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

Migrate Custom Payment Sheet Example to PaymentLauncher #4175

Merged
merged 10 commits into from
Sep 9, 2021

Conversation

skyler-stripe
Copy link
Contributor

Summary

  • Change PaymentController to PaymentLauncher within DefaultFlowController
  • Update tests to reflect changes.

Motivation

PaymentLauncher is a more modern api that gives us a few advantages over PaymentController. PaymentLauncher is more simple to use as it does not require us to handle the interim flows with PaymentFlowProcessor. It also uses ActivityResultLauncher which gives us more reliable result callbacks over startActivityForResult/onActivityResult.

doc on this project

Testing

  • Added tests
  • Modified tests
  • Manually verified

Recording

output

Copy link
Contributor

@ccen-stripe ccen-stripe left a comment

Choose a reason for hiding this comment

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

Now that PaymentController and FlowResultProcessors are no longer needed in any of the PaymentSheet logic, we can delete them from dagger graph.

Line 74-137 here can be all deleted. @brnunes-stripe as discussed, I think this would make it easier to inject productUsage :)

Also we can make remove the @RestrictTo annotation of PaymentController, PaymentFlowResultProcessor and PaymentIntentFlowResultProcessor, then make them internal as they no longer needs to be visible to paymentsheet module. There might be more classes that could be made internal here, cc-ing @michelleb-stripe for more pointers

@skyler-stripe
Copy link
Contributor Author

Now that PaymentController and FlowResultProcessors are no longer needed in any of the PaymentSheet logic, we can delete them from dagger graph.

Line 74-137 here can be all deleted. @brnunes-stripe as discussed, I think this would make it easier to inject productUsage :)

Also we can make remove the @RestrictTo annotation of PaymentController, PaymentFlowResultProcessor and PaymentIntentFlowResultProcessor, then make them internal as they no longer needs to be visible to paymentsheet module. There might be more classes that could be made internal here, cc-ing @michelleb-stripe for more pointers

Does it make sense to do this in this PR? I was thinking I'd do a third PR to clean out the no longer needed stuff and change some of the internals based on @michelleb-stripe 's comment on my last PR. #4159 (comment)

@skyler-stripe
Copy link
Contributor Author

skyler-stripe commented Sep 8, 2021

Now that PaymentController and FlowResultProcessors are no longer needed in any of the PaymentSheet logic, we can delete them from dagger graph.

Line 74-137 here can be all deleted. @brnunes-stripe as discussed, I think this would make it easier to inject productUsage :)

Also we can make remove the @RestrictTo annotation of PaymentController, PaymentFlowResultProcessor and PaymentIntentFlowResultProcessor, then make them internal as they no longer needs to be visible to paymentsheet module. There might be more classes that could be made internal here, cc-ing @michelleb-stripe for more pointers

done in e328c29

will make a follow up PR to look more deeply into things we can restrict.

"Failed to confirm ${stripeIntentResult.intent.javaClass.simpleName}: $it"
)
} ?: RuntimeException("Failed to complete payment.")
RuntimeException("Failed to complete payment.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it ever fall here? Aren't all options covered above?

@skyler-stripe skyler-stripe merged commit 7c2a1ef into master Sep 9, 2021
@skyler-stripe skyler-stripe deleted the paymentLauncherForCustomExample branch September 9, 2021 17:42
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.

5 participants