Skip to content

feat(flags): support mixed targeting in local evaluation#523

Open
patricio-posthog wants to merge 7 commits intomainfrom
feat/mixed-targeting-local-eval
Open

feat(flags): support mixed targeting in local evaluation#523
patricio-posthog wants to merge 7 commits intomainfrom
feat/mixed-targeting-local-eval

Conversation

@patricio-posthog
Copy link
Copy Markdown
Contributor

Problem

Flags using mixed user+group targeting (the beta feature) cannot be evaluated locally by the SDK. The SDK reads aggregation_group_type_index at the flag level only. For mixed flags this is None, so all conditions are evaluated as person targeting, causing group conditions to fail and fall back to an HTTP call.

This breaks customers using flags in environments that cannot make HTTP calls (e.g., Temporal workflows).

Changes

Updated match_feature_flag_properties to resolve aggregation per condition when a condition explicitly sets its own aggregation_group_type_index that differs from the flag level. Each condition uses the correct properties and bucketing value for its aggregation type. Backwards compatible with existing pure person and pure group flags.

How did you test this code?

All 115 existing tests pass unchanged. Added 6 new tests covering mixed targeting scenarios: person match, group match, no match, group skips to person, no groups passed, and correct bucketing per condition.

@patricio-posthog patricio-posthog requested a review from a team as a code owner April 17, 2026 20:52
@patricio-posthog patricio-posthog requested a review from a team April 17, 2026 20:52
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 17, 2026

posthog-python Compliance Report

Date: 2026-04-17 21:12:02 UTC
Duration: 160008ms

✅ All Tests Passed!

30/30 tests passed


Capture Tests

29/29 tests passed

View Details
Test Status Duration
Format Validation.Event Has Required Fields 517ms
Format Validation.Event Has Uuid 1507ms
Format Validation.Event Has Lib Properties 1506ms
Format Validation.Distinct Id Is String 1507ms
Format Validation.Token Is Present 1506ms
Format Validation.Custom Properties Preserved 1506ms
Format Validation.Event Has Timestamp 1506ms
Retry Behavior.Retries On 503 9519ms
Retry Behavior.Does Not Retry On 400 3505ms
Retry Behavior.Does Not Retry On 401 3508ms
Retry Behavior.Respects Retry After Header 9514ms
Retry Behavior.Implements Backoff 23518ms
Retry Behavior.Retries On 500 7517ms
Retry Behavior.Retries On 502 7509ms
Retry Behavior.Retries On 504 7511ms
Retry Behavior.Max Retries Respected 23531ms
Deduplication.Generates Unique Uuids 1497ms
Deduplication.Preserves Uuid On Retry 7513ms
Deduplication.Preserves Uuid And Timestamp On Retry 14521ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry 7503ms
Deduplication.No Duplicate Events In Batch 1508ms
Deduplication.Different Events Have Different Uuids 1507ms
Compression.Sends Gzip When Enabled 1507ms
Batch Format.Uses Proper Batch Structure 1506ms
Batch Format.Flush With No Events Sends Nothing 1005ms
Batch Format.Multiple Events Batched Together 1505ms
Error Handling.Does Not Retry On 403 3508ms
Error Handling.Does Not Retry On 413 3507ms
Error Handling.Retries On 408 7514ms

Feature_Flags Tests

1/1 tests passed

View Details
Test Status Duration
Request Payload.Request With Person Properties Device Id 513ms

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 17, 2026

Prompt To Fix All With AI
This is a comment left during a code review.
Path: posthog/feature_flags.py
Line: 326-331

Comment:
**Redundant guard variable (`has_condition_aggregation`)**

`has_condition_aggregation` is always `True` whenever `condition_aggregation != flag_aggregation` is `True`. When the key is absent from `condition`, `.get("aggregation_group_type_index", flag_aggregation)` returns `flag_aggregation`, so `condition_aggregation == flag_aggregation` is guaranteed — the `!=` test is already `False` before `has_condition_aggregation` is checked. The extra variable adds complexity without changing behavior (simplicity rule 4: no superfluous parts).

```suggestion
            condition_aggregation = condition.get(
                "aggregation_group_type_index", flag_aggregation
            )

            if condition_aggregation != flag_aggregation:
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3829-3867

Comment:
**Duplicate test — identical to `test_mixed_targeting_person_condition_matches`**

`test_mixed_targeting_group_without_groups_skips_to_person` is byte-for-byte the same flag definition, the same `get_feature_flag` call (no `groups` arg, `person_properties={"email": "test@example.com"}`), and the same assertions as `test_mixed_targeting_person_condition_matches` at line 3710. One of them should be removed; the surviving test can carry the inline comment explaining the skip behaviour. This violates the OnceAndOnlyOnce simplicity rule.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3869-3896

Comment:
**Unused mock setup and missing call-count assertion**

`patch_flags.return_value = {"featureFlags": {"mixed-flag": "from-api"}}` is dead code here — since all group conditions skip locally and `is_inconclusive` stays `False`, the function returns `False` without ever calling `flags`, so the mock return value is never read. Every other new test also asserts `patch_flags.call_count == 0` to prove no network fallback occurred; this test omits that guard, leaving the no-fallback behaviour unverified.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: posthog/test/test_feature_flags.py
Line: 3709-3940

Comment:
**Prefer parameterised tests**

Five of the six new tests use an identical flag definition (two conditions: one group, one person). Per the project's coding standards, parameterised tests are preferred. Factoring the common flag fixture out and using `@parameterized.expand` (or a data-driven equivalent) would reduce the duplication significantly and make it straightforward to add further inputs in future.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "chore: ruff format" | Re-trigger Greptile

Comment thread posthog/feature_flags.py Outdated
Comment thread posthog/test/test_feature_flags.py Outdated
Comment thread posthog/test/test_feature_flags.py Outdated
Comment thread posthog/test/test_feature_flags.py Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Enables local evaluation of feature flags that use mixed person + group targeting by resolving aggregation context (properties + bucketing value) per condition rather than only at the flag level.

Changes:

  • Update match_feature_flag_properties to support per-condition aggregation_group_type_index, selecting the correct properties and bucketing value per condition.
  • Pass group context (group_type_mapping, groups, group_properties) from Client._compute_flag_locally into match_feature_flag_properties.
  • Add new unit tests covering mixed targeting scenarios and add a changeset entry for release notes.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
posthog/feature_flags.py Implements per-condition aggregation handling for mixed local flag evaluation (properties + bucketing selection).
posthog/client.py Ensures local evaluation passes group context needed for mixed targeting conditions.
posthog/test/test_feature_flags.py Adds test coverage for mixed person/group targeting behavior and bucketing correctness.
.sampo/changesets/mixed-targeting-local-eval.md Documents the patch change for release automation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants