8000 fix: Forced variation not available in experiment by ozayr-zaviar · Pull Request #292 · optimizely/ruby-sdk · GitHub
[go: up one dir, main page]

Skip to content

fix: Forced variation not available in experiment #292

New issue 8000

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

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 4 commits into from
Dec 21, 2021

Conversation

ozayr-zaviar
Copy link
Contributor
@ozayr-zaviar ozayr-zaviar commented Dec 14, 2021

Summary

  • Crash fixes and order set for variation by flag first then experiment

Test plan

  • All unit tests and FSC should pass

@ozayr-zaviar ozayr-zaviar changed the title Uzair/ruby fixes fix: fix: Forced variation not available in experiment Dec 14, 2021
@ozayr-zaviar ozayr-zaviar changed the title fix: fix: Forced variation not available in experiment fix: Forced variation not available in experiment Dec 14, 2021
@coveralls
Copy link
coveralls commented Dec 14, 2021

Coverage Status

Coverage decreased (-0.1%) to 99.453% when pulling 402763f on uzair/ruby-fixes into ac29ac1 on master.

@msohailhussain msohailhussain marked this pull request as ready for review December 14, 2021 18:50
@msohailhussain msohailhussain requested a review from a team as a code owner December 14, 2021 18:50
Copy link
Contributor
@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor
@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

A fix suggested. Also need a unit test covering the case.

Comment on lines 1103 to 1107
variation_id = if !variation
experiment_id != '' ? config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) : ''
else
variation ? variation['id'] : ''
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
variation_id = if !variation
experiment_id != '' ? config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) : ''
else
variation ? variation['id'] : ''
end
variation_id = variation ? variation['id'] : ''

If it's null from get_flag_variagtion(), then get_variation_id_from_key_by_experiment_id() will also return null.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is independent of flag_key, for a/b experiments this will return non-null and get_flag_variation will return null.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msohailhussain A good point! I still see this code can be refactored. Can consider the same comment for this:
https://github.com/optimizely/php-sdk/pull/237/files#r770133288

8000
@@ -298,8 +298,8 @@ def decide_for_keys(user_context, keys, decide_options = [])
decisions
end

def get_flag_variation_by_key(flag_key, variation_key)
project_config.get_variation_from_flag(flag_key, variation_key)
def get_flag_variation(flag_key, target_value, attribute)
Copy link
Contributor

Choose a reason for hiding this comment

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

please add description of this method.

Copy link
Contributor
@jaeopt jaeopt left a comment

Choose a reason fo 8000 r hiding this comment

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

The unit test looks good! A suggestion for refactoring.

Comment on lines 1103 to 1107
variation_id = if !variation
experiment_id != '' ? config.get_variation_id_from_key_by_experiment_id(experiment_id, variation_key) : ''
else
variation ? variation['id'] : ''
end
Copy link
Contributor

Choose a reason for hiding this comment

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

@msohailhussain A good point! I still see this code can be refactored. Can consider the same comment for this:
https://github.com/optimizely/php-sdk/pull/237/files#r770133288

Copy link
Contributor
@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

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

LGTM

@msohailhussain msohailhussain merged commit be870fe into master Dec 21, 2021
@msohailhussain msohailhussain deleted the uzair/ruby-fixes branch December 21, 2021 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0