-
Notifications
You must be signed in to change notification settings - Fork 659
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
Don't allow PaymentSheet.Configuration to be null. #7625
Don't allow PaymentSheet.Configuration to be null. #7625
Conversation
Diffuse output:
APK
DEX
|
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.
Love this change! Should we do something similar with CustomerSheet
so that we don’t have to resolve the merchant name in the ViewModel?
config = viewModel.state?.config?.copy( | ||
shippingDetails = value | ||
val state = viewModel.state | ||
if (state != null) { |
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.
Why is this different?
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.
We're updating the config only, and the config looks nullable in the new version (because state is nullable). But this didn't change any behavior.
@@ -177,7 +180,7 @@ internal class DefaultFlowController @Inject internal constructor( | |||
) { | |||
configure( | |||
mode = PaymentSheet.InitializationMode.PaymentIntent(paymentIntentClientSecret), | |||
configuration = configuration, | |||
configuration = configuration ?: PaymentSheet.Configuration.default(viewModel.getApplication()), |
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 will make it harder for us to convert this to a non-Android ViewModel. Can we solve this differently? Maybe the default()
method just takes a label
argument?
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.
Yep, it'll make it harder. But flow controller already has access to context. I just pulled it from here (because it was easy) instead of adding a new property to the flow controller that's explicitly a context. But can do that if you prefer.
Yes |
Summary
When I was looking at prototyping an LPM refactor, I realized we had a lot of one off defaults for each of the properties of the payment sheet configuration. This change makes it all happen in the configuration class, and not leak to other parts of the codebase.
Motivation
Simplifying logic in PaymentSheet.