Fix Link default card change ui states #10108

toluo-stripe · 2025-02-07T21:17:25Z

Summary

Fix selected button flicker when default card is changed
Loading bar should be on payment method row, not primary button

Motivation

Payment Method Update Loading State

Default Card Radio Button Flicker

Testing

Added tests
Modified tests
Manually verified

Screenshots

Screen.Recording.2025-02-07.at.5.22.56.PM.mov

Changelog

github-actions · 2025-02-07T21:23:11Z

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 302.6 KiB │ 302.6 KiB │  0 B │ 456.7 KiB │ 456.7 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.8 KiB │  90.8 KiB │ -2 B │   171 KiB │   171 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ -2 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19981 │ 19981 │ 0 (+0 -0) 
   types │  6194 │  6194 │ 0 (+0 -0) 
 classes │  4985 │  4985 │ 0 (+0 -0) 
 methods │ 29823 │ 29823 │ 0 (+0 -0) 
  fields │ 17542 │ 17542 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3624 │ 3624 │  0

APK

   compressed    │   uncompressed   │                        
──────────┬──────┼───────────┬──────┤                        
 size     │ diff │ size      │ diff │ path                   
──────────┼──────┼───────────┼──────┼────────────────────────
 28.6 KiB │ -3 B │  63.2 KiB │  0 B │ ∆ META-INF/CERT.SF     
 25.4 KiB │ +1 B │  63.1 KiB │  0 B │ ∆ META-INF/MANIFEST.MF 
──────────┼──────┼───────────┼──────┼────────────────────────
   54 KiB │ -2 B │ 126.3 KiB │  0 B │ (total)

cttsai-stripe · 2025-02-08T00:43:41Z

paymentsheet/src/main/java/com/stripe/android/link/ui/wallet/WalletViewModel.kt

@@ -96,30 +96,34 @@ internal class WalletViewModel @Inject constructor(
        }
    }

-    private fun loadPaymentDetails(selectedItemId: String? = null) {
+    private fun loadPaymentDetails() {


If the updated function is only used in the init block, should we move

_uiState.update { it.setProcessing() }

out of the function instead?

loadPaymentDetails is only used in the init block, so this is fine. The other calls to load use loadPaymentDetailsHelper

+1 to James' question here. I think we could move the uiState into the init block.

Then we wouldn't need this function which is only intended to be called by the init block. We could rename loadPaymentDetailsHelper to loadPaymentDetails and call it from anywhere.

My concern with the current approach is that it's not easy to tell why you would want to call loadPaymentDetails vs. loadPaymentDetailsHelper, which could lead to confusion/bugs in the future

cttsai-stripe · 2025-02-08T00:56:55Z

paymentsheet/src/test/java/com/stripe/android/link/ui/wallet/WalletScreenTest.kt

+        dispatcher.scheduler.advanceTimeBy(1.1.seconds)
+
+        onWalletPaymentMethodRowLoadingIndicator().assertDoesNotExist()
+        onWalletPayButton().assertExists()


exist or enabled?

cttsai-stripe · 2025-02-08T00:59:06Z

paymentsheet/src/test/java/com/stripe/android/link/ui/wallet/WalletScreenTest.kt

+        composeTestRule.waitForIdle()
+
+        onWalletPaymentMethodRowLoadingIndicator().assertIsDisplayed()


Does this introduce flakiness? Should we use waitUntilExists instead?

I think if we have flakiness after waitForIdle, then the screen is not behaving as expected. The loader state is changed on the main thread before kicking of any coroutines to update the card

cttsai-stripe · 2025-02-08T01:09:12Z

paymentsheet/src/test/java/com/stripe/android/link/ui/wallet/WalletViewModelTest.kt

@@ -392,17 +401,21 @@ class WalletViewModelTest {

        viewModel.onSetDefaultClicked(card1)

+        assertThat(viewModel.uiState.value.cardBeingUpdated).isEqualTo(card1.id)
+


Is there a better way to simulate processing, maybe test dispatcher?
I don't know. Not a blocker for me.

+1 to this question.

If the tests are going to continue to rely on the delay, could we at least use a variable for the delay and then reference that variable in the advanceTimeBy call? Otherwise I was having a hard time figuring out why 1.1 seconds was being used here

I can't think of another way to simulate processing. I'll replace the literals with variable names

amk-stripe

I'm not sure this is the right fix -- on iOS, it looks like the payment methods are not re-ordered when the default payment method changes. Instead, just the default badge moves. I think that this is a better/smoother UI. Could we do the same thing on Android?

toluo-stripe · 2025-02-12T00:33:12Z

This what it looks like now cc @amk-stripe

Screen.Recording.2025-02-11.at.4.55.46.PM.mov

amk-stripe · 2025-02-12T17:38:45Z

paymentsheet/src/test/java/com/stripe/android/link/ui/wallet/WalletViewModelTest.kt

@@ -392,17 +401,21 @@ class WalletViewModelTest {

        viewModel.onSetDefaultClicked(card1)

+        assertThat(viewModel.uiState.value.cardBeingUpdated).isEqualTo(card1.id)
+


+1 to this question.

If the tests are going to continue to rely on the delay, could we at least use a variable for the delay and then reference that variable in the advanceTimeBy call? Otherwise I was having a hard time figuring out why 1.1 seconds was being used here

amk-stripe · 2025-02-12T17:42:12Z

paymentsheet/src/main/java/com/stripe/android/link/ui/wallet/WalletViewModel.kt

@@ -96,30 +96,34 @@ internal class WalletViewModel @Inject constructor(
        }
    }

-    private fun loadPaymentDetails(selectedItemId: String? = null) {
+    private fun loadPaymentDetails() {


+1 to James' question here. I think we could move the uiState into the init block.

Then we wouldn't need this function which is only intended to be called by the init block. We could rename loadPaymentDetailsHelper to loadPaymentDetails and call it from anywhere.

My concern with the current approach is that it's not easy to tell why you would want to call loadPaymentDetails vs. loadPaymentDetailsHelper, which could lead to confusion/bugs in the future

toluo-stripe changed the title ~~Wallet update states~~ Fix Link default card change ui states Feb 7, 2025

toluo-stripe marked this pull request as ready for review February 7, 2025 22:31

toluo-stripe requested review from a team as code owners February 7, 2025 22:31

toluo-stripe requested review from cttsai-stripe and amk-stripe February 7, 2025 22:31

cttsai-stripe reviewed Feb 8, 2025

View reviewed changes

amk-stripe reviewed Feb 8, 2025

View reviewed changes

toluo-stripe force-pushed the tolu/link/wallet_screen_states branch from 36040cb to a17477d Compare February 11, 2025 21:59

toluo-stripe requested review from amk-stripe and cttsai-stripe February 12, 2025 00:33

amk-stripe previously approved these changes Feb 12, 2025

View reviewed changes

toluo-stripe dismissed amk-stripe’s stale review via 13cf9d0 February 12, 2025 19:05

toluo-stripe force-pushed the tolu/link/wallet_screen_states branch from 1066399 to 13cf9d0 Compare February 12, 2025 19:05

toluo-stripe requested a review from amk-stripe February 12, 2025 20:25

amk-stripe previously approved these changes Feb 12, 2025

View reviewed changes

toluo-stripe added 7 commits February 13, 2025 09:52

Wallet default card fix

d6871b3

Remove list reorder

a784b7b

Update WalletScreen.kt

e5d7993

Update paymentsheet.api

1b2571a

Update WalletScreenTest.kt

788d76a

Clean up loader

e498268

Update WalletViewModelTest.kt

fa3db55

toluo-stripe dismissed amk-stripe’s stale review via fa3db55 February 13, 2025 14:55

toluo-stripe force-pushed the tolu/link/wallet_screen_states branch from e375c77 to fa3db55 Compare February 13, 2025 14:55

amk-stripe approved these changes Feb 13, 2025

View reviewed changes

toluo-stripe merged commit f5bec10 into master Feb 13, 2025
13 checks passed

toluo-stripe deleted the tolu/link/wallet_screen_states branch February 13, 2025 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Link default card change ui states #10108

Fix Link default card change ui states #10108

toluo-stripe commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading

cttsai-stripe Feb 8, 2025

toluo-stripe Feb 12, 2025

amk-stripe Feb 12, 2025

cttsai-stripe Feb 8, 2025

cttsai-stripe Feb 8, 2025

toluo-stripe Feb 12, 2025

cttsai-stripe Feb 8, 2025

amk-stripe Feb 12, 2025

toluo-stripe Feb 12, 2025

amk-stripe left a comment

toluo-stripe commented Feb 12, 2025

amk-stripe Feb 12, 2025

amk-stripe Feb 12, 2025

		composeTestRule.waitForIdle()

		onWalletPaymentMethodRowLoadingIndicator().assertIsDisplayed()

		@@ -392,17 +401,21 @@ class WalletViewModelTest {

		viewModel.onSetDefaultClicked(card1)

		assertThat(viewModel.uiState.value.cardBeingUpdated).isEqualTo(card1.id)

Fix Link default card change ui states #10108

Fix Link default card change ui states #10108

Conversation

toluo-stripe commented Feb 7, 2025 • edited Loading

Summary

Motivation

Testing

Screenshots

Changelog

github-actions bot commented Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amk-stripe left a comment

Choose a reason for hiding this comment

toluo-stripe commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toluo-stripe commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025 •

edited

Loading