I think we should replace the callbacks in favor of a single method that handles view actions. A sealed class representing all the view actions can then just be appended to.

    sealed interface ViewAction {
        data class OnBrandChoiceChanged(val cardBrandChoice: CardBrandChoice) : ViewAction
        data class OnCardDetailsChanged(val cardUpdateParams: CardUpdateParams?) : ViewAction
    }

• fun onBrandChoiceChanged(cardBrandChoice: CardBrandChoice) is a function meant to be called from the UI when a new brand is selected.
• val onBrandChoiceChanged: BrandChoiceCallback is meant for analytics purposes. It's fired when the selected brand in the function above is different from the original. The brandChoiceChange function parameter is sent downstream from [Payment|Customer]SheetViewModel to UpdatePaymentMethodInteractor. EditCardDetailsInteractor is handling CBC change now, that's why I added the function parameter
  *val onCardDetailsChanged: CardDetailsCallback is a callback that emits edit card parameters to a consumer (UpdatePaymentMethodInteractor)

I don't think onCardDetailsChanged fits into the viewAction because of the behaviour I described above. I could move the brandChanged function into a ViewAction interface. What do you think?

Is this unintentional? I see two callbacks for handling changes to card brand choice.

The one highlighted in this comments is to send an analytics event upstream. The other is for when the user selects a brand from the dropdown. I expanded more here

I could rename it to something with analytics in its name. What do you think?

This feels redundant since brand choice can be controlled from CardUpdateParams as well. Are we going to combine these later? Could each individual field be a view action?

This callback is for analytics. I don't want the component using this interactor to have to determine when the cardbrand is changed. CardUpdateParams has expiryDate and address values as well, so a new cardUpdateParam does not mean the card brand is changed. The consumer will need to add logic (trivial I admit) to determine if there's a new card brand.

internal data class CardUpdateParams(
    val expiryMonth: Int? = null,
    val expiryYear: Int? = null,
    val cardBrand: CardBrand? = null,
    val billingDetails: PaymentMethod.BillingDetails? = null,
)

Why does it default to CartesBancaires?

The CARD_WITH_NETWORKS test fixture defaults to CartesBancaires. It was probably an arbitrary choice when the fixture was created

private fun handler(
        card: PaymentMethod.Card = PaymentMethodFixtures.CARD_WITH_NETWORKS,
        cardBrandFilter: CardBrandFilter = DefaultCardBrandFilter,

usually tests follow a pattern of:

• setup
• test
• assert

This is also known as: arrange, act, assert

in this case I suppose the test should look like this. Note that there is a single line between each section which acts as a separator between each section: arrange, act and assert.

@Test
   fun  xxx() {
        var cardUpdateParams: CardUpdateParams? = null
        val handler = handler(
            onCardDetailsChanged = {
                cardUpdateParams = it
            }
        )

        handler.brandChanged(CardBrand.Visa)

        assertThat(cardUpdateParams).isEqualTo(cardUpdateParams(cardBrand = CardBrand.Visa))
   }

If we want to test initial states like defaulting to CardBrand.CartesBancaires or cardUpdateParams being null, we should keep that in a different test that is ONLY testing that.

Imho, these comments hurt more than the help -- imho, i'd remove them because the variable names seem to speak for themselves, but i'll leave it up to you guys.

imho, shouldShowCardBrandDropdown or isCardBrandDropdownShown are better names as this is a Boolean

s/BrandChoiceCallback/CardBrandChoiceCallback/
or maybe
s/BrandChoiceCallback/CardBrandCallback/

Calling this "CardDetailsCallback" introduces "Details" as new nomenclature. It seems like we avoid this by just changing the name of this to: CardUpdateParamsCallback

Question: Are we expecting to add parameters to CardDetailsEntry that we shouldn't compare?

• If no, can we instead rely on the generated equals of data class instead of hasChanged?
• If yes, is there anything we can remove or reduce to make it possible?

Also should we just compare entries here?

fun hasChanged(
        cardDetailsEntry: CardDetailsEntry,
    ): Boolean

We're not comparing two versions of CardDetailsEntry, distinctUntilChanged would have sufficed in that scenario. We're comparing against the existing card and billing details (taking billingDetailsCollectionMode into account).

I have the full logic on my prototype branch here - https://github.com/stripe/stripe-android/pull/10366/files#r2025490382

We should be calling Dispatchers.setMain(testDispatcher) in the setup (@BeforeTest) function, but other tests call it here. I followed that pattern to avoid any side-effects.

Does this still need to be exposed? Is the UI going to call it somewhere?

Yh, it needs to be exposed. It is used in the UpdatePaymentMethodInteractor to consume the latest CardUpdateParams

EditCardDetailsInteractor.create(
    card = displayableSavedPaymentMethod.savedPaymentMethod.card,
    onCardUpdateParamsChanged = {
        // CardParams update
        this.cardUpdateParams.value = it
    },
    coroutineScope = coroutineScope,
    isModifiable = displayableSavedPaymentMethod.isModifiable(),
    cardBrandFilter = cardBrandFilter,
    onBrandChoiceChanged = onBrandChoiceSelected,
)

The UpdatePaymentMethodInteractor uses this value to determine if the Save button should be enabled. It also uses this value to execute the update operation.

private suspend fun maybeUpdateCard(): Result<PaymentMethod>? {
    val cardUpdateParams = cardUpdateParams.value
    return if (cardUpdateParams != null) {
        updatePaymentMethodExecutor(
            displayableSavedPaymentMethod.paymentMethod,
            cardUpdateParams
        ).onSuccess {
            this.cardUpdateParams.value = null
        }
    } else {
        null
    }
}

Would passing it through the create method be enough? Is there ever a reason for a consumer to call interactor.onCardUpdateParamsChanged?

Emitting CardUpdateParams is a core part of this interactor's behaviour and is used in testing. An example is this one:
We're testing that the save button is enabled when there is a non-null CardUpdateParams.

@Test
fun updatingCard_updatesStateAsExpected(: {
    var updatedPaymentMethod: PaymentMethod? = null
    val initialPaymentMethod = PaymentMethodFixtures.CARD_WITH_NETWORKS_PAYMENT_METHOD

    runScenario(
        displayableSavedPaymentMethod = initialPaymentMethod.toDisplayableSavedPaymentMethod(),
        updatePaymentMethodExecutor = { paymentMethod, _ ->
            updatedPaymentMethod = paymentMethod
            Result.success(paymentMethod)
        },
        onUpdateSuccess = {},
    ) {
        val editCardDetailsInteractor = interactor.editCardDetailsInteractorHelper()

        editCardDetailsInteractor.onCardUpdateParamsChanged(
            CardUpdateParams(
                cardBrand = CardBrand.CartesBancaires
            )
        )

        interactor.state.test {
            assertThat(awaitItem().isSaveButtonEnabled).isTrue()
        }
    }
}

We could call editCardDetailsInteractor.handleViewAction(...) to update the card brand and expect a new CardUpdateParams, but I feel like that would be testing the behaviour of the editCardDetailsInteractor. That's not what we're trying to do here. DefaultEditCardDetailsInteractorTest is doing that for us already.

Yeah I see what's going on here now. I think exposing an attribute for testing is the wrong solution. Re-introducing the Factory interface is probably better since we can then pass all these parameters through then just provide the factory as a parameter UpdatePaymentMethodInteractor.

I'm peaceful with keeping this for now! Can we follow-up with the factory?