Skip to content

Commit 5ab73c9

Browse files
Forced Bucketing Improvements (#57)
* Added more unit tests for forced bucketing. Added check for invalid user IDs. Also replaced empty with \!isset and is_null to properly reflect the desired behavior of checking for null and emptry string values. * Added some unit tests for activate, track, and getVariation after a setForcedVariation. * Had to fix multiple set for forced variation get/set unit tests. * Checked for empty and null strings in a cleaner way. Fixed a test.
1 parent c9422a9 commit 5ab73c9

File tree

2 files changed

+541
-172
lines changed

2 files changed

+541
-172
lines changed

src/Optimizely/ProjectConfig.php

Lines changed: 45 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,10 @@ class ProjectConfig
113113
private $_errorHandler;
114114

115115
/**
116-
* @var array Associative array of user IDs to an associative array
117-
* of experiments to variations.
116+
* @var array Associative array of user IDs to an associative array
117+
* of experiments to variations. This contains all the forced variations
118+
* set by the user by calling setForcedVariation (it is not the same as the
119+
* whitelisting forcedVariations data structure in the Experiments class).
118120
*/
119121
private $_forcedVariationMap;
120122

@@ -348,23 +350,31 @@ public function isVariationIdValid($experimentKey, $variationId)
348350
}
349351

350352
/**
351-
* Gets the forced variation key for the given user and experiment.
352-
*
353+
* Gets the forced variation key for the given user and experiment.
354+
*
353355
* @param $experimentKey string Key for experiment.
354-
* @param $userId string The user Id.
356+
* @param $userId string The user Id.
355357
*
356-
* @return Variation The variation which the given user and experiment should be forced into.
358+
* @return Variation The variation which the given user and experiment should be forced into.
357359
*/
358360
public function getForcedVariation($experimentKey, $userId)
359361
{
362+
363+
// check for null and empty string user ID
364+
if (strlen($userId) == 0) {
365+
$this->_logger->log(Logger::DEBUG, 'User ID is invalid');
366+
return null;
367+
}
368+
360369
if (!isset($this->_forcedVariationMap[$userId])) {
361370
$this->_logger->log(Logger::DEBUG, sprintf('User "%s" is not in the forced variation map.', $userId));
362371
return null;
363372
}
364373

365374
$experimentToVariationMap = $this->_forcedVariationMap[$userId];
366375
$experimentId = $this->getExperimentFromKey($experimentKey)->getId();
367-
if (empty($experimentId)) {
376+
// check for null and empty string experiment ID
377+
if (strlen($experimentId) == 0) {
368378
// this case is logged in getExperimentFromKey
369379
return null;
370380
}
@@ -375,58 +385,70 @@ public function getForcedVariation($experimentKey, $userId)
375385
}
376386

377387
$variationId = $experimentToVariationMap[$experimentId];
378-
if (empty($variationId)) {
388+
// check for null and empty string variation ID
389+
if (strlen($variationId) == 0) {
379390
$this->_logger->log(Logger::DEBUG, sprintf('No variation mapped to experiment "%s" in the forced variation map.', $experimentKey));
380391
return null;
381392
}
382393

383-
$variationKey = $this->getVariationFromId($experimentKey, $variationId)->getKey();
384-
if (empty($variationKey)) {
385-
// this case is logged in getVariationFromKey
394+
$variation = $this->getVariationFromId($experimentKey, $variationId);
395+
$variationKey = $variation->getKey();
396+
// check if the variation exists in the datafile (a new variation is returned if it is not in the datafile)
397+
if (strlen($variationKey) == 0) {
398+
// this case is logged in getVariationFromId
386399
return null;
387400
}
388401

389402
$this->_logger->log(Logger::DEBUG, sprintf('Variation "%s" is mapped to experiment "%s" and user "%s" in the forced variation map', $variationKey, $experimentKey, $userId));
390403

391-
$variation = $this->getVariationFromKey($experimentKey, $variationKey);
392404
return $variation;
393405
}
394406

395407
/**
396-
* Sets an associative array of user IDs to an associative array of experiments
397-
* to forced variations.
398-
*
408+
* Sets an associative array of user IDs to an associative array of experiments
409+
* to forced variations.
410+
*
399411
* @param $experimentKey string Key for experiment.
400412
* @param $userId string The user Id.
401413
* @param $variationKey string Key for variation. If null, then clear the existing experiment-to-variation mapping.
402414
*
403-
* @return boolean A boolean value that indicates if the set completed successfully.
415+
* @return boolean A boolean value that indicates if the set completed successfully.
404416
*/
405417
public function setForcedVariation($experimentKey, $userId, $variationKey)
406418
{
407-
$experimentId = $this->getExperimentFromKey($experimentKey)->getId();
408-
if (empty($experimentId)) {
419+
// check for null and empty string user ID
420+
if (strlen($userId) == 0) {
421+
$this->_logger->log(Logger::DEBUG, 'User ID is invalid');
422+
return FALSE;
423+
}
424+
425+
$experiment = $this->getExperimentFromKey($experimentKey);
426+
$experimentId = $experiment->getId();
427+
// check if the experiment exists in the datafile (a new experiment is returned if it is not in the datafile)
428+
if (strlen($experimentId) == 0) {
409429
// this case is logged in getExperimentFromKey
410430
return FALSE;
411431
}
412432

413-
if (empty($variationKey)) {
433+
// clear the forced variation if the variation key is null
434+
if (is_null($variationKey)) {
414435
unset($this->_forcedVariationMap[$userId][$experimentId]);
415436
$this->_logger->log(Logger::DEBUG, sprintf('Variation mapped to experiment "%s" has been removed for user "%s".', $experimentKey, $userId));
416437
return TRUE;
417438
}
418439

419-
$variationId = $this->getVariationFromKey($experimentKey, $variationKey)->getId();
420-
if (empty($variationId)) {
440+
$variation = $this->getVariationFromKey($experimentKey, $variationKey);
441+
$variationId = $variation->getId();
442+
// check if the variation exists in the datafile (a new variation is returned if it is not in the datafile)
443+
if (strlen($variationId) == 0) {
421444
// this case is logged in getVariationFromKey
422445
return FALSE;
423446
}
424447

425448
$this->_forcedVariationMap[$userId][$experimentId] = $variationId;
426-
427449
$this->_logger->log(Logger::DEBUG, sprintf('Set variation "%s" for experiment "%s" and user "%s" in the forced variation map.', $variationId, $experimentId, $userId));
428450

429-
return TRUE;
451+
return TRUE;
430452
}
431453

432454
}

0 commit comments

Comments
 (0)