Skip to content

Conversation

@hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Nov 29, 2024

Closes: #13027

Description

When a subscription product that has "One time shipping" enabled get converted into simple product, "One time shipping" label is still shown. This is incorrect because "one time shipping" is only supported in subscription product, not simple.

This PR adds the fix by checking product type and not showing the label if it's simple.

That issue is not tackled here yet.

Steps to reproduce

  1. Create a Simple Subscription product,
  2. Go to Add More Details > Shipping,
  3. Enter a weight value, and enable "One time shipping", then press back,
  4. Confirm that "Shipping" card is shown with "One time shipping: Enabled" text shown,
  5. Publish the product,
  6. Once published, tap "Product Type" card and switch to "Simple Physical Product", confirm the change,
  7. Check the "Shipping" card and ensure it now only says "Weight" and doesn't mention "One time shipping".

Testing information

Tested in API 35 with Simulator.

I also discovered a separate issue when working on this:

Given product details with "one time shipping" enabled but no other shipping values entered, when switching to simple product, Shipping card disappears

This issue is not tackled in this PR.

The tests that have been performed

The listed steps above.

Images/gif

Screen_recording_20241129_163549.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

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 Nov 29, 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
Commite5b5579
Direct Downloadwoocommerce-wear-prototype-build-pr13032-e5b5579.apk

@hafizrahman hafizrahman added this to the 21.3 milestone Nov 29, 2024
@hafizrahman hafizrahman added the feature: product details Related to adding or editing products, includes product settings. label Nov 29, 2024
@hafizrahman hafizrahman marked this pull request as ready for review November 29, 2024 09:37
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 29, 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
Commite5b5579
Direct Downloadwoocommerce-prototype-build-pr13032-e5b5579.apk

@codecov-commenter
Copy link

codecov-commenter commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 40.23%. Comparing base (f109bb9) to head (e5b5579).
Report is 90 commits behind head on trunk.

Files with missing lines Patch % Lines
...id/ui/products/details/ProductDetailCardBuilder.kt 91.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #13032      +/-   ##
============================================
+ Coverage     40.13%   40.23%   +0.10%     
- Complexity     6120     6138      +18     
============================================
  Files          1280     1280              
  Lines         74008    74011       +3     
  Branches      10122    10124       +2     
============================================
+ Hits          29703    29779      +76     
+ Misses        41725    41645      -80     
- Partials       2580     2587       +7     

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

@JorgeMucientes JorgeMucientes self-assigned this Dec 3, 2024
Copy link
Contributor

@JorgeMucientes JorgeMucientes left a comment

Choose a reason for hiding this comment

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

Nice job @hafizrahman it works as expected and code looks good. Thanks for adding the unit tests.

Btw I was about to report the weird issue about the shipping card being gone when switching the product from subscription to simple when I noticed you already created a GH issue for it. Anyway I think it is a very minor thing and very corner case for a user to land in that situation, so I wouldn't spend to much time fixing it, unless it is very straighforward :)

Comment on lines +486 to +487
// Only add "One time shipping" info if product is Subscription types
if (currentProduct.productType == SUBSCRIPTION || currentProduct.productType == VARIABLE_SUBSCRIPTION) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch adding this check!

@JorgeMucientes JorgeMucientes merged commit 300721b into trunk Dec 3, 2024
16 of 18 checks passed
@JorgeMucientes JorgeMucientes deleted the issue/13027-remove-one-time-label-when-switching-to-simple branch December 3, 2024 23:20
@hafizrahman
Copy link
Contributor Author

Btw I was about to report the weird issue about the shipping card being gone when switching the product from subscription to simple when I noticed you already created a GH issue for it.

Yeah! I ended up working on it here #13035

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: product details Related to adding or editing products, includes product settings.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One time shipping label remains visible when the product is changed to a simple product

5 participants