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.

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

Move LinkInlineSignup to confirmation definitions #9870

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

samer-stripe
Copy link
Collaborator

@samer-stripe samer-stripe commented Jan 7, 2025

Summary

This PR moves Link Inline Signup into confirmation definitions.

Motivation

This PR is the final element that was duplicated across FlowController and PaymentSheet that is now shared completely through ConfirmationHandler.

Testing

  • Added tests
  • Modified tests
  • Manually verified

private val _processingState =
MutableSharedFlow<ProcessingState>(replay = 1, extraBufferCapacity = 5)
val processingState: Flow<ProcessingState> = _processingState

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🥳

Copy link
Contributor

github-actions bot commented Jan 7, 2025

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.4 KiB │  90.4 KiB │ -9 B │ 170.7 KiB │ 170.7 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ -9 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19975 │ 19975 │ 0 (+0 -0) 
   types │  6193 │  6193 │ 0 (+0 -0) 
 classes │  4985 │  4985 │ 0 (+0 -0) 
 methods │ 29820 │ 29820 │ 0 (+0 -0) 
  fields │ 17538 │ 17538 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3624 │ 3624 │  0
APK
   compressed    │   uncompressed   │                                           
──────────┬──────┼───────────┬──────┤                                           
 size     │ diff │ size      │ diff │ path                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 25.4 KiB │ -6 B │    63 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
    269 B │ -3 B │     120 B │  0 B │ ∆ META-INF/version-control-info.textproto 
 28.5 KiB │ -1 B │  63.1 KiB │  0 B │ ∆ META-INF/CERT.SF                        
  1.2 KiB │ +1 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA                       
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 55.3 KiB │ -9 B │ 127.4 KiB │  0 B │ (total)

@samer-stripe samer-stripe marked this pull request as ready for review January 8, 2025 16:01
@samer-stripe samer-stripe requested review from a team as code owners January 8, 2025 16:01
@samer-stripe
Copy link
Collaborator Author

I'm hoping this can be split into two PRs: one for the selection changes and other for just the definition but I don't think it'll be trivial. We would need some connecting code in between both PRs. I fear this PR though would be become too big with all the changes together.

@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from 6aed416 to 7e237c5 Compare January 21, 2025 17:57
@samer-stripe samer-stripe changed the title [Prototype] Move Link Inline Signup to definition Move Link Inline Signup to definition Jan 21, 2025
@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from 7e237c5 to d89cdb5 Compare January 23, 2025 05:55
@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from d89cdb5 to be28107 Compare January 23, 2025 05:57
@@ -372,16 +349,7 @@ internal class PaymentSheetViewModel @Inject internal constructor(
) {
this.checkoutIdentifier = identifier
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we can remove checkoutIdentifier entirely now, and have the UI know when to start it's animation because of when the button is clicked, and know when to stop it based on when processing is false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

checkoutIdentifier actually isn't used to show and hide processing. That right now is actually entirely managed in the collect function in the init block already. This is used to identify if we are processing from a button click of a wallet PM or the primary button click and switching between animations there.

Copy link
Collaborator Author

@samer-stripe samer-stripe Jan 23, 2025

Choose a reason for hiding this comment

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

Optionally, we could switch between the animations if we maintain the original confirmation option in the loading state. Then we can always decide in the collect block what animation to use.

@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from be28107 to 5729503 Compare January 23, 2025 17:16
@samer-stripe samer-stripe changed the title Move Link Inline Signup to definition Move Link Inline Signup to confirmation definition Jan 23, 2025
@samer-stripe samer-stripe changed the title Move Link Inline Signup to confirmation definition Move LinkInlineSignup to confirmation definitions Jan 23, 2025
@samer-stripe
Copy link
Collaborator Author

There is a major behavior change now with how signup is handled in FlowController. We are no longer handling signup directly in the sheet but when the user clicks Buy now which is the behavior we have on iOS.

What isn't in this PR is the restoration of the signup details when the user opens the sheet. I am planning on fast-following with that PR.

@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch 2 times, most recently from 1f2eed3 to ae40994 Compare January 23, 2025 18:42
@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from ae40994 to 749e238 Compare January 24, 2025 02:57
linkAnalyticsHelper.onLinkPopupSkipped()

return linkInlineSignupConfirmationOption.toNewOption()
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Forgot to add this use case from LinkHandler. We should really just remove SignIn as a type and only generate a UserInput if the user has valid sign up values (complete and also unused email). Will be a follow-up task

name = "John Doe",
consentAction = SignUpConsentAction.Checkbox,
),
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to change these tests to use SignUp instead of SignIn since the shouldCompleteLinkInlineFlow parameter is no longer supported since moving FlowController sign up out of PaymentOptionsViewModel.

@samer-stripe samer-stripe force-pushed the samer/link-inline-signup-definition branch from d41d00d to b5ce0c7 Compare January 24, 2025 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants