-
Notifications
You must be signed in to change notification settings - Fork 997
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
Refactor embedded view selection flow #4477
base: master
Are you sure you want to change the base?
Conversation
XCTAssertTrue(mockDelegate.didCallHeightDidChange, "didCallHeightDidChange should be called when selection changes to show a mandate") | ||
XCTAssertTrue(mockDelegate.didCallSelectionDidUpdate, "selectionDidUpdate should be called when selection changes") | ||
XCTAssertEqual(mockDelegate.calls, [.didUpdateHeight, .didUpdateSelection, .didTapPaymentMethodRow]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing these tests! I like this array approach b/c it's less verbose and also tests order and cardinality i.e. ensures they're only called once.
embeddedPaymentMethodsView.resetSelectionToLastSelection() | ||
} | ||
embeddedFormViewController.dismiss(animated: true) | ||
} | ||
|
||
func embeddedFormViewControllerShouldClose(_ embeddedFormViewController: EmbeddedFormViewController) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method name implies it would be called when you cancel, since you should close then too, so I renamed it to be clearer
self.formViewController = nil | ||
return | ||
/// Helper method to inform delegate only if the payment option changed | ||
func informDelegateIfPaymentOptionUpdated() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a footgun here where if you call the delegate method directly instead of calling this helper, the helper will break b/c it won't know the correct last updated payment option. Not sure there's a good way to prevent that in code.
Summary
Refactored the
updateSelectionState
into two separate, smaller methods with single responsibilities:embeddedPaymentMethodsViewDidUpdateSelection
, called whenever the selection changesembeddedPaymentMethodsViewDidTapPaymentMethodRow
, called whenever a row is tappedand fixed some bugs from the old implementation.
Motivation
https://jira.corp.stripe.com/browse/MOBILESDK-2974
The old
updateSelectionState
code had some bugs. It should work like this:but instead it:
It was also doing too many different things.
Testing
See expanded view tests.
The old delegate method implementation should probably have been unit tested given its complexity, but IMO they don't need to be now that they're so simple and small.
Changelog
See changelog