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

Add FormActivityViewModel #9983

Merged
merged 13 commits into from
Jan 28, 2025
Merged

Add FormActivityViewModel #9983

merged 13 commits into from
Jan 28, 2025

Conversation

tjclawson-stripe
Copy link
Collaborator

@tjclawson-stripe tjclawson-stripe commented Jan 24, 2025

Summary

Setup FormActivityViewModel and FormActivityComponent and display VerticalModeFormUI

Motivation

https://jira.corp.stripe.com/browse/MOBILESDK-2941

Screen.Recording.2025-01-25.at.5.59.27.PM.mov

Testing

  • Added tests
  • Modified tests
  • Manually verified

import com.stripe.android.model.PaymentMethodCode
import javax.inject.Inject

internal class FormActivityViewModel @Inject constructor(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Named FormActivityViewModel to not conflict with existing FormViewModel during refactors

Copy link
Contributor

github-actions bot commented Jan 24, 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 │ +1 B │ 170.7 KiB │ 170.7 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ +1 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                                      
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
    272 B │ +1 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                        
 25.4 KiB │ -1 B │    63 KiB │  0 B │ ∆ META-INF/MANIFEST.MF                    
──────────┼──────┼───────────┼──────┼───────────────────────────────────────────
 54.2 KiB │ +1 B │ 126.2 KiB │  0 B │ (total)

@tjclawson-stripe tjclawson-stripe force-pushed the tyler/form-viewmodel-setup branch from 32ee872 to a997c6d Compare January 24, 2025 22:13
@tjclawson-stripe tjclawson-stripe changed the title Add FormActivityViewModel Add FormActivityViewModel Scaffolding Jan 24, 2025
@tjclawson-stripe tjclawson-stripe marked this pull request as ready for review January 24, 2025 22:34
@tjclawson-stripe tjclawson-stripe requested review from a team as code owners January 24, 2025 22:35
hostedSurface = HOSTED_SURFACE_PAYMENT_ELEMENT,
instantDebits = instantDebits,
linkMode = paymentMethodMetadata.linkMode,
onBehalfOf = null,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Todo as part of confirmation implementation

@@ -40,7 +47,10 @@ internal class FormActivity : AppCompatActivity() {
finish()
}
) {
Text(localArgs.selectedPaymentMethodCode)
VerticalModeFormUI(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will be moved to a FormActivityUI composable in follow up PRs for headers/mandates

@tjclawson-stripe tjclawson-stripe marked this pull request as draft January 25, 2025 22:59
@tjclawson-stripe tjclawson-stripe changed the title Add FormActivityViewModel Scaffolding Add FormActivityViewModel Jan 26, 2025
@tjclawson-stripe tjclawson-stripe force-pushed the tyler/form-viewmodel-setup branch from 9fbebe5 to 65c4a6b Compare January 26, 2025 19:28
selectedPaymentMethodCode: PaymentMethodCode,
private val selectionHolder: EmbeddedSelectionHolder,
private val embeddedFormHelperFactory: EmbeddedFormHelperFactory,
val eventReporter: EventReporter
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've got an idea on how to remove the need for this. I'm going to attempt it in Manage first, and can follow up here if it's successful.

internal class FormActivityViewModelTest {

@Test
fun `viewmodel initializes interactor correctly`() = runTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly, it feels like we should remove this test, and have the test be in EmbeddedFormInteractorFactory. But even then, I don't think that test is very valuable. It seems like we should just remove this.

This is just wiring code that'll get tested with integration tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

val eventReporter: EventReporter
) : ViewModel() {

val formInteractor = embeddedFormInteractorFactory.create(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to do one of 2 things:

  1. Inject all these things, they should already be in the dependency injection graph.
  2. Use this object somewhere else that can't inject these things. But even then, feels like this should live outside the view model.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update to inject all dependencies and inject interactor directly into FormActivity

formElements = formHelper.formElementsForCode(paymentMethodCode),
onFormFieldValuesChanged = formHelper::onFormFieldValuesChanged,
usBankAccountArguments = usBankAccountFormArguments,
reportFieldInteraction = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this just delegates to eventReporter, right? Any reason not to hook it up now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It delegates to PaymentSheetAnalyticsListener, which requires a CurrentScreen. I'll implement in a follow up

@tjclawson-stripe tjclawson-stripe marked this pull request as ready for review January 28, 2025 17:45
@tjclawson-stripe tjclawson-stripe enabled auto-merge (squash) January 28, 2025 17:45
@tjclawson-stripe tjclawson-stripe merged commit 14c5f1d into master Jan 28, 2025
13 checks passed
@tjclawson-stripe tjclawson-stripe deleted the tyler/form-viewmodel-setup branch January 28, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants