refact: Decide API #314
refact: Decide API #314
Conversation
| return { | ||
| 'user_id': self.user_id, | ||
| 'attributes': self.user_attributes, | ||
| } |
There was a problem hiding this comment.
We need a mutex control for setAttribute and getAttributes.
There was a problem hiding this comment.
What about changing public class names to OptimizelyUserContext, OptimizelyDecision and OptimizelyDecideOption to be consistent with other SDKs and docs?
|
|
||
| if default_decide_options is None: | ||
| self.default_decide_options = [] | ||
|
|
There was a problem hiding this comment.
what if default_decide_option is non-null?
There was a problem hiding this comment.
I have already placed another check below which ensures that it must be a list when non-null
| @@ -1120,7 +1100,7 @@ def decide_all(self, user_context, decide_options=None): | |||
|
|
|||
| # check if SDK is ready | |||
| if not self.is_valid: | |||
| self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('decide_all')) | |||
| self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('decide_all')) | |||
There was a problem hiding this comment.
line #1131: return {} when SDK_NOT_READY
There was a problem hiding this comment.
it already does. Looks like github is not showing you the updated diff.
| @@ -1153,7 +1132,7 @@ def decide_for_keys(self, user_context, keys, decide_options=None): | |||
|
|
|||
| # check if SDK is ready | |||
| if not self.is_valid: | |||
| self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('decide_for_keys')) | |||
| self.logger.error(enums.Errors.INVALID_OPTIMIZELY.format('decide_for_keys')) | |||
There was a problem hiding this comment.
line #1161: should check with "default_decide_options" as well
|
@jaeopt I have tried to address all of your comments both in this PR and the rest. I am yet to take a final look on the unit tests. You may re-review all new changes besides the tests file. |
|
@jaeopt Reasons work has been done. Unit testing is in progress. You may review reasons related work for now. |
optimizely/bucketer.py
Outdated
| ) | ||
| decide_reasons.append(message) |
There was a problem hiding this comment.
This message can be dropped from decision reasons. Too much info.
Can remove "reasons" from this method return.
optimizely/optimizely.py
Outdated
| @@ -1113,7 +1118,7 @@ def _decide_all(self, user_context, decide_options=None): | |||
| if not config: | |||
| self.logger.error(enums.Errors.INVALID_PROJECT_CONFIG.format('decide')) | |||
| reasons.append(OptimizelyDecisionMessage.SDK_NOT_READY) | |||
There was a problem hiding this comment.
No need for update reasons here. We return {}
optimizely/optimizely.py
Outdated
| should_include_reasons = False | ||
| if OptimizelyDecideOption.INCLUDE_REASONS in decide_options: | ||
| should_include_reasons = True |
There was a problem hiding this comment.
| should_include_reasons = False | |
| if OptimizelyDecideOption.INCLUDE_REASONS in decide_options: | |
| should_include_reasons = True | |
| should_include_reasons = OptimizelyDecideOption.INCLUDE_REASONS in decide_options |
optimizely/decision_service.py
Outdated
| return None | ||
| message = 'User "%s" is not in the forced variation map.' % user_id | ||
| self.logger.debug(message) | ||
| decide_reasons.append(message) |
There was a problem hiding this comment.
Can we remove this from reasons? It's the default case.
optimizely/decision_service.py
Outdated
| ) | ||
| return None | ||
| decide_reasons.append(message) |
There was a problem hiding this comment.
Can we remove this from reasons? It's the default case.
optimizely/decision_service.py
Outdated
|
|
||
| def get_variation(self, project_config, experiment, user_id, attributes, ignore_user_profile=False): | ||
| def get_variation( | ||
| self, project_config, experiment, user_id, attributes, ignore_user_profile=False, decide_options=[] |
There was a problem hiding this comment.
remove "decide_options" not used here. If you want to keep it, then you can remove "ignore_user_profile" from arguments.
optimizely/decision_service.py
Outdated
| @@ -225,25 +242,31 @@ def get_variation(self, project_config, experiment, user_id, attributes, ignore_ | |||
| user_id: ID for user. | |||
| attributes: Dict representing user attributes. | |||
| ignore_user_profile: True to ignore the user profile lookup. Defaults to False. | |||
| decideOptions: Options to customize evaluation. | |||
| if variation: | ||
| return variation | ||
| message = 'Returning previously activated variation ID "{}" of experiment ' \ |
tests/test_user_context.py
Outdated
|
|
||
| def test_decide__invalid_flag_key(self): | ||
| opt_obj = optimizely.Optimizely(json.dumps(self.config_dict_with_features)) | ||
| user_context = opt_obj.create_user_context('test_user') |
There was a problem hiding this comment.
Can you add non-empty attributes to this user-context, so we can validate the user-context clone works ok when validating below?
tests/test_user_context.py
Outdated
| 'optimizely.optimizely.Optimizely._send_impression_event' | ||
| ) as mock_send_event: | ||
|
|
||
| user_context = opt_obj.create_user_context('test_user') |
There was a problem hiding this comment.
Here too. Add non-empty attributes to user-context
tests/test_user_context.py
Outdated
| mock_decide.assert_called_with( | ||
| user_context, | ||
| 'test_feature_in_experiment', | ||
| ['EXCLUDE_VARIABLES', 'ENABLED_FLAGS_ONLY'] |
There was a problem hiding this comment.
"decide_for_keys" should not pass the merged options (options + default_decide_options) to "decide()" since decide() itself merge options with default_decide_option. Can you fix this test and the optimizely.
| {'browser': 'chrome'} | ||
| ) | ||
|
|
||
| def test_decide__option__include_reasons__feature_test(self): |
There was a problem hiding this comment.
These tests look good.
Can we add a few more tests validating "reasons" messages for:
- hit on everyone-else rule
- hit on none of the rules
- hit variation from "get_forced_variation"
- hit from "white_list"
- hit from user-profile-service
- any other important cases?
Summary
This PR refactors and fixes the decide implementation in another branch.
todo: reasons work. It will be done separately.
Test plan
Issues