-
Notifications
You must be signed in to change notification settings - Fork 989
Implement feature flag enrolment API #6177
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
Conversation
bc9137e
to
8698ba0
Compare
@@ -55,13 +55,13 @@ class ReactivateUsersExperimentImpl @Inject constructor( | |||
) : ReactivateUsersExperiment { | |||
|
|||
override suspend fun isControl(): Boolean = | |||
reactivateUsersToggles.reactivateUsersExperimentMay25().isEnabled(CONTROL) | |||
reactivateUsersToggles.reactivateUsersExperimentMay25().isEnrolledAndEnabled(CONTROL) |
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.
This experiment is missing enrolling users too, I think we can do it in the evaluate
method of RealDaxPrompts
right after checking the user is eligible (line 55)? cc @anikiki to double check.
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.
Yes, that's correct 👍
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.
@marcosholgado @anikiki can you check the latest commit to see if that's what we want?
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 checked and it's what we want 👍
...rc/main/java/com/duckduckgo/app/browser/senseofprotection/SenseOfProtectionExperimentImpl.kt
Show resolved
Hide resolved
...rc/main/java/com/duckduckgo/app/browser/senseofprotection/SenseOfProtectionExperimentImpl.kt
Show resolved
Hide resolved
@aitorvs I pushed a commit that fixes the |
...ions-impl/src/main/java/com/duckduckgo/subscriptions/impl/ui/SubscriptionWebViewViewModel.kt
Outdated
Show resolved
Hide resolved
5d7fb7b
to
5cd6e2b
Compare
3df3c69
to
d6eb4b7
Compare
2839a64
to
e0cfb55
Compare
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.
LGTM
…esignShieldThenUseNonExperimentAssets
Task/Issue URL: https://app.asana.com/1/137249556945/project/1198194956794324/task/1210415399293026?focus=true
Description
Implement the enrolment API as defined in API Proposal: feature flag enrolment API
Steps to test this PR
We tested the following experiments
Thanks @marcosholgado, @mikescamell , @anikiki , @LukasPaczos