fix: include group context in $feature_flag_called dedupe key#625
Open
gustavohstrassburger wants to merge 2 commits into
Open
fix: include group context in $feature_flag_called dedupe key#625gustavohstrassburger wants to merge 2 commits into
gustavohstrassburger wants to merge 2 commits into
Conversation
In `_capture_feature_flag_called_if_needed`, the per-distinct_id dedupe key only included the flag key and response. For group-scoped flags, this meant that when the same user was evaluated under a different group, no new `$feature_flag_called` event was fired — causing per-group exposure undercount for experiments scoped to a group key. Include the sorted `groups` map in the dedupe key so that the same (distinct_id, flag, response) combination fires once per distinct group context. Repeated calls under the same group context still dedupe, and calls that pass the same map in a different key order still dedupe (the groups are canonicalized via `sorted(groups.items())`). Mirrors the posthog-node fix in PostHog/posthog-js#3658. Generated-By: PostHog Code Task-Id: d94308d9-7655-4bac-8f15-c61478b5fca1
Contributor
posthog-python Compliance ReportDate: 2026-05-25 19:19:43 UTC ✅ All Tests Passed!45/45 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 16/16 tests passed View Details
|
Generated-By: PostHog Code Task-Id: d94308d9-7655-4bac-8f15-c61478b5fca1
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
posthog/test/test_feature_flags.py:5339-5373
**Prefer parameterised tests for the two dedupe cases**
`test_capture_dedupes_repeated_calls_under_same_group_context` and `test_capture_dedupes_when_groups_passed_in_different_key_order` have identical structure — they both assert `flag_called_count == 1` and differ only in the groups supplied. The codebase already uses `@parameterized.expand` (e.g. `test_mixed_targeting` above), so these two cases should be collapsed into a single `@parameterized.expand` test instead of two separate methods.
Reviews (1): Last reviewed commit: "chore: add changeset" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+5339
to
+5373
| @mock.patch.object(Client, "capture") | ||
| @mock.patch("posthog.client.flags") | ||
| def test_capture_dedupes_when_groups_passed_in_different_key_order( | ||
| self, patch_flags, patch_capture | ||
| ): | ||
| client = Client(FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) | ||
| client.feature_flags = [ | ||
| { | ||
| "id": 1, | ||
| "name": "Group flag", | ||
| "key": "group-scoped-flag", | ||
| "active": True, | ||
| "filters": { | ||
| "groups": [{"properties": [], "rollout_percentage": 100}], | ||
| }, | ||
| } | ||
| ] | ||
|
|
||
| client.get_feature_flag( | ||
| "group-scoped-flag", | ||
| "user-1", | ||
| groups={"company": "org-a", "team": "red"}, | ||
| ) | ||
| client.get_feature_flag( | ||
| "group-scoped-flag", | ||
| "user-1", | ||
| groups={"team": "red", "company": "org-a"}, | ||
| ) | ||
|
|
||
| flag_called_count = sum( | ||
| 1 | ||
| for call in patch_capture.call_args_list | ||
| if call.args and call.args[0] == "$feature_flag_called" | ||
| ) | ||
| self.assertEqual(flag_called_count, 1) |
Contributor
There was a problem hiding this comment.
test_capture_dedupes_repeated_calls_under_same_group_context and test_capture_dedupes_when_groups_passed_in_different_key_order have identical structure — they both assert flag_called_count == 1 and differ only in the groups supplied. The codebase already uses @parameterized.expand (e.g. test_mixed_targeting above), so these two cases should be collapsed into a single @parameterized.expand test instead of two separate methods.
Context Used: Do not attempt to comment on incorrect alphabetica... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 5339-5373
Comment:
**Prefer parameterised tests for the two dedupe cases**
`test_capture_dedupes_repeated_calls_under_same_group_context` and `test_capture_dedupes_when_groups_passed_in_different_key_order` have identical structure — they both assert `flag_called_count == 1` and differ only in the groups supplied. The codebase already uses `@parameterized.expand` (e.g. `test_mixed_targeting` above), so these two cases should be collapsed into a single `@parameterized.expand` test instead of two separate methods.
**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
💡 Motivation and Context
In
_capture_feature_flag_called_if_needed, the per-distinct_iddedupe key only included the flag key and response. For group-scoped flags this meant that when the same user was evaluated under a different group, no new$feature_flag_calledevent was fired — causing per-group exposure undercount for experiments scoped to a group key.This mirrors the posthog-node fix in PostHog/posthog-js#3658 (which closes PostHog/posthog-js#3651). Both SDKs share the same dedupe shape, so backend evaluation needs the same change.
💚 How did you test it?
Added three tests covering the new behavior:
test_capture_fires_per_group_context— same user, same flag, same response, two different group contexts → two$feature_flag_calledevents.test_capture_dedupes_repeated_calls_under_same_group_context— repeated access under the same group → one event.test_capture_dedupes_when_groups_passed_in_different_key_order— same groups passed with keys in a different order → still one event (canonicalized viasorted(groups.items())).All 143 existing tests in
test_feature_flags.pystill pass, plus the 29 intest_evaluate_flags.py.📝 Checklist
If releasing new changes
sampo addto generate a changeset fileCreated with PostHog Code