Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Shipping Labels Revamp] Fix custom package dimension units #13214

Merged
merged 7 commits into from
Jan 4, 2025

Conversation

ThomazFB
Copy link
Contributor

@ThomazFB ThomazFB commented Dec 27, 2024

Why

Every Package request comes with the Store Options data. This data is essential for the entire package selection structure, but the data wasn't being held by the main ViewModel, which blocked some scenarios where Store Options were required after the request.

How

Updates the Package selection flow to keep the StoreOptions data returned by the Packages endpoint, and map the dimensions unit in it to the Custom package creation, allowing the Custom package to keep the correct unit type.

How to Test

  1. Simply make sure that all three types of Package selection are working as expected.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 27, 2024

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commita7633b5
Direct Downloadwoocommerce-wear-prototype-build-pr13214-a7633b5.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 27, 2024

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commita7633b5
Direct Downloadwoocommerce-prototype-build-pr13214-a7633b5.apk

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 70.73171% with 12 lines in your changes missing coverage. Please review.

Project coverage is 40.62%. Comparing base (9153b1d) to head (a7633b5).
Report is 8 commits behind head on trunk.

Files with missing lines Patch % Lines
...ckages/datasource/WooShippingLabelPackageMapper.kt 0.00% 8 Missing ⚠️
...ckages/WooShippingLabelPackageCreationViewModel.kt 71.42% 1 Missing and 1 partial ⚠️
...i/orders/wooshippinglabels/packages/ui/UIModels.kt 84.61% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #13214      +/-   ##
============================================
+ Coverage     40.61%   40.62%   +0.01%     
- Complexity     6386     6388       +2     
============================================
  Files          1351     1351              
  Lines         77525    77562      +37     
  Branches      10659    10665       +6     
============================================
+ Hits          31486    31512      +26     
- Misses        43259    43270      +11     
  Partials       2780     2780              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThomazFB ThomazFB added type: task An internally driven task. feature: shipping labels Related to creating, ordering, or printing shipping labels. labels Dec 30, 2024
@ThomazFB ThomazFB added this to the 21.4 milestone Dec 30, 2024
@ThomazFB ThomazFB marked this pull request as ready for review December 30, 2024 19:33
@atorresveiga atorresveiga self-assigned this Jan 3, 2025
@@ -50,6 +52,14 @@ class FetchPredefinedPackagesFromStore @Inject constructor(
.let { Carrier.DHL to it }
)

private fun StoreOptionsDAO.toStoreOptionsForPackages() =
StoreOptionsForPackages(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomazFB, do you think we need other store options class for packages?
We already refresh store options when we start the purchase flow with the ObserveStoreOptions, and we cache that data. I might be missing something, but re-using the use case could help us maintain a single source of truth. WDYT?

Copy link
Contributor Author

@ThomazFB ThomazFB Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @atorresveiga! I considered using it, but the thing is: these store options we're talking about are coming from the Package request we have to do to display the selection data; it comes for free. while re-using the use case would add complexity or maybe an additional request to a data that's already there for us to use. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of what I'm talking about:

image

Copy link
Contributor

@atorresveiga atorresveiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! :shipit:

@atorresveiga atorresveiga enabled auto-merge January 4, 2025 14:51
@atorresveiga atorresveiga merged commit 25b59c5 into trunk Jan 4, 2025
15 checks passed
@atorresveiga atorresveiga deleted the issue/fix-custom-package-units branch January 4, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: shipping labels Related to creating, ordering, or printing shipping labels. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants