-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(feature-flags): v3 json payloads #13623
Conversation
Hey @EDsCODE! 馃憢 |
posthog/models/feature_flag.py
Outdated
@@ -130,12 +138,18 @@ def get_filters(self): | |||
else: | |||
# :TRICKY: Keep this backwards compatible. | |||
# We don't want to migrate to avoid /decide endpoint downtime until this code has been deployed | |||
return { | |||
response = { |
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.
You don't need to add this, since it's for the backwards compatible case. Makes it more confusing 馃槄 .
This is for the case where the groups
key is not in filters, and there's a direct properties
key instead. There's no way for the payload to be here.
We might be able to remove this completely, depending on if any old clients remain.
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.
There's a related failing test here. Checking again
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.
There does seem to be a lot of trash created too though, where filters are completely empty. Should guard against that, I'll add that in a follow up
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.
pending the two comments, looks good to me!
re: failing test, the problem is you're using the old way to create flags (direct property key), with payloads, which should be impossible in the new UI, since we always use {"groups": []} now |
Yeah got it. The backwards compatible code is so hectic |
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.
2 nits okay for followup: ebafa3f#comments
Problem
payloads
for boolean valuespayload
per variantfeatureFlagPayloads
that returns a dictionary of matched keys to payloadChanges
馃憠 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?