Skip to content

refactor: moved validated forced decision to decision service. #293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 7, 2022
Merged

refactor: moved validated forced decision to decision service. #293

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c971e10
moved validated forced decision to decision service
ozayr-zaviar Dec 22, 2021
6cad8da
headers updated
ozayr-zaviar Jan 4, 2022
9b85bc2
find_forced_decision updated to get_forced_decision
ozayr-zaviar Jan 6, 2022
4e43f2a
added project config
ozayr-zaviar Jan 6, 2022
27bf929
function name corrected
ozayr-zaviar Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/optimizely.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2016-2021, Optimizely and contributors
# Copyright 2016-2022, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -199,7 +199,7 @@ def decide(user_context, key, decide_options = [])
experiment = nil
decision_source = Optimizely::DecisionService::DECISION_SOURCES['ROLLOUT']
context = Optimizely::OptimizelyUserContext::OptimizelyDecisionContext.new(key, nil)
variation, reasons_received = user_context.find_validated_forced_decision(context)
variation, reasons_received = @decision_service.validated_forced_decision(context, user_context)
Copy link
Contributor

Choose a reason for hiding this comment

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

please keep the method name find_validated_forced_decision

reasons.push(*reasons_received)

if variation
Expand Down
28 changes: 25 additions & 3 deletions lib/optimizely/decision_service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2017-2021, Optimizely and contributors
# Copyright 2017-2022, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -265,7 +265,7 @@ def get_variation_from_experiment_rule(project_config, flag_key, rule, user, opt
reasons = []

context = Optimizely::OptimizelyUserContext::OptimizelyDecisionContext.new(flag_key, rule['key'])
variation, forced_reasons = user.find_validated_forced_decision(context)
variation, forced_reasons = validated_forced_decision(context, user)
reasons.push(*forced_reasons)

return [variation['id'], reasons] if variation
Expand All @@ -290,7 +290,7 @@ def get_variation_from_delivery_rule(project_config, flag_key, rules, rule_index
skip_to_everyone_else = false
rule = rules[rule_index]
context = Optimizely::OptimizelyUserContext::OptimizelyDecisionContext.new(flag_key, rule['key'])
variation, forced_reasons = user.find_validated_forced_decision(context)
variation, forced_reasons = validated_forced_decision(context, user)
reasons.push(*forced_reasons)

return [variation, skip_to_everyone_else, reasons] if variation
Expand Down Expand Up @@ -417,6 +417,28 @@ def get_forced_variation(project_config, experiment_key, user_id)
[variation, decide_reasons]
end

def validated_forced_decision(context, user_context)
decision = user_context.find_forced_decision(context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove find_forced_decision and use get_forced_decision. Since we remove sanity checking, they're now identical.

flag_key = context[:flag_key]
rule_key = context[:rule_key]
Comment on lines +422 to +423
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move this code inside the if block since it is only used there?

variation_key = decision ? decision[:variation_key] : decision
reasons = []
target = rule_key ? "flag (#{flag_key}), rule (#{rule_key})" : "flag (#{flag_key})"
if variation_key
variation = user_context.optimizely_client.get_flag_variation(flag_key, variation_key, 'key')
Copy link
Contributor

@jaeopt jaeopt Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know what happened to my previous comments on this line. github bug? :)
We need to change this line to same as other SDKs - pass 'config' as a parameter and move get_flag_variation to config, so we can remove dependency to user_context.optimizely_client here.

if variation
reason = "Variation (#{variation_key}) is mapped to #{target} and user (#{user_context.user_id}) in the forced decision map."
reasons.push(reason)
return variation, reasons
Comment on lines +431 to +432
Copy link
Contributor

Choose a reason for hiding this comment

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

this code can be moved to line 436, that way we won't have reasons.push(reason) twice.

else
reason = "Invalid variation is mapped to #{target} and user (#{user_context.user_id}) in the forced decision map."
reasons.push(reason)
end
end

[nil, reasons]
end

private

def get_whitelisted_variation_id(project_config, experiment_id, user_id)
Expand Down
25 changes: 2 additions & 23 deletions lib/optimizely/optimizely_user_context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# frozen_string_literal: true

#
# Copyright 2020, Optimizely and contributors
# Copyright 2020-2022, Optimizely and contributors
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
Expand All @@ -26,6 +26,7 @@ class OptimizelyUserContext
attr_reader :forced_decisions
attr_reader :OptimizelyDecisionContext
attr_reader :OptimizelyForcedDecision
attr_reader :optimizely_client
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed when we are only removing a method here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also update the Copyright header with 2020-2021


OptimizelyDecisionContext = Struct.new(:flag_key, :rule_key)
OptimizelyForcedDecision = Struct.new(:variation_key)
Expand Down Expand Up @@ -156,28 +157,6 @@ def remove_all_forced_decisions
true
end

def find_validated_forced_decision(context)
decision = find_forced_decision(context)
flag_key = context[:flag_key]
rule_key = context[:rule_key]
variation_key = decision ? decision[:variation_key] : decision
reasons = []
target = rule_key ? "flag (#{flag_key}), rule (#{rule_key})" : "flag (#{flag_key})"
if variation_key
variation = @optimizely_client.get_flag_variation(flag_key, variation_key, 'key')
if variation
reason = "Variation (#{variation_key}) is mapped to #{target} and user (#{@user_id}) in the forced decision map."
reasons.push(reason)
return variation, reasons
else
reason = "Invalid variation is mapped to #{target} and user (#{@user_id}) in the forced decision map."
reasons.push(reason)
end
end

[nil, reasons]
end

# Track an event
#
# @param event_key - Event key representing the event which needs to be recorded.
Expand Down