fix: Send correct impression event data for projects with duplicate rule keys (#690)

yavorona · yavorona · commit 3d1a233acf7f · 2021-07-15T11:23:50.000-07:00
* Send correct impression event data + fix unit tests

* Add getExperimentId unit tests

* Add unit test for getVariationIdFromExperimentIdAndVariationKey + clean up

* fix emitNotificationCenterActivate
diff --git a/packages/optimizely-sdk/lib/core/decision/index.tests.js b/packages/optimizely-sdk/lib/core/decision/index.tests.js
@@ -35,6 +35,23 @@ describe('lib/core/decision', function() {
     });
   });
 
+  describe('getExperimentId method', function() {
+    it('should return null when experiment is null', function() {
+      var experimentId = decision.getExperimentId(rolloutDecisionObj);
+      assert.strictEqual(experimentId, null);
+    });
+
+    it('should return null when experiment is not defined', function() {
+      var experimentId = decision.getExperimentId({});
+      assert.strictEqual(experimentId, null);
+    });
+
+    it('should return experiment id when experiment is defined', function() {
+      var experimentId = decision.getExperimentId(featureTestDecisionObj);
+      assert.strictEqual(experimentId, '594098');
+    });
+  });
+
   describe('getVariationKey method', function() {
     it('should return empty string when variation is null', function() {
       var variationKey = decision.getVariationKey(rolloutDecisionObj);
diff --git a/packages/optimizely-sdk/lib/core/decision/index.ts b/packages/optimizely-sdk/lib/core/decision/index.ts
@@ -42,3 +42,12 @@ export function getVariationKey(decisionObj: DecisionObj): string {
 export function getFeatureEnabledFromVariation(decisionObj: DecisionObj): boolean {
    return decisionObj.variation?.featureEnabled ?? false;
 }
+
+/**
+ * Get experiment id from the provided decision object
+ * @param   {DecisionObj} decisionObj       Object representing decision
+ * @returns {string}                        Experiment id or null if experiment is null
+ */
+export function getExperimentId(decisionObj: DecisionObj): string | null {
+   return decisionObj.experiment?.id ?? null;
+}
diff --git a/packages/optimizely-sdk/lib/core/event_builder/event_helpers.tests.js b/packages/optimizely-sdk/lib/core/event_builder/event_helpers.tests.js
@@ -18,6 +18,7 @@ import { assert } from 'chai';
 
 import fns from '../../utils/fns';
 import * as projectConfig from '../project_config';
+import * as decision from '../decision';
 import { buildImpressionEvent, buildConversionEvent } from './event_helpers';
 
 describe('lib/event_builder/event_helpers', function() {
@@ -33,19 +34,21 @@ describe('lib/event_builder/event_helpers', function() {
     };
 
     sinon.stub(projectConfig, 'getEventId');
-    sinon.stub(projectConfig, 'getVariationIdFromExperimentAndVariationKey');
-    sinon.stub(projectConfig, 'getExperimentId');
+    sinon.stub(projectConfig, 'getVariationIdFromExperimentIdAndVariationKey');
     sinon.stub(projectConfig, 'getLayerId');
     sinon.stub(projectConfig, 'getAttributeId');
 
+    sinon.stub(decision, 'getExperimentId');
+
     sinon.stub(fns, 'uuid').returns('uuid');
     sinon.stub(fns, 'currentTimestamp').returns(100);
   });
 
   afterEach(function() {
+    decision.getExperimentId.restore();
+
     projectConfig.getEventId.restore();
-    projectConfig.getVariationIdFromExperimentAndVariationKey.restore();
-    projectConfig.getExperimentId.restore();
+    projectConfig.getVariationIdFromExperimentIdAndVariationKey.restore();
     projectConfig.getLayerId.restore();
     projectConfig.getAttributeId.restore();
 
@@ -56,7 +59,7 @@ describe('lib/event_builder/event_helpers', function() {
   describe('buildImpressionEvent', function() {
     describe('when botFiltering and anonymizeIP are true', function() {
       it('should build an ImpressionEvent with the correct attributes', function() {
-        var decision = {
+        var decisionObj = {
           experiment: {
             key: 'exp1',
             status: 'Running',
@@ -79,17 +82,19 @@ describe('lib/event_builder/event_helpers', function() {
           },
           decisionSource: 'experiment',
         }
-        projectConfig.getVariationIdFromExperimentAndVariationKey
-          .withArgs(configObj, 'exp1', 'var1')
+        decision.getExperimentId.withArgs(decisionObj).returns('exp1-id');
+
+        projectConfig.getVariationIdFromExperimentIdAndVariationKey
+          .withArgs(configObj, 'exp1-id', 'var1')
           .returns('var1-id');
-        projectConfig.getExperimentId.withArgs(configObj, 'exp1').returns('exp1-id');
         projectConfig.getLayerId.withArgs(configObj, 'exp1-id').returns('layer-id');
 
         projectConfig.getAttributeId.withArgs(configObj, 'plan_type').returns('plan_type_id');
 
+
         var result = buildImpressionEvent({
           configObj: configObj,
-          decisionObj: decision,
+          decisionObj: decisionObj,
           enabled: true,
           flagKey: 'flagkey1',
           userId: 'user1',
@@ -148,7 +153,7 @@ describe('lib/event_builder/event_helpers', function() {
 
     describe('when botFiltering and anonymizeIP are undefined', function() {
       it('should create an ImpressionEvent with the correct attributes', function() {
-        var decision = {
+        var decisionObj = {
           experiment: {
             key: 'exp1',
             status: 'Running',
@@ -171,10 +176,11 @@ describe('lib/event_builder/event_helpers', function() {
           },
           decisionSource: 'experiment',
         }
-        projectConfig.getVariationIdFromExperimentAndVariationKey
-          .withArgs(configObj, 'exp1', 'var1')
+        decision.getExperimentId.withArgs(decisionObj).returns('exp1-id');
+
+        projectConfig.getVariationIdFromExperimentIdAndVariationKey
+          .withArgs(configObj, 'exp1-id', 'var1')
           .returns('var1-id');
-        projectConfig.getExperimentId.withArgs(configObj, 'exp1').returns('exp1-id');
         projectConfig.getLayerId.withArgs(configObj, 'exp1-id').returns('layer-id');
 
         projectConfig.getAttributeId.withArgs(configObj, 'plan_type').returns('plan_type_id');
@@ -184,7 +190,7 @@ describe('lib/event_builder/event_helpers', function() {
 
         var result = buildImpressionEvent({
           configObj: configObj,
-          decisionObj: decision,
+          decisionObj: decisionObj,
           flagKey: 'flagkey1',
           enabled: false,
           userId: 'user1',
diff --git a/packages/optimizely-sdk/lib/core/event_builder/event_helpers.ts b/packages/optimizely-sdk/lib/core/event_builder/event_helpers.ts
@@ -25,9 +25,8 @@ import { DecisionObj } from '../decision_service';
 import {
   getAttributeId,
   getEventId,
-  getExperimentId,
   getLayerId,
-  getVariationIdFromExperimentAndVariationKey,
+  getVariationIdFromExperimentIdAndVariationKey,
   ProjectConfig,
 } from '../project_config';
 
@@ -134,10 +133,10 @@ export const buildImpressionEvent = function({
 
   const ruleType = decisionObj.decisionSource;
   const experimentKey = decision.getExperimentKey(decisionObj);
+  const experimentId = decision.getExperimentId(decisionObj);
   const variationKey = decision.getVariationKey(decisionObj);
 
-  const variationId = experimentKey !== '' && variationKey !== '' ? getVariationIdFromExperimentAndVariationKey(configObj, experimentKey, variationKey) : null;
-  const experimentId = experimentKey !== '' ? getExperimentId(configObj, experimentKey) : null;
+  const variationId = experimentId !== null && variationKey !== '' ? getVariationIdFromExperimentIdAndVariationKey(configObj, experimentId, variationKey) : null;
   const layerId = experimentId !== null ? getLayerId(configObj, experimentId) : null;
 
   return {
diff --git a/packages/optimizely-sdk/lib/core/project_config/index.tests.js b/packages/optimizely-sdk/lib/core/project_config/index.tests.js
@@ -392,6 +392,19 @@ describe('lib/core/project_config', function() {
       });
     });
 
+    describe('#getVariationIdFromExperimentIdAndVariationKey', function() {
+      it('should return the variation id for the given experiment id and variation key', function() {
+        assert.strictEqual(
+          projectConfig.getVariationIdFromExperimentIdAndVariationKey(
+            configObj,
+            testData.experiments[0].id,
+            testData.experiments[0].variations[0].key
+          ),
+          testData.experiments[0].variations[0].id
+        );
+      });
+    });
+
     describe('#getSendFlagDecisionsValue', function() {
       it('should return false when sendFlagDecisions is undefined', function() {
         configObj.sendFlagDecisions = undefined;
diff --git a/packages/optimizely-sdk/lib/core/project_config/index.ts b/packages/optimizely-sdk/lib/core/project_config/index.ts
@@ -385,6 +385,26 @@ export const getVariationIdFromExperimentAndVariationKey = function(
   return null;
 };
 
+/**
+ * Get the variation ID given the experiment id and variation key
+ * @param  {ProjectConfig}  projectConfig   Object representing project configuration
+ * @param  {string}         experimentId    Id of the experiment the variation belongs to
+ * @param  {string}         variationKey    The variation key
+ * @return {string|null}    Variation ID or null
+ */
+export const getVariationIdFromExperimentIdAndVariationKey = function(
+  projectConfig: ProjectConfig,
+  experimentId: string,
+  variationKey: string
+): string | null {
+  const experiment = projectConfig.experimentIdMap[experimentId];
+  if (experiment.variationKeyMap.hasOwnProperty(variationKey)) {
+    return experiment.variationKeyMap[variationKey].id;
+  }
+
+  return null;
+};
+
 /**
  * Get experiment from provided experiment key
  * @param  {ProjectConfig}  projectConfig  Object representing project configuration
@@ -719,28 +739,29 @@ export const getSendFlagDecisionsValue = function(projectConfig: ProjectConfig):
 }
 
 export default {
-  createProjectConfig: createProjectConfig,
-  getExperimentId: getExperimentId,
-  getLayerId: getLayerId,
-  getAttributeId: getAttributeId,
-  getEventId: getEventId,
-  getExperimentStatus: getExperimentStatus,
-  isActive: isActive,
-  isRunning: isRunning,
-  getExperimentAudienceConditions: getExperimentAudienceConditions,
-  getVariationKeyFromId: getVariationKeyFromId,
-  getVariationIdFromExperimentAndVariationKey: getVariationIdFromExperimentAndVariationKey,
-  getExperimentFromKey: getExperimentFromKey,
-  getTrafficAllocation: getTrafficAllocation,
-  getExperimentFromId: getExperimentFromId,
-  getFeatureFromKey: getFeatureFromKey,
-  getVariableForFeature: getVariableForFeature,
-  getVariableValueForVariation: getVariableValueForVariation,
-  getTypeCastValue: getTypeCastValue,
-  getSendFlagDecisionsValue: getSendFlagDecisionsValue,
-  getAudiencesById: getAudiencesById,
-  eventWithKeyExists: eventWithKeyExists,
-  isFeatureExperiment: isFeatureExperiment,
-  toDatafile: toDatafile,
-  tryCreatingProjectConfig: tryCreatingProjectConfig,
+  createProjectConfig,
+  getExperimentId,
+  getLayerId,
+  getAttributeId,
+  getEventId,
+  getExperimentStatus,
+  isActive,
+  isRunning,
+  getExperimentAudienceConditions,
+  getVariationKeyFromId,
+  getVariationIdFromExperimentAndVariationKey,
+  getVariationIdFromExperimentIdAndVariationKey,
+  getExperimentFromKey,
+  getTrafficAllocation,
+  getExperimentFromId,
+  getFeatureFromKey,
+  getVariableForFeature,
+  getVariableValueForVariation,
+  getTypeCastValue,
+  getSendFlagDecisionsValue,
+  getAudiencesById,
+  eventWithKeyExists,
+  isFeatureExperiment,
+  toDatafile,
+  tryCreatingProjectConfig,
 };
diff --git a/packages/optimizely-sdk/lib/optimizely/index.ts b/packages/optimizely-sdk/lib/optimizely/index.ts
@@ -341,14 +341,15 @@ export default class Optimizely {
 
     const ruleType = decisionObj.decisionSource;
     const experimentKey = decision.getExperimentKey(decisionObj);
+    const experimentId = decision.getExperimentId(decisionObj);
     const variationKey = decision.getVariationKey(decisionObj);
 
-    let experimentId: string | null = null;
-    let variationId: string | null = null;
+    let variationId = null;
+    let experiment;
 
-    if (experimentKey !== '' && variationKey !== '') {
-      variationId = projectConfig.getVariationIdFromExperimentAndVariationKey(configObj, experimentKey, variationKey);
-      experimentId = projectConfig.getExperimentId(configObj, experimentKey);
+    if (experimentId !== null && variationKey !== '') {
+      variationId = projectConfig.getVariationIdFromExperimentIdAndVariationKey(configObj, experimentId, variationKey);
+      experiment = configObj.experimentIdMap[experimentId];
     }
 
     const impressionEventOptions = {
@@ -366,7 +367,6 @@ export default class Optimizely {
       logger: this.logger,
     };
     const impressionEvent = getImpressionEvent(impressionEventOptions);
-    const experiment = configObj.experimentKeyMap[experimentKey];
     let variation;
     if (experiment && experiment.variationKeyMap && variationKey !== '') {
       variation = experiment.variationKeyMap[variationKey];
