Skip to content

Commit 6ae9ee3

Browse files
oakbanimikeproeng37
oakbani
authored and
mikeproeng37
committed
Bug - isFeatureEnabled always returns false for Rollout (#82)
1 parent deadcde commit 6ae9ee3

File tree

6 files changed

+138
-21
lines changed

6 files changed

+138
-21
lines changed

‎src/Optimizely/Bucketer.php‎

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
* limitations under the License.
1616
*/
1717
namespace Optimizely;
18+
1819
use Monolog\Logger;
1920
use Optimizely\Entity\Experiment;
2021
use Optimizely\Entity\Variation;
@@ -146,30 +147,45 @@ public function bucket(ProjectConfig $config, Experiment $experiment, $bucketing
146147
if ($userExperimentId != $experiment->getId()) {
147148
$this->_logger->log(
148149
Logger::INFO,
149-
sprintf('User "%s" is not in experiment %s of group %s.',
150-
$userId, $experiment->getKey(), $experiment->getGroupId()
151-
));
150+
sprintf(
151+
'User "%s" is not in experiment %s of group %s.',
152+
$userId,
153+
$experiment->getKey(),
154+
$experiment->getGroupId()
155+
)
156+
);
152157
return new Variation();
153158
}
154159

155-
$this->_logger->log(Logger::INFO,
156-
sprintf('User "%s" is in experiment %s of group %s.',
157-
$userId, $experiment->getKey(), $experiment->getGroupId()
158-
));
160+
$this->_logger->log(
161+
Logger::INFO,
162+
sprintf(
163+
'User "%s" is in experiment %s of group %s.',
164+
$userId,
165+
$experiment->getKey(),
166+
$experiment->getGroupId()
167+
)
168+
);
159169
}
160170

161171
// Bucket user if not in whitelist and in group (if any).
162172
$variationId = $this->findBucket($bucketingId, $userId, $experiment->getId(), $experiment->getTrafficAllocation());
163173
if (!empty($variationId)) {
164174
$variation = $config->getVariationFromId($experiment->getKey(), $variationId);
165-
$this->_logger->log(Logger::INFO,
166-
sprintf('User "%s" is in variation %s of experiment %s.',
167-
$userId, $variation->getKey(), $experiment->getKey()
168-
));
175+
176+
$this->_logger->log(
177+
Logger::INFO,
178+
sprintf(
179+
'User "%s" is in variation %s of experiment %s.',
180+
$userId,
181+
$variation->getKey(),
182+
$experiment->getKey()
183+
)
184+
);
169185
return $variation;
170186
}
171-
172-
$this->_logger->log(Logger::INFO, sprintf('User "%s" is in no variation.', $userId));
187+
188+
$this->_logger->log(Logger::INFO, sprintf('User "%s" is in no variation.', $userId));
173189
return new Variation();
174190
}
175191
}

‎src/Optimizely/Entity/Experiment.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -296,4 +296,5 @@ public function isUserInForcedVariation($userId)
296296
$forcedVariations = $this->getForcedVariations();
297297
return !is_null($forcedVariations) && isset($forcedVariations[$userId]);
298298
}
299+
299300
}

‎src/Optimizely/ProjectConfig.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,10 @@ public function __construct($datafile, $logger, $errorHandler)
206206
$this->_experimentKeyMap = array_merge($this->_experimentKeyMap, $experimentsInGroup);
207207
}
208208

209+
$this->_variationKeyMap = [];
210+
$this->_variationIdMap = [];
211+
$this->_experimentIdMap = [];
212+
209213
forEach(array_values($this->_experimentKeyMap) as $experiment) {
210214
$this->_variationKeyMap[$experiment->getKey()] = [];
211215
$this->_variationIdMap[$experiment->getKey()] = [];
@@ -223,10 +227,28 @@ public function __construct($datafile, $logger, $errorHandler)
223227
$audience->setConditionsList($conditionDecoder->getConditionsList());
224228
}
225229

226-
foreach(array_values($this->_rollouts) as $rollout){
230+
$rolloutVariationIdMap = [];
231+
$rolloutVariationKeyMap = [];
232+
foreach($this->_rollouts as $rollout){
233+
227234
$this->_rolloutIdMap[$rollout->getId()] = $rollout;
235+
236+
foreach($rollout->getExperiments() as $rule){
237+
$rolloutVariationIdMap[$rule->getKey()] = [];
238+
$rolloutVariationKeyMap[$rule->getKey()] = [];
239+
240+
$variations = $rule->getVariations();
241+
foreach($variations as $variation){
242+
$rolloutVariationIdMap[$rule->getKey()][$variation->getId()] = $variation;
243+
$rolloutVariationKeyMap[$rule->getKey()][$variation->getKey()] = $variation;
244+
}
245+
}
228246
}
229247

248+
// Add variations for rollout experiments to variationIdMap and variationKeyMap
249+
$this->_variationIdMap = $this->_variationIdMap + $rolloutVariationIdMap;
250+
$this->_variationKeyMap = $this->_variationKeyMap + $rolloutVariationKeyMap;
251+
230252
foreach(array_values($this->_featureFlags) as $featureFlag){
231253
$this->_featureKeyMap[$featureFlag->getKey()] = $featureFlag;
232254
}

‎tests/BucketerTest.php‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
<?php
22
/**
3-
* Copyright 2016, Optimizely
3+
* Copyright 2016-2017, Optimizely
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
2020
use Monolog\Logger;
2121
use Optimizely\Bucketer;
2222
use Optimizely\Entity\Experiment;
23+
use Optimizely\Entity\Rollout;
2324
use Optimizely\Entity\Variation;
2425
use Optimizely\ErrorHandler\NoOpErrorHandler;
2526
use Optimizely\Logger\DefaultLogger;
@@ -369,4 +370,50 @@ public function testBucketVariationGroupedExperimentsWithBucketingId()
369370
)
370371
);
371372
}
373+
374+
public function testBucketWithRolloutRule()
375+
{
376+
$bucketer = new TestBucketer($this->loggerMock);
377+
$bucketer->setBucketValues([4999, 5000]);
378+
379+
$rollout = $this->config->getRolloutFromId('166660');
380+
$rollout_rule = null;
381+
$rollout_rules = $rollout->getExperiments();
382+
foreach ($rollout_rules as $rule) {
383+
if ($rule->getKey() == 'rollout_1_exp_3') {
384+
$rollout_rule = $rule;
385+
}
386+
}
387+
388+
$expectedVariation = new Variation(
389+
'177778',
390+
'177778',
391+
[
392+
[
393+
"id"=> "155556",
394+
"value"=> "false"
395+
]
396+
]
397+
);
398+
399+
$this->assertEquals(
400+
$expectedVariation,
401+
$bucketer->bucket(
402+
$this->config,
403+
$rollout_rule,
404+
'bucketingId',
405+
'userId'
406+
)
407+
);
408+
409+
$this->assertEquals(
410+
new Variation(),
411+
$bucketer->bucket(
412+
$this->config,
413+
$rollout_rule,
414+
'bucketingId',
415+
'userId'
416+
)
417+
);
418+
}
372419
}

‎tests/ProjectConfigTest.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ public function testInit()
145145
// Check variation key map
146146
$variationKeyMap = new \ReflectionProperty(ProjectConfig::class, '_variationKeyMap');
147147
$variationKeyMap->setAccessible(true);
148+
148149
$this->assertEquals([
149150
'test_experiment' => [
150151
'control' => $this->config->getVariationFromKey('test_experiment', 'control'),
@@ -179,6 +180,21 @@ public function testInit()
179180
'test_experiment_integer_feature' => [
180181
'control' => $this->config->getVariationFromKey('test_experiment_integer_feature', 'control'),
181182
'variation' => $this->config->getVariationFromKey('test_experiment_integer_feature', 'variation')
183+
],
184+
'rollout_1_exp_1' => [
185+
'177771' => $this->config->getVariationFromKey('rollout_1_exp_1', '177771')
186+
],
187+
'rollout_1_exp_2' => [
188+
'177773' => $this->config->getVariationFromKey('rollout_1_exp_2', '177773')
189+
],
190+
'rollout_1_exp_3' => [
191+
'177778' => $this->config->getVariationFromKey('rollout_1_exp_3', '177778')
192+
],
193+
'rollout_2_exp_1' => [
194+
'177775' => $this->config->getVariationFromKey('rollout_2_exp_1', '177775')
195+
],
196+
'rollout_2_exp_2' => [
197+
'177780' => $this->config->getVariationFromKey('rollout_2_exp_2', '177780')
182198
]
183199
], $variationKeyMap->getValue($this->config));
184200

@@ -219,6 +235,21 @@ public function testInit()
219235
'test_experiment_integer_feature' => [
220236
'122242' => $this->config->getVariationFromId('test_experiment_integer_feature', '122242'),
221237
'122243' => $this->config->getVariationFromId('test_experiment_integer_feature', '122243')
238+
],
239+
'rollout_1_exp_1' => [
240+
'177771' => $this->config->getVariationFromId('rollout_1_exp_1', '177771')
241+
],
242+
'rollout_1_exp_2' => [
243+
'177773' => $this->config->getVariationFromId('rollout_1_exp_2', '177773')
244+
],
245+
'rollout_1_exp_3' => [
246+
'177778' => $this->config->getVariationFromId('rollout_1_exp_3', '177778')
247+
],
248+
'rollout_2_exp_1' => [
249+
'177775' => $this->config->getVariationFromId('rollout_2_exp_1', '177775')
250+
],
251+
'rollout_2_exp_2' => [
252+
'177780' => $this->config->getVariationFromId('rollout_2_exp_2', '177780')
222253
]
223254
], $variationIdMap->getValue($this->config));
224255

‎tests/TestData.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -601,7 +601,7 @@
601601
"experiments": [
602602
{
603603
"id": "177770",
604-
"key": "177770",
604+
"key": "rollout_1_exp_1",
605605
"status": "Running",
606606
"layerId": "166660",
607607
"audienceIds": [
@@ -628,7 +628,7 @@
628628
},
629629
{
630630
"id": "177772",
631-
"key": "177772",
631+
"key": "rollout_1_exp_2",
632632
"status": "Running",
633633
"layerId": "166660",
634634
"audienceIds": [
@@ -655,7 +655,7 @@
655655
},
656656
{
657657
"id": "177776",
658-
"key": "177776",
658+
"key": "rollout_1_exp_3",
659659
"status": "Running",
660660
"layerId": "166660",
661661
"audienceIds": [
@@ -676,7 +676,7 @@
676676
"trafficAllocation": [
677677
{
678678
"entityId": "177778",
679-
"endOfRange": 10000
679+
"endOfRange": 5000
680680
}
681681
]
682682
}
@@ -687,7 +687,7 @@
687687
"experiments": [
688688
{
689689
"id": "177774",
690-
"key": "177774",
690+
"key": "rollout_2_exp_1",
691691
"status": "Running",
692692
"layerId": "166661",
693693
"audienceIds": [
@@ -711,7 +711,7 @@
711711
},
712712
{
713713
"id": "177779",
714-
"key": "177779",
714+
"key": "rollout_2_exp_2",
715715
"status": "Running",
716716
"layerId": "166661",
717717
"audienceIds": [

0 commit comments

Comments
 (0)