feat: add decide api by thomaszurkan-optimizely · Pull Request #309 · optimizely/python-sdk

thomaszurkan-optimizely · 2020-11-26T00:30:04Z

Summary

Introducing a new primary interface for retrieving feature flag status, configuration and associated experiment decisions for users. The new OptimizelyUserContext class exposes the following APIs:

- set_attribute
- decide
- decide_all
- decide_for_keys
- track_event

Test plan

Unit tests

…into decideApi

coveralls · 2020-11-26T00:47:58Z

Coverage decreased (-0.5%) to 95.883% when pulling 4f00cb5 on decideApi into be63510 on master.

jaeopt

Overall looks good. Changes suggested.

jaeopt · 2020-12-09T23:29:05Z

optimizely/decision/decision.py

+# limitations under the License.
+
+
+class Decision(object):


Can we change the name to "OptimizelyDecision"?

Done in 314

jaeopt · 2020-12-09T23:29:20Z

optimizely/decision/decide_option.py

+# limitations under the License.
+
+
+class DecideOption(object):


Can we change the name to "OptimizelyDecideOption"?

that is not consistent with the rest of the sdk

Done in 314

jaeopt · 2020-12-09T23:46:32Z

optimizely/decision_service.py

        return None

-    def get_variation_for_feature(self, project_config, feature, user_id, attributes=None):
+    def get_variation_for_feature(self, project_config, feature, user_id, attributes=None, ignore_user_profile=False):


It's interesting - I see that python-sdk already has "ignore_user_profile" option. We can keep the current structure to reuse it, but I'm concerned we may need to add options to support other options later.

jaeopt · 2020-12-10T00:06:10Z

optimizely/optimizely.py

+            notification_center=None,
+            event_processor=None,
+            datafile_access_token=None,
+            default_decisions=None


Can we change this to "default_decide_options"?

Done in 314

jaeopt · 2020-12-10T00:13:58Z

optimizely/optimizely.py

+        user_context = UserContext(self, user_id, attributes)
+        return user_context
+
+    def decide(self, user_context, key, decide_options=None):


Can we make "decide", "decide_all" and "decide_for_keys" for internal access only (not public) if access can be controlled in python?
We want to let them use the APIs via UserContext always.

@jaeopt Addressing this in my new PR. Python doesn't have real private methods or access control. The convention is to prefix a method with an underscore, if it's not meant to be called directly.

jaeopt · 2020-12-10T01:36:40Z

tests/test_user_context.py

+            enums.NotificationTypes.DECISION,
+            'feature',
+            'test_user',
+            {},
+            {
+                'feature_key': 'test_feature_in_experiment',
+                'feature_enabled': True,
+                'source': 'feature-test',
+                'source_info': {
+                    'experiment': mock_experiment,
+                    'variation': mock_variation,
+                },
+            },


This notification type/contents should be changed.

jaeopt · 2020-12-10T01:39:04Z

tests/test_user_context.py

+            'time.time', return_value=42
+        ):
+            context = opt_obj.create_user_context(user_id)
+            decision = context.decide('test_feature_in_experiment', [DecideOption.DISABLE_DECISION_EVENT,


Also need the same tests for default_decide_options.

jaeopt · 2020-12-10T01:42:48Z

tests/test_user_context.py

+        )
+
+        # Check that impression event is sent for rollout and send_flag_decisions = True
+        self.assertEqual(1, mock_process.call_count)


Can we validate the event contents (at least metadata for flag decision)?

jaeopt · 2020-12-10T01:44:40Z

tests/test_user_context.py

+
+        # Check that impression event is NOT sent for rollout and send_flag_decisions = True
+        # with disable decision event decision option
+        self.assertEqual(0, mock_process.call_count)


Can we have one more test validating "IGNORE_USER_PROFILE" skips ups save() as well?

jaeopt · 2020-12-10T01:45:45Z

tests/test_user_context.py

+        # with disable decision event decision option
+        self.assertEqual(0, mock_process.call_count)
+
+    def test_decide_options_reasons(self):


A test for "EXCLUDE_VARIABLES" missing?

* WIP * WIP * fix: Passes All FSC * fix: unit tests and cleanup * refact: rename decide classes * fix: merge default decide options in decide for keys * prefix decide methods with _ * fix: decide option import * mutex locks * Apply suggestions from code review Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> * tests: user context tests * tests: WIP * feat: reasons work * tests: refact * tests: Add unit tests * remove reasons from find_bucket * address comments * tests: decide * fix: import * tests: Add reasons tests Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>

oakbani · 2021-02-01T17:59:03Z

@msohailhussain @jaeopt I have merged my PR. This PR should be good to go.

jaeopt · 2021-02-01T19:05:17Z

Tests fail after merge. Can you take a look? I guess this is the final one including all decide-api related changes. We can just close #309, correct?

oakbani · 2021-02-01T20:38:21Z

The unit tests are only failing on pypy and pypy3 due to a missing module. This error at times appear on travis builds. Not due to our change. Yes, this is the final one that should be merged.

jaeopt

LGTM

thomaszurkan-optimizely added 6 commits November 9, 2020 10:06

add user context

35a6eac

cleanup lint

509ff50

update comments

0ef34ee

fixes from ali's comments

61fe356

add all decide objects

c8cabbd

add decide apis

dc226c1

thomaszurkan-optimizely requested a review from a team as a code owner November 26, 2020 00:30

optimizely-codereview-bot assigned thomaszurkan-optimizely Nov 26, 2020

thomaszurkan-optimizely added the wip Work in progress label Nov 26, 2020

thomaszurkan-optimizely added 3 commits November 25, 2020 16:31

Merge branch 'master' into decideApi

366030e

fix lint errors

fd99ec1

Merge branch 'decideApi' of https://github.com/optimizely/python-sdk …

36b0328

…into decideApi

added unit tests and cleanup decide

19b7fae

thomaszurkan-optimizely removed the wip Work in progress label Dec 3, 2020

thomaszurkan-optimizely added 11 commits December 2, 2020 16:22

cleanup lint errors

ffa40d6

fix lint

13ec677

remove reduce to work with python 3.x

5feca2c

add more unit tests for user context

4c4b4a4

another test

460c904

try and fix travis pypy tests

0d4ec50

attempt to fix pypy on travis

8b5d56d

fix travis pypy

a1cbc68

add more tests

67fc4f2

finish python with reasons

f3e3854

make sure we are at the lowest level DEBUG

c38dbfc

jaeopt requested changes Dec 10, 2020

View reviewed changes

thomaszurkan-optimizely and others added 2 commits December 10, 2020 10:17

only capture info messages and up

2c1f8b0

jaeopt approved these changes Feb 1, 2021

View reviewed changes

Merge branch 'master' into decideApi

4f00cb5

jaeopt merged commit 67b9f83 into master Feb 1, 2021

jaeopt deleted the decideApi branch February 1, 2021 22:17

jaeopt mentioned this pull request Feb 1, 2021

feat: add user context #305

Closed

Conversation

thomaszurkan-optimizely commented Nov 26, 2020 • edited by jaeopt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

coveralls commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oakbani commented Feb 1, 2021

Uh oh!

jaeopt commented Feb 1, 2021 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oakbani commented Feb 1, 2021

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thomaszurkan-optimizely commented Nov 26, 2020 •

edited by jaeopt

Loading

coveralls commented Nov 26, 2020 •

edited

Loading

jaeopt commented Feb 1, 2021 via email •

edited

Loading