-
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-1393] Moves Stripe Intent Logic Into It's Own Service #2050
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.
This change looks perfectly good to me on its own, but since we're experimenting with new patterns, I have two thoughts to take it further:
StripeIntentService
should have its own tests- This change helps us to not repeat the API call in multiple classes, which is great, but it still only encapsulates a single signal (the API request). What would it look like if we wrote a service that was a higher-order encapsulation of multiple signals, or signal transformations? What might take this further? One small idea that comes to mind is that it looks like we really just want the
clientSecret
from this API call, so you could probably fold at least amap
operation into it.
@amy-at-kickstarter I added tests for I tried a bit of a different pattern than what we use elsewhere in the app. One callout I have is that we have tests behind the actual API request already that make sure the correct data is being passed in, so what I have might be a bit overkill. Lmk what you think.
I'm not sure about this. For one thing, we need the result of the Signal (the envelope with the client secret or the ErrorEnvelope), not just the client secret. I feel like keeping the |
…in our already large AppEnvironment
* This way it's a better representation of the actual service
…s and update tests
…ts and update tests
…s and update tests
@amy-at-kickstarter, I did the following from our Slack discussion:
|
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 how this shaped up.
@@ -13,7 +13,8 @@ protocol PaymentMethodSettingsViewControllerDelegate: AnyObject { | |||
internal final class PaymentMethodSettingsViewController: UIViewController, | |||
MessageBannerViewControllerPresenting { | |||
private let dataSource = PaymentMethodsDataSource() | |||
private let viewModel: PaymentMethodsViewModelType = PaymentMethodSettingsViewModel() | |||
private let viewModel: PaymentMethodsViewModelType = | |||
PaymentMethodSettingsViewModel(stripeIntentService: StripeIntentService()) |
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 this pattern for dependencies!
assert( | ||
AppEnvironment.current.apiService as? MockService != nil, | ||
"AppEnvironment.current.apiService should be a MockService when used in test." | ||
) |
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.
👍 nice assert
📲 What
Creates a new StripeIntentService to handle creating Payment and Setup Intents
🤔 Why
This logic is used in crowdfunding, late pledges, and account settings payment method flows.
Creating a shared/reusable service makes things cleaner and easier to maintain.
Hopefully, it makes consolidating late pledges and crowdfunding easier as well.
🛠 How
Creates a pretty simple StripeIntentService to handle both intent types.
👀 See
No visible changes
✅ Acceptance criteria