-
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
[NT-359] Integrate Updating Backing mutation #893
[NT-359] Integrate Updating Backing mutation #893
Conversation
// MARK: - Apple Pay | ||
// MARK: - Create Apple Pay Backing | ||
|
||
let willCreateApplePayBacking = Signal.combineLatest( |
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 and willUpdateApplePayBacking
are to ensure that upon completion of these asynchronous events we show the appropriate messaging/view to the user.
|
||
// MARK: - Success/Failure Create | ||
|
||
let createPaymentAuthorizationDidFinishSignal = Signal.zip( |
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.
zip
allows us to ensure that each of these signals will emit at least once before this signal emits. So we know that we started out creating here and not updating because we zip
with the willCreateApplePayBacking
signal.
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.
Initial code pass looks good, I'll test this thoroughly tomorrow once we have an Apple Pay sandbox user set up!
let updateBackingDidCompleteApplePay = updateApplePayBackingSuccess | ||
.ignoreValues() | ||
let updateBackingDidComplete = Signal.zip( | ||
self.submitButtonTappedSignal, |
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.
Same here, do we need this zip
?
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'm no longer convinced it would do what I need it to here, so I'll consider an alternative.
|
||
let createApplePayBackingData = Signal.combineLatest( | ||
createBackingData, | ||
pkPaymentData.skipNil(), | ||
self.stripeTokenSignal.skipNil() | ||
) | ||
.takeWhen(applePayStatusSuccess) | ||
.takeWhen(willCreateApplePayBacking.filter(isTrue)) |
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.
It might make sense to just put this filter(isTrue)
as part of willCreateApplePayBacking
.
let updateBackingDataAndIsApplePay = updateBackingData.takePairWhen( | ||
Signal.merge( | ||
updateButtonTapped.mapConst(false), | ||
willUpdateApplePayBacking.filter(isTrue) |
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.
Same here would it make sense to put this filter(isTrue)
as part of willUpdateApplePayBacking
?
.takeWhen(createApplePayBackingCompleted) | ||
.map(first) | ||
|
||
self.confirmationLabelAttributedText = Signal.merge( |
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 we move this somewhere higher up in the pledge view model? It's kind of separate from the success/failure stuff.
@@ -1256,7 +1258,7 @@ final class PledgeViewModelTests: TestCase { | |||
} | |||
} | |||
|
|||
func testUpdatingConfirmButtonEnabled_ShippingEnabled() { | |||
func testUpdatingSubmitButtonEnabled_ShippingEnabled() { |
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.
Did we change the title of the button? π€
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.
Yes, it's not always a confirm button anymore, sometimes a pledge button. We should also remove the continue button and just use this one.
|
||
self.vm.inputs.applePayButtonTapped() | ||
|
||
self.notifyDelegateUpdatePledgeDidSucceedWithMessage.assertDidNotEmitValue() |
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 we assert here that the output goToApplePayPaymentAuthorization
is called?
self.goToThanks.assertDidNotEmitValue() | ||
|
||
self.vm.inputs.applePayButtonTapped() | ||
|
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.
Same here can we assert that goToApplePayPaymentAuthorization
is called?
self.scheduler.run() | ||
|
||
self.notifyDelegateUpdatePledgeDidSucceedWithMessage.assertDidNotEmitValue() | ||
self.updatePledgeFailedWithError.assertValues([ |
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.
Hmm so if the request errors before the payment authorization VC has dismissed, the error banner might show below the payment authorization VC π€ that's probably not ideal?
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.
π
* Updated validation * Fix snapshot tests, add submitButtonHidden * Add PledgeAmountSummaryViewController * Add updateButtonTapped * Add snapshot test and vm test * lol * Remove unused var * Rename signal, fix payment source ID comparison * Fix tests * Fix tests, simplify hiding of shipping location stackview * Fix snapshots * Commit the actual snapshots * Use local var * [NT-359] Integrate Updating Backing mutation (#893) * Wire up change payment method to mutation for payment source and Apple Pay * Remove payment method when backing with Apple Pay and vice versa * Update tests for apple pay sheet race condition fixes * Replace zip with simpler signals * Add Stripe token error test * Move signals around, improve tests
π² What
A continuation of the work in #887 and #890, this PR is also based off #887's branch.
This wires up the
UpdateBacking
mutation to allow one to change their payment method and Apple Pay backing.π€ Why
This is a continuation of the work to allow a user to manage their pledge.
π How
paymentSourceId
andApplePayParams
toUpdateBackingData
.CreateBacking
mutation, I may have a way to make this even more reusable due to that mutation andUpdateBacking
being so similar.π See
Note: Currently the manage pledge view does not refresh automatically, this is a known issue that will be addressed in a subsequent PR. Also, currently the credit card that is the current payment source for the backing is meant to be selected by default and ordered first. This will also be addressed in an upcoming PR. So, at the moment, the experience is that when you land on Change payment method, you may see the incorrect card selected, but the behaviour of the submit button enabling/disabling should still be correct and allow you to change it.
β Acceptance criteria
Be sure to test this against the native dev environment. The payment method that you use and the project that you back should all be created on that environment. Navigate to Change payment method in the Manage pledge view.
* I was unable to test this unfortunately as I didn't have an iOS 12 device with me at the time and, although I think it should work, Apple Pay doesn't seem to do anything on the simulator. This is possible a Stripe issue? π€