Gracefully handle empty entity IDs in traffic allocation (#35)

aliabbasrizvi · web-flow · commit d033ec389143 · 2017-03-21T13:13:45.000-07:00
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -1,7 +1,7 @@
-#Contributing to the Optimizely PHP SDK
+# Contributing to the Optimizely PHP SDK
 We welcome contributions and feedback! All contributors must sign our [Contributor License Agreement (CLA)](https://docs.google.com/a/optimizely.com/forms/d/e/1FAIpQLSf9cbouWptIpMgukAKZZOIAhafvjFCV8hS00XJLWQnWDFtwtA/viewform) to be eligible to contribute. Please read the [README](README.md) to set up your development environment, then read the guidelines below for information on submitting your code.
 
-##Development process
+## Development process
 
 1. Create a branch off of `devel`: `git checkout -b YOUR_NAME/branch_name`.
 2. Commit your changes. Make sure to add tests!
@@ -10,12 +10,12 @@ We welcome contributions and feedback! All contributors must sign our [Contribut
 5. Open a pull request from `YOUR_NAME/branch_name` to `devel`.
 6. A repository maintainer will review your pull request and, if all goes well, merge it!
 
-##Pull request acceptance criteria
+## Pull request acceptance criteria
 
 * **All code must have test coverage.** We use PHPUnit. Changes in functionality should have accompanying unit tests. Bug fixes should have accompanying regression tests.
 * Tests are located in `tests` with one file per class.
 
-##License
+## License
 
 All contributions are under the CLA mentioned above. For this project, Optimizely uses the Apache 2.0 license, and so asks that by contributing your code, you agree to license your contribution under the terms of the [Apache License v2.0](http://www.apache.org/licenses/LICENSE-2.0). Your contributions should also include the following header:
 
@@ -39,5 +39,5 @@ All contributions are under the CLA mentioned above. For this project, Optimizel
 
 The YEAR above should be the year of the contribution. If work on the file has been done over multiple years, list each year in the section above. Example: Optimizely writes the file and releases it in 2014. No changes are made in 2015. Change made in 2016. YEAR should be “2014, 2016”.
 
-##Contact
+## Contact
 If you have questions, please contact developers@optimizely.com.
diff --git a/README.md b/README.md
@@ -1,35 +1,35 @@
-#Optimizely PHP SDK
+# Optimizely PHP SDK
 [![Build Status](https://travis-ci.org/optimizely/php-sdk.svg?branch=master)](https://travis-ci.org/optimizely/php-sdk)
 [![Coverage Status](https://coveralls.io/repos/github/optimizely/php-sdk/badge.svg?branch=master)](https://coveralls.io/github/optimizely/php-sdk?branch=master)
 [![Total Downloads](https://poser.pugx.org/optimizely/optimizely-sdk/downloads)](https://packagist.org/packages/optimizely/optimizely-sdk)
 [![Apache 2.0](https://img.shields.io/github/license/nebula-plugins/gradle-extra-configurations-plugin.svg)](http://www.apache.org/licenses/LICENSE-2.0)
 
 This repository houses the PHP SDK for Optimizely Full Stack.
 
-##Getting Started
+## Getting Started
 
-###Installing the SDK
+### Installing the SDK
 
 The Optimizely PHP SDK can be installed through [Composer](https://getcomposer.org/). Please use the following command: 
 
 ```
 php composer.phar require optimizely/optimizely-sdk
 ```
 
-###Using the SDK
+### Using the SDK
 See the Optimizely Full Stack [developer documentation](https://developers.optimizely.com/x/solutions/sdks/reference/?language=php) to learn how to set up your first Full Stack project and use the SDK.
 
-##Development
+## Development
 
-###Unit tests
+### Unit tests
 
-#####Running all tests
+##### Running all tests
 You can run all unit tests with:
 
 ```
 ./vendor/bin/phpunit
 ```
 
-###Contributing
+### Contributing
 
 Please see [CONTRIBUTING](CONTRIBUTING.md).
diff --git a/src/Optimizely/Bucketer.php b/src/Optimizely/Bucketer.php
@@ -150,7 +150,7 @@ public function bucket(ProjectConfig $config, Experiment $experiment, $userId)
             }
 
             $userExperimentId = $this->findBucket($userId, $group->getId(), $group->getTrafficAllocation());
-            if (is_null($userExperimentId)) {
+            if (empty($userExperimentId)) {
                 $this->_logger->log(Logger::INFO, sprintf('User "%s" is in no experiment.', $userId));
                 return new Variation();
             }
@@ -172,7 +172,7 @@ public function bucket(ProjectConfig $config, Experiment $experiment, $userId)
 
         // Bucket user if not in whitelist and in group (if any).
         $variationId = $this->findBucket($userId, $experiment->getId(), $experiment->getTrafficAllocation());
-        if (!is_null($variationId)) {
+        if (!empty($variationId)) {
             $variation = $config->getVariationFromId($experiment->getKey(), $variationId);
             $this->_logger->log(Logger::INFO,
                 sprintf('User "%s" is in variation %s of experiment %s.',
diff --git a/tests/BucketerTest.php b/tests/BucketerTest.php
@@ -82,11 +82,28 @@ public function testGenerateBucketValue()
     public function testBucketValidExperimentNotInGroup()
     {
         $bucketer = new TestBucketer($this->loggerMock);
-        $bucketer->setBucketValues([3000, 7000, 9000]);
+        $bucketer->setBucketValues([1000, 3000, 7000, 9000]);
         // Total calls in this test
-        $this->loggerMock->expects($this->exactly(6))
+        $this->loggerMock->expects($this->exactly(8))
             ->method('log');
 
+        // No variation (empty entity ID)
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::DEBUG, 'Assigned bucket 1000 to user "testUserId".');
+        $this->loggerMock->expects($this->at(1))
+            ->method('log')
+            ->with(Logger::INFO, 'User "testUserId" is in no variation.');
+
+        $this->assertEquals(
+            new Variation(),
+            $bucketer->bucket(
+                $this->config,
+                $this->config->getExperimentFromKey('test_experiment'),
+                $this->testUserId
+            )
+        );
+
         // control
         $this->loggerMock->expects($this->at(0))
             ->method('log')
@@ -145,10 +162,10 @@ public function testBucketValidExperimentInGroup()
     {
         $bucketer = new TestBucketer($this->loggerMock);
         // Total calls in this test
-        $this->loggerMock->expects($this->exactly(10))
+        $this->loggerMock->expects($this->exactly(14))
             ->method('log');
 
-        // group_experiment_1 (20% experiment)
+        // group_experiment_1 (15% experiment)
         // variation 1
         $bucketer->setBucketValues([1000, 4000]);
         $this->loggerMock->expects($this->at(0))
@@ -216,6 +233,41 @@ public function testBucketValidExperimentInGroup()
                 $this->testUserId
             )
         );
+
+        // User not in any experiment (previously allocated space)
+        $bucketer->setBucketValues([400]);
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::DEBUG, 'Assigned bucket 400 to user "testUserId".');
+        $this->loggerMock->expects($this->at(1))
+            ->method('log')
+            ->with(Logger::INFO, 'User "testUserId" is in no experiment.');
+
+        $this->assertEquals(
+            new Variation(),
+            $bucketer->bucket(
+                $this->config,
+                $this->config->getExperimentFromKey('group_experiment_1'),
+                $this->testUserId
+            )
+        );
+
+        // User not in any experiment (never allocated space)
+        $bucketer->setBucketValues([9000]);
+        $this->loggerMock->expects($this->at(0))
+            ->method('log')
+            ->with(Logger::DEBUG, 'Assigned bucket 9000 to user "testUserId".');
+        $this->loggerMock->expects($this->at(1))
+            ->method('log')
+            ->with(Logger::INFO, 'User "testUserId" is in no experiment.');
+        $this->assertEquals(
+            new Variation(),
+            $bucketer->bucket(
+                $this->config,
+                $this->config->getExperimentFromKey('group_experiment_1'),
+                $this->testUserId
+            )
+        );
     }
 
     public function testBucketInvalidExperiment()
diff --git a/tests/TestData.php b/tests/TestData.php
@@ -23,7 +23,7 @@
 
 define('DATAFILE',
     '{"experiments": [{"status": "Running", "key": "test_experiment", "layerId": "7719770039", 
-    "trafficAllocation": [{"entityId": "7722370027", "endOfRange": 4000}, 
+    "trafficAllocation": [{"entityId": "", "endOfRange": 1500}, {"entityId": "7722370027", "endOfRange": 4000}, 
     {"entityId": "7721010009", "endOfRange": 8000}], "audienceIds": ["7718080042"], 
     "variations": [{"id": "7722370027", "key": "control"}, {"id": "7721010009", "key": "variation"}], 
     "forcedVariations": {"user1": "control"}, "id": "7716830082"}, {"status": "Paused", "key": "paused_experiment", "layerId": "7719779139", 
@@ -32,7 +32,7 @@
     "variations": [{"id": "7722370427", "key": "control"}, {"id": "7721010509", "key": "variation"}], 
     "forcedVariations": {}, "id": "7716830585"}], "version": "2", 
     "audiences": [{"conditions": "[\"and\", [\"or\", [\"or\", {\"name\": \"device_type\", \"type\": \"custom_attribute\", \"value\": \"iPhone\"}]], [\"or\", [\"or\", {\"name\": \"location\", \"type\": \"custom_attribute\", \"value\": \"San Francisco\"}]]]", "id": "7718080042", "name": "iPhone users in San Francisco"}], 
-    "groups": [{"policy": "random", "trafficAllocation": [{"entityId": "7723330021", "endOfRange": 2000}, {"entityId": "7718750065", "endOfRange": 6000}], "experiments": [{"status": "Running", "key": "group_experiment_1", "layerId": "7721010011", "trafficAllocation": [{"entityId": "7722260071", "endOfRange": 5000}, {"entityId": "7722360022", "endOfRange": 10000}], "audienceIds": [], "variations": [{"id": "7722260071", "key": "group_exp_1_var_1"}, {"id": "7722360022", "key": "group_exp_1_var_2"}], "forcedVariations": {"user1": "group_exp_1_var_1"}, "id": "7723330021"}, {"status": "Running", "key": "group_experiment_2", "layerId": "7721020020", "trafficAllocation": [{"entityId": "7713030086", "endOfRange": 5000}, {"entityId": "7725250007", "endOfRange": 10000}], "audienceIds": [], 
+    "groups": [{"policy": "random", "trafficAllocation": [{"entityId": "", "endOfRange": 500}, {"entityId": "7723330021", "endOfRange": 2000}, {"entityId": "7718750065", "endOfRange": 6000}], "experiments": [{"status": "Running", "key": "group_experiment_1", "layerId": "7721010011", "trafficAllocation": [{"entityId": "7722260071", "endOfRange": 5000}, {"entityId": "7722360022", "endOfRange": 10000}], "audienceIds": [], "variations": [{"id": "7722260071", "key": "group_exp_1_var_1"}, {"id": "7722360022", "key": "group_exp_1_var_2"}], "forcedVariations": {"user1": "group_exp_1_var_1"}, "id": "7723330021"}, {"status": "Running", "key": "group_experiment_2", "layerId": "7721020020", "trafficAllocation": [{"entityId": "7713030086", "endOfRange": 5000}, {"entityId": "7725250007", "endOfRange": 10000}], "audienceIds": [], 
     "variations": [{"id": "7713030086", "key": "group_exp_2_var_1"}, {"id": "7725250007", "key": "group_exp_2_var_2"}], "forcedVariations": {}, "id": "7718750065"}], "id": "7722400015"}], 
     "attributes": [{"id": "7723280020", "key": "device_type"}, {"id": "7723340004", "key": "location"}], 
     "projectId": "7720880029", "accountId": "1592310167", 
