feat(OptimizelyConfig): Add new fields to OptimizelyConfig #230

ozayr-zaviar · 2021-08-22T00:44:54Z

Summary

The following new public properties are added to OptimizelyConfig:

sdkKey
environmentKey
attributes
audiences
events
experimentRules and deliveryRules to OptimizelyFeature
audiences to OptimizelyExperiment

Test plan

All FSC tests OPTIMIZELY_CONFIG_V2 tests should pass.
https://app.travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/235923589

coveralls · 2021-08-22T01:09:26Z

Coverage decreased (-0.9%) to 96.958% when pulling c56a186 on uzair/config-v2 into f15e5f7 on master.

coveralls · 2021-08-22T01:09:26Z

Coverage decreased (-0.9%) to 96.958% when pulling c56a186 on uzair/config-v2 into f15e5f7 on master.

coveralls · 2021-08-22T01:09:26Z

Coverage decreased (-0.9%) to 96.958% when pulling c56a186 on uzair/config-v2 into f15e5f7 on master.

coveralls · 2021-08-22T01:09:27Z

Coverage decreased (-0.9%) to 96.958% when pulling 5304778 on uzair/config-v2 into f15e5f7 on master.

coveralls · 2021-08-22T01:09:27Z

Coverage decreased (-0.9%) to 96.958% when pulling c56a186 on uzair/config-v2 into f15e5f7 on master.

keppel2 · 2021-08-23T06:57:31Z

src/Optimizely/Config/ProjectConfigInterface.php

@@ -51,6 +51,37 @@ public function getBotFiltering();
     */
    public function getRevision();

+    /**
+     * @return string String represnting environment key of the datafile.


Use "representing" instead of "represnting" if you want . If you leave a comment, try to keep in mind the text. This is a minor correction, however--I wouldn't gate an entire review on it.

jaeopt

Looks good. Need some changes.

jaeopt · 2021-08-23T16:41:59Z

src/Optimizely/Config/DatafileProjectConfig.php

+        $this->audiences = isset($config['audiences']) ? $config['audiences'] : [];
+        $this->events = $config['events'] ?: [];


Can't we "?:" consistently for all these props?

jaeopt · 2021-08-23T17:07:37Z

src/Optimizely/Config/DatafileProjectConfig.php

-        $this->_botFiltering = isset($config['botFiltering'])? $config['botFiltering'] : null;
+        $this->attributes = isset($config['attributes']) ? $config['attributes'] : [];


Naming rule? I see inconsistency with and without "_" prefix though they're all private

#188 (comment)

jaeopt · 2021-08-23T18:24:57Z

src/Optimizely/OptimizelyConfig/OptimizelyFeature.php

+     * Map of rollout Experiments Keys to OptimizelyExperiments.
+     *
+     * @var <String, OptimizelyExperiment> associative array


Suggested change

* Map of rollout Experiments Keys to OptimizelyExperiments.

*

* @var <String, OptimizelyExperiment> associative array

* Array of delivery rules.

*

* @var [OptimizelyExperiment]

jaeopt · 2021-08-23T18:25:30Z

src/Optimizely/OptimizelyConfig/OptimizelyFeature.php

+     * Map of Experiment Keys to OptimizelyExperiments.
+     *
+     * @var <String, OptimizelyExperiment> associative array


Suggested change

* Map of Experiment Keys to OptimizelyExperiments.

*

* @var <String, OptimizelyExperiment> associative array

* Array of experiment rules.

*

* @var [OptimizelyExperiment]

jaeopt · 2021-08-23T18:33:03Z

tests/OptimizelyConfigTests/OptimizelyConfigServiceTest.php

+        $experimentMapFlag1 = $optimizelyConfig->getFeaturesMap()['flag1']->getExperimentsMap(); // 9300000007569
+        $experimentMapFlag2 = $optimizelyConfig->getFeaturesMap()['flag2']->getExperimentsMap(); // 9300000007573


Change these to use "->getExperimentRules()" instead of "->getExperimentsMap()"

jaeopt · 2021-08-23T18:44:20Z

tests/OptimizelyConfigTests/OptimizelyConfigServiceTest.php

+            array("3468206642"),
+            array('and', array('or', '3468206642', '3988293899'), '3988293898'),
+            array('and', 'and'),
+            array(),


Can we have one more test for non-matching audience-id: '[“or”, “1”, “100000”]' in TDD

jaeopt · 2021-08-23T18:50:26Z

tests/OptimizelyConfigTests/OptimizelyConfigServiceTest.php

+    }
+
+    public function testgetConfigAudiences()
+    {


I do not see tests for validating typeAudiences and audiences merged properly (filtering duplicate ones and $opt_dummy_audience? Please add if missed.

jaeopt · 2021-08-23T18:56:37Z

src/Optimizely/OptimizelyConfig/OptimizelyConfig.php

+     * @var string sdkKey of the config.
+     */
+    private $sdkKey;
+


(1) add warning message about duplicated keys to "experimentsMap" of OptimizelyConfig
(2) add deprecation warning to "getExperimentsMap" of OptimizelyFeature.

zashraf1985

Overall Comment about Variable and Method doc comments. I see multiple patterns of commenting. Can we decide and use one. In some cases, there is a comment line and a return type line. In others, there is a single line which casually talks about a type.

zashraf1985 · 2021-08-23T19:40:38Z

src/Optimizely/Config/DatafileProjectConfig.php

+    /**
+     * list of Typed Audiences that will be parsed from the datafile
+     *
+     * @var [typed_audience]


I am not sure but assuming it represents the type, should'nt it be [AUDIENCE] instead of [typed_audience]

zashraf1985 · 2021-08-23T19:45:49Z

src/Optimizely/Config/DatafileProjectConfig.php

+        return array_filter(
+            array_values($this->_experimentIdMap),
+            function ($experiment) use ($rolloutExperimentIds) {
+                return !in_array($experiment->getId(), $rolloutExperimentIds);
+            }
+        );


Is this diff needed. it looks like only indentation/formatting change?

zashraf1985 · 2021-08-23T19:52:45Z

src/Optimizely/OptimizelyConfig/OptimizelyConfig.php

+     * Array of events as OptimizelyEvent.
+     *
+     * @var [OptimizelyEvent]
+     */


As a common pattern, i see two ways of variable and function doc comments here. one is this and one is along the lines of @var string sdkKey of the config. why two standards and not one?

zashraf1985 · 2021-08-23T20:34:15Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+    {
+        $finalAudiences = [];
+        $uniqueIdsMap = [];
+        $normalAudiences = $this->projectConfig->getAudiences();


normalAudiences sounds like an awkward variable name. You can call it audiences?

zashraf1985 · 2021-08-23T20:38:50Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+        foreach ($audiencesArray as $audience) {
+            $id = $audience['id'];
+            if ($id != '$opt_dummy_audience') {
+                $optlyAudience = new OptimizelyAudience(
+                    $id,
+                    $audience['name'],
+                    $audience['conditions']
+                );
+                array_push($finalAudiences, $optlyAudience);
+            }
+        }


You dont probably need a third loop if you keep building the finalAudiences array while iterating through the first two

zashraf1985 · 2021-08-23T22:13:20Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+                    if ($finalAudiences !== '') {
+                        $finalAudiences = $finalAudiences . ' ';
+                    } else {
+                        $finalAudiences = $finalAudiences;
+                    }


Suggested change

if ($finalAudiences !== '') {

$finalAudiences = $finalAudiences . ' ';

} else {

$finalAudiences = $finalAudiences;

}

if ($finalAudiences !== '') {

$finalAudiences .= ' ';

}

zashraf1985 · 2021-08-23T22:14:31Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+                }
+            }
+            // Checks if sub audience is empty or not
+            if (strval($subAudience !== '')) {


why is strVal needed when you are already initializing it to empty string at the top?

zashraf1985 · 2021-08-23T22:17:27Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+     *
+     * @param string feature rollout id.
+     *
+     * @return array of optimizelyExperiments as delivery rules .


Suggested change

* @return array of optimizelyExperiments as delivery rules .

* @return array of optimizelyExperiments as delivery rules.

zashraf1985 · 2021-08-23T22:18:23Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+            $expId = $exp->getId();
+            $expKey = $exp->getKey();


NIT: Why do you need to assign these to vars. Cant they be used directly?

But these values are again required below to assign values in $experimentsKeyMap and $experimentsIdMap

zashraf1985 · 2021-08-23T22:19:17Z

src/Optimizely/OptimizelyConfig/OptimizelyEvent.php

+    }
+
+    /**
+     * @return string event conditions.


Can you correct the comment? copy paste error maybe?

msohailhussain

initial review.

msohailhussain · 2021-08-23T21:38:21Z

src/Optimizely/Config/DatafileProjectConfig.php

        $this->_revision = $config['revision'];
        $this->_sendFlagDecisions = isset($config['sendFlagDecisions']) ? $config['sendFlagDecisions'] : false;

        $groups = $config['groups'] ?: [];
        $experiments = $config['experiments'] ?: [];
-        $events = $config['events'] ?: [];


don't understand why these are removed from here. all three.

msohailhussain · 2021-08-23T22:15:13Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+        $typedAudiences = $this->projectConfig->getTypedAudiences();
+        $audiencesArray = $typedAudiences;
+        foreach ($audiencesArray as $key => $typedAudience) {
+            $uniqueIdsMap[$typedAudience['id']] = $typedAudience['id'];


can't we push it in array and check if that id exists or not?

msohailhussain · 2021-08-23T22:24:31Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+     *
+     * @return array of OptimizelyEvents.
+     */
+    protected function getConfigAudiences()


please revise the variable names.
finalAudiences, normalAudiences seems weird.

msohailhussain · 2021-08-23T22:25:13Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

@@ -145,6 +249,7 @@ protected function getVariablesMap(Experiment $experiment, Variation $variation)
        }

        $featureFlag = $this->experimentIdFeatureMap[$experimentId];
+


remove this line.

msohailhussain · 2021-08-23T22:26:38Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+                $subAudience = $this->getSerializedAudiences($var);
+
+                $subAudience = '(' . $subAudience . ')';
+            } elseif (in_array($var, array('and', 'or', 'not'), true)) {


can we have 'and', 'or', 'not' in constants.

msohailhussain · 2021-08-23T22:30:14Z

src/Optimizely/OptimizelyConfig/OptimizelyConfigService.php

+            $optExp = new OptimizelyExperiment(
+                $expId,
+                $expKey,
+                $this->getVariationsMap($exp),


i am not sure, when we get variation, without passing featureFlag, how featurevariables will be merged.

jaeopt

All changes look good.
Deprecation warning for "getExperimentsMap()" should be moved from OptimizelyConfig to OptimizelyFeature.

jaeopt · 2021-08-24T20:56:12Z

src/Optimizely/OptimizelyConfig/OptimizelyConfig.php

@@ -124,6 +128,7 @@ public function getDatafile()
     */
    public function getExperimentsMap()
    {
+        # This experimentsMap is deprecated. Use experimentRules and deliveryRules instead.


Move this deprecation message to "getExperimentsMap()" of OptimizelyFeature. The same-named one here for OptimizelyConfig is not deprecated.

zashraf1985

Looks Good! Great work.

jaeopt

LGTM

zeeshan and jae already done, dismissing my review.

config v2 implementation

df67724

optimizely-codereview-bot assigned ozayr-zaviar Aug 22, 2021

removed decoder package

c56a186

deprecation warning added

4176c4b

ozayr-zaviar removed their assignment Aug 23, 2021

ozayr-zaviar marked this pull request as ready for review August 23, 2021 06:23

ozayr-zaviar requested a review from a team as a code owner August 23, 2021 06:23

keppel2 reviewed Aug 23, 2021

View reviewed changes

typo corrected

b67e2d1

jaeopt requested changes Aug 23, 2021

View reviewed changes

optimizely-codereview-bot assigned ozayr-zaviar Aug 23, 2021

zashraf1985 requested changes Aug 23, 2021

View reviewed changes

msohailhussain previously requested changes Aug 23, 2021

View reviewed changes

ozayr-zaviar added 4 commits August 24, 2021 18:16

comments addressed

563003b

extra spacing removed

ccda595

testcases comments addressed

1df4ab0

lint fixed

8d23eeb

jaeopt requested changes Aug 24, 2021

View reviewed changes

comments addressed

5304778

zashraf1985 approved these changes Aug 24, 2021

View reviewed changes

jaeopt approved these changes Aug 24, 2021

View reviewed changes

msohailhussain merged commit 930400f into master Aug 24, 2021

msohailhussain deleted the uzair/config-v2 branch August 24, 2021 22:58

		$this->audiences = isset($config['audiences']) ? $config['audiences'] : [];
		$this->events = $config['events'] ?: [];

		$this->_botFiltering = isset($config['botFiltering'])? $config['botFiltering'] : null;
		$this->attributes = isset($config['attributes']) ? $config['attributes'] : [];

		$experimentMapFlag1 = $optimizelyConfig->getFeaturesMap()['flag1']->getExperimentsMap(); // 9300000007569
		$experimentMapFlag2 = $optimizelyConfig->getFeaturesMap()['flag2']->getExperimentsMap(); // 9300000007573

	* @return array of optimizelyExperiments as delivery rules .
	* @return array of optimizelyExperiments as delivery rules.

		@@ -145,6 +249,7 @@ protected function getVariablesMap(Experiment $experiment, Variation $variation)
		}

		$featureFlag = $this->experimentIdFeatureMap[$experimentId];

feat(OptimizelyConfig): Add new fields to OptimizelyConfig #230

feat(OptimizelyConfig): Add new fields to OptimizelyConfig #230

Conversation

ozayr-zaviar commented Aug 22, 2021 • edited Loading

Summary

Test plan

coveralls commented Aug 22, 2021

coveralls commented Aug 22, 2021

coveralls commented Aug 22, 2021

coveralls commented Aug 22, 2021 • edited Loading

coveralls commented Aug 22, 2021

Choose a reason for hiding this comment

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

zashraf1985 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

zashraf1985 Aug 23, 2021 • edited Loading

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

msohailhussain 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

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 left a comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

ozayr-zaviar commented Aug 22, 2021 •

edited

Loading

coveralls commented Aug 22, 2021 •

edited

Loading

zashraf1985 Aug 23, 2021 •

edited

Loading