Link Confirmation Handler #9865

toluo-stripe · 2025-01-07T16:55:49Z

Summary

Abstract confirmation logic used for link screens with LinkConfirmationHandler. It will be used in

WalletViewModel
PaymentMethodViewModel

The abstraction helps us generate the args needed to kick off confirmation and handle errors without duplicating logic across these screens.

Motivation

JIRA

Testing

Added tests
Modified tests
Manually verified

Changelog

graphite-app · 2025-01-07T16:56:47Z

...ntsheet/src/main/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandler.kt

+        paymentDetails: ConsumerPaymentDetails.PaymentDetails,
+        linkAccount: LinkAccount
+    ): ConfirmationHandler.Args {
+        configuration.shippingDetails


Line 59 appears to be a dangling statement that accesses configuration.shippingDetails without using the value. This line can be safely removed since it has no effect on the program execution.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-07T16:59:43Z

...eet/src/test/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandlerTest.kt

+    }
+
+    @Test
+    fun `null confirmation yields canceled result`() = runTest(dispatcher) {


The test name doesn't match the assertion being verified - the test confirms that a null confirmation returns Result.Failed, not Result.Canceled. Consider renaming to null confirmation yields failed result to accurately reflect the test behavior.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

github-actions · 2025-01-07T17:02:21Z

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 │ 301.8 KiB │ 301.8 KiB │  0 B │ 455.5 KiB │ 455.5 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.2 KiB │   7.2 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.2 KiB │  90.3 KiB │ +7 B │ 170.3 KiB │ 170.3 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ +7 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19971 │ 19971 │ 0 (+0 -0) 
   types │  6191 │  6191 │ 0 (+0 -0) 
 classes │  4982 │  4982 │ 0 (+0 -0) 
 methods │ 29771 │ 29771 │ 0 (+0 -0) 
  fields │ 17541 │ 17541 │ 0 (+0 -0) 

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

APK

   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 28.5 KiB │ +4 B │  62.9 KiB │  0 B │ ∆ META-INF/CERT.SF                        
 25.3 KiB │ +3 B │  62.8 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
    272 B │ +1 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
  1.2 KiB │ -1 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA                       
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 55.2 KiB │ +7 B │ 127.1 KiB │  0 B │ (total)

paymentsheet/src/main/java/com/stripe/android/link/LinkConfiguration.kt

graphite-app · 2025-01-07T22:09:06Z

...ntsheet/src/main/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandler.kt

+            val args = confirmationArgs(paymentDetails, linkAccount)
+            confirmationHandler.start(args)
+            val result = confirmationHandler.awaitResult()
+            return transformResult(result)


The return keyword here will cause the runCatching block to exit early, bypassing the error handling logic. Consider removing it to ensure errors are properly caught and processed through the getOrElse block.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

amk-stripe · 2025-01-07T22:39:44Z

...ntsheet/src/main/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandler.kt

+    }
+
+    companion object {
+        val NO_CLIENT_SECRET_FOUND = IllegalStateException("no client secret found")


Suggested change

val NO_CLIENT_SECRET_FOUND = IllegalStateException("no client secret found")

val NO_CLIENT_SECRET_FOUND = IllegalStateException("No client secret found.")

amk-stripe · 2025-01-07T22:40:28Z

...ntsheet/src/main/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandler.kt

+        paymentDetails: ConsumerPaymentDetails.PaymentDetails,
+        linkAccount: LinkAccount
+    ): Result {
+        return kotlin.runCatching {


Suggested change

return kotlin.runCatching {

return runCatching {

Is it possible to write this this way? Or why do we need to specify kotlin here?

amk-stripe · 2025-01-07T22:42:20Z

...eet/src/test/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandlerTest.kt

+            linkAccount = TestFactory.LINK_ACCOUNT
+        )
+
+        Truth.assertThat(result).isEqualTo(Result.Succeeded)


can you import assertThat so that you don't need to specify Truth with every assertThat statement?

amk-stripe · 2025-01-07T22:44:08Z

...eet/src/test/java/com/stripe/android/link/confirmation/DefaultLinkConfirmationHandlerTest.kt

+        Truth.assertThat(result).isEqualTo(Result.Succeeded)
+        Truth.assertThat(confirmationHandler.startTurbine.awaitItem())
+            .isEqualTo(
+                ConfirmationHandler.Args(


do we need to assert equality on this entire object? Are there particular fields we care more about actually testing?

I think we have lots of tests that do assert equality on the whole object, but they are harder to maintain. If we update the type of ConfirmationHandler.Args we would need to update all the tests that assert on equality of a ConfirmationHandler.Args object, even if the type update is irrelevant to the impacted test

I trimmed it a little bit, but almost all the fields are important here

graphite-app bot reviewed Jan 7, 2025

View reviewed changes

toluo-stripe force-pushed the tolu/link/confirmation_manager branch from 7a31caf to 60d6884 Compare January 7, 2025 16:58

graphite-app bot reviewed Jan 7, 2025

View reviewed changes

toluo-stripe force-pushed the tolu/link/confirmation_manager branch from 60d6884 to 092a362 Compare January 7, 2025 17:03

toluo-stripe marked this pull request as ready for review January 7, 2025 17:10

toluo-stripe requested review from a team as code owners January 7, 2025 17:10

toluo-stripe requested review from jaynewstrom-stripe, amk-stripe and samer-stripe January 7, 2025 17:10

jaynewstrom-stripe reviewed Jan 7, 2025

View reviewed changes

paymentsheet/src/main/java/com/stripe/android/link/LinkConfiguration.kt Outdated Show resolved Hide resolved

toluo-stripe mentioned this pull request Jan 7, 2025

Refactor Link shippingValues to use AddressDetails #9869

Merged

3 tasks

toluo-stripe added 2 commits January 7, 2025 17:07

Link Confirmation Handler

2ff01f2

Fix savedStateHandle test issue

20e039d

toluo-stripe force-pushed the tolu/link/confirmation_manager branch from 049f1ca to 20e039d Compare January 7, 2025 22:08

graphite-app bot reviewed Jan 7, 2025

View reviewed changes

Remove unneeded return statement

a71adc6

amk-stripe reviewed Jan 7, 2025

View reviewed changes

toluo-stripe added 2 commits January 7, 2025 19:00

Add setup intent case and update tests

27d901e

Remove function parameter

bf6452c

toluo-stripe requested review from amk-stripe and jaynewstrom-stripe January 8, 2025 00:05

amk-stripe approved these changes Jan 8, 2025

View reviewed changes

toluo-stripe merged commit 1b50c6d into master Jan 9, 2025
13 checks passed

toluo-stripe deleted the tolu/link/confirmation_manager branch January 9, 2025 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link Confirmation Handler #9865

Link Confirmation Handler #9865

toluo-stripe commented Jan 7, 2025 •

edited

Loading

graphite-app bot Jan 7, 2025

graphite-app bot Jan 7, 2025

github-actions bot commented Jan 7, 2025 •

edited

Loading

graphite-app bot Jan 7, 2025

amk-stripe Jan 7, 2025

amk-stripe Jan 7, 2025

amk-stripe Jan 7, 2025

amk-stripe Jan 7, 2025

toluo-stripe Jan 8, 2025

	val NO_CLIENT_SECRET_FOUND = IllegalStateException("no client secret found")
	val NO_CLIENT_SECRET_FOUND = IllegalStateException("No client secret found.")

Link Confirmation Handler #9865

Link Confirmation Handler #9865

Conversation

toluo-stripe commented Jan 7, 2025 • edited Loading

Summary

Motivation

Testing

Changelog

graphite-app bot Jan 7, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 7, 2025

Choose a reason for hiding this comment

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

graphite-app bot Jan 7, 2025

Choose a reason for hiding this comment

amk-stripe Jan 7, 2025

Choose a reason for hiding this comment

amk-stripe Jan 7, 2025

Choose a reason for hiding this comment

amk-stripe Jan 7, 2025

Choose a reason for hiding this comment

amk-stripe Jan 7, 2025

Choose a reason for hiding this comment

toluo-stripe Jan 8, 2025

Choose a reason for hiding this comment

toluo-stripe commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading