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 3 | Emitting UI–agnostic payment states in CardReaderPaymentController #12877

Merged
merged 26 commits into from
Nov 19, 2024

Conversation

samiuelson
Copy link
Collaborator

@samiuelson samiuelson commented Nov 6, 2024

Closes: #12825

This is part 3 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 be merged together with the above ones after additional testing of the whole refactor.

Description

This PR introduces a new class modeling a "payment state" — CardReaderPaymentOrRefundState. It is emitted by the paymentState: StateFlow<CardReaderPaymentOrRefundState> property in CardReaderPaymentController, allowing observing the payment state in POS.

The goal of the paymentState property is to replace the viewStateData: LiveData<ViewState> that emits "UI model" instances, designed specifically for the existing dialog-based payment flow UI. For now, the viewStateData is marked deprecated, and supposed to be used only by the existing IPP flow.

Coming up in the next PR:

  1. Removing viewStateData from the controller
  2. Mapping the controller's paymentState to ViewState in CardReaderPaymentViewModel
  3. Covering controller with unit tests

Testing information

As a result of the refactoring done within this PR, 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

This PR will be followed up by removing the viewStateData property from the controller, adding unit tests, and mapping the paymentState to ViewState. That said, this PR should not cause any regression in the existing IPP flow, however, the most important part to test is the code changes.

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

…tController`

The goal is to remove viewStateData property and map paymentState to ViewState in the target [ViewModel]
The goal is to make payment controller return UI-agnostic payment state and map the payment state to UI state in the target VM.
@dangermattic
Copy link
Collaborator

dangermattic commented Nov 6, 2024

3 Errors
🚫 Please add tests for class CardReaderPaymentOrRefundState (or add unit-tests-exemption label to ignore this).
🚫 Please add tests for class CardReaderPaymentState (or add unit-tests-exemption label to ignore this).
🚫 This PR is tagged with status: do not merge label(s).
1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 6, 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
Commit0c4e9ec
Direct Downloadwoocommerce-wear-prototype-build-pr12877-0c4e9ec.apk

@samiuelson samiuelson added this to the 21.2 milestone Nov 7, 2024
@samiuelson samiuelson changed the title [POS] Custom Payment UI - Emit UI–agnostic payment states in CardReaderPaymentController [POS] Custom Payment UI – Part 3. Emit UI–agnostic payment states in CardReaderPaymentController Nov 7, 2024
@samiuelson samiuelson changed the title [POS] Custom Payment UI – Part 3. Emit UI–agnostic payment states in CardReaderPaymentController [POS] Custom Payment UI – Part 3 | Emitting UI–agnostic payment states in CardReaderPaymentController Nov 7, 2024
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Nov 7, 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
Commitf756819
Direct Downloadwoocommerce-prototype-build-pr12877-f756819.apk

@samiuelson samiuelson added feature: point of sale POS project status: do not merge Dependent on another PR, ready for review but not ready for merge. labels Nov 7, 2024
@samiuelson samiuelson changed the title [POS] Custom Payment UI – Part 3 | Emitting UI–agnostic payment states in CardReaderPaymentController [POS] Custom Payment UI — Part 3 | Emitting UI–agnostic payment states in CardReaderPaymentController Nov 8, 2024
@samiuelson
Copy link
Collaborator Author

samiuelson commented Nov 14, 2024

Thanks for the reviews @kidinov and @malinajirka 🙇🏼

As discussed, we're going to keep the callbacks in the CardReaderPaymentOrRefundState. Also, the mapping of CardReaderPaymentOrRefundState to ViewState, and removing viewStateData from controller (bringing the controller to the final shape) is ready for review in the next PR: [POS] Custom payment UI — Part 4 | Switch from viewStateData to paymentState.

In the current PR, I extracted nameForTracking from the payment state model (c26ed71, 7fe2991) and moved the logic of the ::trackCancelledFlow function into a use case class (44115aa).

Let me know what you think!

else -> null
}
if (nameForTracking == null) {
WooLog.e(WooLog.T.CARD_READER, "Invalid state received")
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, I'd consider to track this case too, just instead of passing hard coded string we could pass state's name there

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the idea, but I would prefer to implement it out of this PR, and refactoring process because it would affect the existing business logic—it would require modifying unit tests.

Reported issue: https://github.com/orgs/woocommerce/projects/278/views/1?visibleFields=%5B%22Title%22%2C%22Assignees%22%2C%22Status%22%2C143176801%2C%22Linked+pull+requests%22%5D&pane=issue&itemId=87370857&issue=woocommerce%7Cwoocommerce-android%7C12930

Base automatically changed from custom-payment-ui-2 to feature/custom-payment-ui November 19, 2024 14:37
Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @samiuelson !

The changes look good to me. I haven't run thorough tests since I believe we'll run them on the final PR of this chain of PRs anyway. I haven't discovered any issues when smoke testing the changes with my WisePad 3 (including interac refunds).

@samiuelson
Copy link
Collaborator Author

Thanks for review, @malinajirka!

I haven't run thorough tests since I believe we'll run them on the final PR of this chain of PRs anyway.

That's correct. The build used in the Call for testing (pdfdoF-5Rg-p2) is generated from the next PR that includes payment state to UI state mapping — [POS] Custom payment UI — Part 4 | Switching from viewStateData to paymentState.

@samiuelson samiuelson merged commit a73aa7f into feature/custom-payment-ui Nov 19, 2024
7 of 14 checks passed
@samiuelson samiuelson deleted the custom-payment-ui-3 branch November 19, 2024 15:28
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants