diff --git a/CHANGELOG.md b/CHANGELOG.md index 63b0c111..741010d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 3.7.4 - 2024-11-25 + +1. Fix bug where this SDK incorrectly sent feature flag events with null values when calling `get_feature_flag_payload`. + ## 3.7.3 - 2024-11-25 1. Use personless mode when sending an exception without a provided `distinct_id`. diff --git a/posthog/client.py b/posthog/client.py index 3707d555..b12764c8 100644 --- a/posthog/client.py +++ b/posthog/client.py @@ -173,6 +173,15 @@ def get_feature_payloads( resp_data = self.get_decide(distinct_id, groups, person_properties, group_properties, disable_geoip) return resp_data["featureFlagPayloads"] + def get_feature_flags_and_payloads( + self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None + ): + resp_data = self.get_decide(distinct_id, groups, person_properties, group_properties, disable_geoip) + return { + "featureFlags": resp_data["featureFlags"], + "featureFlagPayloads": resp_data["featureFlagPayloads"], + } + def get_decide(self, distinct_id, groups=None, person_properties=None, group_properties=None, disable_geoip=None): require("distinct_id", distinct_id, ID_TYPES) @@ -746,23 +755,52 @@ def get_feature_flag_payload( groups=groups, person_properties=person_properties, group_properties=group_properties, - send_feature_flag_events=send_feature_flag_events, - only_evaluate_locally=True, + send_feature_flag_events=False, + # Disable automatic sending of feature flag events because we're manually handling event dispatch. + # This prevents sending events with empty data when `get_feature_flag` cannot be evaluated locally. + only_evaluate_locally=True, # Enable local evaluation of feature flags to avoid making multiple requests to `/decide`. disable_geoip=disable_geoip, ) response = None + payload = None if match_value is not None: - response = self._compute_payload_locally(key, match_value) + payload = self._compute_payload_locally(key, match_value) + + flag_was_locally_evaluated = payload is not None + if not flag_was_locally_evaluated and not only_evaluate_locally: + try: + responses_and_payloads = self.get_feature_flags_and_payloads( + distinct_id, groups, person_properties, group_properties, disable_geoip + ) + response = responses_and_payloads["featureFlags"].get(key, None) + payload = responses_and_payloads["featureFlagPayloads"].get(str(key).lower(), None) + except Exception as e: + self.log.exception(f"[FEATURE FLAGS] Unable to get feature flags and payloads: {e}") - if response is None and not only_evaluate_locally: - decide_payloads = self.get_feature_payloads( - distinct_id, groups, person_properties, group_properties, disable_geoip + feature_flag_reported_key = f"{key}_{str(response)}" + + if ( + feature_flag_reported_key not in self.distinct_ids_feature_flags_reported[distinct_id] + and send_feature_flag_events # noqa: W503 + ): + self.capture( + distinct_id, + "$feature_flag_called", + { + "$feature_flag": key, + "$feature_flag_response": response, + "$feature_flag_payload": payload, + "locally_evaluated": flag_was_locally_evaluated, + f"$feature/{key}": response, + }, + groups=groups, + disable_geoip=disable_geoip, ) - response = decide_payloads.get(str(key).lower(), None) + self.distinct_ids_feature_flags_reported[distinct_id].add(feature_flag_reported_key) - return response + return payload def _compute_payload_locally(self, key, match_value): payload = None diff --git a/posthog/test/test_feature_flags.py b/posthog/test/test_feature_flags.py index 90a857e3..1cb5b20a 100644 --- a/posthog/test/test_feature_flags.py +++ b/posthog/test/test_feature_flags.py @@ -1632,9 +1632,10 @@ def test_boolean_feature_flag_payloads_local(self, patch_decide): ) self.assertEqual(patch_decide.call_count, 0) + @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") - def test_boolean_feature_flag_payload_decide(self, patch_decide): - patch_decide.return_value = {"featureFlagPayloads": {"person-flag": 300}} + def test_boolean_feature_flag_payload_decide(self, patch_decide, patch_capture): + patch_decide.return_value = {"featureFlags": {"person-flag": True}, "featureFlagPayloads": {"person-flag": 300}} self.assertEqual( self.client.get_feature_flag_payload( "person-flag", "some-distinct-id", person_properties={"region": "USA"} @@ -1649,6 +1650,8 @@ def test_boolean_feature_flag_payload_decide(self, patch_decide): 300, ) self.assertEqual(patch_decide.call_count, 2) + self.assertEqual(patch_capture.call_count, 1) + patch_capture.reset_mock() @mock.patch("posthog.client.decide") def test_multivariate_feature_flag_payloads(self, patch_decide): @@ -2334,6 +2337,88 @@ def test_capture_is_called(self, patch_decide, patch_capture): disable_geoip=None, ) + @mock.patch.object(Client, "capture") + @mock.patch("posthog.client.decide") + def test_capture_is_called_in_get_feature_flag_payload(self, patch_decide, patch_capture): + patch_decide.return_value = { + "featureFlags": {"person-flag": True}, + "featureFlagPayloads": {"person-flag": 300}, + } + client = Client(api_key=FAKE_TEST_API_KEY, personal_api_key=FAKE_TEST_API_KEY) + + client.feature_flags = [ + { + "id": 1, + "name": "Beta Feature", + "key": "person-flag", + "is_simple_flag": False, + "active": True, + "filters": { + "groups": [ + { + "properties": [{"key": "region", "value": "USA"}], + "rollout_percentage": 100, + } + ], + }, + } + ] + + # Call get_feature_flag_payload with match_value=None to trigger get_feature_flag + client.get_feature_flag_payload( + key="person-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} + ) + + # Assert that capture was called once, with the correct parameters + self.assertEqual(patch_capture.call_count, 1) + patch_capture.assert_called_with( + "some-distinct-id", + "$feature_flag_called", + { + "$feature_flag": "person-flag", + "$feature_flag_response": True, + "$feature_flag_payload": 300, + "locally_evaluated": False, + "$feature/person-flag": True, + }, + groups={}, + disable_geoip=None, + ) + + # Reset mocks for further tests + patch_capture.reset_mock() + patch_decide.reset_mock() + + # Call get_feature_flag_payload again for the same user; capture should not be called again because we've already reported an event for this distinct_id + flag + client.get_feature_flag_payload( + key="person-flag", distinct_id="some-distinct-id", person_properties={"region": "USA", "name": "Aloha"} + ) + + self.assertEqual(patch_capture.call_count, 0) + patch_capture.reset_mock() + + # Call get_feature_flag_payload for a different user; capture should be called + client.get_feature_flag_payload( + key="person-flag", distinct_id="some-distinct-id2", person_properties={"region": "USA", "name": "Aloha"} + ) + + self.assertEqual(patch_capture.call_count, 1) + patch_capture.assert_called_with( + "some-distinct-id2", + "$feature_flag_called", + { + "$feature_flag": "person-flag", + "$feature_flag_response": True, + "$feature_flag_payload": 300, + "locally_evaluated": False, + "$feature/person-flag": True, + }, + groups={}, + disable_geoip=None, + ) + + patch_capture.reset_mock() + @mock.patch.object(Client, "capture") @mock.patch("posthog.client.decide") def test_disable_geoip_get_flag_capture_call(self, patch_decide, patch_capture):