Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ForcedDecisions): add forced-decisions APIs to OptimizelyUserContext #233

Merged
merged 28 commits into from
Nov 4, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
8607ff2
Work in progress
NomanShoaib Sep 21, 2021
e013fea
fix
NomanShoaib Sep 21, 2021
0b1f517
implemented forcedDecision
NomanShoaib Sep 22, 2021
cd4176c
fixed issues
NomanShoaib Sep 24, 2021
5c753aa
fixed all errors
NomanShoaib Sep 27, 2021
0fd717b
fixed old tests and logs errors
NomanShoaib Sep 27, 2021
7e2400f
fix
NomanShoaib Sep 28, 2021
9cb1d03
fix
NomanShoaib Sep 28, 2021
ae48a61
added remove forcedecision removeall force decision apis
NomanShoaib Sep 28, 2021
6a9a5f0
Unit tests added and fixed some bugs
NomanShoaib Oct 1, 2021
60ae438
Added unit tests of forceddecision
NomanShoaib Oct 4, 2021
df1c82f
Comments fixed
NomanShoaib Oct 6, 2021
80e5026
return true in removeallforcedecision even if forceddecision list is …
NomanShoaib Oct 6, 2021
979a0e7
Added depreciation warning in
NomanShoaib Oct 6, 2021
d22ad11
Fixed experimentId null or empty error in impression event
NomanShoaib Oct 6, 2021
a5843c2
fixed logging and !=null comments
NomanShoaib Oct 6, 2021
f387f89
lint fix
NomanShoaib Oct 6, 2021
3557793
removed depreciation warning
NomanShoaib Oct 7, 2021
8797195
made rule key compulsory instead of optional and moved it to 2nd arg …
NomanShoaib Oct 11, 2021
83ef326
- moved the two methods of remove and remove all forceddecision after…
NomanShoaib Oct 11, 2021
b7b980d
Added Decided by forceddecision reason
NomanShoaib Oct 12, 2021
280371c
Added reasons in test case
NomanShoaib Oct 13, 2021
9168430
DatafileprojectConfig line 248 added verification that redundant vari…
NomanShoaib Oct 14, 2021
2bb8937
fixed logging and unit tests
NomanShoaib Oct 15, 2021
0df9732
lint fix
NomanShoaib Oct 20, 2021
bc3bb03
OptimizelyDecisionContext and OptimizelyForcedDecision implemented
ozayr-zaviar Oct 29, 2021
7566d55
impression corrected (#234)
ozayr-zaviar Nov 3, 2021
32122b7
comments addressed
ozayr-zaviar Nov 4, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Optimizely/DecisionService/DecisionService.php
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ public function getVariationFromDeliveryRule(ProjectConfigInterface $projectConf
$skipToEveryoneElse = false;
// check forced-decision first
$rule = $rules[$ruleIndex];
list($forcedDecisionResponse, $reasons) = $user->findValidatedForcedDecision($flagKey, $rule->getKey(), $options);
list($forcedDecisionResponse, $reasons) = $user->findValidatedForcedDecision($flagKey, $rule->getKey());

$decideReasons = array_merge($decideReasons, $reasons);
if ($forcedDecisionResponse) {
Expand Down
16 changes: 11 additions & 5 deletions src/Optimizely/Optimizely.php
Original file line number Diff line number Diff line change
Expand Up @@ -353,8 +353,7 @@ public function decide(OptimizelyUserContext $userContext, $key, array $decideOp
// get decision
$decision = null;
// check forced-decisions first
list($forcedDecisionResponse, $reasons) = $userContext->findValidatedForcedDecision($flagKey, $ruleKey, $decideOptions);
$decideReasons = array_merge($decideReasons, $reasons);
list($forcedDecisionResponse, $reasons) = $userContext->findValidatedForcedDecision($flagKey, $ruleKey);
if ($forcedDecisionResponse) {
$decision = new FeatureDecision(null, $forcedDecisionResponse, FeatureDecision::DECISION_SOURCE_FEATURE_TEST, $decideReasons);
} else {
Expand All @@ -366,8 +365,8 @@ public function decide(OptimizelyUserContext $userContext, $key, array $decideOp
$decideOptions
);
}

$decideReasons = $decision->getReasons();
$decideReasons = array_merge($decideReasons, $reasons);
$decideReasons = array_merge($decideReasons, $decision->getReasons());
$variation = $decision->getVariation();

if ($variation) {
Expand Down Expand Up @@ -1263,7 +1262,14 @@ private function getFeatureVariableValueFromVariation($featureFlagKey, $variable
*/
public function isValid()
{
return $this->getConfig() !== null;
if (!$this->getConfig()) {
$this->_logger->log(
Logger::ERROR,
"Optimizely SDK not configured properly yet."

Choose a reason for hiding this comment

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

Is there a static message for this that can be referenced?

);
return false;
}
return true;
}

/**
Expand Down
70 changes: 36 additions & 34 deletions src/Optimizely/OptimizelyUserContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,77 +35,76 @@ public function __construct(Optimizely $optimizelyClient, $userId, array $attrib
$this->forcedDecisions = $forcedDecisions;
}

public function removeForcedDecision($flagKey, $ruleKey = null)
public function setForcedDecision($flagKey, $ruleKey, $variationKey)
{
// check if SDK is ready
if (!$this->optimizelyClient->isValid()) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add error log message here and below for sdkNotReady?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can not add logging in optimizelyUsercontext in order to do so we have to pass logger over here.

}
if (empty($flagKey)) {
return false;
}
$index = $this->findExistingRuleAndFlagKey($flagKey, $ruleKey);
if ($index != -1) {
array_splice($this->forcedDecisions, $index, 1);
return true;
$this->forcedDecisions[$index]->setVariationKey($variationKey);
} else {
if (!$this->forcedDecisions) {
$this->forcedDecisions = array();
}
array_push($this->forcedDecisions, new ForcedDecision($flagKey, $ruleKey, $variationKey));
}
return false;
return true;
}

public function removeAllForcedDecisions()
public function getForcedDecision($flagKey, $ruleKey = null)
{
// check if SDK is ready
if (!$this->optimizelyClient->isValid()) {
return false;
return null;
}

$this->forcedDecisions = [];
return true;
return $this->findForcedDecision($flagKey, $ruleKey);
}

public function setForcedDecision($flagKey, $variationKey, $ruleKey = null)
public function removeForcedDecision($flagKey, $ruleKey = null)
{
// check if SDK is ready
if (!$this->optimizelyClient->isValid()) {
return false;
}
if (empty($flagKey)) {
return false;
}
$index = $this->findExistingRuleAndFlagKey($flagKey, $ruleKey);
if ($index != -1) {
$this->forcedDecisions[$index]->setVariationKey($variationKey);
} else {
if (!$this->forcedDecisions) {
$this->forcedDecisions = array();
}
array_push($this->forcedDecisions, new ForcedDecision($flagKey, $ruleKey, $variationKey));
array_splice($this->forcedDecisions, $index, 1);
return true;
}
return true;
return false;
}

public function getForcedDecision($flagKey, $ruleKey = null)
public function removeAllForcedDecisions()
{
// check if SDK is ready
if (!$this->optimizelyClient->isValid()) {
return null;
return false;
}
return $this->findForcedDecision($flagKey, $ruleKey);

$this->forcedDecisions = [];
return true;
}

public function findValidatedForcedDecision($flagKey, $ruleKey, array $options = [])
public function findValidatedForcedDecision($flagKey, $ruleKey)
{
$decideReasons = [];
$variationKey = $this->findForcedDecision($flagKey, $ruleKey);
$variation = null;
if ($variationKey) {
$variation = $this->optimizelyClient->getFlagVariationByKey($flagKey, $variationKey);
$message = sprintf('Variation "%s" is mapped to %s and user "%s" in the forced decision map.', $variationKey, $ruleKey? 'flag "'.$flagKey.'", rule "'.$ruleKey.'"': 'flag "'.$flagKey.'"', $this->userId);

$decideReasons[] = $message;
return [$variation, $decideReasons];
} else {
$message = sprintf('Invalid variation is mapped to %s and user "%s" in the forced decision map.', $ruleKey? 'flag "'.$flagKey.'", rule "'.$ruleKey.'"': 'flag "'.$flagKey.'"', $this->userId);

$decideReasons[] = $message;
if ($variation) {
array_push($decideReasons, 'Decided by forced decision.');
array_push($decideReasons, sprintf('Variation "%s" is mapped to %s and user "%s" in the forced decision map.', $variationKey, $ruleKey? 'flag "'.$flagKey.'", rule "'.$ruleKey.'"': 'flag "'.$flagKey.'"', $this->userId));
} else {
array_push($decideReasons, sprintf('Invalid variation is mapped to %s and user "%s" in the forced decision map.', $ruleKey? 'flag "'.$flagKey.'", rule "'.$ruleKey.'"': 'flag "'.$flagKey.'"', $this->userId));
}
}
return [null, $decideReasons];
return [$variation, $decideReasons];
}

private function findExistingRuleAndFlagKey($flagKey, $ruleKey)
Expand All @@ -123,7 +122,10 @@ private function findExistingRuleAndFlagKey($flagKey, $ruleKey)
public function findForcedDecision($flagKey, $ruleKey)
{
$foundVariationKey = null;
if ($this->forcedDecisions && count($this->forcedDecisions) == 0) {
if (!$this->forcedDecisions) {

Choose a reason for hiding this comment

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

Any reason to do this vs. isset?

return null;
}
if (count($this->forcedDecisions) == 0) {
return null;
}
$index = $this->findExistingRuleAndFlagKey($flagKey, $ruleKey);
Expand Down
2 changes: 1 addition & 1 deletion tests/ConfigTests/DatafileProjectConfigTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2020,2021, Optimizely
* Copyright 2016-2020, 2021, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down
8 changes: 0 additions & 8 deletions tests/DecisionServiceTests/DecisionServiceTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1080,11 +1080,9 @@ public function testGetVariationForFeatureRolloutWhenUserIsNotBucketedInTheTarge
$expected_variation,
FeatureDecision::DECISION_SOURCE_ROLLOUT,
[
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_1" and user "user_1" in the forced decision map.',
'Audiences for rule 1 collectively evaluated to TRUE.',
'User "user_1" meets condition for targeting rule "1".',
'User "user_1" is not in the traffic group for targeting rule "1". Checking Everyone Else rule now.',
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_3" and user "user_1" in the forced decision map.',
'Audiences for rule Everyone Else collectively evaluated to TRUE.',
'User "user_1" meets condition for targeting rule "Everyone Else".',
'User "user_1" is in the traffic group of targeting rule "Everyone Else".'
Expand Down Expand Up @@ -1168,13 +1166,10 @@ public function testGetVariationForFeatureRolloutWhenUserDoesNotQualifyForAnyTar
$expected_variation,
FeatureDecision::DECISION_SOURCE_ROLLOUT,
[
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_1" and user "user_1" in the forced decision map.',
'Audiences for rule 1 collectively evaluated to FALSE.' ,
'User "user_1" does not meet conditions for targeting rule "1".',
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_2" and user "user_1" in the forced decision map.',
'Audiences for rule 2 collectively evaluated to FALSE.',
'User "user_1" does not meet conditions for targeting rule "2".',
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_3" and user "user_1" in the forced decision map.',
'Audiences for rule Everyone Else collectively evaluated to TRUE.',
'User "user_1" meets condition for targeting rule "Everyone Else".',
'User "user_1" is in the traffic group of targeting rule "Everyone Else".'
Expand Down Expand Up @@ -1262,13 +1257,10 @@ public function testGetVariationForFeatureRolloutLogging()
$expected_variation,
FeatureDecision::DECISION_SOURCE_ROLLOUT,
[
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_1" and user "user_1" in the forced decision map.',
'Audiences for rule 1 collectively evaluated to FALSE.',
'User "user_1" does not meet conditions for targeting rule "1".',
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_2" and user "user_1" in the forced decision map.',
'Audiences for rule 2 collectively evaluated to FALSE.',
'User "user_1" does not meet conditions for targeting rule "2".',
'Invalid variation is mapped to flag "boolean_single_variable_feature", rule "rollout_1_exp_3" and user "user_1" in the forced decision map.',
'Audiences for rule Everyone Else collectively evaluated to TRUE.',
'User "user_1" meets condition for targeting rule "Everyone Else".',
'User "user_1" is in the traffic group of targeting rule "Everyone Else".'
Expand Down
8 changes: 3 additions & 5 deletions tests/OptimizelyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ public function testSetForcedDecisionExperimentRuleToDecisionSendImpressionAndNo

$optimizelyMock->notificationCenter = $this->notificationCenterMock;

$this->assertTrue($userContext->setForcedDecision('double_single_variable_feature', 'variation', 'test_experiment_double_feature'));
$this->assertTrue($userContext->setForcedDecision('double_single_variable_feature', 'test_experiment_double_feature', 'variation'));
$optimizelyDecision = $optimizelyMock->decide($userContext, 'double_single_variable_feature');
$expectedOptimizelyDecision = new OptimizelyDecision(
'variation',
Expand Down Expand Up @@ -642,7 +642,7 @@ public function testSetForcedDecisionDeliveryRuleToDecisionSendImpressionAndNoti

$optimizelyMock->notificationCenter = $this->notificationCenterMock;

$this->assertTrue($userContext->setForcedDecision('boolean_single_variable_feature', '177773', 'rollout_1_exp_2'));
$this->assertTrue($userContext->setForcedDecision('boolean_single_variable_feature', 'rollout_1_exp_2', '177773'));
$optimizelyDecision = $optimizelyMock->decide($userContext, 'boolean_single_variable_feature');
$expectedOptimizelyDecision = new OptimizelyDecision(
'177773',
Expand Down Expand Up @@ -722,7 +722,7 @@ public function testSetForcedDecisionFlagToDecisionSendImpressionAndNotification

$optimizelyMock->notificationCenter = $this->notificationCenterMock;

$this->assertTrue($userContext->setForcedDecision('double_single_variable_feature', 'variation'));
$this->assertTrue($userContext->setForcedDecision('double_single_variable_feature', null, 'variation'));
$optimizelyDecision = $optimizelyMock->decide($userContext, 'double_single_variable_feature');
$expectedOptimizelyDecision = new OptimizelyDecision(
'variation',
Expand Down Expand Up @@ -1914,7 +1914,6 @@ public function testDecideOptionIncludeReasons()
->getMock();

$expectedReasons = [
'Invalid variation is mapped to flag "double_single_variable_feature", rule "test_experiment_double_feature" and user "test_user" in the forced decision map.',
'Audiences for experiment "test_experiment_double_feature" collectively evaluated to TRUE.',
'Assigned bucket 4513 to user "test_user" with bucketing ID "test_user".',
'User "test_user" is in variation control of experiment test_experiment_double_feature.',
Expand Down Expand Up @@ -1987,7 +1986,6 @@ public function testDecideLogsWhenAudienceEvalFails()
->getMock();

$expectedReasons = [
'Invalid variation is mapped to flag "multi_variate_feature", rule "test_experiment_multivariate" and user "test_user" in the forced decision map.',
'Audiences for experiment "test_experiment_multivariate" collectively evaluated to FALSE.',
'User "test_user" does not meet conditions to be in experiment "test_experiment_multivariate".',
"The user 'test_user' is not bucketed into any of the experiments using the feature 'multi_variate_feature'.",
Expand Down
Loading