-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add Input Screen onboarding wide event #7239
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
base: develop
Are you sure you want to change the base?
Add Input Screen onboarding wide event #7239
Conversation
# Conflicts: # app/src/main/java/com/duckduckgo/app/pixels/remoteconfig/AndroidBrowserConfigFeature.kt
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| viewModelScope.launch(dispatchers.io()) { | ||
| onboardingStore.storeInputScreenSelection(inputScreenSelected) | ||
| if (inputScreenSelected) { | ||
| inputScreenOnboardingWideEvent.onInputScreenEnabledDuringOnboarding() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love this, ie. the fact that we use something from the impl module :(
I haven't found a good way to have this exposed as a public API that would make sense though -- we're also kinda bypassing the public API proposal process 💃
I leave it up to you.
| getCurrentWideEventId()?.let { wideEventId -> | ||
| wideEventClient.flowFinish( | ||
| wideEventId = wideEventId, | ||
| status = FlowStatus.Unknown, | ||
| ) | ||
| cachedFlowId = null | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this? isn't this method supposed to be called only once?
Is it just a safe guard?
| .flowStart( | ||
| name = INPUT_SCREEN_ONBOARDING_FEATURE_NAME, | ||
| flowEntryPoint = "onboarding", | ||
| cleanupPolicy = OnProcessStart(ignoreIfIntervalTimeoutPresent = true), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think should remove the cleanupPolicy. If I understand correctly, when doing that, if 7d pass an event will be sent with status unknown.
| .flowStart( | |
| name = INPUT_SCREEN_ONBOARDING_FEATURE_NAME, | |
| flowEntryPoint = "onboarding", | |
| cleanupPolicy = OnProcessStart(ignoreIfIntervalTimeoutPresent = true), | |
| ) | |
| .flowStart( | |
| name = INPUT_SCREEN_ONBOARDING_FEATURE_NAME, | |
| flowEntryPoint = "onboarding", | |
| ) |
If we do this we can tell how many users went through onboarding but never saw the toggle in the next 7d.
If we do this, we could send FlowStatus.Failure in line 59 above to distinguish them (?)
264395b to
e1920c1
Compare
This reverts commit 60df703.
e1920c1 to
edfd7b1
Compare
edfd7b1 to
fcc17e2
Compare
fcc17e2 to
0f57bed
Compare

Task/Issue URL: https://app.asana.com/1/137249556945/project/488551667048375/task/1212197026199574?focus=true
Description
Steps to test this PR
Feature 1
UI changes