-
Notifications
You must be signed in to change notification settings - Fork 135
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
[Payment Method Improvements] Tapping Order Complete button should be equivalent tapping Mark as Paid #11587
[Payment Method Improvements] Tapping Order Complete button should be equivalent tapping Mark as Paid #11587
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11587 +/- ##
============================================
- Coverage 40.34% 40.34% -0.01%
Complexity 5199 5199
============================================
Files 1087 1087
Lines 63250 63265 +15
Branches 8669 8671 +2
============================================
+ Hits 25519 25524 +5
- Misses 35436 35445 +9
- Partials 2295 2296 +1 ☔ View full report in Codecov by Sentry. |
…ng-order-complete-button-equivalent-tapping-mark-as-paid
@@ -70,6 +74,16 @@ | |||
<argument | |||
android:name="orderId" | |||
app:argType="long" /> | |||
|
|||
<action | |||
android:id="@+id/action_changeDueCalculatorFragment_to_selectPaymentMethodFragment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shell, you go back and bring result with "is order paid = true/false" instead of going forward and passing "true" always? Check the setupResultHandlers
method in SelectPaymentMethodFragment
- I believe they do pretty much the same as what you need in order to deliver the result back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I'll do it that way instead as you suggested. Added to this ticket for my draft PR:
#11585
Overall some things I noted (sorry if this is already fixed in the following PRs and I repeat myself):
But I still see
|
...tlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few notes I left which I think worth attention, but you can fix them in the open PR so not blocking this one
Sorry for the confusion @kidinov my PR description was innaccurate. Those changes are in this PR: https://github.com/woocommerce/woocommerce-android/pull/11593/commits I will update the margin and padding there as well. |
Closes: #11583
Description
This PR is to update the change due correctly based on the user input for how much cash was given.
unit-tests-exemption
label as this PR is dependent on others and will likely change. Updating the test is ticketed here:[Payment] Testing: Add implementation so order details load successfully, emits success state and all tests pass #11542
Testing instructions
Expected:
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.