-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[MBL-1491] [MBL-1663] Handle 3DS authentication with informational snackbar #2236
Conversation
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.
LGTM.
I have some thoughts/suggestions/nits/discussions, but nothing that's blocking.
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModel.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModel.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/CardView/PPOProjectCardModel.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOContainerViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOContainerViewModel.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModel.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModel.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModelTests.swift
Outdated
Show resolved
Hide resolved
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.
I like the banner changes - very nice! I am concerned about this approach to 3DS - I know we talked about the complication with where to present what (considering banners) before, but it feels weird to me to have a view model presenting UI instead of a view controller. I trust your judgement, though. But I do think it'd be helpful to test this end to end now, while you still have alternatives fresh in your mind, in case the 3DS UI doesn't get presented correctly or something.
Kickstarter-iOS/Features/PledgedProjectsOverview/CardView/PPOProjectCardModel.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOViewModel.swift
Outdated
Show resolved
Hide resolved
6082cf0
to
c21dfa9
Compare
SwiftFormat found issues:
Generated by 🚫 Danger |
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.
LGTM! I like the refactor. A couple non-blocking nits and commentary.
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOContainerViewController.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOContainerViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/PledgedProjectsOverview/PPOContainerViewModel.swift
Outdated
Show resolved
Hide resolved
4e6303d
to
b11d9ef
Compare
… it up through to PPOContainerViewController
b11d9ef
to
e0656e7
Compare
📲 What
This PR adds support for letting a user handle 3DS authentication via Stripe.
🤔 Why
Stripe won't process some transactions otherwise.
🛠 How
When the user taps the "authorize card" button, I added a loading indicator to the button and updated the enum cases to include a way to turn that loading indicator off.
Then I added fetching the client secret from GraphQL and passing that through the enum case for triggering 3DS authentication.
With those pieces in place, I updated PPOViewModel to call Stripe's authentication check. In debug mode, this is simulated for ease of testing. When the callback completes, the app informs the user that the call has either succeeded or failed. This was done with a touched up version of the MessageBannerView that already existed and had some basic hookups in the PPOViewModel.
If successful, the item is hidden. This was done by adding filtering capability to PPOViewModel and tying it to the existing Publisher chain for populating results.
I also added some convenience helpers for clarity, such as
withEmptyValues()
on Publisher, a pattern that I was using in a few places, and a way to map the values of a Paginator's Results (in this case, to facilitate filtering).👀 See
Figma
✅ Acceptance criteria
Tests should pass.
End-to-end testing should succeed.