woocommerce / woocommerce-android

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

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

Closed samiuelson closed 2 days ago

samiuelson commented 3 weeks ago

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.

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:

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

Generated by :no_entry_sign: Danger

wpmobilebot commented 3 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
Commitb51daa9a9fdcac58e22dccf46cc41e1034011c1f
Direct Downloadwoocommerce-wear-prototype-build-pr12853-b51daa9.apk
wpmobilebot commented 3 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
Commitb51daa9a9fdcac58e22dccf46cc41e1034011c1f
Direct Downloadwoocommerce-prototype-build-pr12853-b51daa9.apk
codecov-commenter commented 2 weeks ago

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 :warning:
...s/cardreader/payment/CardReaderPaymentViewModel.kt 94.44% 2 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

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