fix(flags): allow overrides for disabled flags #18408
Conversation
| flag.feature_flag.key in localOverrides ? localOverrides[flag.feature_flag.key] : flag.value | ||
| flag.feature_flag.key in localOverrides | ||
| ? localOverrides[flag.feature_flag.key] | ||
| : flag.feature_flag.active && flag.value |
There was a problem hiding this comment.
why do you need these .active checks here and above? Don't seem to be doing anything?
There was a problem hiding this comment.
Need them here because the "flag.value" that comes back is based on whether the flag matches which seem to return True anyway for flags that are disabled. Which results in a flag having a "on" toggle when it's disabled sooo that's super confusing.
There was a problem hiding this comment.
If you literally remove it to see, you can see how buggy it is.
There was a problem hiding this comment.
which seem to return True anyway for flags that are disabled.
I think this is the bug to fix then, right?
Ah, it's the same reason as my below comment, it's calling an internal function to get this value which doesn't do the active check, but get_all_feature_flags does
There was a problem hiding this comment.
Well I don't have full context on where my_flags is also used in so I didn't want to change it at the API level but if it's just being used in toolbar feature flags then I can make the change there instead of in the frontend.
There was a problem hiding this comment.
get_all_flags seems to solve the issue fully anyway and I don't really see my_flags being called anywhere else so LGTM?
There was a problem hiding this comment.
Yep, LGTM! Even if it were being used elsewhere, since you were already removing the active=True filter, it would have borked things elsewhere too.
And since it was doing the wrong thing anyway, always better to fix at the source, i.e. endpoint level.
| @@ -460,7 +460,7 @@ def my_flags(self, request: request.Request, **kwargs): | |||
| raise exceptions.NotAuthenticated() | |||
|
|
|||
| feature_flags = ( | |||
| FeatureFlag.objects.filter(team=self.team, active=True, deleted=False) | |||
| FeatureFlag.objects.filter(team=self.team, deleted=False) | |||
There was a problem hiding this comment.
in a follow up, you should also replace L479 with get_all_feature_flags, because right now this gives incorrect responses for some flags because it's missing logic for hash key overrides, for example.
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
that didn't work, maybe need to merge master? Closing for now, re-open when you're online :) |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
remaking this PR... why is it so broken... |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Problem
#17616
Allow users to toggle disabled flags to test with too
Changes
Screen.Recording.2023-11-06.at.12.20.16.AM.mov
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?