Skip to content

Conversation

@joshheald
Copy link
Contributor

Part of: #8082

Description

Apologies for the length: this PR duplicates an existing file.

The connection flows for BuiltIn and Bluetooth card readers are different, but similar. As a first step towards modelling this, I've duplicated the CardReaderConnectionController in order to make a BuiltIn variant.

Once both variants are working, I'll factor out the shared code.

No rush on these: I've put them into next week's milestone

Testing instructions

Check you can still take a payment using a bluetooth reader (with the feature flag off)
Run unit tests


  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Adds built-in reader duplicate of connection controller. Will be substantially simplified.
@joshheald joshheald added type: task An internally driven task. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Dec 1, 2022
@joshheald joshheald added this to the 11.6 milestone Dec 1, 2022
@wpmobilebot
Copy link
Collaborator

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8280-bec3cb5 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@joshheald joshheald requested a review from toupper December 1, 2022 15:15
@joshheald joshheald marked this pull request as ready for review December 1, 2022 15:15
@@ -0,0 +1,769 @@
import Combine
Copy link
Contributor

Choose a reason for hiding this comment

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

This class looks massive, probably because of the enums and comments inside, but I think it's doing many things in one place. Did you consider splitting it into several other classes/files to make it easier to maintain and work with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. It's a duplicate of the existing massive CardReaderConnectionController, and my immediate plan is to share the common code between them. This should help with the size, and once that's done I think it'll be worth addressing whether it's doing too much.

In general, it's covering a well-scoped responsibility (connecting to a reader), but perhaps the software updates could be moved out of here, or the error handling.

Another approach (which would be a separate project really) would be to turn it all the way in to a state machine. It looks like it's built with that in mind, but it doesn't have the core of the state machine which decides how to handle transitions from x state based on y input, the transitions are all individually coded.

@toupper
Copy link
Contributor

toupper commented Dec 2, 2022

The code looks great (thanks a lot for all the comments) and it tests well. I just added a small suggestion there. :shipit:

Base automatically changed from task/restore-tap-on-mobile-entitlement to trunk December 5, 2022 09:43
@joshheald joshheald merged commit b007212 into trunk Dec 5, 2022
@joshheald joshheald deleted the issue/8082-duplicate-CardReaderConnectionController-for-built-in-reader-flow branch December 5, 2022 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants