woocommerce / woocommerce-android

WooCommerce Android app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
277 stars 135 forks source link

[Order creation] Update coupons display order details #12855

Closed malinajirka closed 2 weeks ago

malinajirka commented 2 weeks ago

Closes: #12619 #12848 Ref: pe5pgL-5G4-p2#comment-4599

This PR updates the how coupons are displayed during Order creation to match the iOS behavior.

Changes:

Notes:

I planned to add unit tests for the added code. However, I haven't figured out how to test the code in the current state without significant refactoring. It felt like the unit tests would be tightly coupled with the inner workings of the VM, which felt off. In any case, if you have some ideas how to write reasonable tests or if you believe it's worth spending more time on, I will be happy to look into it more.

P.S. I also noticed shipping lines are editable at all times which is a bug I reported here.

Before After
Screenshot_1730470897 Screenshot_1730470794

Testing

I tested the following flows on my phone and tablet.

Case: Add coupon to order success:

Case: Delete coupon from order:

Case: Add coupon to order error

Case: Editing an existing order

Case: Editing an existing PAID order

I also tested adding and removing shipping lines in combination with coupons with gift card extension installed/uninstalled.


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:

wpmobilebot commented 2 weeks ago
📲 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
Commiteb1f773d9bf0bf7fb2b56512c2b29f3cd2e915ed
Direct Downloadwoocommerce-wear-prototype-build-pr12855-eb1f773.apk
wpmobilebot commented 2 weeks ago

📲 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
Commiteb1f773d9bf0bf7fb2b56512c2b29f3cd2e915ed
Direct Downloadwoocommerce-prototype-build-pr12855-eb1f773.apk
codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 17.43119% with 90 lines in your changes missing coverage. Please review.

Project coverage is 40.20%. Comparing base (995e65a) to head (eb1f773). Report is 50 commits behind head on trunk.

Files with missing lines Patch % Lines
...ui/orders/creation/coupon/CouponLineFormSection.kt 0.00% 81 Missing :warning:
...rders/creation/shipping/ShippingLineFormSection.kt 0.00% 4 Missing :warning:
...s/creation/totals/OrderCreateEditTotalsFullView.kt 0.00% 3 Missing :warning:
...oid/ui/orders/creation/OrderCreateEditViewModel.kt 84.61% 0 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## trunk #12855 +/- ## ============================================ - Coverage 40.24% 40.20% -0.04% - Complexity 5784 5786 +2 ============================================ Files 1247 1249 +2 Lines 71160 71247 +87 Branches 9933 9954 +21 ============================================ + Hits 28638 28647 +9 - Misses 39889 39966 +77 - Partials 2633 2634 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dangermattic commented 2 weeks ago
1 Warning
:warning: This PR is assigned to the milestone 21.1. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by :no_entry_sign: Danger

malinajirka commented 2 weeks ago

Thanks so much for the thorough review @samiuelson ! I have implemented all the changes except of the performance optimization.

One more thing that caught my attention is the singular form of the "Coupon" label in the bottom sheet despite multiple coupons applied (isn't it slightly confusing?). Maybe worth considering reporting an issue:

I noticed this as well but then forgot about it :X. I updated it in 18e079db3fe4f946ba006148e351589c180868c3. We use plural for products no matter whether there is one or multiple, so the change brings it to consistency.