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.

Sign up for GitHub

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

Jump to bottom

refact(decisionService): Move ProjectConfig from DecisionService constructor to individual methods #175

Merged
merged 3 commits into from
Jun 27, 2019
Merged

refact(decisionService): Move ProjectConfig from DecisionService constructor to individual methods #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b23840
Move ProjectConfig from DecisionService constructor to individual met…
mfahadahmed Jun 17, 2019
630fcdd
Add strict type.
mfahadahmed Jun 18, 2019
d9c580b
Merge branch 'master' into fahad/move-project-config
mikeproeng37 Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
98 changes: 49 additions & 49 deletions src/Optimizely/DecisionService/DecisionService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,7 @@ class DecisionService
* @var LoggerInterface
*/
private $_logger;

/**
* @var ProjectConfig
*/
private $_projectConfig;


/**
* @var Bucketer
*/
Expand All @@ -70,13 +65,12 @@ class DecisionService
/**
* DecisionService constructor.
*
* @param LoggerInterface $logger
* @param ProjectConfig $projectConfig
* @param LoggerInterface $logger
* @param UserProfileServiceInterface $userProfileService
*/
public function __construct(LoggerInterface $logger, ProjectConfig $projectConfig, UserProfileServiceInterface $userProfileService = null)
public function __construct(LoggerInterface $logger, UserProfileServiceInterface $userProfileService = null)
{
$this->_logger = $logger;
$this->_projectConfig = $projectConfig;
$this->_bucketer = new Bucketer($logger);
$this->_userProfileService = $userProfileService;
}
Expand Down Expand Up @@ -105,13 +99,14 @@ protected function getBucketingId($userId, $userAttributes)
/**
* Determine which variation to show the user.
*
* @param $experiment Experiment Experiment to get the variation for.
* @param $userId string User identifier.
* @param $attributes array Attributes of the user.
* @param $projectConfig ProjectConfig ProjectConfig instance.
* @param $experiment Experiment Experiment to get the variation for.
* @param $userId string User identifier.
* @param $attributes array Attributes of the user.
*
* @return Variation Variation which the user is bucketed into.
*/
public function getVariation(Experiment $experiment, $userId, $attributes = null)
public function getVariation(ProjectConfig $projectConfig, Experiment $experiment, $userId, $attributes = null)
{
$bucketingId = $this->getBucketingId($userId, $attributes);

Expand All @@ -121,13 +116,13 @@ public function getVariation(Experiment $experiment, $userId, $attributes = null
}

// check if a forced variation is set
$forcedVariation = $this->_projectConfig->getForcedVariation($experiment->getKey(), $userId);
$forcedVariation = $projectConfig->getForcedVariation($experiment->getKey(), $userId);
if (!is_null($forcedVariation)) {
return $forcedVariation;
}

// check if the user has been whitelisted
$variation = $this->getWhitelistedVariation($experiment, $userId);
$variation = $this->getWhitelistedVariation($projectConfig, $experiment, $userId);
if (!is_null($variation)) {
return $variation;
}
Expand All @@ -138,22 +133,22 @@ public function getVariation(Experiment $experiment, $userId, $attributes = null
$storedUserProfile = $this->getStoredUserProfile($userId);
if (!is_null($storedUserProfile)) {
$userProfile = $storedUserProfile;
$variation = $this->getStoredVariation($experiment, $userProfile);
$variation = $this->getStoredVariation($projectConfig, $experiment, $userProfile);
if (!is_null($variation)) {
return $variation;
}
}
}

if (!Validator::isUserInExperiment($this->_projectConfig, $experiment, $attributes, $this->_logger)) {
if (!Validator::isUserInExperiment($projectConfig, $experiment, $attributes, $this->_logger)) {
$this->_logger->log(
Logger::INFO,
sprintf('User "%s" does not meet conditions to be in experiment "%s".', $userId, $experiment->getKey())
);
return null;
}

$variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketingId, $userId);
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketingId, $userId);
if (!is_null($variation)) {
$this->saveVariation($experiment, $variation, $userProfile);
}
Expand All @@ -163,26 +158,27 @@ public function getVariation(Experiment $experiment, $userId, $attributes = null
/**
* Get the variation the user is bucketed into for the given FeatureFlag
*
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user ID
* @param array $userAttributes user attributes
* @param ProjectConfig $projectConfig ProjectConfig instance.
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user ID
* @param array $userAttributes user attributes
* @return Decision if getVariationForFeatureExperiment or getVariationForFeatureRollout returns a Decision
* null otherwise
*/
public function getVariationForFeature(FeatureFlag $featureFlag, $userId, $userAttributes)
public function getVariationForFeature(ProjectConfig $projectConfig, FeatureFlag $featureFlag, $userId, $userAttributes)
{
//Evaluate in this order:
//1. Attempt to bucket user into experiment using feature flag.
//2. Attempt to bucket user into rollout using the feature flag.

// Check if the feature flag is under an experiment and the the user is bucketed into one of these experiments
$decision = $this->getVariationForFeatureExperiment($featureFlag, $userId, $userAttributes);
$decision = $this->getVariationForFeatureExperiment($projectConfig, $featureFlag, $userId, $userAttributes);
if ($decision) {
return $decision;
}

// Check if the feature flag has rollout and the user is bucketed into one of it's rules
$decision = $this->getVariationForFeatureRollout($featureFlag, $userId, $userAttributes);
$decision = $this->getVariationForFeatureRollout($projectConfig, $featureFlag, $userId, $userAttributes);
if ($decision) {
$this->_logger->log(
Logger::INFO,
Expand All @@ -203,13 +199,14 @@ public function getVariationForFeature(FeatureFlag $featureFlag, $userId, $userA
/**
* Get the variation if the user is bucketed for one of the experiments on this feature flag
*
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user id
* @param array $userAttributes user userAttributes
* @param ProjectConfig $projectConfig ProjectConfig instance.
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user id
* @param array $userAttributes user userAttributes
* @return Decision if a variation is returned for the user
* null if feature flag is not used in any experiments or no variation is returned for the user
*/
public function getVariationForFeatureExperiment(FeatureFlag $featureFlag, $userId, $userAttributes)
public function getVariationForFeatureExperiment(ProjectConfig $projectConfig, FeatureFlag $featureFlag, $userId, $userAttributes)
{
$featureFlagKey = $featureFlag->getKey();
$experimentIds = $featureFlag->getExperimentIds();
Expand All @@ -225,13 +222,13 @@ public function getVariationForFeatureExperiment(FeatureFlag $featureFlag, $user

// Evaluate each experiment ID and return the first bucketed experiment variation
foreach ($experimentIds as $experiment_id) {
$experiment = $this->_projectConfig->getExperimentFromId($experiment_id);
$experiment = $projectConfig->getExperimentFromId($experiment_id);
if ($experiment && !($experiment->getKey())) {
// Error logged and exception thrown in ProjectConfig-getExperimentFromId
continue;
}

$variation = $this->getVariation($experiment, $userId, $userAttributes);
$variation = $this->getVariation($projectConfig, $experiment, $userId, $userAttributes);
if ($variation && $variation->getKey()) {
$this->_logger->log(
Logger::INFO,
Expand All @@ -255,15 +252,16 @@ public function getVariationForFeatureExperiment(FeatureFlag $featureFlag, $user
* Evaluate the user for rules in priority order by seeing if the user satisfies the audience.
* Fall back onto the everyone else rule if the user is ever excluded from a rule due to traffic allocation.
*
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user id
* @param array $userAttributes user userAttributes
* @param ProjectConfig $projectConfig ProjectConfig instance.
* @param FeatureFlag $featureFlag The feature flag the user wants to access
* @param string $userId user id
* @param array $userAttributes user userAttributes
* @return Decision if a variation is returned for the user
* null if feature flag is not used in a rollout or
* no rollout found against the rollout ID or
* no variation is returned for the user
*/
public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId, $userAttributes)
public function getVariationForFeatureRollout(ProjectConfig $projectConfig, FeatureFlag $featureFlag, $userId, $userAttributes)
{
$bucketing_id = $this->getBucketingId($userId, $userAttributes);
$featureFlagKey = $featureFlag->getKey();
Expand All @@ -275,7 +273,7 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
);
return null;
}
$rollout = $this->_projectConfig->getRolloutFromId($rollout_id);
$rollout = $projectConfig->getRolloutFromId($rollout_id);
if ($rollout && !($rollout->getId())) {
// Error logged and thrown in getRolloutFromId
return null;
Expand All @@ -291,7 +289,7 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
$experiment = $rolloutRules[$i];

// Evaluate if user meets the audience condition of this rollout rule
if (!Validator::isUserInExperiment($this->_projectConfig, $experiment, $userAttributes, $this->_logger)) {
if (!Validator::isUserInExperiment($projectConfig, $experiment, $userAttributes, $this->_logger)) {
$this->_logger->log(
Logger::DEBUG,
sprintf("User '%s' did not meet the audience conditions to be in rollout rule '%s'.", $userId, $experiment->getKey())
Expand All @@ -301,7 +299,7 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
}

// Evaluate if user satisfies the traffic allocation for this rollout rule
$variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketing_id, $userId);
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketing_id, $userId);
if ($variation && $variation->getKey()) {
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT);
}
Expand All @@ -311,15 +309,15 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
$experiment = $rolloutRules[sizeof($rolloutRules)-1];

// Evaluate if user meets the audience condition of Everyone Else Rule / Last Rule now
if (!Validator::isUserInExperiment($this->_projectConfig, $experiment, $userAttributes, $this->_logger)) {
if (!Validator::isUserInExperiment($projectConfig, $experiment, $userAttributes, $this->_logger)) {
$this->_logger->log(
Logger::DEBUG,
sprintf("User '%s' did not meet the audience conditions to be in rollout rule '%s'.", $userId, $experiment->getKey())
);
return null;
}

$variation = $this->_bucketer->bucket($this->_projectConfig, $experiment, $bucketing_id, $userId);
$variation = $this->_bucketer->bucket($projectConfig, $experiment, $bucketing_id, $userId);
if ($variation && $variation->getKey()) {
return new FeatureDecision($experiment, $variation, FeatureDecision::DECISION_SOURCE_ROLLOUT);
}
Expand All @@ -329,18 +327,19 @@ public function getVariationForFeatureRollout(FeatureFlag $featureFlag, $userId,
/**
* Determine variation the user has been forced into.
*
* @param $experiment Experiment Experiment in which user is to be bucketed.
* @param $userId string string
* @param $projectConfig ProjectConfig ProjectConfig instance.
* @param $experiment Experiment Experiment in which user is to be bucketed.
* @param $userId string string
*
* @return null|Variation Representing the variation the user is forced into.
*/
private function getWhitelistedVariation(Experiment $experiment, $userId)
private function getWhitelistedVariation(ProjectConfig $projectConfig, Experiment $experiment, $userId)
{
// Check if user is whitelisted for a variation.
$forcedVariations = $experiment->getForcedVariations();
if (!is_null($forcedVariations) && isset($forcedVariations[$userId])) {
$variationKey = $forcedVariations[$userId];
$variation = $this->_projectConfig->getVariationFromKey($experiment->getKey(), $variationKey);
$variation = $projectConfig->getVariationFromKey($experiment->getKey(), $variationKey);
if ($variationKey && !empty($variation->getKey())) {
$this->_logger->log(
Logger::INFO,
Expand Down Expand Up @@ -395,12 +394,13 @@ private function getStoredUserProfile($userId)
/**
* Get the stored variation for the given experiment from the user profile.
*
* @param $experiment Experiment The experiment for which we are getting the stored variation.
* @param $userProfile UserProfile The user profile from which we are getting the stored variation.
* @param $projectConfig ProjectConfig ProjectConfig instance.
* @param $experiment Experiment The experiment for which we are getting the stored variation.
* @param $userProfile UserProfile The user profile from which we are getting the stored variation.
*
* @return null|Variation the stored variation or null if not found.
*/
private function getStoredVariation(Experiment $experiment, UserProfile $userProfile)
private function getStoredVariation(ProjectConfig $projectConfig, Experiment $experiment, UserProfile $userProfile)
{
$experimentKey = $experiment->getKey();
$userId = $userProfile->getUserId();
Expand All @@ -414,7 +414,7 @@ private function getStoredVariation(Experiment $experiment, UserProfile $userPro
return null;
}

if (!$this->_projectConfig->isVariationIdValid($experimentKey, $variationId)) {
if (!$projectConfig->isVariationIdValid($experimentKey, $variationId)) {
$this->_logger->log(
Logger::INFO,
sprintf(
Expand All @@ -427,7 +427,7 @@ private function getStoredVariation(Experiment $experiment, UserProfile $userPro
return null;
}

$variation = $this->_projectConfig->getVariationFromId($experimentKey, $variationId);
$variation = $projectConfig->getVariationFromId($experimentKey, $variationId);
$this->_logger->log(
Logger::INFO,
sprintf(
Expand Down
8 changes: 4 additions & 4 deletions src/Optimizely/Optimizely.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public function __construct(
}

$this->_eventBuilder = new EventBuilder($this->_logger);
$this->_decisionService = new DecisionService($this->_logger, $this->_config, $userProfileService);
$this->_decisionService = new DecisionService($this->_logger, $userProfileService);
$this->notificationCenter = new NotificationCenter($this->_logger, $this->_errorHandler);
}

Expand Down Expand Up @@ -405,7 +405,7 @@ public function getVariation($experimentKey, $userId, $attributes = null)
return null;
}

$variation = $this->_decisionService->getVariation($experiment, $userId, $attributes);
$variation = $this->_decisionService->getVariation($this->_config, $experiment, $userId, $attributes);
$variationKey = ($variation === null) ? null : $variation->getKey();

if ($this->_config->isFeatureExperiment($experiment->getId())) {
Expand Down Expand Up @@ -520,7 +520,7 @@ public function isFeatureEnabled($featureFlagKey, $userId, $attributes = null)
}

$featureEnabled = false;
$decision = $this->_decisionService->getVariationForFeature($featureFlag, $userId, $attributes);
$decision = $this->_decisionService->getVariationForFeature($this->_config, $featureFlag, $userId, $attributes);
$variation = $decision->getVariation();
if ($variation) {
$experiment = $decision->getExperiment();
Expand Down Expand Up @@ -650,7 +650,7 @@ public function getFeatureVariableValueForType(
}

$featureEnabled = false;
$decision = $this->_decisionService->getVariationForFeature($featureFlag, $userId, $attributes);
$decision = $this->_decisionService->getVariationForFeature($this->_config, $featureFlag, $userId, $attributes);
$variableValue = $variable->getDefaultValue();

if ($decision->getVariation() === null) {
Expand Down