From 5e0a5eca81e2b2ba66e0e32f1e67c2eba7274ddb Mon Sep 17 00:00:00 2001 From: esrakartalOpt Date: Tue, 21 Jul 2026 09:33:51 -0500 Subject: [PATCH] [FSSDK-12735] SDK Work: Implement holdout exclusion logic for Targeted Delivery rules --- lib/optimizely/decision_service.rb | 34 +++-- lib/optimizely/helpers/constants.rb | 6 + spec/decision_service_holdout_spec.rb | 197 ++++++++++++++++++++++++++ spec/spec_params.rb | 91 ++++++++++++ 4 files changed, 318 insertions(+), 10 deletions(-) diff --git a/lib/optimizely/decision_service.rb b/lib/optimizely/decision_service.rb index ef4d0448..4a2bc2c5 100644 --- a/lib/optimizely/decision_service.rb +++ b/lib/optimizely/decision_service.rb @@ -198,27 +198,35 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt # Check global holdouts first (flag level) — these apply to all rules across all flags holdouts = project_config.global_holdouts + global_holdout_decision = nil holdouts.each do |holdout| - holdout_decision = get_variation_for_holdout(holdout, user_context, project_config) - reasons.push(*holdout_decision.reasons) + holdout_result = get_variation_for_holdout(holdout, user_context, project_config) + reasons.push(*holdout_result.reasons) - next unless holdout_decision.decision + next unless holdout_result.decision message = "The user '#{user_id}' is bucketed into holdout '#{holdout['key']}' for feature flag '#{feature_flag['key']}'." @logger.log(Logger::INFO, message) reasons.push(message) - return DecisionResult.new(holdout_decision.decision, false, reasons) + + if holdout['exclude_targeted_deliveries'] + global_holdout_decision = holdout_result.decision + else + return DecisionResult.new(holdout_result.decision, false, reasons) + end end - # Check if the feature flag has an experiment and the user is bucketed into that experiment - experiment_decision = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options) - reasons.push(*experiment_decision.reasons) + unless global_holdout_decision + # Check if the feature flag has an experiment and the user is bucketed into that experiment + experiment_decision = get_variation_for_feature_experiment(project_config, feature_flag, user_context, user_profile_tracker, decide_options) + reasons.push(*experiment_decision.reasons) - return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision + return DecisionResult.new(experiment_decision.decision, experiment_decision.error, reasons) if experiment_decision.decision - # If there's an error (e.g., CMAB error), return immediately without falling back to rollout - return DecisionResult.new(nil, experiment_decision.error, reasons) if experiment_decision.error + # If there's an error (e.g., CMAB error), return immediately without falling back to rollout + return DecisionResult.new(nil, experiment_decision.error, reasons) if experiment_decision.error + end # Check if the feature flag has a rollout and the user is bucketed into that rollout rollout_decision = get_variation_for_feature_rollout(project_config, feature_flag, user_context) @@ -237,6 +245,10 @@ def get_decision_for_flag(feature_flag, user_context, project_config, decide_opt DecisionResult.new(rollout_decision.decision, rollout_decision.error, reasons) else + if global_holdout_decision + return DecisionResult.new(global_holdout_decision, false, reasons) + end + message = "The user '#{user_id}' is not bucketed into a rollout for feature flag '#{feature_flag['key']}'." @logger.log(Logger::INFO, message) DecisionResult.new(nil, false, reasons) @@ -493,6 +505,8 @@ def get_variation_from_delivery_rule(project_config, flag_key, rules, rule_index # Step 2: Local holdout check local_holdouts = project_config.get_holdouts_for_rule(rule['id']) local_holdouts.each do |holdout| + next if holdout['exclude_targeted_deliveries'] + holdout_decision = get_variation_for_holdout(holdout, user_context, project_config) reasons.push(*holdout_decision.reasons) return [holdout_decision.decision, nil, skip_to_everyone_else, reasons] if holdout_decision.decision diff --git a/lib/optimizely/helpers/constants.rb b/lib/optimizely/helpers/constants.rb index b23955b1..88b06833 100644 --- a/lib/optimizely/helpers/constants.rb +++ b/lib/optimizely/helpers/constants.rb @@ -349,6 +349,9 @@ module Constants }, 'includedRules' => { 'type' => %w[array null] + }, + 'exclude_targeted_deliveries' => { + 'type' => 'boolean' } } } @@ -369,6 +372,9 @@ module Constants }, 'includedRules' => { 'type' => %w[array null] + }, + 'exclude_targeted_deliveries' => { + 'type' => 'boolean' } } } diff --git a/spec/decision_service_holdout_spec.rb b/spec/decision_service_holdout_spec.rb index 370fd4b8..ef198983 100644 --- a/spec/decision_service_holdout_spec.rb +++ b/spec/decision_service_holdout_spec.rb @@ -1032,5 +1032,202 @@ expect(result.variation_id).not_to eq('local_var_1') # must NOT be holdout variation end end + + describe 'exclude_targeted_deliveries' do + let(:config_with_etd) do + Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_EXCLUDE_TARGETED_JSON, + spy_logger, + error_handler + ) + end + + let(:project_with_etd) do + Optimizely::Project.new( + datafile: OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_EXCLUDE_TARGETED_JSON, + logger: spy_logger, + error_handler: error_handler + ) + end + + let(:decision_service_etd) do + Optimizely::DecisionService.new(spy_logger, spy_cmab_service, spy_user_profile_service) + end + + after(:example) do + project_with_etd&.close + end + + describe 'global holdout with exclude_targeted_deliveries=true' do + it 'blocks experiment rules and returns holdout decision' do + user_ctx = project_with_etd.create_user_context('some-user', {}) + feature_flag = config_with_etd.feature_flag_key_map['boolean_feature'] + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) + .and_return([{ 'id' => 'var_etd_1', 'key' => 'control', 'featureEnabled' => false }, []]) + + result = decision_service_etd.get_decision_for_flag( + feature_flag, user_ctx, config_with_etd, [], nil + ) + + expect(result.decision).not_to be_nil + expect(result.decision.source).to eq('holdout') + expect(result.decision.variation['id']).to eq('var_etd_1') + end + + it 'allows delivery/rollout rules to proceed normally' do + feature_flag = config_with_etd.feature_flag_key_map['string_single_variable_feature'] + user_ctx = project_with_etd.create_user_context('some-user', {}) + + holdout_variation = { 'id' => 'var_etd_1', 'key' => 'control', 'featureEnabled' => false } + rollout_variation = { 'id' => '177780', 'key' => '177780', 'featureEnabled' => true, 'variables' => [] } + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) do |_bucketer, _config, exp, *_rest| + if exp['id'] == 'holdout_etd_global' + [holdout_variation, []] + elsif exp['id'] == 'holdout_no_etd_global' + [nil, []] + else + [rollout_variation, []] + end + end + allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?) + .and_return([true, []]) + + result = decision_service_etd.get_decision_for_flag( + feature_flag, user_ctx, config_with_etd, [], nil + ) + + expect(result.decision).not_to be_nil + expect(result.decision.source).to eq('rollout') + end + + it 'returns holdout decision when no rollout matches' do + feature_flag = config_with_etd.feature_flag_key_map['boolean_feature'] + user_ctx = project_with_etd.create_user_context('some-user', {}) + + holdout_variation = { 'id' => 'var_etd_1', 'key' => 'control', 'featureEnabled' => false } + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) do |_bucketer, _config, exp, *_rest| + if exp['id'] == 'holdout_etd_global' + [holdout_variation, []] + else + [nil, []] + end + end + + result = decision_service_etd.get_decision_for_flag( + feature_flag, user_ctx, config_with_etd, [], nil + ) + + expect(result.decision).not_to be_nil + expect(result.decision.source).to eq('holdout') + end + end + + describe 'global holdout without exclude_targeted_deliveries' do + it 'returns holdout immediately blocking all rules' do + user_ctx = project_with_etd.create_user_context('some-user', {}) + feature_flag = config_with_etd.feature_flag_key_map['boolean_feature'] + + holdout_etd_var = { 'id' => 'var_etd_1', 'key' => 'control', 'featureEnabled' => false } + holdout_no_etd_var = { 'id' => 'var_no_etd_1', 'key' => 'control', 'featureEnabled' => false } + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) do |_bucketer, _config, exp, *_rest| + if exp['id'] == 'holdout_etd_global' + [nil, []] + elsif exp['id'] == 'holdout_no_etd_global' + [holdout_no_etd_var, []] + else + raise 'Should not evaluate experiment rules when holdout blocks' + end + end + + result = decision_service_etd.get_decision_for_flag( + feature_flag, user_ctx, config_with_etd, [], nil + ) + + expect(result.decision).not_to be_nil + expect(result.decision.source).to eq('holdout') + expect(result.decision.variation['id']).to eq('var_no_etd_1') + end + end + + describe 'local holdout with exclude_targeted_deliveries=true on delivery rule' do + it 'skips holdout and serves delivery variation' do + user_ctx = project_with_etd.create_user_context('some-user', {}) + feature_flag = config_with_etd.feature_flag_key_map['string_single_variable_feature'] + rollout = config_with_etd.get_rollout_from_id(feature_flag['rolloutId']) + rules = rollout['experiments'] + rule_index = 0 + + rollout_variation = rules[rule_index]['variations'].first + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) + .and_return([rollout_variation, []]) + allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?) + .and_return([true, []]) + + _holdout_decision, variation, _skip, _reasons = decision_service_etd.get_variation_from_delivery_rule( + config_with_etd, feature_flag['key'], rules, rule_index, user_ctx + ) + + expect(variation).not_to be_nil + expect(variation['id']).to eq(rollout_variation['id']) + end + end + + describe 'local holdout without exclude_targeted_deliveries on experiment rule' do + it 'applies holdout and returns holdout variation' do + user_ctx = project_with_etd.create_user_context('some-user', {}) + experiment = config_with_etd.experiment_id_map['122227'] + user_profile_tracker = Optimizely::UserProfileTracker.new('some-user', nil, spy_logger) + + holdout_variation = { 'id' => 'local_var_no_etd_1', 'key' => 'holdout', 'featureEnabled' => false } + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) + .and_return([holdout_variation, []]) + allow(Optimizely::Audience).to receive(:user_meets_audience_conditions?) + .and_return([true, []]) + + result = decision_service_etd.get_variation_from_experiment_rule( + config_with_etd, 'boolean_feature', experiment, user_ctx, user_profile_tracker + ) + + expect(result.holdout_decision).not_to be_nil + expect(result.holdout_decision.source).to eq('holdout') + end + end + + describe 'backward compatibility' do + it 'treats absent exclude_targeted_deliveries as false' do + config_no_etd = Optimizely::DatafileProjectConfig.new( + OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + spy_logger, + error_handler + ) + project_no_etd = Optimizely::Project.new( + datafile: OptimizelySpec::CONFIG_BODY_WITH_HOLDOUTS_JSON, + logger: spy_logger, + error_handler: error_handler + ) + + user_ctx = project_no_etd.create_user_context('some-user', {}) + feature_flag = config_no_etd.feature_flag_key_map['boolean_feature'] + + allow_any_instance_of(Optimizely::Bucketer).to receive(:bucket) + .and_return([{ 'id' => 'var_1', 'key' => 'control', 'featureEnabled' => true }, []]) + + result = decision_service_etd.get_decision_for_flag( + feature_flag, user_ctx, config_no_etd, [], nil + ) + + expect(result.decision).not_to be_nil + expect(result.decision.source).to eq('holdout') + + project_no_etd.close + end + end + end end end diff --git a/spec/spec_params.rb b/spec/spec_params.rb index 1249f648..62f75b21 100644 --- a/spec/spec_params.rb +++ b/spec/spec_params.rb @@ -2118,6 +2118,97 @@ module OptimizelySpec CONFIG_BODY_WITH_GLOBAL_HOLDOUTS_ONLY_JSON = JSON.dump(CONFIG_BODY_WITH_GLOBAL_HOLDOUTS_ONLY).freeze + CONFIG_BODY_WITH_HOLDOUTS_EXCLUDE_TARGETED = VALID_CONFIG_BODY.merge( + { + 'holdouts' => [ + { + 'id' => 'holdout_etd_global', + 'key' => 'global_holdout_exclude_td', + 'status' => 'Running', + 'audiences' => [], + 'exclude_targeted_deliveries' => true, + 'variations' => [ + { + 'id' => 'var_etd_1', + 'key' => 'control', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'var_etd_1', + 'endOfRange' => 10_000 + } + ] + }, + { + 'id' => 'holdout_no_etd_global', + 'key' => 'global_holdout_no_exclude_td', + 'status' => 'Running', + 'audiences' => [], + 'variations' => [ + { + 'id' => 'var_no_etd_1', + 'key' => 'control', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'var_no_etd_1', + 'endOfRange' => 10_000 + } + ] + } + ], + 'localHoldouts' => [ + { + 'id' => 'holdout_etd_local', + 'key' => 'local_holdout_exclude_td', + 'status' => 'Running', + 'audiences' => [], + 'exclude_targeted_deliveries' => true, + 'includedRules' => ['177774'], + 'variations' => [ + { + 'id' => 'local_var_etd_1', + 'key' => 'holdout', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'local_var_etd_1', + 'endOfRange' => 10_000 + } + ] + }, + { + 'id' => 'holdout_no_etd_local', + 'key' => 'local_holdout_no_exclude_td', + 'status' => 'Running', + 'audiences' => [], + 'includedRules' => ['122227'], + 'variations' => [ + { + 'id' => 'local_var_no_etd_1', + 'key' => 'holdout', + 'featureEnabled' => false + } + ], + 'trafficAllocation' => [ + { + 'entityId' => 'local_var_no_etd_1', + 'endOfRange' => 10_000 + } + ] + } + ] + } + ).freeze + + CONFIG_BODY_WITH_HOLDOUTS_EXCLUDE_TARGETED_JSON = JSON.dump(CONFIG_BODY_WITH_HOLDOUTS_EXCLUDE_TARGETED).freeze + def self.deep_clone(obj) obj.dup.tap do |new_obj| case new_obj