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

Refactor embedded view selection flow #4477

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

yuki-stripe
Copy link
Collaborator

Summary

Refactored the updateSelectionState into two separate, smaller methods with single responsibilities:

  • embeddedPaymentMethodsViewDidUpdateSelection, called whenever the selection changes
  • embeddedPaymentMethodsViewDidTapPaymentMethodRow, called whenever a row is tapped

and 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:

  • If the customer taps the card row, we should present the form, send a "tapped" analytic, etc.
  • If we reset the selection back to the card row, we shouldn't do those things.

but instead it:

  • Always try to present the form
  • Always send a "tapped" analytic if the selection changed

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

Comment on lines -56 to +53
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])
Copy link
Collaborator Author

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) {
Copy link
Collaborator Author

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() {
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants