-
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-1307] Show processing view while payment is processing #2013
Conversation
@@ -296,6 +311,7 @@ final class PostCampaignCheckoutViewController: UIViewController, MessageBannerV | |||
guard error == nil, status == .succeeded else { | |||
self.messageBannerViewController? | |||
.showBanner(with: .error, message: Strings.Something_went_wrong_please_try_again()) | |||
self.viewModel.inputs.checkoutTerminated() |
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 don't like how many different places we're calling checkoutTerminated
in this - it feels messy. But I can't think of a better way to implement this. Let me know if y'all have any ideas.
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.
You could write a wrapper method like
checkoutTerminated(withErrorMessage message: String?) {
self.messageBannerViewController?
.showBanner(with: .error, message: message ?? Strings.Something_went_wrong_please_try_again())
self.viewModel.inputs.checkoutTerminated()
}
which would consolidate two of these.
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.
That feels more confusing to me actually, since then the checkoutTerminated
would sometimes be nested and sometimes not. I think I'm going to leave this as-is if you're okay with it. I also think it's clearer now that I got rid of the extraneous calls, so thanks for that call-out!
The other thing I tried was having the processing view tied directly to api calls the way the PledgeViewModel does, but it did disappear and reappear between linked calls, so I say that very much did not work.
validateCheckoutError.mapConst(true), // Card validation error terminates flow. | ||
self.checkoutTerminatedProperty.signal.mapConst(true), // Error/cancellation in VC. | ||
checkoutCompleteSignal.signal.mapConst(true) // Checkout completed. | ||
) |
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 see what you mean about worrying about edge cases in here. I don't see any obvious errors - but for readability, you could sort the merge
into clearer groups of true
and false
.
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.
Done!
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.
Some suggestions for clarity but overall LGTM
@@ -391,10 +403,25 @@ public class PostCampaignCheckoutViewModel: PostCampaignCheckoutViewModelType, | |||
.map { $0 } | |||
|
|||
self.checkoutError = checkoutCompleteSignal.signal.errors() | |||
|
|||
self.processingViewIsHidden = Signal.merge( | |||
initialData.mapConst(true), |
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.
Because this property starts as true
I don't think this one is necessary.
@@ -12,11 +12,14 @@ final class PostCampaignCheckoutViewModelTests: XCTestCase { | |||
Never |
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.
Can you add more tests for the processing view in here?
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.
Yeah, of course! I wanted to make sure we were on the same page about going with this approach before I wrote proper tests. But I think we're good now!
@@ -296,6 +311,7 @@ final class PostCampaignCheckoutViewController: UIViewController, MessageBannerV | |||
guard error == nil, status == .succeeded else { | |||
self.messageBannerViewController? | |||
.showBanner(with: .error, message: Strings.Something_went_wrong_please_try_again()) | |||
self.viewModel.inputs.checkoutTerminated() |
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.
You could write a wrapper method like
checkoutTerminated(withErrorMessage message: String?) {
self.messageBannerViewController?
.showBanner(with: .error, message: message ?? Strings.Something_went_wrong_please_try_again())
self.viewModel.inputs.checkoutTerminated()
}
which would consolidate two of these.
@@ -430,6 +447,7 @@ extension PostCampaignCheckoutViewController: STPApplePayContextDelegate { | |||
completion: @escaping StripeApplePay.STPIntentClientSecretCompletionBlock | |||
) { | |||
guard let paymentIntentClientSecret = self.applePayPaymentIntent else { | |||
self.viewModel.inputs.checkoutTerminated() |
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.
These calls aren't needed - AFAIK everywhere that completion(nil, error)
is called will end up below in case: .error
of func applePayContext(didCompleteWith:error:)
. Tested this with a breakpoint and by breaking one of the calls.
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.
Oh, thanks! That makes this nicer :) I wasn't sure how to generate one of these errors so I really appreciate that!
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.
Ha, me neither, but I just set something to nil
and checked what happened 😆
40b5a85
to
90cb1ca
Compare
* Show processing view while payment is processing * Support rest of payment flow * Test processing view in apple pay flow * Minor edits for clarity and more tests
📲 What
Show processing view while payment is processing. This prevents further user interaction until it's dismissed.
Note: It's very important that we make sure that regardless of how checkout goes/where it fails, the processing view will get dismissed. It will lock the app otherwise. The original pledge VC ties the processing view to specific API calls, but we can't do that for post campaign pledges, since we require multiple different API calls to finish the payment flow. Please help me double check that there aren't edge cases that would leave the processing screen up indefinitely!
Note: The processing screen does hide immediately once apple pay is dismissed - I have no idea why the gif is showing it weird.
👀 See
Jira
✅ Acceptance criteria
⏰ TODO