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

Additional "default browser" prompts: enroll only onboarded users #5523

Conversation

LukasPaczos
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/1208671518894266/1209220388656415/f

Description

We only want to enroll users that are already onboarded and have seen the default browser prompt that's part of onboarding.

Steps to test this PR

You can follow instructions from #5476 but when you're in the onboarding experience, minimize the app, change the date forward, and resume the app. Previously the experiment's default browser dialog would show, with the proposed changes it won't, because we're only enrolling users after onboarding is done.

@LukasPaczos LukasPaczos requested a review from malmstein January 23, 2025 12:10
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-variant-2 branch from 4a97b9f to 6c7a3cd Compare January 23, 2025 13:19
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-enroll-after-onboarding branch from a63c7ff to e002bd3 Compare January 23, 2025 13:20
Copy link
Contributor

@malmstein malmstein left a comment

Choose a reason for hiding this comment

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

I assume you’ve checked regarding the Onboarding state. Approving this pre-emptively.

@@ -154,9 +165,10 @@ class DefaultBrowserPromptsExperimentImpl @Inject constructor(
}

private suspend fun evaluate() {
val isOnboardingComplete = userStageStore.getUserAppStage() == AppStage.ESTABLISHED
Copy link
Contributor

Choose a reason for hiding this comment

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

Has @nalcalag looked at this? In the past we’ve had issues understanding if onboarding was complete in some edge cases, so just want to double check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the heads-up @malmstein . This check is correct, when onboarding is completed, the UserAppStage should be set to ESTABLISHED. This means all onboarding dialogs, both in-context and on the new tab page, will have already been shown.

There are other methods to determine if parts of the onboarding process have been completed, such as checking for dialogs on the new tab page, etc. I’m not sure about the specific requirements for this feature, but feel free to reach out to me if needed @LukasPaczos .

@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-variant-2 branch 3 times, most recently from 1397fda to cc2c9a8 Compare January 31, 2025 12:59
Base automatically changed from feature/lukasz-p/default-browser-prompts-variant-2 to develop January 31, 2025 13:32
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-enroll-after-onboarding branch from e002bd3 to bc30cad Compare January 31, 2025 13:36
@LukasPaczos LukasPaczos force-pushed the feature/lukasz-p/default-browser-prompts-enroll-after-onboarding branch from bc30cad to 58b330f Compare January 31, 2025 14:11
@LukasPaczos LukasPaczos merged commit 324f597 into develop Jan 31, 2025
5 checks passed
@LukasPaczos LukasPaczos deleted the feature/lukasz-p/default-browser-prompts-enroll-after-onboarding branch January 31, 2025 14:29
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.

3 participants