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.

Sign up for GitHub

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

Add set as default feature enabled and disabled state to CustomerState #10046

Merged
merged 7 commits into from
Feb 3, 2025
+107 −124
Merged

Add set as default feature enabled and disabled state to CustomerState #10046

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
de56fbd
Simplify CustomerStateTest
amk-stripe Jan 30, 2025
46a4866
Simplify DefaultPaymentElementLoaderTest
amk-stripe Jan 30, 2025
adbb7c2
Pure refactor -- replace CustomerState.defaultPaymentMethodId field w…
amk-stripe Jan 30, 2025
1555850
Pure refactor -- add enabled/disabled state to DefaultPaymentMethodState
amk-stripe Jan 31, 2025
9694251
Use enabled/disabled state from DefaultPaymentMethodState
amk-stripe Jan 31, 2025
c2a7c1f
Lint fixes
amk-stripe Jan 31, 2025
80ca7e4
API dump
amk-stripe Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions paymentsheet/api/paymentsheet.api
View file
Original file line number Diff line number Diff line change
@@ -2494,6 +2494,22 @@ public final class com/stripe/android/paymentsheet/state/CustomerState$Creator :
public synthetic fun newArray (I)[Ljava/lang/Object;
}

public final class com/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Disabled$Creator : android/os/Parcelable$Creator {
public fun <init> ()V
public final fun createFromParcel (Landroid/os/Parcel;)Lcom/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Disabled;
public synthetic fun createFromParcel (Landroid/os/Parcel;)Ljava/lang/Object;
public final fun newArray (I)[Lcom/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Disabled;
public synthetic fun newArray (I)[Ljava/lang/Object;
}

public final class com/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Enabled$Creator : android/os/Parcelable$Creator {
public fun <init> ()V
public final fun createFromParcel (Landroid/os/Parcel;)Lcom/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Enabled;
public synthetic fun createFromParcel (Landroid/os/Parcel;)Ljava/lang/Object;
public final fun newArray (I)[Lcom/stripe/android/paymentsheet/state/CustomerState$DefaultPaymentMethodState$Enabled;
public synthetic fun newArray (I)[Ljava/lang/Object;
}

public final class com/stripe/android/paymentsheet/state/CustomerState$Permissions$Creator : android/os/Parcelable$Creator {
public fun <init> ()V
public final fun createFromParcel (Landroid/os/Parcel;)Lcom/stripe/android/paymentsheet/state/CustomerState$Permissions;
6 changes: 5 additions & 1 deletion paymentsheet/src/main/java/com/stripe/android/paymentsheet/SavedPaymentMethodMutator.kt
View file
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ import com.stripe.android.paymentsheet.analytics.EventReporter
import com.stripe.android.paymentsheet.model.PaymentSelection
import com.stripe.android.paymentsheet.navigation.PaymentSheetScreen
import com.stripe.android.paymentsheet.repositories.CustomerRepository
import com.stripe.android.paymentsheet.state.CustomerState
import com.stripe.android.paymentsheet.ui.DefaultAddPaymentMethodInteractor
import com.stripe.android.paymentsheet.ui.DefaultUpdatePaymentMethodInteractor
import com.stripe.android.paymentsheet.ui.PaymentMethodRemovalDelayMillis
@@ -50,7 +51,10 @@ internal class SavedPaymentMethodMutator(
private val isEmbedded: Boolean,
) {
val defaultPaymentMethodId: StateFlow<String?> = customerStateHolder.customer.mapAsStateFlow { customerState ->
customerState?.defaultPaymentMethodId
when (val defaultPaymentMethodState = customerState?.defaultPaymentMethodState) {
is CustomerState.DefaultPaymentMethodState.Enabled -> defaultPaymentMethodState.defaultPaymentMethodId
is CustomerState.DefaultPaymentMethodState.Disabled, null -> null
}
}

val providePaymentMethodName: (code: String?) -> ResolvableString = { code ->
26 changes: 19 additions & 7 deletions paymentsheet/src/main/java/com/stripe/android/paymentsheet/state/CustomerState.kt
View file
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ internal data class CustomerState(
val customerSessionClientSecret: String?,
val paymentMethods: List<PaymentMethod>,
val permissions: Permissions,
val defaultPaymentMethodId: String?
val defaultPaymentMethodState: DefaultPaymentMethodState,
) : Parcelable {
@Parcelize
data class Permissions(
@@ -24,6 +24,15 @@ internal data class CustomerState(
val canRemoveDuplicates: Boolean,
) : Parcelable

@Parcelize
sealed class DefaultPaymentMethodState : Parcelable {
@Parcelize
data class Enabled(val defaultPaymentMethodId: String?) : DefaultPaymentMethodState()

@Parcelize
data object Disabled : DefaultPaymentMethodState()
}

internal companion object {
/**
* Creates a [CustomerState] instance using an [ElementsSession.Customer] response.
@@ -54,6 +63,12 @@ internal data class CustomerState(
else -> false
}

val defaultPaymentMethodState = if (FeatureFlags.enableDefaultPaymentMethods.isEnabled) {
DefaultPaymentMethodState.Enabled(customer.defaultPaymentMethod)
} else {
DefaultPaymentMethodState.Disabled
}

return CustomerState(
id = customer.session.customerId,
ephemeralKeySecret = customer.session.apiKey,
@@ -67,11 +82,7 @@ internal data class CustomerState(
// Should always remove duplicates when using `customer_session`
canRemoveDuplicates = true,
),
defaultPaymentMethodId = if (FeatureFlags.enableDefaultPaymentMethods.isEnabled) {
customer.defaultPaymentMethod
} else {
null
}
defaultPaymentMethodState = defaultPaymentMethodState
)
}

@@ -114,7 +125,8 @@ internal data class CustomerState(
*/
canRemoveDuplicates = false,
),
defaultPaymentMethodId = null
// This is a customer sessions only feature, so it's always disabled when using a legacy ephemeral key.
defaultPaymentMethodState = DefaultPaymentMethodState.Disabled
)
}
}
54 changes: 25 additions & 29 deletions paymentsheet/src/main/java/com/stripe/android/paymentsheet/state/PaymentElementLoader.kt
View file
Original file line number Diff line number Diff line change
@@ -9,7 +9,6 @@ import com.stripe.android.common.model.CommonConfiguration
import com.stripe.android.core.Logger
import com.stripe.android.core.exception.StripeException
import com.stripe.android.core.injection.IOContext
import com.stripe.android.core.utils.FeatureFlags
import com.stripe.android.core.utils.UserFacingLogger
import com.stripe.android.googlepaylauncher.GooglePayEnvironment
import com.stripe.android.googlepaylauncher.GooglePayRepository
@@ -200,7 +199,7 @@ internal class DefaultPaymentElementLoader @Inject constructor(
val initialPaymentSelection = async {
retrieveInitialPaymentSelection(
savedSelection = savedSelection,
customer = customer,
customerDeferred = customer,
isGooglePayReady = isGooglePayReady,
)
}
@@ -355,7 +354,7 @@ internal class DefaultPaymentElementLoader @Inject constructor(
paymentMethods = state.paymentMethods
.withDefaultPaymentMethodOrLastUsedPaymentMethodFirst(
savedSelection = savedSelection,
defaultPaymentMethodId = state.defaultPaymentMethodId
defaultPaymentMethodState = state.defaultPaymentMethodState,
).filter { cardBrandFilter.isAccepted(it) }
)
}
@@ -558,29 +557,29 @@ internal class DefaultPaymentElementLoader @Inject constructor(

private suspend fun retrieveInitialPaymentSelection(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I re-worked this function and also withDefaultPaymentMethodOrLastUsedPaymentMethodFirst.

Accessing the defaultPaymentMethodId is a bit more complicated now and the code became hard to read unless I adjusted a bit. The behavior is the same though

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth breaking this code out into another object, testing the defaulting logic in isolation, and injecting that object into the loader for use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Testing the defaulting logic in isolation sounds like a good idea to me. I think we could also achieve that by just making this function @VisibleForTesting. What's the advantage of breaking it out into another object? Want to make sure I'm fully understanding your suggestion here.

I can follow up with this change!

savedSelection: Deferred<SavedSelection>,
customer: Deferred<CustomerState?>,
customerDeferred: Deferred<CustomerState?>,
isGooglePayReady: Boolean,
): PaymentSelection? {
// get default payment method if dpm enabled, otherwise try to get savedSelection
val primaryPaymentSelection = if (FeatureFlags.enableDefaultPaymentMethods.isEnabled) {
customer.await()?.defaultPaymentMethodId?.let {
customer.await()?.paymentMethods?.firstOrNull {
it.id == customer.await()?.defaultPaymentMethodId
val customer = customerDeferred.await()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference between customerDeferred.await() inside retrieveInitialPaymentSelection

and

retrieveInitialPaymentSelection(
                savedSelection = savedSelection,
                customer = customer.await(),
                isGooglePayReady = isGooglePayReady,
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh good point! I don't think so. I can follow up with this change (I have another PR that will build on top of this one and I don't want that PR to be blocked on this)

val primaryPaymentSelection = when (val defaultPaymentMethodState = customer?.defaultPaymentMethodState) {
is CustomerState.DefaultPaymentMethodState.Enabled ->
customer.paymentMethods.firstOrNull {
it.id == defaultPaymentMethodState.defaultPaymentMethodId
}?.toPaymentSelection()
}
} else {
when (val selection = savedSelection.await()) {
is SavedSelection.GooglePay -> PaymentSelection.GooglePay
is SavedSelection.Link -> PaymentSelection.Link
is SavedSelection.PaymentMethod -> {
customer.await()?.paymentMethods?.find { it.id == selection.id }?.toPaymentSelection()
CustomerState.DefaultPaymentMethodState.Disabled, null -> {
when (val selection = savedSelection.await()) {
is SavedSelection.GooglePay -> PaymentSelection.GooglePay
is SavedSelection.Link -> PaymentSelection.Link
is SavedSelection.PaymentMethod -> {
customer?.paymentMethods?.find { it.id == selection.id }?.toPaymentSelection()
}
is SavedSelection.None -> null
}
is SavedSelection.None -> null
}
}

return primaryPaymentSelection
?: customer.await()?.paymentMethods?.firstOrNull()?.toPaymentSelection()
?: customer?.paymentMethods?.firstOrNull()?.toPaymentSelection()
?: PaymentSelection.GooglePay.takeIf { isGooglePayReady }
}

@@ -750,21 +749,18 @@ internal class DefaultPaymentElementLoader @Inject constructor(

private suspend fun List<PaymentMethod>.withDefaultPaymentMethodOrLastUsedPaymentMethodFirst(
savedSelection: Deferred<SavedSelection>,
defaultPaymentMethodId: String?
defaultPaymentMethodState: CustomerState.DefaultPaymentMethodState,
): List<PaymentMethod> {
val primaryPaymentMethodIndex = if (FeatureFlags.enableDefaultPaymentMethods.isEnabled) {
defaultPaymentMethodId?.let {
indexOfFirst { it.id == defaultPaymentMethodId }
}
} else {
val selection = savedSelection.await()
(selection as? SavedSelection.PaymentMethod)?.let {
indexOfFirst { it.id == selection.id }.takeIf { it != -1 }
val primaryPaymentMethodId = when (defaultPaymentMethodState) {
is CustomerState.DefaultPaymentMethodState.Enabled -> defaultPaymentMethodState.defaultPaymentMethodId
CustomerState.DefaultPaymentMethodState.Disabled -> {
(savedSelection.await() as? SavedSelection.PaymentMethod)?.id
}
}

return primaryPaymentMethodIndex?.let {
val primaryPaymentMethod = get(primaryPaymentMethodIndex)
val primaryPaymentMethod = this.firstOrNull { it.id == primaryPaymentMethodId }
Copy link
Contributor

Choose a reason for hiding this comment

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

This is better than what I wrote

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, firstOrNull is really helpful here, I just didn't think of it until I was trying to make this work with this new type!


return primaryPaymentMethod?.let {
listOf(primaryPaymentMethod) + (this - primaryPaymentMethod)
} ?: this
}
4 changes: 3 additions & 1 deletion ...eet/src/main/java/com/stripe/android/paymentsheet/viewmodels/PaymentOptionsItemsMapper.kt
View file
Original file line number Diff line number Diff line change
@@ -28,7 +28,9 @@ internal class PaymentOptionsItemsMapper(
paymentMethods = customerState?.paymentMethods ?: listOf(),
isLinkEnabled = isLinkEnabled,
isGooglePayReady = isGooglePayReady,
defaultPaymentMethodId = customerState?.defaultPaymentMethodId
defaultPaymentMethodId = (
customerState?.defaultPaymentMethodState as? CustomerState.DefaultPaymentMethodState.Enabled
)?.defaultPaymentMethodId
) ?: emptyList()
}
}
2 changes: 1 addition & 1 deletion paymentsheet/src/test/java/com/stripe/android/paymentsheet/CustomerState.kt
View file
Original file line number Diff line number Diff line change
@@ -18,6 +18,6 @@ internal fun createCustomerState(
canRemoveDuplicates = true,
canRemoveLastPaymentMethod = canRemoveLastPaymentMethod,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled,
)
}
4 changes: 2 additions & 2 deletions paymentsheet/src/test/java/com/stripe/android/paymentsheet/PaymentSheetFixtures.kt
View file
Original file line number Diff line number Diff line change
@@ -105,7 +105,7 @@ internal object PaymentSheetFixtures {
canRemoveLastPaymentMethod = true,
canRemoveDuplicates = false,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled,
)

internal val CONFIG_GOOGLEPAY
@@ -169,7 +169,7 @@ internal object PaymentSheetFixtures {
canRemoveLastPaymentMethod = true,
canRemoveDuplicates = false,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled
),
config = config.asCommonConfiguration(),
paymentSelection = paymentSelection,
2 changes: 1 addition & 1 deletion paymentsheet/src/test/java/com/stripe/android/paymentsheet/PaymentSheetViewModelTest.kt
View file
Original file line number Diff line number Diff line change
@@ -353,7 +353,7 @@ internal class PaymentSheetViewModelTest {
canRemoveLastPaymentMethod = true,
canRemoveDuplicates = false,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled,
),
customerRepository = customerRepository
)
2 changes: 1 addition & 1 deletion paymentsheet/src/test/java/com/stripe/android/paymentsheet/SavedPaymentMethodMutatorTest.kt
View file
Original file line number Diff line number Diff line change
@@ -575,7 +575,7 @@ class SavedPaymentMethodMutatorTest {
canRemoveLastPaymentMethod = true,
canRemoveDuplicates = shouldRemoveDuplicates,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled,
)
)

79 changes: 29 additions & 50 deletions paymentsheet/src/test/java/com/stripe/android/paymentsheet/state/CustomerStateTest.kt
View file
Original file line number Diff line number Diff line change
@@ -40,21 +40,10 @@ class CustomerStateTest {
customerSessionClientSecret = "cuss_123",
)

assertThat(customerState).isEqualTo(
CustomerState(
id = "cus_1",
ephemeralKeySecret = "ek_1",
customerSessionClientSecret = "cuss_123",
paymentMethods = paymentMethods,
permissions = CustomerState.Permissions(
canRemovePaymentMethods = false,
canRemoveLastPaymentMethod = false,
// Always true for `customer_session`
canRemoveDuplicates = true,
),
defaultPaymentMethodId = null
)
)
assertThat(customerState.permissions.canRemovePaymentMethods).isFalse()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated a few of these tests to only assert on the fields relevant to the test, so that I wouldn't need to update the defaultPaymentMethodId field in as many places

Copy link
Contributor

@tianzhao-stripe tianzhao-stripe Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I realized that we can remove

            customerId = "cus_1",
            ephemeralKeySecret = "ek_1",

from the calls of createElementsSessionCustomer b/c createElementsSessionCustomer has

        customerId: String = "cus_1",
        ephemeralKeySecret: String = "ek_1",

As the default values already

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh nice! I'm probably not going to make this update, because I don't think including them is adding much maintenance overhead, but it's good to know we don't need it for new tests or if we wanted to update these in the future

assertThat(customerState.permissions.canRemoveLastPaymentMethod).isFalse()
// Always true for `customer_session`
assertThat(customerState.permissions.canRemoveDuplicates).isTrue()
}

@Test
@@ -79,21 +68,10 @@ class CustomerStateTest {
customerSessionClientSecret = "cuss_123",
)

assertThat(customerState).isEqualTo(
CustomerState(
id = "cus_1",
ephemeralKeySecret = "ek_1",
customerSessionClientSecret = "cuss_123",
paymentMethods = paymentMethods,
permissions = CustomerState.Permissions(
canRemovePaymentMethods = true,
canRemoveLastPaymentMethod = true,
// Always true for `customer_session`
canRemoveDuplicates = true,
),
defaultPaymentMethodId = null
)
)
assertThat(customerState.permissions.canRemovePaymentMethods).isTrue()
assertThat(customerState.permissions.canRemoveLastPaymentMethod).isTrue()
// Always true for `customer_session`
assertThat(customerState.permissions.canRemoveDuplicates).isTrue()
}

@Test
@@ -118,21 +96,10 @@ class CustomerStateTest {
customerSessionClientSecret = "cuss_123",
)

assertThat(customerState).isEqualTo(
CustomerState(
id = "cus_3",
ephemeralKeySecret = "ek_3",
customerSessionClientSecret = "cuss_123",
paymentMethods = paymentMethods,
permissions = CustomerState.Permissions(
canRemovePaymentMethods = false,
canRemoveLastPaymentMethod = false,
// Always true for `customer_session`
canRemoveDuplicates = true,
),
defaultPaymentMethodId = null
)
)
assertThat(customerState.permissions.canRemovePaymentMethods).isFalse()
assertThat(customerState.permissions.canRemoveLastPaymentMethod).isFalse()
// Always true for `customer_session`
assertThat(customerState.permissions.canRemoveDuplicates).isTrue()
}

private fun createCustomerSessionForTestingDefaultPaymentMethod(defaultPaymentMethodId: String?): CustomerState {
@@ -173,7 +140,13 @@ class CustomerStateTest {
defaultPaymentMethodId = defaultPaymentMethodId
)

assertThat(customerState.defaultPaymentMethodId).isEqualTo(defaultPaymentMethodId)
assertThat(customerState.defaultPaymentMethodState).isInstanceOf(
CustomerState.DefaultPaymentMethodState.Enabled::class.java
)
val actualDefaultPaymentMethodId = (
customerState.defaultPaymentMethodState as CustomerState.DefaultPaymentMethodState.Enabled
).defaultPaymentMethodId
assertThat(actualDefaultPaymentMethodId).isEqualTo(defaultPaymentMethodId)
}

@Test
@@ -185,7 +158,7 @@ class CustomerStateTest {
defaultPaymentMethodId = defaultPaymentMethodId
)

assertThat(customerState.defaultPaymentMethodId).isNull()
assertThat(customerState.defaultPaymentMethodState).isEqualTo(CustomerState.DefaultPaymentMethodState.Disabled)
}

@Test
@@ -197,7 +170,13 @@ class CustomerStateTest {
defaultPaymentMethodId = defaultPaymentMethodId
)

assertThat(customerState.defaultPaymentMethodId).isNull()
assertThat(customerState.defaultPaymentMethodState).isInstanceOf(
CustomerState.DefaultPaymentMethodState.Enabled::class.java
)
val actualDefaultPaymentMethodId = (
customerState.defaultPaymentMethodState as CustomerState.DefaultPaymentMethodState.Enabled
).defaultPaymentMethodId
assertThat(actualDefaultPaymentMethodId).isEqualTo(defaultPaymentMethodId)
}

@Test
@@ -209,7 +188,7 @@ class CustomerStateTest {
defaultPaymentMethodId = defaultPaymentMethodId
)

assertThat(customerState.defaultPaymentMethodId).isNull()
assertThat(customerState.defaultPaymentMethodState).isEqualTo(CustomerState.DefaultPaymentMethodState.Disabled)
}

@Test
@@ -238,7 +217,7 @@ class CustomerStateTest {
// Always 'false' for legacy ephemeral keys
canRemoveDuplicates = false,
),
defaultPaymentMethodId = null
defaultPaymentMethodState = CustomerState.DefaultPaymentMethodState.Disabled
)
)
}
Loading