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

ozayr-zaviar · 2021-07-29T13:12:35Z

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://travis-ci.com/github/optimizely/fullstack-sdk-compatibility-suite/builds/235036023

coveralls · 2021-07-29T13:41:53Z

Coverage increased (+0.009%) to 99.486% when pulling 4edf298 on uzair/opt-config-v2 into bc46127 on master.

msohailhussain · 2021-08-04T21:19:58Z

lib/optimizely/optimizely_config.rb

+      optly_typed_audiences = []
+
+      @project_config.audiences.each do |old_audience|
+        next unless old_audience['id'] != '$opt_dummy_audience'


not sure, this is the right assumption. it may come in the middle of the array.

msohailhussain · 2021-08-04T21:20:27Z

lib/optimizely/optimizely_config.rb

+        optly_typed_audiences.push(
+          'id' => type_audience['id'],
+          'name' => type_audience['name'],
+          'conditions' => type_audience['conditions'].to_json


this should be string

msohailhussain · 2021-08-04T21:21:21Z

lib/optimizely/optimizely_config.rb

      config = {
+        'sdk_key' => @project_config.sdk_key,


msohailhussain · 2021-08-04T21:21:31Z

lib/optimizely/optimizely_config.rb

+        'attributes' => get_attributes_list(@project_config.attributes),
+        'audiences' => @audiences,
+        'events' => get_events_list(@project_config.events),
+        'environment_key' => @project_config.environment_key


should be camelcase.

zashraf1985 · 2021-08-05T22:07:51Z

lib/optimizely/optimizely_config.rb

          }
        )
      end
    end
+
+    def get_attributes_list(attributes)
+      attributes.reduce([]) do |attributes_list, attribute|


map makes more sense here as you are transforming an array to another array

zashraf1985 · 2021-08-05T22:08:07Z

lib/optimizely/optimizely_config.rb

+    end
+
+    def get_events_list(events)
+      events.reduce([]) do |events_list, event|


use map here instead of reduce

zashraf1985 · 2021-08-05T22:10:41Z

lib/optimizely/optimizely_config.rb

+    end
+
+    def lookup_name_from_id(audience_id, audiences_map)
+      name = audiences_map[audience_id] || audience_id


Can you check if only audiences_map[audience_id] || audience_id works?

zashraf1985 · 2021-08-05T22:20:03Z

lib/optimizely/optimizely_config.rb

+      name
+    end
+
+    def stringify_conditions(conditions, audiences_map)


This code looks complex. It will be great if you can add detailed comments inside the method with each branch condition.

zashraf1985 · 2021-08-05T22:21:55Z

lib/optimizely/optimizely_config.rb

+    end
+
+    def replace_ids_with_names(conditions, audiences_map)
+      if !conditions.empty?


NIT! Could use ternary operator here?

zashraf1985 · 2021-08-05T22:22:53Z

lib/optimizely/optimizely_config.rb

+        experiments.each do |experiment|
+          optly_exp = {
+            'id' => experiment['id'],
+            'key' => experiment['key'],
+            'variationsMap' => get_variation_map(feature_id, experiment, feature_variables_map),
+            'audiences' => replace_ids_with_names(experiment.fetch('audienceConditions', []), audiences_id_map) || ''
+          }
+          delivery_rules.push(optly_exp)
+        end


could use experiment.map and directly assign to delivery_rules

zashraf1985 · 2021-08-05T23:31:53Z

lib/optimizely/optimizely_config.rb

+      @audiences = []
+      type_audiences = @project_config.typed_audiences
+      optly_typed_audiences = []
+      id_lookup_dict = {}


I assume it holds audiences only, if yes, the name should reflect that.

zashraf1985 · 2021-08-05T23:37:30Z

lib/optimizely/optimizely_config.rb

+      type_audiences = @project_config.typed_audiences
+      optly_typed_audiences = []


whats the difference between type_audiences and optly_typed_audiences. It should be clear from the variable names that why there are two arrays being used. Actually you dont even need to use type_audiences one. You can use #project_config.typed_audiences directly to loop below. optly_typed_audiences does not sound consistent with others as no other variable are prefixed with optly. You should use typed_audiences to hold temp audiences and directly reference @project_config.typed_audiences to loop through the original array.

zashraf1985 · 2021-08-05T23:39:35Z

lib/optimizely/optimizely_config.rb

+      experiments_key_map = {}
+      experiments_id_map.each do |_, experiment|
+        experiments_key_map[experiment['key']] = experiment
+      end
+      experiments_key_map


You could probably use reduce here?

keppel2 · 2021-08-11T01:06:19Z

A passing continuous integration (CI) can be helpful for reviews, but I will check to see if there is anything I can help with.

keppel2 · 2021-08-11T01:06:52Z

lib/optimizely/optimizely_config.rb

+          'featureEnabled' => variation['featureEnabled'],
+          'variablesMap' => get_merged_variables_map(variation, feature_id, feature_variables_map)
+        }
+#         variation_object['featureEnabled'] = variation['featureEnabled'] if @project_config.feature_experiment?(experiment['id']) || @project_config.rollout_experiment?(experiment['id'])


It looks like doing so will help with rubocop.

keppel2 · 2021-08-11T01:19:17Z

rkeppel@unknown- ruby-sdk % rubocop
Inspecting 85 files
.............................................C..............................C.....C..

Offenses:

lib/optimizely/optimizely_config.rb:109:1: C: Layout/CommentIndentation: Incorrect indentation detected (column 0 instead of 8).
#         variation_object['featureEnabled'] = variation['featureEnabled'] if @project_config.feature_experiment?(experiment['id']) || @project_config.rollout_experiment?(experiment['id'])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/optimizely_config_spec.rb:764:1: C: Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.
spec/spec_params.rb:1428:8: C: Layout/MultilineArrayBraceLayout: The closing array brace must be on the line after the last array element when the opening brace is on a separate line from the first array element.
      }],
       ^

85 files inspected, 3 offenses detected
rkeppel@unknown- ruby-sdk % echo $?
1

keppel2 · 2021-08-11T01:20:26Z

rubocop worked for me on my MacBook. You can run it before pushing in changes to see if it will pass.

keppel2 · 2021-08-11T01:23:56Z

diff --git a/lib/optimizely/optimizely_config.rb b/lib/optimizely/optimizely_config.rb
index 6537ccc..9226ea9 100644
--- a/lib/optimizely/optimizely_config.rb
+++ b/lib/optimizely/optimizely_config.rb
@@ -106,7 +106,6 @@ module Optimizely
           'featureEnabled' => variation['featureEnabled'],
           'variablesMap' => get_merged_variables_map(variation, feature_id, feature_variables_map)
         }
-#         variation_object['featureEnabled'] = variation['featureEnabled'] if @project_config.feature_experiment?(experiment['id']) || @project_config.rollout_experiment?(experiment['id'])
         variations_map.update(variation['key'] => variation_object)
       end
     end
diff --git a/spec/optimizely_config_spec.rb b/spec/optimizely_config_spec.rb
index a0b372a..07ba5ae 100644
--- a/spec/optimizely_config_spec.rb
+++ b/spec/optimizely_config_spec.rb
@@ -761,7 +761,6 @@ describe Optimizely::OptimizelyConfig do
       result = optimizely_config.send(:replace_ids_with_names, audience_condition, audiences_map)
       expect(result).to eq(expected_audience_outputs[index])
     end
-
   end
 
   it 'should return correct config revision' do
diff --git a/spec/spec_params.rb b/spec/spec_params.rb
index eab7ad8..4647f4e 100644
--- a/spec/spec_params.rb
+++ b/spec/spec_params.rb
@@ -1425,7 +1425,8 @@ module OptimizelySpec
         'variables' => [],
         'id' => '2027',
         'key' => 'flag_1'
-      }],
+      }
+    ],
     'experiments' => [],
     'audiences' => [
       {

This seems to quiet rubocop.

keppel2 · 2021-08-11T08:32:29Z

FEATURE_ENABLED is a known issue @msohailhussain . This appears to be the only thing wrong in the FSC.

keppel2 · 2021-08-11T08:34:24Z

Looks good--nice job with passing the PR tests.

keppel2 · 2021-08-11T08:38:26Z

Fullstack-prod-suite seems stuck in travis. Another push may wake it up.

zashraf1985

LGTM! Added a few more minor comments. Preapproving based on the assumption that those comments will be addressed before merging

zashraf1985 · 2021-08-11T14:16:00Z

lib/optimizely/optimizely_config.rb

+        audience_id_lookup_dict[typed_audience['id']] = typed_audience['id']
+      end
+
+      @project_config.audiences.each do |old_audience|


NIT: Why name it old_audience. why cant you just call it audience?

zashraf1985 · 2021-08-11T14:19:08Z

lib/optimizely/optimizely_config.rb

@@ -111,10 +161,107 @@ def get_features_map(all_experiments_map)
                  'value' => variable['defaultValue']
                }
              )
-            end
+            end,
+            'experimentRules' => feature['experimentIds'].reduce([]) do |experiments_map, experiment_id|


use map here. you are transforming array into an array here, not reducing to a value or object.

jaeopt

LGTM
A couple of comments about deprecation suggested.

lib/optimizely/optimizely_config.rb

jaeopt · 2021-08-11T17:28:24Z

lib/optimizely/optimizely_config.rb

      config = {
+        'sdkKey' => @project_config.sdk_key,
        'datafile' => @project_config.datafile,
        'experimentsMap' => experiments_map_object,


Can we add a note about key-conflict issue on top of 'experimentsMap':

// This experimentsMap is for experiments of legacy projects only.
// For flag projects, experiment keys are not guaranteed to be unique
// across multiple flags, so this map may not include all experiments
// when keys conflict.

Need a period at the end of the first sentence - "This experimentsMap is for experiments of legacy projects only"

jaeopt

Deprecation message should be changed

jaeopt · 2021-08-12T18:32:51Z

lib/optimizely/optimizely_config.rb

+            # This experimentsMap is for experiments of legacy projects only.
+            # For flag projects, experiment keys are not guaranteed to be unique
+            # across multiple flags, so this map may not include all experiments
+            # when keys conflict. Use experimentRules and deliveryRules instead.


This message should be chanaged to:
"This experimentsMap is deprecated. Use experimentRules and deliveryRules instead."

The one at top level ("experimentsMap" in OptimizleyConfig) looks good.

jaeopt

LGTM

ozayr-zaviar added 2 commits July 27, 2021 20:21

init

06ba039

experiment and delivery rules implemented

61706dc

optimizely-codereview-bot assigned ozayr-zaviar Jul 29, 2021

rubocop error fixed

f065fa4

ozayr-zaviar added 5 commits July 29, 2021 18:50

variable name corrected

2099a46

syntax error fixed

9fa1c8c

testapp creash case fixed

9c25e55

FSC test cases fixed

5e89329

refactoring

7101864

msohailhussain reviewed Aug 4, 2021

View reviewed changes

lib/optimizely/optimizely_config.rb Outdated

config = {

'sdk_key' => @project_config.sdk_key,

Copy link

Contributor

msohailhussain Aug 4, 2021

Choose a reason for hiding this comment

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

sdkKey

msohailhussain reviewed Aug 4, 2021

View reviewed changes

ozayr-zaviar added 2 commits August 5, 2021 05:49

comments addressed

e9bfc9a

unit tests added

f8843dd

zashraf1985 suggested changes Aug 5, 2021

View reviewed changes

ozayr-zaviar and others added 5 commits August 6, 2021 19:15

comments addressed and unit test for audiences added

d3ac144

rollout condition added

c9fd3fe

experiment in rollout fucntion added

50ed1fd

no need to check featureEnabled.

f3f9cae

Merge branch 'master' into uzair/opt-config-v2

fe21ff7

msohailhussain marked this pull request as ready for review August 10, 2021 21:48

msohailhussain requested a review from a team as a code owner August 10, 2021 21:48

msohailhussain requested a review from jaeopt August 10, 2021 21:48

msohailhussain unassigned ozayr-zaviar Aug 10, 2021

optimizely-codereview-bot requested review from msohailhussain and zashraf1985 August 10, 2021 21:49

keppel2 reviewed Aug 11, 2021

View reviewed changes

ozayr-zaviar added 2 commits August 11, 2021 12:01

comments addressed

8938534

unit test fixed

a12b39e

zashraf1985 approved these changes Aug 11, 2021

View reviewed changes

jaeopt approved these changes Aug 11, 2021

View reviewed changes

ozayr-zaviar added 2 commits August 12, 2021 00:17

comments addressed

94ca43d

rollout experiment unit test added

e6b1e8d

keppel2 self-requested a review August 11, 2021 22:31

keppel2 approved these changes Aug 11, 2021

View reviewed changes

depreciation comments added

eb45b1a

jaeopt requested changes Aug 12, 2021

View reviewed changes

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

rephrased comment.

4edf298

jaeopt approved these changes Aug 12, 2021

View reviewed changes

msohailhussain merged commit 3a30a00 into master Aug 12, 2021

msohailhussain deleted the uzair/opt-config-v2 branch August 12, 2021 23:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ozayr-zaviar commented Jul 29, 2021 •

edited

Loading

coveralls commented Jul 29, 2021 •

edited

Loading

msohailhussain Aug 4, 2021

msohailhussain Aug 4, 2021

msohailhussain Aug 4, 2021

msohailhussain Aug 4, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021

zashraf1985 Aug 5, 2021 •

edited

Loading

zashraf1985 Aug 5, 2021 •

edited

Loading

keppel2 commented Aug 11, 2021

keppel2 Aug 11, 2021

keppel2 Aug 11, 2021

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021 •

edited

Loading

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021 •

edited

Loading

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021

zashraf1985 left a comment

zashraf1985 Aug 11, 2021

zashraf1985 Aug 11, 2021

jaeopt left a comment

jaeopt Aug 11, 2021 •

edited

Loading

jaeopt Aug 11, 2021

jaeopt left a comment

jaeopt Aug 12, 2021

jaeopt left a comment

		type_audiences = @project_config.typed_audiences
		optly_typed_audiences = []

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

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

Conversation

ozayr-zaviar commented Jul 29, 2021 • edited Loading

Summary

Test plan

coveralls commented Jul 29, 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

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 5, 2021 • edited Loading

Choose a reason for hiding this comment

zashraf1985 Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

keppel2 commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021 • edited Loading

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021 • edited Loading

keppel2 commented Aug 11, 2021

keppel2 commented Aug 11, 2021

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

jaeopt left a comment

Choose a reason for hiding this comment

jaeopt Aug 11, 2021 • edited Loading

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

jaeopt left a comment

Choose a reason for hiding this comment

ozayr-zaviar commented Jul 29, 2021 •

edited

Loading

coveralls commented Jul 29, 2021 •

edited

Loading

zashraf1985 Aug 5, 2021 •

edited

Loading

zashraf1985 Aug 5, 2021 •

edited

Loading

keppel2 commented Aug 11, 2021 •

edited

Loading

keppel2 commented Aug 11, 2021 •

edited

Loading

jaeopt Aug 11, 2021 •

edited

Loading