diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index be2be2c5..1f59b67b 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -733,30 +733,7 @@ def get_decision_for_flag( reasons = decide_reasons.copy() if decide_reasons else [] user_id = user_context.user_id - # Check holdouts - holdouts = project_config.get_holdouts_for_flag(feature_flag.key) - for holdout in holdouts: - holdout_decision = self.get_variation_for_holdout(holdout, user_context, project_config) - reasons.extend(holdout_decision['reasons']) - - decision = holdout_decision['decision'] - # Check if user was bucketed into holdout (has a variation) - if decision.variation is None: - continue - - message = ( - f"The user '{user_id}' is bucketed into holdout '{holdout.key}' " - f"for feature flag '{feature_flag.key}'." - ) - self.logger.info(message) - reasons.append(message) - return { - 'decision': holdout_decision['decision'], - 'error': False, - 'reasons': reasons - } - - # If no holdout decision, check experiments then rollouts + # Check experiments then rollouts if feature_flag.experimentIds: for experiment_id in feature_flag.experimentIds: experiment = project_config.get_experiment_from_id(experiment_id) diff --git a/optimizely/entities.py b/optimizely/entities.py index 589ca984..f67dee89 100644 --- a/optimizely/entities.py +++ b/optimizely/entities.py @@ -222,8 +222,6 @@ def __init__( variations: list[VariationDict], trafficAllocation: list[TrafficAllocation], audienceIds: list[str], - includedFlags: Optional[list[str]] = None, - excludedFlags: Optional[list[str]] = None, audienceConditions: Optional[Sequence[str | list[str]]] = None, **kwargs: Any ): @@ -234,8 +232,6 @@ def __init__( self.trafficAllocation = trafficAllocation self.audienceIds = audienceIds self.audienceConditions = audienceConditions - self.includedFlags = includedFlags or [] - self.excludedFlags = excludedFlags or [] def get_audience_conditions_or_ids(self) -> Sequence[str | list[str]]: """Returns audienceConditions if present, otherwise audienceIds. diff --git a/optimizely/helpers/types.py b/optimizely/helpers/types.py index 7076dc4d..6b704c36 100644 --- a/optimizely/helpers/types.py +++ b/optimizely/helpers/types.py @@ -130,5 +130,3 @@ class HoldoutDict(ExperimentDict): Extends ExperimentDict with holdout-specific properties. """ holdoutStatus: HoldoutStatus - includedFlags: list[str] - excludedFlags: list[str] diff --git a/optimizely/project_config.py b/optimizely/project_config.py index 5b752538..0397cd4b 100644 --- a/optimizely/project_config.py +++ b/optimizely/project_config.py @@ -93,10 +93,6 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdouts_data: list[types.HoldoutDict] = config.get('holdouts', []) self.holdouts: list[entities.Holdout] = [] self.holdout_id_map: dict[str, entities.Holdout] = {} - self.global_holdouts: list[entities.Holdout] = [] - self.included_holdouts: dict[str, list[entities.Holdout]] = {} - self.excluded_holdouts: dict[str, list[entities.Holdout]] = {} - self.flag_holdouts_map: dict[str, list[entities.Holdout]] = {} # Convert holdout dicts to Holdout entities for holdout_data in holdouts_data: @@ -104,33 +100,13 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): holdout = entities.Holdout(**holdout_data) self.holdouts.append(holdout) - # Only process Running holdouts but doing it here for efficiency like the original Python implementation) + # Only process Running holdouts if not holdout.is_activated: continue # Map by ID for quick lookup self.holdout_id_map[holdout.id] = holdout - # Categorize as global vs flag-specific - # Global holdouts: apply to all flags unless explicitly excluded - # Flag-specific holdouts: only apply to explicitly included flags - if not holdout.includedFlags: - # This is a global holdout - self.global_holdouts.append(holdout) - - # Track which flags this global holdout excludes - if holdout.excludedFlags: - for flag_id in holdout.excludedFlags: - if flag_id not in self.excluded_holdouts: - self.excluded_holdouts[flag_id] = [] - self.excluded_holdouts[flag_id].append(holdout) - else: - # This holdout applies to specific flags only - for flag_id in holdout.includedFlags: - if flag_id not in self.included_holdouts: - self.included_holdouts[flag_id] = [] - self.included_holdouts[flag_id].append(holdout) - # Utility maps for quick lookup self.group_id_map: dict[str, entities.Group] = self._generate_key_map(self.groups, 'id', entities.Group) self.experiment_id_map: dict[str, entities.Experiment] = self._generate_key_map( @@ -263,22 +239,6 @@ def __init__(self, datafile: str | bytes, logger: Logger, error_handler: Any): everyone_else_variation.variables, 'id', entities.Variation.VariableUsage ) - flag_id = feature.id - applicable_holdouts: list[entities.Holdout] = [] - - # Add global holdouts first, excluding any that are explicitly excluded for this flag - excluded_holdouts = self.excluded_holdouts.get(flag_id, []) - for holdout in self.global_holdouts: - if holdout not in excluded_holdouts: - applicable_holdouts.append(holdout) - - # Add flag-specific local holdouts AFTER global holdouts - if flag_id in self.included_holdouts: - applicable_holdouts.extend(self.included_holdouts[flag_id]) - - if applicable_holdouts: - self.flag_holdouts_map[feature.key] = applicable_holdouts - rollout = None if len(feature.rolloutId) == 0 else self.rollout_id_map[feature.rolloutId] if rollout: for exp in rollout.experiments: @@ -912,20 +872,6 @@ def get_flag_variation( return None - def get_holdouts_for_flag(self, flag_key: str) -> list[entities.Holdout]: - """ Helper method to get holdouts from an applied feature flag. - - Args: - flag_key: Key of the feature flag. - - Returns: - The holdouts that apply for a specific flag as Holdout entity objects. - """ - if not self.holdouts: - return [] - - return self.flag_holdouts_map.get(flag_key, []) - def get_holdout(self, holdout_id: str) -> Optional[entities.Holdout]: """ Helper method to get holdout from holdout ID. diff --git a/tests/test_bucketing_holdout.py b/tests/test_bucketing_holdout.py index e3d9bcb3..8a5e7c78 100644 --- a/tests/test_bucketing_holdout.py +++ b/tests/test_bucketing_holdout.py @@ -62,8 +62,6 @@ def setUp(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -92,8 +90,6 @@ def setUp(self): 'id': 'holdout_empty_1', 'key': 'empty_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [], 'trafficAllocation': [] diff --git a/tests/test_config.py b/tests/test_config.py index c21f9b34..29b1c49e 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1384,10 +1384,6 @@ def setUp(self): # Create config with holdouts config_body_with_holdouts = self.config_dict_with_features.copy() - # Use correct feature flag IDs from the datafile - boolean_feature_id = '91111' # boolean_single_variable_feature - multi_variate_feature_id = '91114' # test_feature_in_experiment_and_rollout - config_body_with_holdouts['holdouts'] = [ { 'id': 'holdout_1', @@ -1395,9 +1391,7 @@ def setUp(self): 'status': 'Running', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [], - 'excludedFlags': [boolean_feature_id] + 'audienceIds': [] }, { 'id': 'holdout_2', @@ -1405,9 +1399,7 @@ def setUp(self): 'status': 'Running', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [multi_variate_feature_id], - 'excludedFlags': [] + 'audienceIds': [] }, { 'id': 'holdout_3', @@ -1415,9 +1407,7 @@ def setUp(self): 'status': 'Inactive', 'variations': [], 'trafficAllocation': [], - 'audienceIds': [], - 'includedFlags': [boolean_feature_id], - 'excludedFlags': [] + 'audienceIds': [] } ] @@ -1425,55 +1415,6 @@ def setUp(self): opt_obj = optimizely.Optimizely(self.config_json_with_holdouts) self.config_with_holdouts = opt_obj.config_manager.get_config() - def test_get_holdouts_for_flag__non_existent_flag(self): - """ Test that get_holdouts_for_flag returns empty array for non-existent flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('non_existent_flag') - self.assertEqual([], holdouts) - - def test_get_holdouts_for_flag__returns_global_and_specific_holdouts(self): - """ Test that get_holdouts_for_flag returns global holdouts that do not exclude the flag - and specific holdouts that include the flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - self.assertEqual(2, len(holdouts)) - - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNotNone(global_holdout) - self.assertEqual('holdout_1', global_holdout['id']) - - specific_holdout = next((h for h in holdouts if h['key'] == 'specific_holdout'), None) - self.assertIsNotNone(specific_holdout) - self.assertEqual('holdout_2', specific_holdout['id']) - - def test_get_holdouts_for_flag__excludes_global_holdouts_for_excluded_flags(self): - """ Test that get_holdouts_for_flag does not return global holdouts that exclude the flag. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('boolean_single_variable_feature') - self.assertEqual(0, len(holdouts)) - - global_holdout = next((h for h in holdouts if h['key'] == 'global_holdout'), None) - self.assertIsNone(global_holdout) - - def test_get_holdouts_for_flag__caches_results(self): - """ Test that get_holdouts_for_flag caches results for subsequent calls. """ - - holdouts1 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - holdouts2 = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment_and_rollout') - - # Should be the same object (cached) - self.assertIs(holdouts1, holdouts2) - self.assertEqual(2, len(holdouts1)) - - def test_get_holdouts_for_flag__returns_only_global_for_non_targeted_flags(self): - """ Test that get_holdouts_for_flag returns only global holdouts for flags not specifically targeted. """ - - holdouts = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_rollout') - - # Should only include global holdout (not excluded and no specific targeting) - self.assertEqual(1, len(holdouts)) - self.assertEqual('global_holdout', holdouts[0]['key']) - def test_get_holdout__returns_holdout_for_valid_id(self): """ Test that get_holdout returns holdout when valid ID is provided. """ @@ -1516,36 +1457,6 @@ def test_get_holdout__does_not_log_when_found(self): self.assertIsNotNone(result) mock_logger.error.assert_not_called() - def test_holdout_initialization__categorizes_holdouts_properly(self): - """ Test that holdouts are properly categorized during initialization. """ - - self.assertIn('holdout_1', self.config_with_holdouts.holdout_id_map) - self.assertIn('holdout_2', self.config_with_holdouts.holdout_id_map) - # Check if a holdout with ID 'holdout_1' exists in global_holdouts - holdout_ids_in_global = [h.id for h in self.config_with_holdouts.global_holdouts] - self.assertIn('holdout_1', holdout_ids_in_global) - - # Use correct feature flag IDs - boolean_feature_id = '91111' - multi_variate_feature_id = '91114' - - self.assertIn(multi_variate_feature_id, self.config_with_holdouts.included_holdouts) - self.assertTrue(len(self.config_with_holdouts.included_holdouts[multi_variate_feature_id]) > 0) - self.assertNotIn(boolean_feature_id, self.config_with_holdouts.included_holdouts) - - self.assertIn(boolean_feature_id, self.config_with_holdouts.excluded_holdouts) - self.assertTrue(len(self.config_with_holdouts.excluded_holdouts[boolean_feature_id]) > 0) - - def test_holdout_initialization__only_processes_running_holdouts(self): - """ Test that only running holdouts are processed during initialization. """ - - self.assertNotIn('holdout_3', self.config_with_holdouts.holdout_id_map) - self.assertNotIn('holdout_3', self.config_with_holdouts.global_holdouts) - - boolean_feature_id = '91111' - included_for_boolean = self.config_with_holdouts.included_holdouts.get(boolean_feature_id) - self.assertIsNone(included_for_boolean) - class FeatureRolloutConfigTest(base.BaseTest): """Tests for Feature Rollout support in ProjectConfig parsing.""" diff --git a/tests/test_decision_service_holdout.py b/tests/test_decision_service_holdout.py index e286ba5c..74cbd9cb 100644 --- a/tests/test_decision_service_holdout.py +++ b/tests/test_decision_service_holdout.py @@ -38,16 +38,11 @@ def setUp(self): # Create a config dict with holdouts and feature flags config_dict_with_holdouts = self.config_dict_with_features.copy() - # Get feature flag ID for test_feature_in_experiment - test_feature_id = '91111' - config_dict_with_holdouts['holdouts'] = [ { 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -76,8 +71,6 @@ def setUp(self): 'id': 'holdout_2', 'key': 'excluded_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [test_feature_id], 'audienceIds': [], 'variations': [ { @@ -268,60 +261,6 @@ def test_holdout_populates_decision_reasons(self): # get_variation_for_feature with holdouts tests - def test_user_bucketed_into_holdout_returns_before_experiments(self): - """ - When user is bucketed into holdout, should return holdout decision - before checking experiments or rollouts. - """ - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - self.assertIsNotNone(feature_flag) - - user_context = self.opt_obj.create_user_context('testUserId', {}) - - # Mock get_holdouts_for_flag to return holdouts - holdout = self.config_with_holdouts.holdouts[0] if self.config_with_holdouts.holdouts else None - self.assertIsNotNone(holdout) - - holdout_variation = holdout.variations[0] - - # Create a holdout decision - mock_holdout_decision = decision_service.Decision( - experiment=holdout, - variation=holdout_variation, - source=enums.DecisionSources.HOLDOUT, - cmab_uuid=None - ) - - mock_holdout_result = { - 'decision': mock_holdout_decision, - 'error': False, - 'reasons': [] - } - - # Mock get_holdouts_for_flag to return holdouts so the holdout path is taken - with mock.patch.object( - self.config_with_holdouts, - 'get_holdouts_for_flag', - return_value=[holdout] - ): - with mock.patch.object( - self.opt_obj.decision_service, - 'get_variation_for_holdout', - return_value=mock_holdout_result - ): - decision_result = self.opt_obj.decision_service.get_variation_for_feature( - self.config_with_holdouts, - feature_flag, - user_context - ) - - self.assertIsNotNone(decision_result) - - # Decision should be valid and from holdout - decision = decision_result['decision'] - self.assertEqual(decision.source, enums.DecisionSources.HOLDOUT) - self.assertIsNotNone(decision.variation) - def test_no_holdout_decision_falls_through_to_experiment_and_rollout(self): """When holdout returns no decision, should fall through to experiment and rollout evaluation.""" feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') @@ -376,42 +315,6 @@ def test_evaluates_holdouts_before_experiments(self): self.assertIsNotNone(decision_result) - def test_evaluates_global_holdouts_for_all_flags(self): - """Should evaluate global holdouts for all flags.""" - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - self.assertIsNotNone(feature_flag) - - # Get global holdouts - global_holdouts = [ - h for h in self.config_with_holdouts.holdouts - if not h.includedFlags or len(h.includedFlags) == 0 - ] - - if global_holdouts: - user_context = self.opt_obj.create_user_context('testUserId', {}) - - result = self.decision_service_with_holdouts.get_variations_for_feature_list( - self.config_with_holdouts, - [feature_flag], - user_context, - [] - ) - - self.assertIsNotNone(result) - self.assertIsInstance(result, list) - - def test_respects_included_and_excluded_flags_configuration(self): - """Should respect included and excluded flags configuration.""" - feature_flag = self.config_with_holdouts.get_feature_from_key('test_feature_in_experiment') - - if feature_flag: - # Get holdouts for this flag - holdouts_for_flag = self.config_with_holdouts.get_holdouts_for_flag('test_feature_in_experiment') - - # Should not include holdouts that exclude this flag - excluded_holdout = next((h for h in holdouts_for_flag if h.key == 'excluded_holdout'), None) - self.assertIsNone(excluded_holdout) - # Holdout logging and error handling tests def test_logs_when_holdout_evaluation_starts(self): @@ -618,8 +521,6 @@ def test_decide_with_holdout_sends_impression_event(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -727,8 +628,6 @@ def test_decide_with_holdout_does_not_send_impression_when_disabled(self): 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -801,8 +700,6 @@ def notification_callback(notification_type, user_id, user_attributes, decision_ 'id': 'holdout_1', 'key': 'test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -930,8 +827,6 @@ def test_decide_all_with_global_holdout(self): 'id': 'global_holdout', 'key': 'global_test_holdout', 'status': 'Running', - 'includedFlags': [], # Global - applies to all flags - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -977,8 +872,6 @@ def test_decide_all_with_included_flags(self): 'id': 'included_holdout', 'key': 'specific_holdout', 'status': 'Running', - 'includedFlags': [feature1_id], # Only applies to feature1 - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1022,8 +915,6 @@ def test_decide_all_with_excluded_flags(self): 'id': 'excluded_holdout', 'key': 'global_except_one', 'status': 'Running', - 'includedFlags': [], # Global - 'excludedFlags': [feature1_id], # Except feature1 'audienceIds': [], 'variations': [ { @@ -1068,8 +959,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'global_holdout', 'key': 'global_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1091,8 +980,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'specific_holdout', 'key': 'specific_holdout', 'status': 'Running', - 'includedFlags': [feature2_id], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1114,8 +1001,6 @@ def test_decide_all_with_multiple_holdouts(self): 'id': 'excluded_holdout', 'key': 'excluded_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [feature1_id], 'audienceIds': [], 'variations': [ { @@ -1154,8 +1039,6 @@ def test_decide_all_with_enabled_flags_only_option(self): 'id': 'disabled_holdout', 'key': 'disabled_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1203,8 +1086,6 @@ def test_holdout_impression_event_has_correct_metadata(self): 'id': 'metadata_holdout', 'key': 'metadata_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1257,8 +1138,6 @@ def test_holdout_impression_respects_send_flag_decisions_false(self): 'id': 'send_flag_holdout', 'key': 'send_flag_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1307,8 +1186,6 @@ def test_holdout_not_running_does_not_apply(self): 'id': 'draft_holdout', 'key': 'draft_holdout', 'status': 'Draft', # Not Running - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1347,8 +1224,6 @@ def test_holdout_concluded_status_does_not_apply(self): 'id': 'concluded_holdout', 'key': 'concluded_holdout', 'status': 'Concluded', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1387,8 +1262,6 @@ def test_holdout_archived_status_does_not_apply(self): 'id': 'archived_holdout', 'key': 'archived_holdout', 'status': 'Archived', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': [], 'variations': [ { @@ -1431,8 +1304,6 @@ def test_holdout_with_audience_match(self): 'id': 'audience_holdout', 'key': 'audience_test_holdout', 'status': 'Running', - 'includedFlags': [], - 'excludedFlags': [], 'audienceIds': ['11154'], # Requires browser_type=chrome 'variations': [ {