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

[Payment Method Improvements] Implement record transaction details to order note functionality #11594

Merged
merged 34 commits into from
May 30, 2024

Conversation

backwardstruck
Copy link
Contributor

@backwardstruck backwardstruck commented May 27, 2024

Closes: #11585

Description

This PR implements the functionality to record transaction details as an order note upon returning to the order screen after marking the order as complete.

Also updated the unit test based on this suggestion:
#11586 (comment)

And addresses feedback from this PR:
#11585

Testing instructions

  1. Enable OTHER_PAYMENT_METHODS
  2. Open an order and proceed to payment.
    • Choose the cash payment option.
    • Switch the Implement record transaction details to order note toggle to enabled (and try again without switching it)
  3. Mark the order as complete

Expected:

  • Ensure that order notes are added correctly for different amounts paid by the customer.
  • Validate the correctness of the change due calculation in the note.
  • Specifically, a note detailing "The order was paid by cash. Customer paid x. The change due was x." is added to the order to accurately capture the cash payment details.

Images

Addressed feedback so 16.dp is consistently used for horizontal padding across various elements to align them.
Used 8.dp or 16.dp for vertical paddings ensuring consistent spacing within the UI elements.
Ensured the 'Change Due' section and record transaction note have proper padding and spacing.

Aligned the button and input field using consistent horizontal padding.

Before After
Screenshot_20240528_160957 Screenshot_20240528_161052
  • 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.

@dangermattic
Copy link
Collaborator

dangermattic commented May 27, 2024

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ This PR is assigned to the milestone 18.9. 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.
1 Message
📖

This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

@backwardstruck backwardstruck added this to the 18.9 milestone May 27, 2024
@backwardstruck backwardstruck added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. unit-tests-exemption labels May 27, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 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
FlavorJalapeno
Build TypeDebug
Commita1e4a99
Direct Downloadwoocommerce-prototype-build-pr11594-a1e4a99.apk

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2024

Codecov Report

Attention: Patch coverage is 27.27273% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 40.22%. Comparing base (5208114) to head (b607180).

Current head b607180 differs from pull request most recent head a1e4a99

Please upload reports for the commit a1e4a99 to get more accurate results.

Files Patch % Lines
...hangeduecalculator/ChangeDueCalculatorViewModel.kt 28.12% 23 Missing ⚠️
...ts/methodselection/SelectPaymentMethodViewModel.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #11594      +/-   ##
============================================
+ Coverage     40.13%   40.22%   +0.08%     
+ Complexity     5219     5202      -17     
============================================
  Files          1093     1089       -4     
  Lines         63900    63523     -377     
  Branches       8793     8724      -69     
============================================
- Hits          25646    25549      -97     
+ Misses        35932    35670     -262     
+ Partials       2322     2304      -18     

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

Base automatically changed from 11583-tapping-order-complete-button-equivalent-tapping-mark-as-paid to trunk May 28, 2024 15:11
@backwardstruck backwardstruck marked this pull request as ready for review May 29, 2024 03:10
@backwardstruck backwardstruck added the category: tracks Related to analytics, including Tracks Events. label May 29, 2024
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

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

@backwardstruck 👋

In order to move forward and land it on the current release I

  • Resolved merge conflicts
  • Adjusted margins and some UI stuff
  • Moved network call out from the main scope
  • Added loading indicator to a button
  • Removed unused nav action
  • Made the release note public, not internal as this a user-facing change
  • Color theme of the toggle (used to be red or something like that)

Feel free to revert/do any changes if that doesn't make sense to you

05-30--10-26.mp4
image

@kidinov kidinov enabled auto-merge May 30, 2024 08:35
@kidinov kidinov merged commit 5cc1837 into trunk May 30, 2024
13 of 14 checks passed
@kidinov kidinov deleted the 11585-implement-record-transaction-details branch May 30, 2024 09:41
@backwardstruck
Copy link
Contributor Author

Thanks a lot for the help! I checked all those changes you made. Some were pretty serious and I don't see any that need to be reverted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tracks Related to analytics, including Tracks Events. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. type: enhancement A request for an enhancement. unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Payment Method Improvements] Implement record transaction details to order note functionality
5 participants