Restore Link Sign Up viewmodel and tests #9377

toluo-stripe · 2024-10-01T19:56:57Z

Summary

Link SignUp ViewModel
hasUserLoggedOut(email) for LinkAccountManager

Motivation

Testing

Added tests
Modified tests
Manually verified

Screenshots

Before	After
before screenshot	after screenshot

Changelog

github-actions · 2024-10-01T20:03:03Z

Diffuse output:

OLD: paymentsheet-example-release-master.apk (signature: V1, V2)
NEW: paymentsheet-example-release-pr.apk (signature: V1, V2)

          │          compressed           │         uncompressed         
          ├───────────┬───────────┬───────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff  │ old       │ new       │ diff 
──────────┼───────────┼───────────┼───────┼───────────┼───────────┼──────
      dex │   3.9 MiB │   3.9 MiB │ -41 B │   8.6 MiB │   8.6 MiB │ -8 B 
     arsc │   2.3 MiB │   2.3 MiB │   0 B │   2.3 MiB │   2.3 MiB │  0 B 
 manifest │   5.1 KiB │   5.1 KiB │   0 B │  25.6 KiB │  25.6 KiB │  0 B 
      res │ 933.6 KiB │ 933.6 KiB │   0 B │   1.5 MiB │   1.5 MiB │  0 B 
   native │   2.6 MiB │   2.6 MiB │   0 B │     6 MiB │     6 MiB │  0 B 
    asset │   2.9 MiB │   2.9 MiB │   0 B │   2.9 MiB │   2.9 MiB │  0 B 
    other │   196 KiB │   196 KiB │  +7 B │ 430.6 KiB │ 430.6 KiB │  0 B 
──────────┼───────────┼───────────┼───────┼───────────┼───────────┼──────
    total │  12.8 MiB │  12.8 MiB │ -34 B │  21.7 MiB │  21.7 MiB │ -8 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 42625 │ 42625 │ 0 (+1 -1) 
   types │ 14130 │ 14130 │ 0 (+0 -0) 
 classes │ 11764 │ 11764 │ 0 (+0 -0) 
 methods │ 60402 │ 60402 │ 0 (+0 -0) 
  fields │ 40064 │ 40064 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  242 │  242 │  0   
 entries │ 6253 │ 6253 │  0

APK

    compressed    │   uncompressed   │                        
──────────┬───────┼───────────┬──────┤                        
 size     │ diff  │ size      │ diff │ path                   
──────────┼───────┼───────────┼──────┼────────────────────────
  3.9 MiB │ -41 B │   8.6 MiB │ -8 B │ ∆ classes.dex          
 53.4 KiB │  +5 B │ 118.3 KiB │  0 B │ ∆ META-INF/CERT.SF     
 50.1 KiB │  +2 B │ 118.2 KiB │  0 B │ ∆ META-INF/MANIFEST.MF 
──────────┼───────┼───────────┼──────┼────────────────────────
    4 MiB │ -34 B │   8.8 MiB │ -8 B │ (total)

DEX

STRINGS:

   old   │ new   │ diff      
  ───────┼───────┼───────────
   42625 │ 42625 │ 0 (+1 -1) 
  
  + ~~R8{"backend":"dex","compilation-mode":"release","has-checksums":false,"min-api":21,"pg-map-id":"74c4ce8","r8-mode":"full","version":"8.5.35"}
  
  - ~~R8{"backend":"dex","compilation-mode":"release","has-checksums":false,"min-api":21,"pg-map-id":"410b8d6","r8-mode":"full","version":"8.5.35"}

toluo-stripe · 2024-10-02T19:53:51Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

+            }
+        }.collectLatest { email ->
+            delay(LOOKUP_DEBOUNCE)
+            if (email != null) {


Collection block will be cancelled and replaced when new email is emitted. Debounce is meant to delay lookup in case new email gets emitted quickly after a previous emit. Old version used a debouncer as well.

amk-stripe · 2024-10-02T22:03:48Z

link/src/test/java/com/stripe/android/link/ui/signup/SignUpViewModelTest.kt

+    private val navController: NavHostController = mock()
+    private val linkAccountManager: LinkAccountManager = mock()
+    private val linkEventsReporter: LinkEventsReporter = mock()
+    private val logger = mock<Logger>()


can you use fakes or real implementations for these instead of mocks, unless there is some reason that isn't possible?

LinkAccountManager and NavController aren't interfaces so we can't have fakes for this. I'm using fakes for logger and linkEventsReporter now.

I suspect it's a fairly simple refactor to make LinkAccountManager an interface and it's likely worth it, given how many tests are modifying its behavior and validating that it's called correctly.

You can refactor like this:

rename the existing LinkAccountManager to something like RealLinkAccountManager or DefaultLinkAccountManager

add an interface called LinkAccountManager, which RealLinkAccountManager implements

use a fake in the new tests you added

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

link/src/main/java/com/stripe/android/link/account/LinkAccountManager.kt

amk-stripe · 2024-10-02T22:19:09Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpScreenState.kt

+        val errorMessage: ResolvableString? = null
+    ) : SignUpScreenState
+
+    data object Loading : SignUpScreenState


Why is this needed? I would expect we'd have a loading state for Link as a whole, and then each individual screen shouldn't need to have a loading state.

If there's specific data we are waiting on, do we need the whole screen to be in a loading state? Or can we either:

wait to transition to the screen until we have the info we need or

keep the specific UI that's dependent on the async state in a loading state, rather than keeping the whole UI in a loading state

I took this out. The original link implementation checked for log in state on both the LinkViewModel and SignUpViewModel, but you're right, it shouldn't be duplicated.

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

amk-stripe · 2024-10-03T00:07:01Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpScreenState.kt

+        val emailController: TextFieldController,
+        val phoneNumberController: PhoneNumberController,
+        val nameController: TextFieldController,


is it possible to not use these controllers? I feel like it should be possible to use their validation logic without actually using the controllers + UI directly. Motivation here is just that these are overly complicated imo, when we know exactly the fields we need to use and how they should look

I tried to avoid the controllers, but the textfields associated with these are complicated, especially PhoneNumberTextField. Creating TextFields that match the existing would be increase the scope.

amk-stripe · 2024-10-07T18:27:39Z

link/src/test/java/com/stripe/android/link/analytics/LinkAnalyticsHelperTest.kt

Rather than increasing the visibility of the fake within this file, can you move it into its own file? I think it's a bit weird for one test to depend on another test

amk-stripe · 2024-10-07T18:29:18Z

link/src/test/java/com/stripe/android/link/analytics/LinkAnalyticsHelperTest.kt

-    override fun onInlineSignupCheckboxChecked() {
-        throw NotImplementedError()
-    }
+    override fun onInlineSignupCheckboxChecked() = Unit


The NotImplementedErrors here were serving a purpose, which is ensuring that those analytics functions are not called from tests where we don't expect them to be called. Can this change be reverted?

amk-stripe · 2024-10-07T19:14:21Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

+        linkAccountManager.lookupConsumer(email).fold(
+            onSuccess = {
+                if (it != null) {
+                    onAccountFetched(it)


should this be done before we even get to this screen? It seems like this would always navigate away from the sign up screen, so I wonder if we should do it while loading or something instead

This will be checked on the LinkVM. The user will not get to this screen if we find an account. The first lookup here will return null (most likely) since we already checked beforehand.

This VM also listens to email changes. If the lookup is successful after an email change, we would navigate to the next screen as well. This is not likely and I'm certain it won't be the case since we don't have persistent log in anymore. I decided to keep in case we change our mind to add persistence later on. The old link implementation had persistence and did a lookup anytime the email changed.

Gotcha, it sounds like this check is unnecessary right now then, is that right? If we add persistence back, we should add this code snippet back. But I think we should avoid adding this unless it's needed.

link/src/main/java/com/stripe/android/link/ui/signup/SignUpScreenState.kt

amk-stripe · 2024-10-07T19:24:37Z

link/src/test/java/com/stripe/android/link/ui/signup/SignUpViewModelTest.kt

+    private val navController: NavHostController = mock()
+    private val linkAccountManager: LinkAccountManager = mock()
+    private val linkEventsReporter: LinkEventsReporter = mock()
+    private val logger = mock<Logger>()


I suspect it's a fairly simple refactor to make LinkAccountManager an interface and it's likely worth it, given how many tests are modifying its behavior and validating that it's called correctly.

You can refactor like this:

rename the existing LinkAccountManager to something like RealLinkAccountManager or DefaultLinkAccountManager

add an interface called LinkAccountManager, which RealLinkAccountManager implements

use a fake in the new tests you added

amk-stripe

None of these comments are blocking, so feel free to merge this and follow up on the comments if that's easier!

amk-stripe · 2024-10-09T17:01:37Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

+            ).fold(
+                onSuccess = {
+                    onAccountFetched(it)
+                    linkEventsReporter.onSignupCompleted()


(for a follow up PR) should the analytics be handled as part of link account manager instead of here? I'd expect the analytics to be the same wherever linkAccountManager.signUp is called from

amk-stripe · 2024-10-09T17:03:45Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

+    }
+
+    private fun onError(error: Throwable) {
+        logger.error("Error: ", error)


should this logger indicate somehow that the error is coming from within the signup VM?

amk-stripe · 2024-10-09T17:07:09Z

link/src/test/java/com/stripe/android/link/ui/signup/SignUpViewModelTest.kt

+                override suspend fun lookupConsumer(email: String, startSession: Boolean): Result<LinkAccount?> {
+                    calledCount += 1
+                    emails.add(email)
+                    delay(100)


(just clarifying) why do we need the delay here? I thought that the delay was handled within the VM, so we wouldn't also need one here

amk-stripe · 2024-10-09T17:07:45Z

link/src/test/java/com/stripe/android/link/ui/signup/SignUpViewModelTest.kt

+            assertThat(linkAccountManager.emails.size).isEqualTo(1)
+            assertThat(linkAccountManager.emails.firstOrNull()).isEqualTo("[email protected]")


you might be able to use assertThat(linkAccountManager.emails).containsExactly("[email protected]") to simplify these asserts

amk-stripe · 2024-10-09T17:13:18Z

link/src/main/java/com/stripe/android/link/ui/signup/SignUpViewModel.kt

+        linkAccountManager.lookupConsumer(email).fold(
+            onSuccess = {
+                if (it != null) {
+                    onAccountFetched(it)


Gotcha, it sounds like this check is unnecessary right now then, is that right? If we add persistence back, we should add this code snippet back. But I think we should avoid adding this unless it's needed.

toluo-stripe commented Oct 2, 2024

View reviewed changes

toluo-stripe force-pushed the tolu/link_signup_vm branch from 748d028 to c5921fe Compare October 2, 2024 20:03

toluo-stripe changed the title ~~Tolu/link signup vm~~ Link Sign Up viewmodel and tests Oct 2, 2024

toluo-stripe marked this pull request as ready for review October 2, 2024 20:12

toluo-stripe requested review from a team as code owners October 2, 2024 20:12

toluo-stripe requested review from amk-stripe and carlosmuvi-stripe October 2, 2024 20:12

toluo-stripe changed the title ~~Link Sign Up viewmodel and tests~~ Restore Link Sign Up viewmodel and tests Oct 2, 2024

samer-stripe previously approved these changes Oct 2, 2024

View reviewed changes

amk-stripe reviewed Oct 2, 2024

View reviewed changes

amk-stripe reviewed Oct 3, 2024

View reviewed changes

toluo-stripe dismissed samer-stripe’s stale review via 02ca344 October 7, 2024 17:12

toluo-stripe force-pushed the tolu/link_signup_vm branch from c5921fe to 02ca344 Compare October 7, 2024 17:12

toluo-stripe requested a review from a team as a code owner October 7, 2024 17:12

toluo-stripe requested a review from jaynewstrom-stripe October 7, 2024 17:12

amk-stripe reviewed Oct 7, 2024

View reviewed changes

toluo-stripe added 11 commits October 8, 2024 16:29

LinkActivity, ViewModel and Navigator

fee10f5

Add Sign Up screen skeleton

9e3be14

rename VM

d5eb7e7

remove mvi-viewmodel

b6dca29

Delete StateAndEffectsVM.kt

628af3d

Delete unused code

f1eaa42

Fix potential memory leak

11d0d8a

link sign up viewmodel

88e4c02

clean up

c69c28b

Remove logged out check

efebf86

Update SignUpVM tests

16ee79b

toluo-stripe force-pushed the tolu/link_signup_vm branch from 02ca344 to 16ee79b Compare October 8, 2024 21:09

Fix event fakes

ceb4a54

toluo-stripe requested a review from amk-stripe October 9, 2024 16:45

amk-stripe approved these changes Oct 9, 2024

View reviewed changes

toluo-stripe merged commit f837bac into master Oct 9, 2024
16 checks passed

toluo-stripe deleted the tolu/link_signup_vm branch October 9, 2024 19:07

toluo-stripe added the Link label Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore Link Sign Up viewmodel and tests #9377

Restore Link Sign Up viewmodel and tests #9377

toluo-stripe commented Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading

toluo-stripe Oct 2, 2024 •

edited

Loading

amk-stripe Oct 2, 2024

toluo-stripe Oct 7, 2024

amk-stripe Oct 7, 2024

amk-stripe Oct 2, 2024

toluo-stripe Oct 7, 2024

amk-stripe Oct 3, 2024

toluo-stripe Oct 4, 2024

amk-stripe Oct 7, 2024

amk-stripe Oct 7, 2024

amk-stripe Oct 7, 2024

toluo-stripe Oct 9, 2024

amk-stripe Oct 9, 2024

amk-stripe Oct 7, 2024

amk-stripe left a comment

amk-stripe Oct 9, 2024

amk-stripe Oct 9, 2024

amk-stripe Oct 9, 2024

amk-stripe Oct 9, 2024

amk-stripe Oct 9, 2024

		assertThat(linkAccountManager.emails.size).isEqualTo(1)
		assertThat(linkAccountManager.emails.firstOrNull()).isEqualTo("[email protected]")

Restore Link Sign Up viewmodel and tests #9377

Restore Link Sign Up viewmodel and tests #9377

Conversation

toluo-stripe commented Oct 1, 2024 • edited Loading

Summary

Motivation

Testing

Screenshots

Changelog

github-actions bot commented Oct 1, 2024 • edited Loading

toluo-stripe Oct 2, 2024 • 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

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

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

toluo-stripe commented Oct 1, 2024 •

edited

Loading

github-actions bot commented Oct 1, 2024 •

edited

Loading

toluo-stripe Oct 2, 2024 •

edited

Loading