return min amount of available custom variables across tables instead of maximum

5 files changed, 100 insertions, 25 deletions

diff --git a/plugins/CustomVariables/Archiver.php b/plugins/CustomVariables/Archiver.php
index b613f65d02..bc43cbc8dd 100644
--- a/plugins/CustomVariables/Archiver.php
+++ b/plugins/CustomVariables/Archiver.php
@@ -66,7 +66,7 @@ class Archiver extends \Piwik\Plugin\Archiver
     {
         $this->dataArray = new DataArray();
 
-        $maxCustomVariables = CustomVariables::getMaxCustomVariables();
+        $maxCustomVariables = CustomVariables::getNumUsableCustomVariables();
         for ($i = 1; $i <= $maxCustomVariables; $i++) {
             $this->aggregateCustomVariable($i);
         }
diff --git a/plugins/CustomVariables/Commands/Info.php b/plugins/CustomVariables/Commands/Info.php
index bf300e357a..e5dfce5d5b 100644
--- a/plugins/CustomVariables/Commands/Info.php
+++ b/plugins/CustomVariables/Commands/Info.php
@@ -28,7 +28,7 @@ class Info extends ConsoleCommand
 
     protected function execute(InputInterface $input, OutputInterface $output)
     {
-        $maxVars = CustomVariables::getMaxCustomVariables();
+        $maxVars = CustomVariables::getNumUsableCustomVariables();
 
         if ($this->hasEverywhereSameAmountOfVariables()) {
             $this->writeSuccessMessage($output, array(
diff --git a/plugins/CustomVariables/CustomVariables.php b/plugins/CustomVariables/CustomVariables.php
index 64a7c9ea05..7a37808909 100644
--- a/plugins/CustomVariables/CustomVariables.php
+++ b/plugins/CustomVariables/CustomVariables.php
@@ -40,7 +40,7 @@ class CustomVariables extends \Piwik\Plugin
     {
         $customVariables = array();
 
-        $maxCustomVariables = self::getMaxCustomVariables();
+        $maxCustomVariables = self::getNumUsableCustomVariables();
 
         for ($i = 1; $i <= $maxCustomVariables; $i++) {
             if (!empty($details['custom_var_k' . $i])) {
@@ -63,25 +63,41 @@ class CustomVariables extends \Piwik\Plugin
         return 200;
     }
 
-    public static function getMaxCustomVariables()
+    /**
+     * Returns the number of available custom variables that can be used.
+     *
+     * "Can be used" is identifed by the minimum number of available custom variables across all relevant tables. Eg
+     * if there are 6 custom variables installed in log_visit but only 5 in log_conversion, we consider only 5 custom
+     * variables as usable.
+     * @return int
+     */
+    public static function getNumUsableCustomVariables()
     {
         $cache    = Cache::getCacheGeneral();
-        $cacheKey = 'CustomVariables.MaxNumCustomVariables';
+        $cacheKey = 'CustomVariables.NumUsableCustomVariables';
 
         if (!array_key_exists($cacheKey, $cache)) {
 
-            $maxCustomVar = 0;
+            $minCustomVar = null;
 
             foreach (Model::getScopes() as $scope) {
                 $model = new Model($scope);
                 $highestIndex = $model->getHighestCustomVarIndex();
 
-                if ($highestIndex > $maxCustomVar) {
-                    $maxCustomVar = $highestIndex;
+                if (!isset($minCustomVar)) {
+                    $minCustomVar = $highestIndex;
                 }
+
+                if ($highestIndex < $minCustomVar) {
+                    $minCustomVar = $highestIndex;
+                }
+            }
+
+            if (!isset($minCustomVar)) {
+                $minCustomVar = 0;
             }
 
-            $cache[$cacheKey] = $maxCustomVar;
+            $cache[$cacheKey] = $minCustomVar;
             Cache::setCacheGeneral($cache);
         }
 
@@ -90,7 +106,7 @@ class CustomVariables extends \Piwik\Plugin
 
     public function getSegmentsMetadata(&$segments)
     {
-        $maxCustomVariables = self::getMaxCustomVariables();
+        $maxCustomVariables = self::getNumUsableCustomVariables();
 
         for ($i = 1; $i <= $maxCustomVariables; $i++) {
             $segments[] = array(
diff --git a/plugins/CustomVariables/tests/Commands/SetNumberOfCustomVariablesTest.php b/plugins/CustomVariables/tests/Commands/SetNumberOfCustomVariablesTest.php
index e70c3515bb..a17a0683a5 100644
--- a/plugins/CustomVariables/tests/Commands/SetNumberOfCustomVariablesTest.php
+++ b/plugins/CustomVariables/tests/Commands/SetNumberOfCustomVariablesTest.php
@@ -65,7 +65,7 @@ class SetNumberOfCustomVariablesTest extends IntegrationTestCase
 
     public function testExecute_ShouldAddMaxCustomVars_IfNumberIsHigherThanActual()
     {
-        $this->assertEquals(5, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(5, CustomVariables::getNumUsableCustomVariables());
 
         $result = $this->executeCommand(6);
 
@@ -77,13 +77,13 @@ class SetNumberOfCustomVariablesTest extends IntegrationTestCase
         $this->assertContains('Added a variable in scope "Conversion" having the index 6', $result);
         $this->assertContains('Your Piwik is now configured for 6 custom variables.', $result);
 
-        $this->assertEquals(6, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(6, CustomVariables::getNumUsableCustomVariables());
     }
 
     public function testExecute_ShouldRemoveMaxCustomVars_IfNumberIsLessThanActual()
     {
         $this->executeCommand(6, true);
-        $this->assertEquals(6, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(6, CustomVariables::getNumUsableCustomVariables());
 
         $result = $this->executeCommand(5);
 
@@ -95,18 +95,18 @@ class SetNumberOfCustomVariablesTest extends IntegrationTestCase
         $this->assertContains('Removed a variable in scope "Conversion" having the index 6', $result);
         $this->assertContains('Your Piwik is now configured for 5 custom variables.', $result);
 
-        $this->assertEquals(5, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(5, CustomVariables::getNumUsableCustomVariables());
     }
 
     public function testExecute_AddMultiple_RemoveMultiple()
     {
-        $this->assertEquals(5, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(5, CustomVariables::getNumUsableCustomVariables());
 
         $this->executeCommand(9);
-        $this->assertEquals(9, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(9, CustomVariables::getNumUsableCustomVariables());
 
         $this->executeCommand(6);
-        $this->assertEquals(6, CustomVariables::getMaxCustomVariables());
+        $this->assertEquals(6, CustomVariables::getNumUsableCustomVariables());
     }
 
     /**
diff --git a/plugins/CustomVariables/tests/Integration/CustomVariablesTest.php b/plugins/CustomVariables/tests/Integration/CustomVariablesTest.php
index 75832efe37..e24bbf0090 100644
--- a/plugins/CustomVariables/tests/Integration/CustomVariablesTest.php
+++ b/plugins/CustomVariables/tests/Integration/CustomVariablesTest.php
@@ -8,6 +8,7 @@
 
 namespace Piwik\Plugins\CustomVariables\tests;
 use Piwik\Plugins\CustomVariables\CustomVariables;
+use Piwik\Plugins\CustomVariables\Model;
 use Piwik\Tracker\Cache;
 use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
 
@@ -18,27 +19,85 @@ use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
  */
 class CustomVariablesTest extends IntegrationTestCase
 {
-    public function testGetMaxCustomVariables_ShouldDetectCorrectNumberOfVariables()
+    public function test_getNumUsableCustomVariables_ShouldDetectCorrectNumberOfVariables()
     {
         Cache::clearCacheGeneral();
-        $this->assertSame(5, CustomVariables::getMaxCustomVariables());
+        $this->assertSame(5, CustomVariables::getNumUsableCustomVariables());
     }
 
-    public function testGetMaxCustomVariables_ShouldCacheTheResult()
+    public function test_getNumUsableCustomVariables_ShouldCacheTheResult()
     {
-        CustomVariables::getMaxCustomVariables();
+        CustomVariables::getNumUsableCustomVariables();
         $cache = Cache::getCacheGeneral();
 
-        $this->assertSame(5, $cache['CustomVariables.MaxNumCustomVariables']);
+        $this->assertSame(5, $cache['CustomVariables.NumUsableCustomVariables']);
     }
 
-    public function testGetMaxCustomVariables_ShouldReadFromCacheIfPossible()
+    public function test_getNumUsableCustomVariables_ShouldReadFromCacheIfPossible()
     {
         $cache = Cache::getCacheGeneral();
-        $cache['CustomVariables.MaxNumCustomVariables'] = 10;
+        $cache['CustomVariables.NumUsableCustomVariables'] = 10;
         Cache::setCacheGeneral($cache);
 
-        $this->assertSame(10, CustomVariables::getMaxCustomVariables());
+        $this->assertSame(10, CustomVariables::getNumUsableCustomVariables());
     }
 
+    public function test_getNumUsableCustomVariables_ShouldReturnMinVariables_IfOneTableHasLessEntriesThanOthers()
+    {
+        $this->assertEquals(5, CustomVariables::getNumUsableCustomVariables());
+
+        $scopes = Model::getScopes();
+
+        // removing custom vars step by step... as soon as one custom var is removed,
+        // it should return the min count of available variables
+        for ($i = 4; $i != -1; $i--) {
+            foreach ($scopes as $scope) {
+                $this->dropCustomVar($scope);
+                $this->assertSame($i, CustomVariables::getNumUsableCustomVariables());
+            }
+        }
+
+        $this->assertEquals(0, CustomVariables::getNumUsableCustomVariables());
+
+        // add custom var, only once all custom vars are written it should write return a higher custom var number
+        for ($i = 1; $i != 7; $i++) {
+            foreach ($scopes as $index => $scope) {
+                $isLastIndex = $index === (count($scopes) - 1);
+
+                $this->addCustomVar($scope);
+
+                if ($isLastIndex) {
+                    $this->assertSame($i, CustomVariables::getNumUsableCustomVariables());
+                    // all scopes have been added, it should consider all custom var counts
+                } else {
+                    $this->assertSame($i - 1, CustomVariables::getNumUsableCustomVariables());
+                    // at least one scope is not added and should therefore return the old custom var count until all
+                    // tables have been updated
+                }
+            }
+        }
+
+        $this->assertEquals(6, CustomVariables::getNumUsableCustomVariables());
+    }
+
+    private function dropCustomVar($scope)
+    {
+        $this->clearCache();
+        $model = new Model($scope);
+        $model->removeCustomVariable();
+    }
+
+    private function addCustomVar($scope)
+    {
+        $this->clearCache();
+        $model = new Model($scope);
+        $model->addCustomVariable();
+    }
+
+    private function clearCache()
+    {
+        Cache::clearCacheGeneral();
+    }
+
+
 }