[Link holdback] Add ahead lookup call for exposure logging. #10565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…osures-on-mpe' into carlosmuvi/add-v2-reporting-to-mpe
|
Diffuse output: APKDEX |
mats-stripe
reviewed
Apr 4, 2025
| val experimentsData = requireNotNull( | ||
| elementsSession.experimentsData | ||
| ) { "Experiments data required to log exposures" } | ||
| val holdbackOn = elementsSession.linkSettings?.linkGlobalHoldbackOn == true |
There was a problem hiding this comment.
AFAIK, we should instead look at elementsSession.experimentsData.experimentAssignments["link_global_holdback"] to get the group. In testmode, this key will not be present, so we treat that as though it were the control group. Confirming here though: https://stripe.slack.com/archives/C05RD8RBF5L/p1743774491875609?thread_ts=1743189375.360319&cid=C05RD8RBF5L
With this, I don't think we need the linkGlobalHoldbackOn field on link_settings for anything?
There was a problem hiding this comment.
updated it here - 8a30e76 (including removing linkGlobalHoldbackOn)
mats-stripe
reviewed
Apr 4, 2025
| group = group, | ||
| name = "link_global_holdback", | ||
| dimensions = mapOf( | ||
| "integration_type" to "mpe", |
There was a problem hiding this comment.
Are we doing ios / android integration types? Or grouping all of MPE together?
Suggested change
| "integration_type" to "mpe", | |
| "integration_type" to "mpe_android", |
There was a problem hiding this comment.
…onsumer-for-exposure # Conflicts: # paymentsheet/src/main/java/com/stripe/android/common/analytics/experiment/LogLinkGlobalHoldbackExposure.kt # paymentsheet/src/main/java/com/stripe/android/common/analytics/experiment/LoggableExperiment.kt # paymentsheet/src/test/java/com/stripe/android/common/analytics/experiment/LogLinkGlobalHoldbackExposureTest.kt # paymentsheet/src/test/java/com/stripe/android/paymentsheet/analytics/DefaultEventReporterTest.kt
Comment on lines
173
to
177
| val avoidConsumerLoggingParams: Map<String, Boolean> = if (doNotLogConsumerFunnelEvent) { | ||
| mapOf("TEMPORARY_AND_DANGEROUS__do_not_log_consumer_funnel_event" to true) | ||
| } else { | ||
| emptyMap() | ||
| } |
There was a problem hiding this comment.
this is unfortunate, but should go away after the holdback.
|
|
||
| internal class LogLinkGlobalHoldbackExposure @Inject constructor( | ||
| internal interface LogLinkGlobalHoldbackExposure { |
There was a problem hiding this comment.
Interface to facilitate adding a Fake.
tillh-stripe
reviewed
Apr 7, 2025
| @@ -45,6 +45,7 @@ interface ConsumersApiService { | |||
| suspend fun lookupConsumerSession( | |||
| email: String, | |||
| requestSurface: String, | |||
| doNotLogConsumerFunnelEvent: Boolean = false, | |||
There was a problem hiding this comment.
Nit: Should we remove the default param here, so that callers are forced to think about which values to provide?
Comment on lines
90
to
97
| .map { it.exists } | ||
| .fold( | ||
| onSuccess = { it }, | ||
| onFailure = { | ||
| logger.error("Failed to check if user is returning", it) | ||
| false | ||
| } | ||
| ) |
There was a problem hiding this comment.
Nit: No need to fold.
Suggested change
| .map { it.exists } | |
| .fold( | |
| onSuccess = { it }, | |
| onFailure = { | |
| logger.error("Failed to check if user is returning", it) | |
| false | |
| } | |
| ) | |
| .map { it.exists } | |
| .onFailure { | |
| logger.error("Failed to check if user is returning", it) | |
| false | |
| } |
There was a problem hiding this comment.
onFailure does not allow error recovery, but getOrElse does the trick!
|
|
||
| val customerEmail = state.getEmail() | ||
|
|
||
| val isReturningUser: Boolean = customerEmail?.let { isReturningUser(email = it) } == true |
There was a problem hiding this comment.
Nit: We can use to compiler to our advantage and clean this up a bit.
Suggested change
| val isReturningUser: Boolean = customerEmail?.let { isReturningUser(email = it) } == true | |
| val isReturningUser = customerEmail != null && isReturningUser(customerEmail) |
Comment on lines
101
to
111
| paymentMethodMetadata.linkState?.configuration?.customerInfo?.email | ||
| ?: config.defaultBillingDetails?.email | ||
| ?: customer?.let { | ||
| customerRepository.retrieveCustomer( | ||
| CustomerRepository.CustomerInfo( | ||
| id = it.id, | ||
| ephemeralKeySecret = it.ephemeralKeySecret, | ||
| customerSessionClientSecret = it.customerSessionClientSecret | ||
| ) | ||
| ) | ||
There was a problem hiding this comment.
We have similar code here to determine the actual customer email.
Can we extract that into a method and then use it in both the PaymentElementLoader and in here?
| @@ -27,6 +27,15 @@ internal interface LinkRepository { | |||
| email: String, | |||
| ): Result<ConsumerSessionLookup> | |||
|
|
|||
| /** | |||
| * Performs a lookup of a consumer session without triggering any | |||
There was a problem hiding this comment.
Suggested change
| * Performs a lookup of a consumer session without triggering any | |
| * Performs a lookup of a consumer session without triggering any |
| val exposureCall = eventReporter.experimentExposureCalls.awaitItem() | ||
| assertTrue(exposureCall.experiment is LoggableExperiment.LinkGlobalHoldback) | ||
| assertEquals(exposureCall.experiment.group, "holdback") | ||
| assertTrue((exposureCall.experiment.isReturningLinkConsumer)) |
There was a problem hiding this comment.
((…))
Let’s use Truth’s assertThat() instead.
| val exposureCall = eventReporter.experimentExposureCalls.awaitItem() | ||
| assertTrue(exposureCall.experiment is LoggableExperiment.LinkGlobalHoldback) | ||
| assertEquals(exposureCall.experiment.group, "holdback") | ||
| assertFalse((exposureCall.experiment.isReturningLinkConsumer)) |
There was a problem hiding this comment.
((…))
Let’s use Truth’s assertThat() instead.
…onsumer-for-exposure
|
@tillh-stripe addressed all feedback here! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
no-cache,no-backend-logginglookup if we can retrieve an email when retrieving the session. On a follow-up PR, this will be put behind a killswitch so we can turn it off after the experiment is done.Motivation
Testing
Screenshots
Changelog