Skip to content

fix(set_forced_variation): Treats empty variation key as invalid and does not reset forced variation. #149

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 1 commit into from
Nov 13, 2018
Merged

fix(set_forced_variation): Treats empty variation key as invalid and does not reset forced variation. #149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion optimizely/project_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

from .helpers import condition as condition_helper
from .helpers import enums
from .helpers import validator
from . import entities
from . import exceptions

Expand Down Expand Up @@ -499,7 +500,7 @@ def set_forced_variation(self, experiment_key, user_id, variation_key):
return False

experiment_id = experiment.id
if not variation_key:
if variation_key is None:
if user_id in self.forced_variation_map:
experiment_to_variation_map = self.forced_variation_map.get(user_id)
if experiment_id in experiment_to_variation_map:
Expand All @@ -517,6 +518,10 @@ def set_forced_variation(self, experiment_key, user_id, variation_key):
self.logger.debug('Nothing to remove. User "%s" does not exist in the forced variation map.' % user_id)
return True

if not validator.is_non_empty_string(variation_key):
self.logger.debug('Variation key is invalid.')
return False

forced_variation = self.get_variation_from_key(experiment_key, variation_key)
if not forced_variation:
# The invalid variation key will be logged inside this call.
Expand Down
5 changes: 4 additions & 1 deletion tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,9 +1055,12 @@ def test_set_forced_variation__invalid_variation_key(self):

self.assertFalse(self.project_config.set_forced_variation('test_experiment', 'test_user',
'variation_not_in_datafile'))
self.assertTrue(self.project_config.set_forced_variation('test_experiment', 'test_user', ''))
self.assertTrue(self.project_config.set_forced_variation('test_experiment', 'test_user', None))

with mock.patch.object(self.project_config, 'logger') as mock_config_logging:
self.assertIs(self.project_config.set_forced_variation('test_experiment', 'test_user', ''), False)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. Use self.assertFalse instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

assertFalse asserts Falsy value which includes None. assert_is ensures that we are returning strictly False from the method.

mock_config_logging.debug.assert_called_once_with('Variation key is invalid.')

def test_set_forced_variation__multiple_sets(self):
""" Test multiple sets of experiments for one and multiple users work """

Expand Down