fix: include group context in $feature_flag_called dedupe key#153
Open
gustavohstrassburger wants to merge 2 commits into
Open
fix: include group context in $feature_flag_called dedupe key#153gustavohstrassburger 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` hash 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 `groups.sort.to_json` before being mixed into the dedup key). 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. Generated-By: PostHog Code Task-Id: d94308d9-7655-4bac-8f15-c61478b5fca1
Generated-By: PostHog Code Task-Id: d94308d9-7655-4bac-8f15-c61478b5fca1
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
spec/posthog/client_spec.rb:619-682
**Duplicated test setup across all three new examples**
The `api_feature_flag_res` hash, `stub_request`, and `Client.new` lines are copy-pasted verbatim in each of the three new `it` blocks. Extracting them into `let`/`before` inside a shared `context` would satisfy the OnceAndOnlyOnce rule and align with the team preference for parameterised tests. The deduplication pair (same-context → one event, different-context → two events) are also natural candidates for a parameterised example to remove repetition between tests 2 and 3.
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
+619
to
+682
| it '$feature_flag_called dedupes across repeated calls under the same group context' do | ||
| api_feature_flag_res = { | ||
| 'flags' => [ | ||
| { | ||
| 'id' => 1, | ||
| 'name' => 'Group flag', | ||
| 'key' => 'group-flag', | ||
| 'active' => true, | ||
| 'filters' => { | ||
| 'groups' => [ | ||
| { 'properties' => [], 'rollout_percentage' => 100 } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| stub_request( | ||
| :get, | ||
| 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' | ||
| ).to_return(status: 200, body: api_feature_flag_res.to_json) | ||
|
|
||
| c = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) | ||
| allow(c).to receive(:capture) | ||
|
|
||
| expect(c).to receive(:capture).with(hash_including( | ||
| event: '$feature_flag_called', | ||
| groups: { organization: 'org-a' } | ||
| )).exactly(1).times | ||
|
|
||
| c.get_feature_flag('group-flag', 'user-1', groups: { organization: 'org-a' }) | ||
| c.get_feature_flag('group-flag', 'user-1', groups: { organization: 'org-a' }) | ||
| end | ||
|
|
||
| it '$feature_flag_called dedupes when same groups are passed in different key order' do | ||
| api_feature_flag_res = { | ||
| 'flags' => [ | ||
| { | ||
| 'id' => 1, | ||
| 'name' => 'Group flag', | ||
| 'key' => 'group-flag', | ||
| 'active' => true, | ||
| 'filters' => { | ||
| 'groups' => [ | ||
| { 'properties' => [], 'rollout_percentage' => 100 } | ||
| ] | ||
| } | ||
| } | ||
| ] | ||
| } | ||
|
|
||
| stub_request( | ||
| :get, | ||
| 'https://us.i.posthog.com/flags/definitions?token=testsecret&send_cohorts=true' | ||
| ).to_return(status: 200, body: api_feature_flag_res.to_json) | ||
|
|
||
| c = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true) | ||
| allow(c).to receive(:capture) | ||
|
|
||
| expect(c).to receive(:capture).with(hash_including(event: '$feature_flag_called')).exactly(1).times | ||
|
|
||
| c.get_feature_flag('group-flag', 'user-1', groups: { organization: 'org-a', team: 'red' }) | ||
| c.get_feature_flag('group-flag', 'user-1', groups: { team: 'red', organization: 'org-a' }) | ||
| end |
There was a problem hiding this comment.
The api_feature_flag_res hash, stub_request, and Client.new lines are copy-pasted verbatim in each of the three new it blocks. Extracting them into let/before inside a shared context would satisfy the OnceAndOnlyOnce rule and align with the team preference for parameterised tests. The deduplication pair (same-context → one event, different-context → two events) are also natural candidates for a parameterised example to remove repetition between tests 2 and 3.
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: spec/posthog/client_spec.rb
Line: 619-682
Comment:
**Duplicated test setup across all three new examples**
The `api_feature_flag_res` hash, `stub_request`, and `Client.new` lines are copy-pasted verbatim in each of the three new `it` blocks. Extracting them into `let`/`before` inside a shared `context` would satisfy the OnceAndOnlyOnce rule and align with the team preference for parameterised tests. The deduplication pair (same-context → one event, different-context → two events) are also natural candidates for a parameterised example to remove repetition between tests 2 and 3.
**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(lib/posthog/client.rb), 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.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 rspec examples covering the new behavior in
spec/posthog/client_spec.rb:$feature_flag_called fires per group context (group-scoped dedup)— same user, same flag, two different group contexts → two events.$feature_flag_called dedupes across repeated calls under the same group context— repeated access under the same group → one event.$feature_flag_called dedupes when same groups are passed in different key order— same groups passed with keys in a different order → one event (canonicalized viagroups.sort.to_json).All 78 existing client tests pass, and all 35
feature_flag_evaluations_spectests pass.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset fileCreated with PostHog Code