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

[POS] Custom payment UI — Part 1 | Separating payment collection management from Android Framework #12853

Merged
merged 16 commits into from
Nov 19, 2024

Conversation

samiuelson
Copy link
Collaborator

@samiuelson samiuelson commented Oct 31, 2024

Custom payment UI

Separating payment collection management from Android Framework

Closes: #12824

💡 I tried to keep this PR as small as possible, but I wasn't able to make it into the 300 loc limit. The code was only restructured though—extracted to a new class.

This is part 1 of 5 PRs refactoring the Payment flow:

  1. [POS] Custom payment UI — Part 1 | Separating payment collection management from Android Framework
  2. [POS] Custom payment UI — Part 2 | Separating CardReaderPaymentController's events from MultiLiveEvent
  3. [POS] Custom Payment UI – Part 3 | Emitting UI–agnostic payment states in CardReaderPaymentController
  4. [POS] Custom payment UI — Part 4 | Switch from viewStateData to paymentState
  5. [POS] Custom payment UI — Part 5 | Unit tests clean up

⚠️ Don't merge — the branch will serve as a base branch for merging the above PRs together, after additional testing.

Description

The intent of this PR is to separate the business logic controlling payment collection and processing process from the framework APIs (like ViewModel.viewModelScope, SavedStateHandle, FragmentArgs, and DialogFragment-specific events).

  1. This was achieved by extracting all the logic from CardReaderPaymentViewModel: ScopedViewModel to CardReaderPaymentController — plain Kotlin class.
    5. Additionally, CardReaderPaymentController emits its own events that CardReaderPaymentViewModel is mapping to its own ones. Thanks this separation CardReaderPaymentController doesn't depend on the MultiLiveEvent making it easier to decouple the whole POS mode from the existing app (e.g. making it easier to extract into a standalone app in the future). Moved to the next PR

Testing information

The refactor introduced by this PR is mostly a code restructuring. As a result, the app should work without any change. It's crucial to test the IPP flow in the store management and POS modes against regression. It may be useful to base on the test plan (pdfdoF-5Jz-p2).

The tests that have been performed

I tested the payment collection flow in both POS and store management modes, using card-present payment and TTP; verified that the IPP flow works and is not changed.

  • 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.

@dangermattic
Copy link
Collaborator

dangermattic commented Oct 31, 2024

1 Error
🚫 This PR is tagged with status: do not merge label(s).
2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class CardReaderPaymentController is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 31, 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
Commitb51daa9
Direct Downloadwoocommerce-wear-prototype-build-pr12853-b51daa9.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 31, 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
Commitb51daa9
Direct Downloadwoocommerce-prototype-build-pr12853-b51daa9.apk

@samiuelson samiuelson added this to the 21.2 milestone Oct 31, 2024
@samiuelson samiuelson added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Oct 31, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 91.25000% with 49 lines in your changes missing coverage. Please review.

Project coverage is 40.38%. Comparing base (9c96e74) to head (f2efc1a).
Report is 133 commits behind head on trunk.

Files with missing lines Patch % Lines
.../payment/controller/CardReaderPaymentController.kt 91.03% 19 Missing and 28 partials ⚠️
...s/cardreader/payment/CardReaderPaymentViewModel.kt 94.44% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##              trunk   #12853      +/-   ##
============================================
+ Coverage     40.34%   40.38%   +0.03%     
- Complexity     5773     5783      +10     
============================================
  Files          1245     1246       +1     
  Lines         70799    70848      +49     
  Branches       9893     9892       -1     
============================================
+ Hits          28563    28609      +46     
- Misses        39604    39607       +3     
  Partials       2632     2632              

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

@samiuelson samiuelson changed the title Custom payment UI — separating payment collection management from Android Framework [POS] Custom payment UI – Part 1. Separating payment collection management from Android Framework Nov 7, 2024
@samiuelson samiuelson changed the title [POS] Custom payment UI – Part 1. Separating payment collection management from Android Framework [POS] Custom payment UI — Part 1 | Separating payment collection management from Android Framework Nov 7, 2024
@samiuelson samiuelson requested a review from kidinov November 7, 2024 19:10
@samiuelson samiuelson marked this pull request as ready for review November 7, 2024 19:10
@kidinov kidinov self-assigned this Nov 8, 2024
private const val CANADA_FEE_FLAT_IN_CENTS = 15L

@Suppress("LongParameterList", "LargeClass")
class CardReaderPaymentController(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not in this PR but I think we'll need to move the tests from the VM to this class. Now we test it kinda as side effect from the tests from the VM

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.

I tested main flows - no regression found. We will need to retest it again when other PRs will be merged here

I left a couple of ideas, please take a look

@kidinov kidinov requested a review from malinajirka November 18, 2024 14:36
@samiuelson samiuelson changed the base branch from trunk to feature/custom-payment-ui November 19, 2024 14:34
@samiuelson samiuelson merged commit a92319d into feature/custom-payment-ui Nov 19, 2024
14 of 17 checks passed
@samiuelson samiuelson deleted the custom-payment-ui-1 branch November 19, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: point of sale POS project status: do not merge Dependent on another PR, ready for review but not ready for merge. unit-tests-exemption
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extract the business logic from CardReaderPayment VM, to an external controller class
6 participants