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.

Author: alda-optimizely

src/Optimizely/ProjectConfig.php

@@ -113,8 +113,10 @@ class ProjectConfig
     private $_errorHandler;
 
     /**
-     * @var array Associative array of user IDs to an associative array 
-     * of experiments to variations.
+     * @var array Associative array of user IDs to an associative array
+     * of experiments to variations. This contains all the forced variations
+     * set by the user by calling setForcedVariation (it is not the same as the
+     * whitelisting forcedVariations data structure in the Experiments class).
      */
     private $_forcedVariationMap;
 
@@ -348,23 +350,31 @@ public function isVariationIdValid($experimentKey, $variationId)
     }
 
     /**
-     * Gets the forced variation key for the given user and experiment.  
-     * 
+     * Gets the forced variation key for the given user and experiment.
+     *
      * @param $experimentKey string Key for experiment.
-     * @param $userId string The user Id.    
+     * @param $userId string The user Id.
      *
-     * @return Variation The variation which the given user and experiment should be forced into. 
+     * @return Variation The variation which the given user and experiment should be forced into.
      */
     public function getForcedVariation($experimentKey, $userId)
     {
+
+        // check for null and empty string user ID
+        if (strlen($userId) == 0) {
+            $this->_logger->log(Logger::DEBUG, 'User ID is invalid');
+            return null;
+        }
+
         if (!isset($this->_forcedVariationMap[$userId])) {
             $this->_logger->log(Logger::DEBUG, sprintf('User "%s" is not in the forced variation map.', $userId));
             return null;
         }
 
         $experimentToVariationMap = $this->_forcedVariationMap[$userId];
         $experimentId = $this->getExperimentFromKey($experimentKey)->getId();
-        if (empty($experimentId)) {
+        // check for null and empty string experiment ID
+        if (strlen($experimentId) == 0) {
             // this case is logged in getExperimentFromKey
             return null;
         }
@@ -375,58 +385,70 @@ public function getForcedVariation($experimentKey, $userId)
         }
 
         $variationId = $experimentToVariationMap[$experimentId];
-        if (empty($variationId)) {
+        // check for null and empty string variation ID
+        if (strlen($variationId) == 0) {
             $this->_logger->log(Logger::DEBUG, sprintf('No variation mapped to experiment "%s" in the forced variation map.', $experimentKey));
             return null;
         }
 
-        $variationKey = $this->getVariationFromId($experimentKey, $variationId)->getKey();
-        if (empty($variationKey)) {
-            // this case is logged in getVariationFromKey
+        $variation = $this->getVariationFromId($experimentKey, $variationId);
+        $variationKey = $variation->getKey();
+        // check if the variation exists in the datafile (a new variation is returned if it is not in the datafile)
+        if (strlen($variationKey) == 0) {
+            // this case is logged in getVariationFromId
             return null;
         }
 
         $this->_logger->log(Logger::DEBUG, sprintf('Variation "%s" is mapped to experiment "%s" and user "%s" in the forced variation map', $variationKey, $experimentKey, $userId));
 
-        $variation = $this->getVariationFromKey($experimentKey, $variationKey);
         return $variation;
     }
 
     /**
-     * Sets an associative array of user IDs to an associative array of experiments 
-     * to forced variations. 
-     * 
+     * Sets an associative array of user IDs to an associative array of experiments
+     * to forced variations.
+     *
      * @param $experimentKey string Key for experiment.
      * @param $userId string The user Id.
      * @param $variationKey string Key for variation. If null, then clear the existing experiment-to-variation mapping.
      *
-     * @return boolean A boolean value that indicates if the set completed successfully. 
+     * @return boolean A boolean value that indicates if the set completed successfully.
      */
     public function setForcedVariation($experimentKey, $userId, $variationKey)
     {
-        $experimentId = $this->getExperimentFromKey($experimentKey)->getId();
-        if (empty($experimentId)) {
+        // check for null and empty string user ID
+        if (strlen($userId) == 0) {
+            $this->_logger->log(Logger::DEBUG, 'User ID is invalid');
+            return FALSE;
+        }
+
+        $experiment = $this->getExperimentFromKey($experimentKey);
+        $experimentId = $experiment->getId();
+        // check if the experiment exists in the datafile (a new experiment is returned if it is not in the datafile)
+        if (strlen($experimentId) == 0) {
             // this case is logged in getExperimentFromKey
             return FALSE;
         }
 
-        if (empty($variationKey)) {
+        // clear the forced variation if the variation key is null
+        if (is_null($variationKey)) {
             unset($this->_forcedVariationMap[$userId][$experimentId]);
             $this->_logger->log(Logger::DEBUG, sprintf('Variation mapped to experiment "%s" has been removed for user "%s".', $experimentKey, $userId));
             return TRUE;
         }
 
-        $variationId = $this->getVariationFromKey($experimentKey, $variationKey)->getId();
-        if (empty($variationId)) {
+        $variation = $this->getVariationFromKey($experimentKey, $variationKey);
+        $variationId = $variation->getId();
+        // check if the variation exists in the datafile (a new variation is returned if it is not in the datafile)
+        if (strlen($variationId) == 0) {
             // this case is logged in getVariationFromKey
             return FALSE;
         }
 
         $this->_forcedVariationMap[$userId][$experimentId] = $variationId;
-
         $this->_logger->log(Logger::DEBUG, sprintf('Set variation "%s" for experiment "%s" and user "%s" in the forced variation map.', $variationId, $experimentId, $userId));
 
-        return TRUE;   
+        return TRUE;
     }
 
 }