Skip to content

Commit 299f482

Browse files
rashidspmikeproeng37
rashidsp
authored and
mikeproeng37
committed
Fixes variable getter not returning the correct value bug (#86)
1 parent 5567b1e commit 299f482

File tree

6 files changed

+88
-46
lines changed

6 files changed

+88
-46
lines changed

‎src/Optimizely/DecisionService/DecisionService.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ public function getVariationForFeatureExperiment(FeatureFlag $featureFlag, $user
239239
"The user '{$userId}' is bucketed into experiment '{$experiment->getKey()}' of feature '{$feature_flag_key}'."
240240
);
241241

242-
return new FeatureDecision($experiment->getId(), $variation->getId(), FeatureDecision::DECISION_SOURCE_EXPERIMENT);
242+
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_EXPERIMENT);
243243
}
244244
}
245245

@@ -308,7 +308,7 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
308308
// Evaluate if user satisfies the traffic allocation for this rollout rule
309309
$variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketing_id, $userId);
310310
if ($variation && $variation->getKey()) {
311-
return new FeatureDecision($experiment->getId(), $variation->getId(), FeatureDecision::DECISION_SOURCE_ROLLOUT);
311+
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT);
312312
} else {
313313
$this->_logger->log(
314314
Logger::DEBUG,
@@ -321,7 +321,7 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
321321
$experiment = $rolloutRules[sizeof($rolloutRules)-1];
322322
$variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketing_id, $userId);
323323
if ($variation && $variation->getKey()) {
324-
return new FeatureDecision($experiment->getId(), $variation->getId(), FeatureDecision::DECISION_SOURCE_ROLLOUT);
324+
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT);
325325
} else {
326326
$this->_logger->log(
327327
Logger::DEBUG,

‎src/Optimizely/DecisionService/FeatureDecision.php‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,16 @@ class FeatureDecision
2222
const DECISION_SOURCE_ROLLOUT = 'rollout';
2323

2424
/**
25-
* @var string The ID experiment in this decision.
25+
* The experiment in this decision.
26+
* @var Experiment
2627
*/
27-
private $_experimentId;
28+
private $_experiment;
2829

2930
/**
30-
* @var string The ID variation in this decision.
31+
* The variation in this decision.
32+
* @var Variation
3133
*/
32-
private $_variationId;
34+
private $_variation;
3335

3436
/**
3537
* The source of the decision. Either DECISION_SOURCE_EXPERIMENT or DECISION_SOURCE_ROLLOUT
@@ -40,25 +42,25 @@ class FeatureDecision
4042
/**
4143
* FeatureDecision constructor.
4244
*
43-
* @param $experimentId
44-
* @param $variationId
45+
* @param $experiment
46+
* @param $variation
4547
* @param $source
4648
*/
47-
public function __construct($experimentId, $variationId, $source)
49+
public function __construct($experiment, $variation, $source)
4850
{
49-
$this->_experimentId = $experimentId;
50-
$this->_variationId = $variationId;
51+
$this->_experiment = $experiment;
52+
$this->_variation = $variation;
5153
$this->_source = $source;
5254
}
5355

54-
public function getExperimentId()
56+
public function getExperiment()
5557
{
56-
return $this->_experimentId;
58+
return $this->_experiment;
5759
}
5860

59-
public function getVariationId()
61+
public function getVariation()
6062
{
61-
return $this->_variationId;
63+
return $this->_variation;
6264
}
6365

6466
public function getSource()

‎src/Optimizely/Optimizely.php‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -483,13 +483,10 @@ public function isFeatureEnabled($featureFlagKey, $userId, $attributes = null)
483483
return false;
484484
}
485485

486-
$experiment_id = $decision->getExperimentId();
487-
$variation_id = $decision->getVariationId();
486+
$experiment = $decision->getExperiment();
487+
$variation = $decision->getVariation();
488488

489489
if ($decision->getSource() == FeatureDecision::DECISION_SOURCE_EXPERIMENT) {
490-
$experiment = $this->_config->getExperimentFromId($experiment_id);
491-
$variation = $this->_config->getVariationFromId($experiment->getKey(), $variation_id);
492-
493490
$this->sendImpressionEvent($experiment->getKey(), $variation->getKey(), $userId, $attributes);
494491

495492
} else {
@@ -586,10 +583,7 @@ public function getFeatureVariableValueForType(
586583
$this->_logger->log(Logger::INFO, "User '{$userId}'is not in any variation, ".
587584
"returning default value '{$variable_value}'.");
588585
} else {
589-
$experiment_id = $decision->getExperimentId();
590-
$variation_id = $decision->getVariationId();
591-
$experiment = $this->_config->getExperimentFromId($experiment_id);
592-
$variation = $this->_config->getVariationFromId($experiment->getKey(), $variation_id);
586+
$variation = $decision->getVariation();
593587
$variable_usage = $variation->getVariableUsageById($variable->getId());
594588
if ($variable_usage) {
595589
$variable_value = $variable_usage->getValue();

‎src/Optimizely/ProjectConfig.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ public function __construct($datafile, $logger, $errorHandler)
209209
$this->_variationKeyMap = [];
210210
$this->_variationIdMap = [];
211211
$this->_experimentIdMap = [];
212-
212+
213213
forEach(array_values($this->_experimentKeyMap) as $experiment) {
214214
$this->_variationKeyMap[$experiment->getKey()] = [];
215215
$this->_variationIdMap[$experiment->getKey()] = [];

‎tests/DecisionServiceTests/DecisionServiceTest.php‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -682,7 +682,7 @@ public function testGetVariationForFeatureExperimentGivenNonMutexGroupAndUserIsB
682682
->will($this->returnValue($variation));
683683

684684
$feature_flag = $this->config->getFeatureFlagFromKey('multi_variate_feature');
685-
$expected_decision = new FeatureDecision($experiment->getId(), $variation->getId(), FeatureDecision::DECISION_SOURCE_EXPERIMENT);
685+
$expected_decision = new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_EXPERIMENT);
686686

687687
$this->loggerMock->expects($this->at(0))
688688
->method('log')
@@ -708,7 +708,7 @@ public function testGetVariationForFeatureExperimentGivenMutexGroupAndUserIsBuck
708708

709709
$mutex_exp = $this->config->getExperimentFromKey('group_experiment_1');
710710
$variation = $mutex_exp->getVariations()[0];
711-
$expected_decision = new FeatureDecision($mutex_exp->getId(), $variation->getId(), FeatureDecision::DECISION_SOURCE_EXPERIMENT);
711+
$expected_decision = new FeatureDecision($mutex_exp, $variation, FeatureDecision::DECISION_SOURCE_EXPERIMENT);
712712

713713
$feature_flag = $this->config->getFeatureFlagFromKey('boolean_feature');
714714
$this->loggerMock->expects($this->at(0))
@@ -760,8 +760,8 @@ public function testGetVariationForFeatureWhenTheUserIsBucketedIntoFeatureExperi
760760
$expected_experiment = $this->config->getExperimentFromId($expected_experiment_id);
761761
$expected_variation = $expected_experiment->getVariations()[0];
762762
$expected_decision = new FeatureDecision(
763-
$expected_experiment->getId(),
764-
$expected_variation->getId(),
763+
$expected_experiment,
764+
$expected_variation,
765765
FeatureDecision::DECISION_SOURCE_EXPERIMENT
766766
);
767767

@@ -789,8 +789,8 @@ public function testGetVariationForFeatureWhenBucketedToFeatureRollout()
789789
$experiment = $rollout->getExperiments()[0];
790790
$expected_variation = $experiment->getVariations()[0];
791791
$expected_decision = new FeatureDecision(
792-
$experiment->getId(),
793-
$expected_variation->getId(),
792+
$experiment,
793+
$expected_variation,
794794
FeatureDecision::DECISION_SOURCE_ROLLOUT
795795
);
796796

@@ -925,8 +925,8 @@ public function testGetVariationForFeatureRolloutWhenUserIsBucketedInTheTargetin
925925
$experiment = $rollout->getExperiments()[0];
926926
$expected_variation = $experiment->getVariations()[0];
927927
$expected_decision = new FeatureDecision(
928-
$experiment->getId(),
929-
$expected_variation->getId(),
928+
$experiment,
929+
$expected_variation,
930930
FeatureDecision::DECISION_SOURCE_ROLLOUT
931931
);
932932

@@ -966,8 +966,8 @@ public function testGetVariationForFeatureRolloutWhenUserIsNotBucketedInTheTarge
966966
$experiment2 = $rollout->getExperiments()[2];
967967
$expected_variation = $experiment2->getVariations()[0];
968968
$expected_decision = new FeatureDecision(
969-
$experiment2->getId(),
970-
$expected_variation->getId(),
969+
$experiment2,
970+
$expected_variation,
971971
FeatureDecision::DECISION_SOURCE_ROLLOUT
972972
);
973973

@@ -1074,8 +1074,8 @@ public function testGetVariationForFeatureRolloutWhenUserDoesNotQualifyForAnyTar
10741074
$experiment2 = $rollout->getExperiments()[2];
10751075
$expected_variation = $experiment2->getVariations()[0];
10761076
$expected_decision = new FeatureDecision(
1077-
$experiment2->getId(),
1078-
$expected_variation->getId(),
1077+
$experiment2,
1078+
$expected_variation,
10791079
FeatureDecision::DECISION_SOURCE_ROLLOUT
10801080
);
10811081

‎tests/OptimizelyTest.php‎

Lines changed: 54 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2084,8 +2084,8 @@ public function testIsFeatureEnabledGivenFeatureFlagIsEnabledAndUserIsBeingExper
20842084
$variation = $this->projectConfig->getVariationFromKey('test_experiment_double_feature', 'control');
20852085

20862086
$expected_decision = new FeatureDecision(
2087-
$experiment->getId(),
2088-
$variation->getId(),
2087+
$experiment,
2088+
$variation,
20892089
FeatureDecision::DECISION_SOURCE_EXPERIMENT
20902090
);
20912091

@@ -2129,8 +2129,8 @@ public function testIsFeatureEnabledGivenFeatureFlagIsEnabledAndUserIsNotBeingEx
21292129
$experiment = $rollout->getExperiments()[0];
21302130
$variation = $experiment->getVariations()[0];
21312131
$expected_decision = new FeatureDecision(
2132-
$experiment->getId(),
2133-
$variation->getId(),
2132+
$experiment,
2133+
$variation,
21342134
FeatureDecision::DECISION_SOURCE_ROLLOUT
21352135
);
21362136

@@ -2412,8 +2412,8 @@ public function testGetFeatureVariableValueForTypeGivenFeatureFlagIsEnabledForUs
24122412
$experiment = $this->projectConfig->getExperimentFromKey('test_experiment_double_feature');
24132413
$variation = $this->projectConfig->getVariationFromKey('test_experiment_double_feature', 'control');
24142414
$expected_decision = new FeatureDecision(
2415-
$experiment->getId(),
2416-
$variation->getId(),
2415+
$experiment,
2416+
$variation,
24172417
FeatureDecision::DECISION_SOURCE_EXPERIMENT
24182418
);
24192419

@@ -2435,6 +2435,52 @@ public function testGetFeatureVariableValueForTypeGivenFeatureFlagIsEnabledForUs
24352435
);
24362436
}
24372437

2438+
public function testGetFeatureVariableValueForTypeWithRolloutRule()
2439+
{
2440+
// should return specific value
2441+
$decisionServiceMock = $this->getMockBuilder(DecisionService::class)
2442+
->setConstructorArgs(array($this->loggerMock, $this->projectConfig))
2443+
->setMethods(array('getVariationForFeature'))
2444+
->getMock();
2445+
2446+
$decisionService = new \ReflectionProperty(Optimizely::class, '_decisionService');
2447+
$decisionService->setAccessible(true);
2448+
$decisionService->setValue($this->optimizelyObject, $decisionServiceMock);
2449+
2450+
$featureFlag = $this->projectConfig->getFeatureFlagFromKey('boolean_single_variable_feature');
2451+
$rolloutId = $featureFlag->getRolloutId();
2452+
$rollout = $this->projectConfig->getRolloutFromId($rolloutId);
2453+
$experiment = $rollout->getExperiments()[0];
2454+
$expectedVariation = $experiment->getVariations()[0];
2455+
$expectedDecision = new FeatureDecision(
2456+
$experiment,
2457+
$expectedVariation,
2458+
FeatureDecision::DECISION_SOURCE_ROLLOUT
2459+
);
2460+
2461+
$decisionServiceMock->expects($this->exactly(1))
2462+
->method('getVariationForFeature')
2463+
->will($this->returnValue($expectedDecision));
2464+
2465+
$this->loggerMock->expects($this->exactly(1))
2466+
->method('log')
2467+
->with(
2468+
Logger::INFO,
2469+
"Returning variable value 'true' for variation '177771' ".
2470+
"of feature flag 'boolean_single_variable_feature'"
2471+
);
2472+
2473+
$this->assertSame(
2474+
$this->optimizelyObject->getFeatureVariableBoolean(
2475+
'boolean_single_variable_feature',
2476+
'boolean_variable',
2477+
'user_id',
2478+
[]
2479+
),
2480+
true
2481+
);
2482+
}
2483+
24382484
public function testGetFeatureVariableValueForTypeGivenFeatureFlagIsEnabledForUserAndVariableNotInVariation()
24392485
{
24402486
// should return default value
@@ -2452,8 +2498,8 @@ public function testGetFeatureVariableValueForTypeGivenFeatureFlagIsEnabledForUs
24522498
$experiment = $this->projectConfig->getExperimentFromKey('test_experiment_integer_feature');
24532499
$variation = $this->projectConfig->getVariationFromKey('test_experiment_integer_feature', 'control');
24542500
$expected_decision = new FeatureDecision(
2455-
$experiment->getId(),
2456-
$variation->getId(),
2501+
$experiment,
2502+
$variation,
24572503
FeatureDecision::DECISION_SOURCE_EXPERIMENT
24582504
);
24592505

0 commit comments

Comments
 (0)