Create ApplePayTokenUseCase and tests #2258
Conversation
| @@ -279,7 +279,6 @@ | |||
| 33AF01BC2D2D74B30085A153 /* PledgePaymentIncrementState+Extension.swift in Sources */ = {isa = PBXBuildFile; fileRef = 33AF01BB2D2D748B0085A153 /* PledgePaymentIncrementState+Extension.swift */; }; | |||
| 33AF01BF2D2E72430085A153 /* PledgeOverTimePaymentScheduleViewModelTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 33AF01BD2D2E71E30085A153 /* PledgeOverTimePaymentScheduleViewModelTest.swift */; }; | |||
| 33AF01C22D2E973A0085A153 /* PledgeOverTimePaymentScheduleViewControllerTest.swift in Sources */ = {isa = PBXBuildFile; fileRef = 33AF01C02D2E95FD0085A153 /* PledgeOverTimePaymentScheduleViewControllerTest.swift */; }; | |||
| 33C9F0BE2CF5104C00B62E14 /* PledgePaymentPlanOptionView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 33C9F0BC2CED6DEF00B62E14 /* PledgePaymentPlanOptionView.swift */; }; | |||
There was a problem hiding this comment.
Must have been an automated change, I didn't touch this.
There was a problem hiding this comment.
Interesting, the same change showed up on my branch as well, maybe it was a merge issue or something as it appeared twice in the pbxproj. Agreed that it appears to be automated, should be fine to include it IMO
| } | ||
|
|
||
| public protocol ApplePayTokenUseCaseOutputs { | ||
| var goToApplePayPaymentAuthorization: Signal<PKPaymentRequest, Never> { get } |
There was a problem hiding this comment.
Slight tweak; I made this a PKPaymentRequest instead of PaymentAuthorizationData.
| */ | ||
|
|
||
| public final class ApplePayTokenUseCase: ApplePayTokenUseCaseInputs, ApplePayTokenUseCaseOutputs { | ||
| init(initialData: Signal<PaymentAuthorizationData, Never>) { |
There was a problem hiding this comment.
This is largely copy-pasted from NoShippingPledgeViewModel.
| ) | ||
| .mapConst(PKPaymentAuthorizationStatus.failure) | ||
|
|
||
| self.createApplePayBackingStatusProperty <~ Signal.merge( |
There was a problem hiding this comment.
TIL this is a ReactiveSwift operator that means bind.
There was a problem hiding this comment.
is it fundamentally the same as =? I guess I'm wondering why use a different approach suddenly for this output.
There was a problem hiding this comment.
Not quite - here's the comment on it in ReactiveSwift: https://github.com/ReactiveCocoa/ReactiveSwift/blob/master/Sources/UnidirectionalBinding.swift#L68
My guess is that we're doing this because Signal.merge outputs a Signal, but createApplePayBackingStatusProperty is a MutableProperty - so you couldn't assign this with =.
More broadly, though, it's not clear to me whether using MutableProperty everywhere is appropriate in our codebase. I don't think we ever read the current value of a mutable property, we just observe them as if they were signals. This is something I'd like to understand better about the guts of ReactiveSwift.
amy-at-kickstarter
changed the title
| To complete an ApplePay payment, the use case should be used in this order: | ||
| - `input.applePayButtonTapped()` - The ApplePay button has been tapped | ||
| - `output.goToApplePayPaymentAuthorization` - The view controller should display a `PKPaymentAuthorizationViewController` with the sent `PKPaymentRequest` | ||
| - `input.paymentAuthorizationDidAuthorizePayment(paymentData:)` - The `PKPaymentAuthorizationViewController` successfully authorized a payment | ||
| - `input.stripeTokenCreated(token:error:)` - Stripe successfully turned the `PKPayment` into a Stripe token. Returns a status, which is also sent by the `applePayAuthorizationStatus` signal. | ||
| - `input.paymentAuthorizationViewControllerDidFinish()` - The `PKPaymentAuthorizationViewController` was dismissed | ||
| - `output.applePayParams` - Sends parameters which can be used in `CreateBacking` or `UpdateBacking`. Sends an initial `nil`value, by default. |
There was a problem hiding this comment.
Not necessary for this PR, but long term it might be good to build a little enum-based state machine for this if these must be called in order. Then the compiler can enforce they are called correctly.
There was a problem hiding this comment.
Love this idea. Would be great to make this enforced in code instead of with hopes and comments.
| .map { pkPayment -> PKPaymentData? in | ||
| guard let displayName = pkPayment.displayName, let network = pkPayment.network else { | ||
| return nil | ||
| } | ||
|
|
||
| return (displayName, network, pkPayment.transactionIdentifier) | ||
| } |
There was a problem hiding this comment.
Looks like all the downstream signal processing is skipping nils. You could move the skipNils to this step or use compactMap instead of map here.
| } | ||
|
|
||
| /** | ||
| A use case for ApplePay transactions in the regular (not post!) pledge flow. |
There was a problem hiding this comment.
nit: I know that sometimes we use live instead of post. IMO late is a better naming convention, but either way It'd be a good idea to align on one and stick to it. You used live in your other PR so maybe we can go with that here.
| ) | ||
| .mapConst(PKPaymentAuthorizationStatus.failure) | ||
|
|
||
| self.createApplePayBackingStatusProperty <~ Signal.merge( |
There was a problem hiding this comment.
is it fundamentally the same as =? I guess I'm wondering why use a different approach suddenly for this output.
amy-at-kickstarter
force-pushed
the
feat/adyer/mbl-1965
branch
from
f54efdf to
9e0909e
Compare
📲 What
Create a use case that will handle ApplePay in the regular (live) pledge flow.
🤔 Why
This puts a lot of ApplePay code in one place, so we can pull it out of
NoShippingPledgeViewModelin the next release.More broadly: our goal is to simplify our checkout code. One way we can do this is by breaking it down into smaller, more understandable parts. We're following Android's lead here and building 'Use Cases', which are small chunks of code related to a single behavior or feature. These little pieces are much easier to understand, debug, and test.
It's worth noting that ApplePay in late + live pledges is fundamentally different - they use different Stripe APIs. It would be nice to consolidate that, but that would be out of scope without further API changes. This is just a refactoring for niceness and modularity, instead of code re-use.