Skip to content

Commit 72116b4

Browse files
yavoronayavorona
authored
feat: Implement DecisionResponse in decision hierarchy (#640)
* Create DecisionResponse interface * Refactor bucketer.bucket reasons and partially decision_service reasons * Create class DecisionResponse instead of an interface * Update getVariation and getVariationForFeature methods in decision service * Update getForcedVariation method in decision service * Clean up and start fixing decision_service tests * Update decision_service unit tests * Update optimizely unit tests * Update bucketer unit tests * Update Copyright year to include 2021 * Fix reported reasons and begin adding new unit tests * Add test case when returning stored variation * Add unit test when user is in forced variation * Add more unit tests * Change __checkIfUserIsInAudience method to return decision response and update unit tests * Add unit test when user is not bucketed into targeting rule * Refactor test setup and add more tests * Clean up and add bucketing in group test * Complete INCLUDE_REASONS default option tests * Add decision response unit tests * Incorporate comments part 1 * Add missing unit tests when user has forced variation * Change DecisionReponse class to interface and update unit tests * Remove console.log statement
1 parent b94149b commit 72116b4

File tree

9 files changed

+1450
-354
lines changed

9 files changed

+1450
-354
lines changed

‎packages/optimizely-sdk/lib/core/bucketer/index.js‎

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Copyright 2016, 2019-2020, Optimizely
2+
* Copyright 2016, 2019-2021, Optimizely
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -34,20 +34,22 @@ var RANDOM_POLICY = 'random';
3434

3535
/**
3636
* Determines ID of variation to be shown for the given input params
37-
* @param {Object} bucketerParams
38-
* @param {string} bucketerParams.experimentId
39-
* @param {string} bucketerParams.experimentKey
40-
* @param {string} bucketerParams.userId
41-
* @param {Object[]} bucketerParams.trafficAllocationConfig
42-
* @param {Array} bucketerParams.experimentKeyMap
43-
* @param {Object} bucketerParams.groupIdMap
44-
* @param {Object} bucketerParams.variationIdMap
45-
* @param {string} bucketerParams.varationIdMap[].key
46-
* @param {Object} bucketerParams.logger
47-
* @param {string} bucketerParams.bucketingId
48-
* @return Variation ID that user has been bucketed into, null if user is not bucketed into any experiment
37+
* @param {Object} bucketerParams
38+
* @param {string} bucketerParams.experimentId
39+
* @param {string} bucketerParams.experimentKey
40+
* @param {string} bucketerParams.userId
41+
* @param {Object[]} bucketerParams.trafficAllocationConfig
42+
* @param {Array} bucketerParams.experimentKeyMap
43+
* @param {Object} bucketerParams.groupIdMap
44+
* @param {Object} bucketerParams.variationIdMap
45+
* @param {string} bucketerParams.varationIdMap[].key
46+
* @param {Object} bucketerParams.logger
47+
* @param {string} bucketerParams.bucketingId
48+
* @return {Object} DecisionResponse DecisionResponse containing variation ID that user has been bucketed into,
49+
* null if user is not bucketed into any experiment and the decide reasons.
4950
*/
5051
export var bucket = function(bucketerParams) {
52+
var decideReasons = [];
5153
// Check if user is in a random group; if so, check if user is bucketed into a specific experiment
5254
var experiment = bucketerParams.experimentKeyMap[bucketerParams.experimentKey];
5355
var groupId = experiment['groupId'];
@@ -73,7 +75,11 @@ export var bucket = function(bucketerParams) {
7375
groupId
7476
);
7577
bucketerParams.logger.log(LOG_LEVEL.INFO, notbucketedInAnyExperimentLogMessage);
76-
return null;
78+
decideReasons.push(notbucketedInAnyExperimentLogMessage);
79+
return {
80+
result: null,
81+
reasons: decideReasons,
82+
};
7783
}
7884

7985
// Return if user is bucketed into a different experiment than the one specified
@@ -86,7 +92,11 @@ export var bucket = function(bucketerParams) {
8692
groupId
8793
);
8894
bucketerParams.logger.log(LOG_LEVEL.INFO, notBucketedIntoExperimentOfGroupLogMessage);
89-
return null;
95+
decideReasons.push(notBucketedIntoExperimentOfGroupLogMessage);
96+
return {
97+
result: null,
98+
reasons: decideReasons,
99+
};
90100
}
91101

92102
// Continue bucketing if user is bucketed into specified experiment
@@ -98,6 +108,7 @@ export var bucket = function(bucketerParams) {
98108
groupId
99109
);
100110
bucketerParams.logger.log(LOG_LEVEL.INFO, bucketedIntoExperimentOfGroupLogMessage);
111+
decideReasons.push(bucketedIntoExperimentOfGroupLogMessage);
101112
}
102113
}
103114
var bucketingId = sprintf('%s%s', bucketerParams.bucketingId, bucketerParams.experimentId);
@@ -110,18 +121,26 @@ export var bucket = function(bucketerParams) {
110121
bucketerParams.userId
111122
);
112123
bucketerParams.logger.log(LOG_LEVEL.DEBUG, bucketedUserLogMessage);
124+
decideReasons.push(bucketedUserLogMessage);
113125

114126
var entityId = this._findBucket(bucketValue, bucketerParams.trafficAllocationConfig);
115127

116128
if (!bucketerParams.variationIdMap.hasOwnProperty(entityId)) {
117129
if (entityId) {
118130
var invalidVariationIdLogMessage = sprintf(LOG_MESSAGES.INVALID_VARIATION_ID, MODULE_NAME);
119131
bucketerParams.logger.log(LOG_LEVEL.WARNING, invalidVariationIdLogMessage);
132+
decideReasons.push(invalidVariationIdLogMessage);
120133
}
121-
return null;
134+
return {
135+
result: null,
136+
reasons: decideReasons,
137+
};
122138
}
123139

124-
return entityId;
140+
return {
141+
result: entityId,
142+
reasons: decideReasons,
143+
};
125144
};
126145

127146
/**

‎packages/optimizely-sdk/lib/core/bucketer/index.tests.js‎

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Copyright 2016-2017, 2019-2020, Optimizely
2+
* Copyright 2016-2017, 2019-2021, Optimizely
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -69,10 +69,11 @@ describe('lib/core/bucketer', function() {
6969
bucketer._generateBucketValue.restore();
7070
});
7171

72-
it('should return correct variation ID when provided bucket value', function() {
72+
it('should return decision response with correct variation ID when provided bucket value', function() {
7373
var bucketerParamsTest1 = cloneDeep(bucketerParams);
7474
bucketerParamsTest1.userId = 'ppid1';
75-
expect(bucketer.bucket(bucketerParamsTest1)).to.equal('111128');
75+
var decisionResponse = bucketer.bucket(bucketerParamsTest1);
76+
expect(decisionResponse.result).to.equal('111128');
7677

7778
var bucketedUser_log1 = createdLogger.log.args[0][1];
7879
expect(bucketedUser_log1).to.equal(
@@ -81,7 +82,7 @@ describe('lib/core/bucketer', function() {
8182

8283
var bucketerParamsTest2 = cloneDeep(bucketerParams);
8384
bucketerParamsTest2.userId = 'ppid2';
84-
expect(bucketer.bucket(bucketerParamsTest2)).to.equal(null);
85+
expect(bucketer.bucket(bucketerParamsTest2).result).to.equal(null);
8586

8687
var notBucketedUser_log1 = createdLogger.log.args[1][1];
8788

@@ -126,11 +127,12 @@ describe('lib/core/bucketer', function() {
126127
};
127128
});
128129

129-
it('should return the proper variation for a user in a grouped experiment', function() {
130+
it('should return decision response with the proper variation for a user in a grouped experiment', function() {
130131
bucketerStub.onFirstCall().returns(50);
131132
bucketerStub.onSecondCall().returns(50);
132133

133-
expect(bucketer.bucket(bucketerParams)).to.equal('551');
134+
var decisionResponse = bucketer.bucket(bucketerParams);
135+
expect(decisionResponse.result).to.equal('551');
134136

135137
sinon.assert.calledTwice(bucketerStub);
136138
sinon.assert.callCount(createdLogger.log, 3);
@@ -157,10 +159,11 @@ describe('lib/core/bucketer', function() {
157159
);
158160
});
159161

160-
it('should return null when a user is bucketed into a different grouped experiment than the one speicfied', function() {
162+
it('should return decision response with variation null when a user is bucketed into a different grouped experiment than the one speicfied', function() {
161163
bucketerStub.returns(5000);
162164

163-
expect(bucketer.bucket(bucketerParams)).to.equal(null);
165+
var decisionResponse = bucketer.bucket(bucketerParams);
166+
expect(decisionResponse.result).to.equal(null);
164167

165168
sinon.assert.calledOnce(bucketerStub);
166169
sinon.assert.calledTwice(createdLogger.log);
@@ -181,10 +184,11 @@ describe('lib/core/bucketer', function() {
181184
);
182185
});
183186

184-
it('should return null when a user is not bucketed into any experiments in the random group', function() {
187+
it('should return decision response with variation null when a user is not bucketed into any experiments in the random group', function() {
185188
bucketerStub.returns(50000);
186189

187-
expect(bucketer.bucket(bucketerParams)).to.equal(null);
190+
var decisionResponse = bucketer.bucket(bucketerParams);
191+
expect(decisionResponse.result).to.equal(null);
188192

189193
sinon.assert.calledOnce(bucketerStub);
190194
sinon.assert.calledTwice(createdLogger.log);
@@ -197,10 +201,11 @@ describe('lib/core/bucketer', function() {
197201
expect(log2).to.equal(sprintf(LOG_MESSAGES.USER_NOT_IN_ANY_EXPERIMENT, 'BUCKETER', 'testUser', '666'));
198202
});
199203

200-
it('should return null when a user is bucketed into traffic space of deleted experiment within a random group', function() {
204+
it('should return decision response with variation null when a user is bucketed into traffic space of deleted experiment within a random group', function() {
201205
bucketerStub.returns(9000);
202206

203-
expect(bucketer.bucket(bucketerParams)).to.equal(null);
207+
var decisionResponse = bucketer.bucket(bucketerParams);
208+
expect(decisionResponse.result).to.equal(null);
204209

205210
sinon.assert.calledOnce(bucketerStub);
206211
sinon.assert.calledTwice(createdLogger.log);
@@ -238,10 +243,11 @@ describe('lib/core/bucketer', function() {
238243
};
239244
});
240245

241-
it('should return a variation when a user falls into an experiment within an overlapping group', function() {
246+
it('should return decision response with variation when a user falls into an experiment within an overlapping group', function() {
242247
bucketerStub.returns(0);
243248

244-
expect(bucketer.bucket(bucketerParams)).to.equal('553');
249+
var decisionResponse = bucketer.bucket(bucketerParams);
250+
expect(decisionResponse.result).to.equal('553');
245251

246252
sinon.assert.calledOnce(bucketerStub);
247253
sinon.assert.calledOnce(createdLogger.log);
@@ -250,10 +256,11 @@ describe('lib/core/bucketer', function() {
250256
expect(log1).to.equal(sprintf(LOG_MESSAGES.USER_ASSIGNED_TO_EXPERIMENT_BUCKET, 'BUCKETER', '0', 'testUser'));
251257
});
252258

253-
it('should return null when a user does not fall into an experiment within an overlapping group', function() {
259+
it('should return decision response with variation null when a user does not fall into an experiment within an overlapping group', function() {
254260
bucketerStub.returns(3000);
255261

256-
expect(bucketer.bucket(bucketerParams)).to.equal(null);
262+
var decisionResponse = bucketer.bucket(bucketerParams);
263+
expect(decisionResponse.result).to.equal(null);
257264
});
258265
});
259266
});
@@ -281,10 +288,11 @@ describe('lib/core/bucketer', function() {
281288
};
282289
});
283290

284-
it('should return null', function() {
291+
it('should return decision response with variation null', function() {
285292
var bucketerParamsTest1 = cloneDeep(bucketerParams);
286293
bucketerParamsTest1.userId = 'ppid1';
287-
expect(bucketer.bucket(bucketerParamsTest1)).to.equal(null);
294+
var decisionResponse = bucketer.bucket(bucketerParamsTest1);
295+
expect(decisionResponse.result).to.equal(null);
288296
});
289297

290298
it('should not log an invalid variation ID warning', function() {
@@ -320,10 +328,11 @@ describe('lib/core/bucketer', function() {
320328
};
321329
});
322330

323-
it('should return null', function() {
331+
it('should return decision response with variation null', function() {
324332
var bucketerParamsTest1 = cloneDeep(bucketerParams);
325333
bucketerParamsTest1.userId = 'ppid1';
326-
expect(bucketer.bucket(bucketerParamsTest1)).to.equal(null);
334+
var decisionResponse = bucketer.bucket(bucketerParamsTest1);
335+
expect(decisionResponse.result).to.equal(null);
327336
});
328337
});
329338
});
@@ -373,7 +382,7 @@ describe('lib/core/bucketer', function() {
373382
bucketerParams1['bucketingId'] = '123456789';
374383
bucketerParams1['experimentKey'] = 'testExperiment';
375384
bucketerParams1['experimentId'] = '111127';
376-
expect(bucketer.bucket(bucketerParams1)).to.equal('111129');
385+
expect(bucketer.bucket(bucketerParams1).result).to.equal('111129');
377386
});
378387

379388
it('check that a null bucketing ID defaults to bucketing with the userId', function() {
@@ -382,7 +391,7 @@ describe('lib/core/bucketer', function() {
382391
bucketerParams2['bucketingId'] = null;
383392
bucketerParams2['experimentKey'] = 'testExperiment';
384393
bucketerParams2['experimentId'] = '111127';
385-
expect(bucketer.bucket(bucketerParams2)).to.equal('111128');
394+
expect(bucketer.bucket(bucketerParams2).result).to.equal('111128');
386395
});
387396

388397
it('check that bucketing works with an experiment in group', function() {
@@ -391,7 +400,7 @@ describe('lib/core/bucketer', function() {
391400
bucketerParams4['bucketingId'] = '123456789';
392401
bucketerParams4['experimentKey'] = 'groupExperiment2';
393402
bucketerParams4['experimentId'] = '443';
394-
expect(bucketer.bucket(bucketerParams4)).to.equal('111128');
403+
expect(bucketer.bucket(bucketerParams4).result).to.equal('111128');
395404
});
396405
});
397406
});

‎packages/optimizely-sdk/lib/core/condition_tree_evaluator/index.tests.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/****************************************************************************
2-
* Copyright 2018, 2020, Optimizely, Inc. and contributors *
2+
* Copyright 2018, 2020-2021, Optimizely, Inc. and contributors *
33
* *
44
* Licensed under the Apache License, Version 2.0 (the "License"); *
55
* you may not use this file except in compliance with the License. *
@@ -16,7 +16,7 @@
1616
import sinon from 'sinon';
1717
import { assert } from 'chai';
1818

19-
import * as conditionTreeEvaluator from './';
19+
import * as conditionTreeEvaluator from './';
2020

2121
var conditionA = {
2222
name: 'browser_type',

‎packages/optimizely-sdk/lib/core/decision_service/index.d.ts‎

Lines changed: 35 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* Copyright 2020, Optimizely
2+
* Copyright 2020-2021, Optimizely
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -23,6 +23,11 @@ import {
2323
Variation
2424
} from '../../shared_types';
2525

26+
interface DecisionResponse<T> {
27+
readonly result: T;
28+
readonly reasons: string[];
29+
}
30+
2631
/**
2732
* Creates an instance of the DecisionService.
2833
* @param {Options} options Configuration options
@@ -33,41 +38,42 @@ export function createDecisionService(options: Options): DecisionService;
3338
export interface DecisionService {
3439

3540
/**
36-
* Gets variation where visitor will be bucketed.
37-
* @param {ProjectConfig} configObj The parsed project configuration object
38-
* @param {string} experimentKey
39-
* @param {string} userId
40-
* @param {UserAttributes} attributes
41-
* @return {string|null} The variation the user is bucketed into.
41+
* Gets a decision response containing variation where visitor will be bucketed and the decide reasons.
42+
* @param {ProjectConfig} configObj The parsed project configuration object
43+
* @param {string} experimentKey
44+
* @param {string} userId
45+
* @param {UserAttributes} attributes
46+
* @return {DecisionResponse} DecisionResponse DecisionResponse containing variation
47+
* the user is bucketed into and the decide reasons.
4248
*/
4349
getVariation(
4450
configObj: ProjectConfig,
4551
experimentKey: string,
4652
userId: string,
4753
attributes?: UserAttributes
48-
): string | null;
54+
): DecisionResponse<string | null>;
4955

5056
/**
51-
* Given a feature, user ID, and attributes, returns an object representing a
52-
* decision. If the user was bucketed into a variation for the given feature
53-
* and attributes, the returned decision object will have variation and
57+
* Given a feature, user ID, and attributes, returns a decision response containing an
58+
* object representing a decision and decide reasons. If the user was bucketed into a variation
59+
* for the given feature and attributes, the decision object will have variation and
5460
* experiment properties (both objects), as well as a decisionSource property.
5561
* decisionSource indicates whether the decision was due to a rollout or an
5662
* experiment.
57-
* @param {ProjectConfig} configObj The parsed project configuration object
58-
* @param {FeatureFlag} feature A feature flag object from project configuration
59-
* @param {string} userId A string identifying the user, for bucketing
60-
* @param {unknown} attributes Optional user attributes
61-
* @return {DecisionObj} An object with experiment, variation, and decisionSource
62-
* properties. If the user was not bucketed into a variation, the variation
63-
* property is null.
63+
* @param {ProjectConfig} configObj The parsed project configuration object
64+
* @param {FeatureFlag} feature A feature flag object from project configuration
65+
* @param {string} userId A string identifying the user, for bucketing
66+
* @param {unknown} attributes Optional user attributes
67+
* @return {DecisionResponse} DecisionResponse DecisionResponse containing an object with experiment, variation, and decisionSource
68+
* properties and the decide reasons. If the user was not bucketed into a variation, the
69+
* variation property in decision object is null.
6470
*/
6571
getVariationForFeature(
6672
configObj: ProjectConfig,
6773
feature: FeatureFlag,
6874
userId: string,
6975
attributes: unknown
70-
): DecisionObj;
76+
): DecisionResponse<DecisionObj>;
7177

7278
/**
7379
* Removes forced variation for given userId and experimentKey
@@ -80,12 +86,17 @@ export interface DecisionService {
8086

8187
/**
8288
* Gets the forced variation key for the given user and experiment.
83-
* @param {ProjectConfig} configObj Object representing project configuration
84-
* @param {string} experimentKey Key for experiment.
85-
* @param {string} userId The user Id.
86-
* @return {string|null} Variation key that specifies the variation which the given user and experiment should be forced into.
89+
* @param {ProjectConfig} configObj Object representing project configuration
90+
* @param {string} experimentKey Key for experiment.
91+
* @param {string} userId The user Id.
92+
* @return {DecisionResponse} DecisionResponse DecisionResponse contaning variation key that specifies the variation which
93+
* the given user and experiment should be forced into and decide options.
8794
*/
88-
getForcedVariation(configObj: ProjectConfig, experimentKey: string, userId: string): string | null;
95+
getForcedVariation(
96+
configObj: ProjectConfig,
97+
experimentKey: string,
98+
userId: string
99+
): DecisionResponse<string | null>;
89100

90101
/**
91102
* Sets the forced variation for a user in a given experiment

0 commit comments

Comments
 (0)