Skip to content
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

Merged
merged 21 commits into from May 14, 2024

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Apr 29, 2024

πŸ“² 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

  • No changes to late pledge flow
  • No changes to crowdfunding flow
  • No changes to managing payment method in account settings

@scottkicks scottkicks self-assigned this Apr 29, 2024
@scottkicks scottkicks marked this pull request as ready for review April 29, 2024 23:00
Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a 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:

  1. StripeIntentService should have its own tests
  2. 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 a map operation into it.

@scottkicks
Copy link
Contributor Author

scottkicks commented May 7, 2024

@amy-at-kickstarter I added tests for StripeIntentService and also made it a class instead of a struct. My goal is to ensure that intent requests are made with the correct data and called only the expected number of times.

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.

  1. 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 a map operation into it.

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 .map in the calling method makes more sense so that it can handle the result appropriately.

@scottkicks
Copy link
Contributor Author

@amy-at-kickstarter, I did the following from our Slack discussion:

  • Removed the service from Environment and injected it only into the ViewModels needing it.
  • Updated MockStripeIntentService so that it uses apiService and keeps track of the number of requests made
  • Added test assertions in all relevant ViewModels that guarantee that we're only requesting the right intents and making those requests the correct number of times.

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter left a 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())
Copy link
Contributor

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."
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ‘ nice assert

@scottkicks scottkicks merged commit 52f6afe into main May 14, 2024
5 checks passed
@scottkicks scottkicks deleted the scott/post-campaign-vm-refactor branch May 14, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants