Skip to content
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.

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

Add forced-decisions APIs to OptimizelyUserContext #361

Merged
merged 42 commits into from
Dec 6, 2021

Conversation

Mat001
Copy link
Contributor

@Mat001 Mat001 commented Oct 1, 2021

Summary

Add a set of new APIs for forced-decisions to OptimizelyUserContext:

  • set_forced_decision
  • get_forced_decision
  • remove_forced_decision
  • remove_all_forced_decisions

Test plan

Issues

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.

Some early comments :)

optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely.py Outdated Show resolved Hide resolved
optimizely/helpers/enums.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 20, 2021

Coverage Status

Coverage decreased (-0.3%) to 95.829% when pulling 201548f on mpirnovar/forced_decisions into a1e31eb on master.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

rest of the comments, @mnoman09 will verify.

optimizely/decision_service.py Show resolved Hide resolved
optimizely/decision_service.py Show resolved Hide resolved
optimizely/decision_service.py Show resolved Hide resolved
optimizely/event/user_event_factory.py Outdated Show resolved Hide resolved
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

few more comments.

optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/entities.py Show resolved Hide resolved
optimizely/optimizely.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/project_config.py Outdated Show resolved Hide resolved
optimizely/project_config.py Outdated Show resolved Hide resolved
optimizely/project_config.py Show resolved Hide resolved
optimizely/project_config.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mnoman09 mnoman09 left a comment

Choose a reason for hiding this comment

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

Suggested some changes.

optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
optimizely/decision_service.py Show resolved Hide resolved
tests/test_config.py Outdated Show resolved Hide resolved
optimizely/decision_service.py Outdated Show resolved Hide resolved
@Mat001
Copy link
Contributor Author

Mat001 commented Nov 24, 2021

@msohailhussain @mnoman09 I consolidated the functions into one. Here:

def get_flag_variation(self, flag_key, variation_attribute, target_value):
"""
Gets variation by specified variation attribute.
For example if variation_attribute is id, the function gets variation by using variation_id.
If object_attribute is key, the function gets variation by using variation_key.
We used to have two separate functions:
get_flag_variation_by_id()
get_flag_variation_by_key()
This function consolidates both functions into one.
Important to always relate object_attribute to the target value.
Should never enter for example object_attribute=key and target_value=variation_id.
Correct is object_attribute=key and target_value=variation_key.
Args:
flag_key: flag key
variation_attribute: id or key for example. The part after the dot notation (id in variation.id)
target_value: target value we want to get for example variation_id or variation_key
Returns:
Variation as a map.
"""
if not flag_key:
return None
variations = self.flag_variations_map.get(flag_key)
for variation in variations:
if getattr(variation, variation_attribute) == target_value:
return variation
return None

When you review this pls look carefully. The two functions I consolidated were each called on a different object:

get_flag_variation_by_id() was called on project_config. ----> in user_event_factory.py
get_flag_variation_by_key() was called on self.client. --------> inside find_validated_forced_decision()

So I made a little change in find_validated_forced_decision() here, so the single function is only called on project_config: https://github.com/optimizely/python-sdk/pull/361/files#diff-4df94492754236d1784d8b2987b84acd6af0ea3d97a0a2e54e61fb3f03acb04cR265-R268

unit tests and FSC tests pass.

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 few changes suggested

optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/decision_service.py Outdated Show resolved Hide resolved
optimizely/optimizely_user_context.py Show resolved Hide resolved
optimizely/project_config.py Outdated Show resolved Hide resolved
Mat001 and others added 6 commits December 1, 2021 15:37
* project config refactor

* use existing loop to generate flag_variation_map

* get_variation_from_experiment_rule and get_variation_from_delivery_rule removed

* fsc test fix

* comment addressed

* commented code removed

* comments from main forced decision PR resolved

Co-authored-by: ozayr-zaviar <[email protected]>
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.

All look good except for a small change suggested!

optimizely/optimizely_user_context.py Outdated Show resolved Hide resolved
Copy link
Contributor

@The-inside-man The-inside-man left a comment

Choose a reason for hiding this comment

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

LGTM!

@Mat001
Copy link
Contributor Author

Mat001 commented Dec 6, 2021

@msohailhussain @mnoman09 @yasirfolio3 Can you please approve this PR so I can merge it? (mark requested changes as resolved) TY

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 dismissed stale reviews from mnoman09 and yasirfolio3 December 6, 2021 18:38

already approved by 2 devx members.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

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

lgtm on behalf of yasir and noman.

@Mat001 Mat001 merged commit d85d272 into master Dec 6, 2021
@Mat001 Mat001 deleted the mpirnovar/forced_decisions branch December 6, 2021 18:46
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.

8 participants