-
Notifications
You must be signed in to change notification settings - Fork 331
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
feat: refactor feature flags #10234
feat: refactor feature flags #10234
Conversation
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
…eature-flag-refactor
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Sorry the PR is so large! I tried to break it down by splitting it into a few PRs, but I don't think there's a good way to make this one smaller. |
// } | ||
export const featureFlagByOwnerId = (parent: RootDataLoader) => { | ||
return new DataLoader< | ||
{ownerId: string; ownerType: 'User' | 'Team' | 'Organization'; featureName: string}, |
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.
ownerType
looks like a footgun,
see convo here: #10184 (comment)
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
featureFlags { | ||
suggestGroups | ||
} | ||
hasSuggestGroupsFlag: featureFlag(featureName: "suggestGroups") |
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.
+1 nit on alias convention-- sometimes you have a suffix like publicTeamsEnabled
, sometimes it's just the name of the feature aiTemplate
and here we have a has
prefix. no right or wrong, but always nice to set a convention for others to follow
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.
Makes sense! I like the most explicit option of has[name]Flag
, so I've updated all of them to follow that convention
...ges/client/modules/email/components/SummaryEmail/MeetingSummaryEmail/WholeMeetingSummary.tsx
Outdated
Show resolved
Hide resolved
""" | ||
Get all of the feature flags for a user, team, or org | ||
""" | ||
getFeatureFlags(userId: ID, teamId: ID, orgId: ID): GetFeatureFlagPayload! |
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.
-1 i think you meant to put this under query instead of mutation?
What's the use case for this one again? I can't remember.
Is there a way to query all feature flags in the system, e.g. if I'm a sales person & i want to apply a feature flag but I can't remember the name of the flag, how would i query 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.
Thanks, i've removed this and added getAllFeatureFlags
as a private query so that a sales person can query it to see all of the feature flags. I'll add a Notion doc with instructions once this is merged
packages/server/graphql/public/typeDefs/FeatureFlagScope.graphql
Outdated
Show resolved
Hide resolved
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
.selectFrom('FeatureFlag') | ||
.innerJoin('FeatureFlagOwner', 'FeatureFlag.id', 'FeatureFlagOwner.featureFlagId') | ||
.where((eb) => | ||
eb.and([ |
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.
+1 you could simplify this a little because FeatureFlag tells you what column to look in so you don't have to look in all 3 columns.
You also don't have to destructure the (featureName, ownerId)
tuple. You could do something like:
WHERE
("featureName", CASE "FeatureFlag"."scope"
WHEN 'User' THEN "FeatureFlagOwner"."userId"
WHEN 'Team' THEN "FeatureFlagOwner"."teamId"
WHEN 'Organization' THEN "FeatureFlagOwner"."orgId"
END) in :keys;
This will execute as something like WHERE ("featureName", "userId") in (("insights", "user123"), ("insights", "user456"))
. That way you won't overfetch (albeit the overfetch is pretty minimal in this case)
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.
Looks good! 1 little change on the SQL piece, other than that good to go!
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
6cc5887
into
feat/apply-new-feature-flag-mutation
Fix #10099
This PR refactors feature flags from the array of enums to the new table approach.
The parent PRs added the new feature flag tables and mutations. Here I'm updating all of the references.
This PR can't be merged until we migrate old necessary feature flags over to the new tables, see: #10248
I'll also create a subsequent PR to notify the team via cron job when a feature flag is expiring: #10249.
To test
addFeatureFlag
mutationapplyFeatureFlag