feat(Decide): Add Decide API #220

oakbani · 2020-11-24T13:53:06Z

Summary

Added new APIs to support the decide feature. Introduced a new OptimizelyUserContext class through createUserContext class api. This creates an optimizely instance with memoized user context and exposes the following APIs

setAttribute
decide
decideAll
decideForKeys
trackEvent

Test plan

Manually tested thoroughly.
Added unit tests to cover new functionality.
Tested with FSC manually.

Testapp PR: https://github.com/optimizely/php-testapp/pull/66

coveralls · 2020-11-24T14:01:49Z

Coverage decreased (-2.4%) to 95.706% when pulling bc47a63 on oakbani/decide-api into 5f73a01 on master.

coveralls · 2020-11-24T14:01:49Z

Coverage decreased (-2.4%) to 95.706% when pulling bc47a63 on oakbani/decide-api into 5f73a01 on master.

coveralls · 2020-11-24T14:01:49Z

Coverage decreased (-2.4%) to 95.706% when pulling bc47a63 on oakbani/decide-api into 5f73a01 on master.

coveralls · 2020-11-24T14:01:49Z

Coverage decreased (-0.5%) to 97.61% when pulling b6d0bd5 on oakbani/decide-api into 5f73a01 on master.

jaeopt

It looks great!
A couple of fixes necessary and more tests suggested.

jaeopt · 2020-12-10T23:28:01Z

src/Optimizely/DecisionService/DecisionService.php

-            $this->_logger->log(Logger::DEBUG, sprintf('No experiment "%s" mapped to user "%s" in the forced variation map.', $experimentKey, $userId));
+            $message = sprintf('No experiment "%s" mapped to user "%s" in the forced variation map.', $experimentKey, $userId);
+            $this->_logger->log(Logger::DEBUG, $message);
+            $decideReasons[] = $message;


I do not think we should include this message into reasons. It may be too much.

jaeopt · 2020-12-10T23:44:51Z

src/Optimizely/Optimizely.php

+            $userId,
+            $userAttributes,
+            $decideOptions,
+            $decideReasons


We do not need collect reasons if INCLUDE_REASONS not set.
Any optimization instead of collecting and throwing away?

I think this is a clean approach.

Here decideReasons is passed by reference. If we choose not to pass here based on INCLUDE_REASONS flag, each internal method would create a default array inside method, push log into it and eventually lose it's scope.

The other thing that we can do is to pass decideOptions in each method with the decideReasons. And check for INCLUDE_REASONS flag inside decideOptions before pushing any log in decideReasons array. This would add an additonal array item check before each log.

src/Optimizely/Optimizely.php

jaeopt · 2020-12-10T23:53:20Z

src/Optimizely/Optimizely.php

+            return [];
+        }
+
+        $enabledFlagsOnly = in_array(OptimizelyDecideOption::ENABLED_FLAGS_ONLY, $decideOptions);


Should check with "default_decide_options" as well

jaeopt · 2020-12-10T23:55:01Z

src/Optimizely/Optimizely.php

+     *
+     * @return OptimizelyDecision A decision result
+     */
+    public function decide(OptimizelyUserContext $userContext, $key, array $decideOptions = [])


We should not open "decide", "decide_all" and "decide_for_keys" to public.
We'll let them use those apis through userContext always.

If I am correct, this won't be possible in PHP as it doesn't provide package level visibility. Making any of these methods protected / private, would make them unreachable from OptimizelyUserContext class.

jaeopt · 2020-12-11T00:11:34Z

tests/OptimizelyUserContextTests.php

+        $optUserContext->setAttribute('browser', 'firefox');
+        $this->assertEquals(["browser" => "firefox"], $optUserContext->getAttributes());
+    }
+


Can we add a test:

no attributes given at init and added later with setAttribute

tests/OptimizelyTest.php

jaeopt · 2020-12-11T00:29:47Z

tests/OptimizelyTest.php

+        $this->assertEquals($expectedOptimizelyDecision->getRuleKey(), $optimizelyDecision->getRuleKey());
+        $this->assertEquals($expectedOptimizelyDecision->getFlagKey(), $optimizelyDecision->getFlagKey());
+        $this->assertEquals($expectedOptimizelyDecision->getUserContext(), $optimizelyDecision->getUserContext());
+        $this->assertEquals($expectedOptimizelyDecision->getFlagKey(), $optimizelyDecision->getFlagKey());


This line is redundant. "reasons" not compared. Is it intentional?

Yes, left out reasons comparison intentionally. As in all those tests, where INCLUDE_REASONS flag is included, we will have to create a verbose expected OptimizelyDecision object. Reasons are asserted separately.

jaeopt · 2020-12-11T00:45:14Z

tests/OptimizelyTest.php

+            ->with(
+                $this->projectConfig,
+                'test_experiment_double_feature',
+                'control',
+                'double_single_variable_feature',
+                'test_experiment_double_feature',
+                FeatureDecision::DECISION_SOURCE_FEATURE_TEST,
+                true,
+                'test_user',
+                ['device_type' => 'iPhone']
+            );


jaeopt · 2020-12-11T00:46:51Z

tests/OptimizelyTest.php

+        $optimizelyDecision = $optimizelyMock->decide($userContext, 'double_single_variable_feature', ['DISABLE_DECISION_EVENT', 'ENABLED_FLAGS_ONLY', 'IGNORE_USER_PROFILE_SERVICE', 'INCLUDE_REASONS', 'EXCLUDE_VARIABLES']);
+        $this->compareOptimizelyDecisions($expectedOptimizelyDecision, $optimizelyDecision);
+    }
+


I do not see tests for

INCLUDE_REASONS

IGNORE_USER_PROFILE (skip read/write UPS both)

Also need the same set of tests for "default_decide_options".

And I'd like to see a test validating options in "default_decide_options" (EXCLUDE_VARIABLES) and decide parameter option (DISABLE_DECIDE_EVENTS) work together ok.

jaeopt

All changes look good!
One last missing test is IGNORE_USER_PROFILE option testing. Add a test validating both reading and updating skipped on the option.

jaeopt

LGTM

…ameter (#221) * impact * fix Decision Service Tests * fix: test cases affected due to bucketer * fix: unit tests affected by reasons * fix: incompatible syntax for < php 7.0 * fix: some more tests * audience eval logs * tests: bucketer reasons * tests: Decision Service reasons * tests: decision service further * update: copyright headers

oakbani · 2021-01-20T07:15:00Z

@msohailhussain @jaeopt ready to merge

oakbani added 8 commits November 18, 2020 19:04

init decide

bfb6bc4

Added tests for UserContext and createUserContext

7045bc5

changes in bucketer

cf08f7a

changes in DS

b62b1b2

WIP

e93d8b8

Decide verified with FSC PR

e9a281c

WIP

141192d

WIP

bc47a63

optimizely-codereview-bot assigned oakbani Nov 24, 2020

oakbani and others added 7 commits November 24, 2020 19:10

add new unit tests

4218774

testcases for decide api

8cc49df

Merge branch 'master' into oakbani/decide-api

d385561

unit tests WIP

22e42de

decide doc string added

beaf8de

revised all unit tests;

db2e453

finalize

bc7ba1c

oakbani marked this pull request as ready for review December 9, 2020 14:53

oakbani requested a review from a team as a code owner December 9, 2020 14:53

oakbani removed their assignment Dec 9, 2020

test on php 7.3.24

4681d87

optimizely-codereview-bot added 🤖 4 hrs 🤖 8 hrs 🤖 12 hrs and removed 🤖 4 hrs 🤖 8 hrs labels Dec 9, 2020

jaeopt requested changes Dec 11, 2020

View reviewed changes

oakbani removed their assignment Dec 11, 2020

optimizely-codereview-bot requested a review from jaeopt December 11, 2020 16:05

jaeopt requested changes Dec 11, 2020

View reviewed changes

optimizely-codereview-bot assigned oakbani Dec 11, 2020

tests: ignore user profile

af0fbc2

oakbani removed their assignment Dec 14, 2020

optimizely-codereview-bot requested a review from jaeopt December 14, 2020 07:56

jaeopt approved these changes Dec 14, 2020

View reviewed changes

oakbani added 3 commits January 7, 2021 16:27

update: headers

7ebb702

fix: variation and rule keys

4562445

oakbani closed this Jan 20, 2021

oakbani reopened this Jan 20, 2021

optimizely-codereview-bot assigned oakbani Jan 20, 2021

oakbani removed their assignment Jan 20, 2021

jaeopt closed this Jan 20, 2021

jaeopt reopened this Jan 20, 2021

optimizely-codereview-bot assigned oakbani Jan 20, 2021

jaeopt closed this Jan 20, 2021

jaeopt reopened this Jan 20, 2021

update: coveralls version

b6d0bd5

jaeopt closed this Jan 21, 2021

jaeopt reopened this Jan 21, 2021

jaeopt closed this Jan 21, 2021

jaeopt reopened this Jan 21, 2021

jaeopt closed this Jan 21, 2021

jaeopt reopened this Jan 21, 2021

jaeopt merged commit c687e53 into master Jan 21, 2021

jaeopt deleted the oakbani/decide-api branch January 21, 2021 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Decide): Add Decide API #220

feat(Decide): Add Decide API #220

oakbani commented Nov 24, 2020 •

edited

Loading

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020 •

edited

Loading

jaeopt left a comment

jaeopt Dec 10, 2020

jaeopt Dec 10, 2020

oakbani Dec 11, 2020

jaeopt Dec 10, 2020

jaeopt Dec 10, 2020

oakbani Dec 11, 2020

jaeopt Dec 11, 2020

jaeopt Dec 11, 2020

oakbani Dec 11, 2020

jaeopt Dec 11, 2020

jaeopt Dec 11, 2020

jaeopt Dec 11, 2020

jaeopt left a comment

jaeopt left a comment

oakbani commented Jan 20, 2021

feat(Decide): Add Decide API #220

feat(Decide): Add Decide API #220

Conversation

oakbani commented Nov 24, 2020 • edited Loading

Summary

Test plan

Testapp PR: https://github.com/optimizely/php-testapp/pull/66

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020

coveralls commented Nov 24, 2020 • edited Loading

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

oakbani commented Jan 20, 2021

oakbani commented Nov 24, 2020 •

edited

Loading

coveralls commented Nov 24, 2020 •

edited

Loading