Add Audiences to OptimizelyConfig and expose in OptimizelyExperiment by The-inside-man · Pull Request #342 · optimizely/python-sdk

The-inside-man · 2021-07-02T19:34:29Z

Summary

Add Audiences to OptimizelyConfig
Expose audiences string through OptimizelyExperiment
Add additional tests for new additions

The “why”, or other context.

Test plan

compatibility tests

Issues

“OASIS-7812"

- assertRaisesRegexp -> assertRaisesRegex - assertEquals -> assertEqual - isAlive() -> is_alive() - Check added to base.py to confirm attribute assertRaisesRegex for backwards compatibility to Python2.7

…t_user_context.

…key and environment_key. Move decide tests to test_user_context from test_optimizely.

coveralls · 2021-07-02T19:40:09Z

Coverage increased (+0.1%) to 95.758% when pulling 6ce98e3 on jbrown/oasis-7812 into 4935717 on master.

The-inside-man · 2021-07-02T19:46:43Z

Looks like some CI issues pulling in python files... @msohailhussain is this part of what you were talking about with some failures?

The-inside-man · 2021-07-02T19:48:10Z

As draft for now (Used a label) But please take a look at let me know thoughts.

The-inside-man · 2021-07-02T19:51:57Z

Found the issue. Fixing shortly

jaeopt

I see some confusions about OptimizelyConfig entities and ProjectConfig entities. See my comments to clarify them.

optimizely/entities.py

optimizely/optimizely_config.py

jaeopt · 2021-07-02T20:31:22Z

optimizely/optimizely_config.py

+            audiences_map[audience.get('id')] = audience.get('name')
+
+        # Updating each OptimizelyExperiment based on the ID from entities.Experiment found in the experiment_key_map
+        for ent_exp in project_config.experiment_key_map.values():


This covers "experiment" rules. We also need to do the same to experiments in rollout ("delivery" rules) as well.

Delivery Rules added to OptimizelyConfig and test cases to cover

optimizely/optimizely_config.py

tests/test_optimizely_config.py

…utes list and events list.

… one operand and one ID, remove unneed get for delivery_rules.

optimizely/optimizely_config.py

The-inside-man · 2021-07-13T18:51:02Z

optimizely/optimizely_config.py

+    def get_delivery_rules(self):
+        """ Get the delivery rules list associated with OptimizelyConfig
+
+        Returns:
+            List of OptimizelyExperiments as DeliveryRules from Rollouts.
+        """
+        return self.delivery_rules
+


This function isnt needed, I meant to remove. Thanks for spotting!

The-inside-man · 2021-07-13T18:56:49Z

tests/base.py

+                    'delivery_rules': [],
+                    'experiment_rules': [],


The testing in Python will fail if these are not there because it compares the expected_config which is a dict of the OptimizelyConfig with the actual_config, a dict of the actual OptimizelyConfig. Base.py homes other scenarios to create OptimizelyConfig which it compares those dicts directly with. If that makes sense. We can chat more if you like.

tests/test_optimizely_config.py

The-inside-man · 2021-07-13T20:24:41Z

@jaeopt All changes and concerns addressed. Please see me comments.

jaeopt

All changes look good. We can discuss a couple of more issues tomorrow.

tests/test_optimizely_config.py

jaeopt · 2021-07-14T00:32:13Z

tests/base.py

+                    'delivery_rules': [],
+                    'experiment_rules': [],


It looks like a wrong place to add them. We can discuss.

jaeopt · 2021-07-14T00:34:59Z

optimizely/optimizely_config.py

+    def get_delivery_rules(self):
+        """ Get the delivery rules list associated with OptimizelyConfig
+
+        Returns:
+            List of OptimizelyExperiments as DeliveryRules from Rollouts.
+        """
+        return self.delivery_rules
+


I also have questions about other get functions other than get_datafile().

The-inside-man · 2021-07-14T18:51:15Z

removed experiment_rules and delivery_rules from base.py and all good now.

jaeopt

All changes look good except that get functions not cleaned up yet.

jaeopt

LGTM

keppel2 · 2021-07-15T13:51:11Z

Changes are fine.

The-inside-man · 2021-07-15T18:24:31Z

Prod Suite with updated test app not using getter for sdk_key and environment_key:

https://travis-ci.com/github/optimizely/fullstack-prod-suite/builds/232876085

The-inside-man · 2021-07-15T18:25:35Z

FSC Suite - Using updated Test App not using getts for sdk_key or environment_key:

https://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/232876040

yasirfolio3

Functionality looks good, just some minor suggestions.

optimizely/optimizely_config.py

…y for another edge case ['and'], changed method to combine typed_audiences and audiences from project_config to use lookup map rather than build a list and check length. Reduces time complexity.

yasirfolio3

LGTM, just one nit.

optimizely/optimizely_config.py

keppel2 · 2021-08-02T15:35:31Z

optimizely/optimizely_config.py

+            The typed_audiences has higher precedence.
+        '''
+
+        typed_audiences = project_config.typed_audiences[:]


@The-inside-man Minor, but we don't mutate typed_audiences and thus can do a reference copy.

Good point! Thanks for catching that!

ozayr-zaviar and others added 14 commits July 2, 2021 13:00

added sdk and environment key

e80fdfb

[MAINTENANCE] Remove Deprecated warnings during build

d58b71a

- assertRaisesRegexp -> assertRaisesRegex - assertEquals -> assertEqual - isAlive() -> is_alive() - Check added to base.py to confirm attribute assertRaisesRegex for backwards compatibility to Python2.7

[OASIS-7757] Fix spelling of environment to fix testcases from failing

a583ea5

[OASIS-7757] - Added additional test cases to test_optimizely and tes…

5b2196c

…t_user_context.

[OASIS-7800] Updated optimizely_config with attributes and events

f964f94

[OASIS-7757] - update copyright years and add more testcases for sdk_…

f60ab0a

…key and environment_key. Move decide tests to test_user_context from test_optimizely.

[OASIS-7800] Updated optimizely_config with attributes and events

df1b081

Run autopep8 to fix formatting issues after rebase

9c9bcea

[OASIS-7800] - Added test cases for attribute map and events map

61d5c24

Remove unused import from optimizely config

13703ca

Corrected comment wording in get functions for events.

ef95cd8

[OASIS-7812] - Add Audiences to OpotimizelyConfig to expose to users

b8828b9

Remove unused import for Experiment in OptimizelyConfig

c3bc816

Update Formatting issues for lint

f1bd533

The-inside-man added the draft Work in progress label Jul 2, 2021

The-inside-man requested a review from a team as a code owner July 2, 2021 19:34

optimizely-codereview-bot assigned The-inside-man Jul 2, 2021

The-inside-man removed the request for review from a team July 2, 2021 19:34

The-inside-man requested review from jaeopt, keppel2 and msohailhussain July 2, 2021 19:46

Update Import for conditions in optimizely_config.py

0ad43d0

The-inside-man removed the draft Work in progress label Jul 2, 2021

The-inside-man requested a review from Mat001 July 2, 2021 20:18

jaeopt requested changes Jul 2, 2021

View reviewed changes

Additional changes for audiences and added functions to create attrib…

942bfe9

…utes list and events list.

Updated changes - reuse test logic, modify stringify to proper handle…

bf92fd7

… one operand and one ID, remove unneed get for delivery_rules.

The-inside-man commented Jul 13, 2021

View reviewed changes

The-inside-man requested a review from jaeopt July 13, 2021 20:24

jaeopt requested changes Jul 14, 2021

View reviewed changes

The-inside-man added 5 commits July 14, 2021 08:58

Change to single test to run all cases for stringify.

2d6def3

Move config_service out of loop.

bec791a

Added missed step in delivery rules.

286f178

Update to properly refect datafile examples.

c3faba3

Remove unused import

199a35f

The-inside-man requested a review from jaeopt July 14, 2021 19:01

jaeopt requested changes Jul 14, 2021

View reviewed changes

Cleaned up get functions till overall cleanup of code.

4fa5d05

jaeopt approved these changes Jul 14, 2021

View reviewed changes

Merge branch 'master' into jbrown/oasis-7812

4356a4d

Merge branch 'master' into jbrown/oasis-7812

a273d11

Mat001 approved these changes Jul 16, 2021

View reviewed changes

yasirfolio3 suggested changes Jul 19, 2021

View reviewed changes

optimizely/optimizely_config.py Outdated Show resolved Hide resolved

optimizely/optimizely_config.py Show resolved Hide resolved

optimizely/optimizely_config.py Show resolved Hide resolved

Added comments to stringify_conditions, added testcase to test string…

c8d68f7

…y for another edge case ['and'], changed method to combine typed_audiences and audiences from project_config to use lookup map rather than build a list and check length. Reduces time complexity.

The-inside-man requested a review from yasirfolio3 July 19, 2021 14:19

Correct spelling of precedence

a9ac50c

yasirfolio3 approved these changes Jul 20, 2021

View reviewed changes

optimizely/optimizely_config.py Outdated Show resolved Hide resolved

Update comment to be more readable.

6ce98e3

The-inside-man merged commit 4aad6b7 into master Jul 20, 2021

The-inside-man deleted the jbrown/oasis-7812 branch July 20, 2021 14:32

keppel2 reviewed Aug 2, 2021

View reviewed changes

Conversation

The-inside-man commented Jul 2, 2021

Summary

Test plan

Issues

Uh oh!

coveralls commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

The-inside-man commented Jul 2, 2021

Uh oh!

The-inside-man commented Jul 2, 2021

Uh oh!

The-inside-man commented Jul 2, 2021

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

The-inside-man commented Jul 13, 2021

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

The-inside-man commented Jul 14, 2021

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

keppel2 commented Jul 15, 2021

Uh oh!

The-inside-man commented Jul 15, 2021

Uh oh!

The-inside-man commented Jul 15, 2021

Uh oh!

yasirfolio3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yasirfolio3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

coveralls commented Jul 2, 2021 •

edited

Loading

yasirfolio3 left a comment •

edited

Loading