From 1196447c73918b41a61bb38e17044067ca9d88a6 Mon Sep 17 00:00:00 2001
From: Thomas Steur <thomas.steur@gmail.com>
Date: Sat, 29 Aug 2015 11:02:01 +0000
Subject: [PATCH] return min amount of available custom variables across tables
instead of maximum
---
core/Tracker/GoalManager.php | 2 +-
core/Tracker/Request.php | 2 +-
core/Tracker/TrackerCodeGenerator.php | 2 +-
core/Tracker/VisitorRecognizer.php | 2 +-
plugins/CoreAdminHome/Controller.php | 2 +-
plugins/CustomVariables/Archiver.php | 2 +-
plugins/CustomVariables/Commands/Info.php | 2 +-
plugins/CustomVariables/CustomVariables.php | 32 ++++++--
.../SetNumberOfCustomVariablesTest.php | 14 ++--
.../tests/Integration/CustomVariablesTest.php | 75 +++++++++++++++++--
plugins/Live/Model.php | 2 +-
plugins/Live/Visitor.php | 2 +-
12 files changed, 107 insertions(+), 32 deletions(-)
diff --git a/core/Tracker/GoalManager.php b/core/Tracker/GoalManager.php
index 7bd5e97f1e..95779e0a76 100644
--- a/core/Tracker/GoalManager.php
+++ b/core/Tracker/GoalManager.php
@@ -216,7 +216,7 @@ class GoalManager
// Copy Custom Variables from Visit row to the Goal conversion
// Otherwise, set the Custom Variables found in the cookie sent with this request
$goal += $visitCustomVariables;
- $maxCustomVariables = CustomVariables::getMaxCustomVariables();
+ $maxCustomVariables = CustomVariables::getNumUsableCustomVariables();
for ($i = 1; $i <= $maxCustomVariables; $i++) {
if (isset($visitorInformation['custom_var_k' . $i])
diff --git a/core/Tracker/Request.php b/core/Tracker/Request.php
index b5d358f25d..489ee313e7 100644
--- a/core/Tracker/Request.php
+++ b/core/Tracker/Request.php
@@ -540,7 +540,7 @@ class Request
}
$customVariables = array();
- $maxCustomVars = CustomVariables::getMaxCustomVariables();
+ $maxCustomVars = CustomVariables::getNumUsableCustomVariables();
foreach ($customVar as $id => $keyValue) {
$id = (int)$id;
diff --git a/core/Tracker/TrackerCodeGenerator.php b/core/Tracker/TrackerCodeGenerator.php
index 7a0204dbda..ceca8a3e71 100644
--- a/core/Tracker/TrackerCodeGenerator.php
+++ b/core/Tracker/TrackerCodeGenerator.php
@@ -63,7 +63,7 @@ class TrackerCodeGenerator
if ($mergeSubdomains || $mergeAliasUrls) {
$options .= $this->getJavascriptTagOptions($idSite, $mergeSubdomains, $mergeAliasUrls);
}
- $maxCustomVars = CustomVariables::getMaxCustomVariables();
+ $maxCustomVars = CustomVariables::getNumUsableCustomVariables();
if ($visitorCustomVariables && count($visitorCustomVariables) > 0) {
$options .= ' // you can set up to ' . $maxCustomVars . ' custom variables for each visitor' . PHP_EOL;
diff --git a/core/Tracker/VisitorRecognizer.php b/core/Tracker/VisitorRecognizer.php
index ff91f57d7e..d5f5e3c502 100644
--- a/core/Tracker/VisitorRecognizer.php
+++ b/core/Tracker/VisitorRecognizer.php
@@ -256,7 +256,7 @@ class VisitorRecognizer
array_unshift($fields, 'visit_first_action_time');
array_unshift($fields, 'visit_last_action_time');
- for ($index = 1; $index <= CustomVariables::getMaxCustomVariables(); $index++) {
+ for ($index = 1; $index <= CustomVariables::getNumUsableCustomVariables(); $index++) {
$fields[] = 'custom_var_k' . $index;
$fields[] = 'custom_var_v' . $index;
}
diff --git a/plugins/CoreAdminHome/Controller.php b/plugins/CoreAdminHome/Controller.php
index cb8f862643..ea8b42da33 100644
--- a/plugins/CoreAdminHome/Controller.php
+++ b/plugins/CoreAdminHome/Controller.php
@@ -292,7 +292,7 @@ class Controller extends ControllerAdmin
$view->defaultReportSiteName = Site::getNameFor($view->idSite);
$view->defaultSiteRevenue = \Piwik\Metrics\Formatter::getCurrencySymbol($view->idSite);
- $view->maxCustomVariables = CustomVariables::getMaxCustomVariables();
+ $view->maxCustomVariables = CustomVariables::getNumUsableCustomVariables();
$allUrls = APISitesManager::getInstance()->getSiteUrlsFromId($view->idSite);
if (isset($allUrls[1])) {
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();
+ }
+
+
}
diff --git a/plugins/Live/Model.php b/plugins/Live/Model.php
index 3a06c58d45..8f7087405e 100644
--- a/plugins/Live/Model.php
+++ b/plugins/Live/Model.php
@@ -33,7 +33,7 @@ class Model
*/
public function queryActionsForVisit($idVisit, $actionsLimit)
{
- $maxCustomVariables = CustomVariables::getMaxCustomVariables();
+ $maxCustomVariables = CustomVariables::getNumUsableCustomVariables();
$sqlCustomVariables = '';
for ($i = 1; $i <= $maxCustomVariables; $i++) {
diff --git a/plugins/Live/Visitor.php b/plugins/Live/Visitor.php
index dae839cd9a..edafacd896 100644
--- a/plugins/Live/Visitor.php
+++ b/plugins/Live/Visitor.php
@@ -250,7 +250,7 @@ class Visitor implements VisitorInterface
$actionDetails = $model->queryActionsForVisit($idVisit, $actionsLimit);
$formatter = new Formatter();
- $maxCustomVariables = CustomVariables::getMaxCustomVariables();
+ $maxCustomVariables = CustomVariables::getNumUsableCustomVariables();
foreach ($actionDetails as $actionIdx => &$actionDetail) {
$actionDetail =& $actionDetails[$actionIdx];
--
GitLab