-
Notifications
You must be signed in to change notification settings - Fork 432
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
add support for feature flags overrides #3892
Conversation
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.
Manual testing was failing and I realized that in places where I was casting the cohort it was failing (e.g guard let cohort = freeTrialsExperiment.getCohortIfEnabled() as? FreeTrialsFeatureFlagExperiment.Cohort
). So, we need to remove the FreeTrialsFeatureFlagExperiment.Cohort
type and in places where it was being used update it to the new cohort you defined in BSK. Then also need to update unit tests where the original cohort was used.
I actually made the changes and tried to push, but there had been more changes and also a rebase, so probs just easier if you make them.
Thanks for baring with me… sorry I completely missed it cause the tests were passing… I just pushed the changes. Hopefully all is ok now |
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.
@SabrinaTardio I tested the Free Trial related changes and I was able to retrieve both control and treatment cohorts using a custom JSONBlob config. So, changes look good to me. As discussed on MM, I leave the main review, testing and approval to @ayoy .
Thank you so much for taking the time to test it! |
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! Works as expected 💪 I've fixed protocol compliance in unit tests and am leaving a comment about Localizable.xcstrings file. Thanks @SabrinaTardio!
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.
We probably aren't interested in checking this file in.
Task/Issue URL: https://app.asana.com/0/1204186595873227/1209266907041599/f iOS PR: duckduckgo/iOS#3892 macOS PR: duckduckgo/macos-browser#3798 What kind of version bump will this require?: Major **Optional**: Tech Design URL: https://app.asana.com/0/1204186595873227/1209266907041602/f **Description**: - Experiment types will be defined as FeatureFlag enum values, with their cohort types defined as per FeatureFlagDescribing protocol. - Add support for overrides of experiment cohorts, Cohort type can be used to populate local overrides menu
Task/Issue URL: https://app.asana.com/0/1204186595873227/1209266907041599/f
Tech Design URL: https://app.asana.com/0/1204186595873227/1209266907041602/f
CC:
Description: Moves experiment flag to be part of FeatureFlag (conforming to FeatureFlagDescribing). This allows to extend macOS local overrides to experiment (that will be also added to iOS in a follow up PR)
Steps to test this PR:
Non Internal user
Feature Flag Override
Experiment Override
Reset All Overrides
Test text Zoom -? Default state should be true, Try to toggle it off and check that when visiting a site there is no option to zoom in the settings, use reset and/or toggle to turn it on again and check the Zoom option reappears
and check it behaves as expected and the menu item reports the expected state
You should see one experiment with a default value that can be either control or treatment: (you’ll see COHORT A or COHORT B printed accordingly)
Try and select a cohort to override and check the print is changed accordingly
Test that “Remove override” comes back to the default state
Test that switch to [cohort] behaves as expected (print and menu item state]
Check that "Remove all overrides” works for both feature flags and experiments.
<!—
Before submitting a PR, please ensure you have tested the combinations you expect the reviewer to test, then delete configurations you know do not need explicit testing.
Using a simulator where a physical device is unavailable is acceptable.
—>
Definition of Done (Internal Only):
Copy Testing:
’
rather than’
Orientation Testing:
Device Testing:
OS Testing:
Theme Testing:
—
Internal references:
Software Engineering Expectations
Technical Design Template