More intuitive flag behaviour #4967
Merged
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
|
Preview URL 🚀 : https://blurts-server-pr-4967-mgjlpikfea-uk.a.run.app |
rhelmer
approved these changes
Aug 21, 2024
| @@ -56,7 +56,7 @@ export default async function WelcomeToPlusPage() { | |||
| }; | |||
|
|
|||
| const enabledFeatureFlags = await getEnabledFeatureFlags({ | |||
| ignoreAllowlist: false, | |||
| isLoggedOut: false, | |||
There was a problem hiding this comment.
Some places in the code say "signed in/out" and some say "logged in/out", not a huge deal but would be nice to be consistent. I think Mozilla Accounts tends to use the former.
There was a problem hiding this comment.
Oh good callout. Next-Auth uses "sign in/out" as well, so I've updated to align with that too: 7d54a46
The allowlist now only applies to disabled flags, allowing us to enable those flags for specific users. The code now also makes more explicit that you should always pass the user's email address, the only exception being if they're not logged in. (It makes intuitive sense that a flag can't be enabled for a specific user if that user is not logged in.)
Vinnl
force-pushed
the
MNTOR-3528-intuitive-flags
branch
from
7d54a46
to
f3e8397
Compare
|
Cleanup completed - database 'blurts-server-pr-4967' destroyed, cloud run service 'blurts-server-pr-4967' destroyed |
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.
References:
Jira: MNTOR-3528
Figma:
Description
The allowlist now only applies to disabled flags, allowing us to enable those flags for specific users. The code now also makes more explicit that you should always pass the user's email address, the only exception being if they're not logged in. (It makes intuitive sense that a flag can't be enabled for a specific user if that user is not logged in.)
In other words, enabling a flag now always enables it for everyone, regardless of the allowlist.
Also note that I moved disabled feature flags to the top, since those are the ones we'll usually want to take action on, whereas we don't usually want to disable flags once they are enabled. (And once we remove their code, disabling them doesn't do anything anymore.)
This is the right moment to make this change, because the only flag with an allowlist at the moment is
RedesignedEmails, which is already set to be removed with the next deploy.Screenshot (if applicable)
How to test
Checklist (Definition of Done)