[Migrate simple payments] Fix card reader payment methods from Menu > Payments > Collect Payment

[jaclync]

Closes: #12790

Why

A merchant reported that the IPP flow from Menu > Payments > Collect Payment entry point stopped working, while the same flow in the orders tab worked. With @joshheald's helpful information peaMlT-Ec-p2#comment-1526, the issue was from not passing a rootViewController to PaymentMethodsView. It was probably missed from the beginning, but the card reader flow wasn't tested before the release since "card reader" and "Tap to Pay" are the only two payment methods that use the rootViewController with UIKit views (I thought I tested this but maybe at an early stage).

How

To fix this, I created PaymentMethodsWrapperHostingController (UIKit) and PaymentMethodsWrapperHosted (SwiftUI) to provide a root view controller to PaymentMethodsView to fix card reader payment method flow (as suggested in Josh's first point peaMlT-Ec-p2#comment-1526). There is also another existing hosting controller and its SwiftUI version in PaymentMethodsHostingController/PaymentMethodsHostingView, but there's more logic in it than what's needed in the Payments menu flow so I created "wrapper" versions (feel free to suggest other names).

It's quite dangerous how PaymentMethodsView.rootViewController isn't easy to enforce and missing it could result in a broken IPP flow. I thought about assertionFailure somewhere but couldn't find a good place to do it. Not mixing SwiftUI and UIKit with optional view controller setup would be great in the future.

Testing instructions

• @jaclync tests on a device for card reader payment method
• @jaclync tests on a device for Tap To Pay payment method

---

Prerequisite: a card reader is available

• Go to Menu > Payments > Collect Payment
• Enter a custom amount, or add some products to the order
• Tap Collect Payment in the order form
• Tap Card Reader --> the card reader flow should start
• Continue to the payment flow --> the payment should be successful and it should navigate back to the Payments menu

Screenshots

Simulator.Screen.Recording.-.iPad.10th.generation.-.2024-05-21.at.13.24.22.mp4

---

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

[wpmobilebot]

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr12801-6e205a0
	Version	18.7
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	6e205a0
	App Center Build	WooCommerce - Prototype Builds #9184

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

[iamgabrielma]

Looks good!

It's quite dangerous how PaymentMethodsView.rootViewController isn't easy to enforce and missing it could result in a broken IPP flow. I thought about assertionFailure somewhere but couldn't find a good place to do it. Not mixing SwiftUI and UIKit with optional view controller setup would be great in the future.

I agree with asserting, perhaps through an Unit Test? At least to assert that the root is not nil.

Additionally, I'd also say that we should think of crashing on purpose in production if we land in this scenario, if we can: The weak reference to rootViewController here is unsafe since we may retain the reference to the class but have deallocated already the reference to the UIViewController. The problem I see is not that the app could crash by itself if tried to access the object that already has been deallocated (that would be the best), but that there's something else in memory and could trigger undefined behaviour.

[joshheald]

Looks good!

It's quite dangerous how PaymentMethodsView.rootViewController isn't easy to enforce and missing it could result in a broken IPP flow. I thought about assertionFailure somewhere but couldn't find a good place to do it. Not mixing SwiftUI and UIKit with optional view controller setup would be great in the future.

I agree with asserting, perhaps through an Unit Test? At least to assert that the root is not nil.

Additionally, I'd also say that we should think of crashing on purpose in production if we land in this scenario, if we can: The weak reference to rootViewController here is unsafe since we may retain the reference to the class but have deallocated already the reference to the UIViewController. The problem I see is not that the app could crash by itself if tried to access the object that already has been deallocated (that would be the best), but that there's something else in memory and could trigger undefined behaviour.

A simpler/more effective check would probably be requiring the vc at init:

    init(dismiss: @escaping () -> Void, 
         rootViewController: UIViewController,
         viewModel: PaymentMethodsViewModel) {
        self.dismiss = dismiss
        self.rootViewController = rootViewController
        self.viewModel = viewModel
    }

That works but it's a bit annoying because requires the chain of SwiftUI callers to unwrap their rootViewController references, which all have to be optional because they're weak.

I think the main issue is that we're passing view controllers around :(

[jaclync]

Re @iamgabrielma:

I agree with asserting, perhaps through an Unit Test? At least to assert that the root is not nil.

Hmm as the problem is PaymentMethodsView usage inside InPersonPaymentsMenu (both SwiftUI views), I'm not sure if it's a common practice to test a SwiftUI view since we can't access subviews easily to check that PaymentMethodsView's rootViewController is set.

Additionally, I'd also say that we should think of crashing on purpose in production if we land in this scenario, if we can: The weak reference to rootViewController here is unsafe since we may retain the reference to the class but have deallocated already the reference to the UIViewController. The problem I see is not that the app could crash by itself if tried to access the object that already has been deallocated (that would be the best), but that there's something else in memory and could trigger undefined behaviour.

I agree some form of crashing/logging would be helpful for us to detect this misconfiguration early. In 6e205a0, when PaymentMethodsView appears, it now checks if rootViewController is nil and if so it:

• Logs a non-fatal error in Sentry
• Crash in debug builds only (assertionFailure)

Lemme know if you think crashing in production would be better!

---

Re @joshheald:

A simpler/more effective check would probably be requiring the vc at init:

    init(dismiss: @escaping () -> Void, 
         rootViewController: UIViewController,
         viewModel: PaymentMethodsViewModel) {
        self.dismiss = dismiss
        self.rootViewController = rootViewController
        self.viewModel = viewModel
    }

That works but it's a bit annoying because requires the chain of SwiftUI callers to unwrap their rootViewController references, which all have to be optional because they're weak.

This would be ideal, but in the wrapper hosting controller, self isn't available when initializing PaymentMethodsView 😞 Lemme know if you have other ideas to get around it.

I think the main issue is that we're passing view controllers around :(

Soo true!

---

I'm planning to merge the PR today, but I can open another PR for any further suggestions.

[iamgabrielma]

Lemme know if you think crashing in production would be better!

In this case, since we haven't seen the problem while developing I think I gravitate more towards crashing in prod and having this recorded explicitly than allowing for undefined behaviour and crashing somewhere else pointing to the wrong trace just because it's using the same memory block. But since since we're logging the case anyway in Sentry I have no strong preference.

[jaclync]

In this case, since we haven't seen the problem while developing I think I gravitate more towards crashing in prod and having this recorded explicitly than allowing for undefined behaviour and crashing somewhere else pointing to the wrong trace just because it's using the same memory block. But since since we're logging the case anyway in Sentry I have no strong preference.

Okay let's go with logging a non-fatal error in Sentry for now, it's a tradeoff where the app won't crash but we don't get alerted right away. Non-fatal events should still appear as issues in each release.