Bar graph should only have Users and Unique Visitors options when they are valid (#14332)

* Hide the nb_uniq_visitors metric from user when date range doesn't support it

* Refactoring

* Fix logic for choosing columns to display in bar graphs

* Further improvements/bug fixes when column not available in pie or bar graph

* Refactoring

* update custom dimensions submodule

* Prevent valid columns being removed from columns_to_display, add a sprinkling of unit tets

* minor tweak cause label might not always be the first key

4 files changed, 233 insertions, 19 deletions

diff --git a/plugins/CoreVisualizations/Visualizations/Graph.php b/plugins/CoreVisualizations/Visualizations/Graph.php
index 86cfd15787..2efecda529 100644
--- a/plugins/CoreVisualizations/Visualizations/Graph.php
+++ b/plugins/CoreVisualizations/Visualizations/Graph.php
@@ -117,8 +117,22 @@ abstract class Graph extends Visualization
     {
         $this->determineWhichRowsAreSelectable();
 
+        // set default selectable columns, if none specified
+        $selectableColumns = $this->config->selectable_columns;
+        if (false === $selectableColumns) {
+            $this->generateSelectableColumns();
+        }
+
+        $this->ensureValidColumnsToDisplay();
+
+        $this->addTranslations();
+
         $this->config->selectable_rows = array_values($this->selectableRows);
 
+    }
+
+    protected function addTranslations()
+    {
         if ($this->config->add_total_row) {
             $totalTranslation = Piwik::translate('General_Total');
             $this->config->selectable_rows[] = array(
@@ -135,26 +149,64 @@ abstract class Graph extends Visualization
             ));
         }
 
-        // set default selectable columns, if none specified
-        $selectableColumns = $this->config->selectable_columns;
-        if (false === $selectableColumns) {
-            $selectableColumns = array('nb_visits', 'nb_actions', 'nb_uniq_visitors', 'nb_users');
-
-            if ($this->config->show_goals) {
-                $goalMetrics       = array('nb_conversions', 'revenue');
-                $selectableColumns = array_merge($selectableColumns, $goalMetrics);
-            }
-        }
-
         $transformed = array();
-        foreach ($selectableColumns as $column) {
+        foreach ($this->config->selectable_columns as $column) {
             $transformed[] = array(
                 'column'      => $column,
                 'translation' => @$this->config->translations[$column],
                 'displayed'   => in_array($column, $this->config->columns_to_display)
             );
         }
-
         $this->config->selectable_columns = $transformed;
     }
+
+    protected function generateSelectableColumns()
+    {
+        $defaultColumns = array(
+            'nb_visits',
+            'nb_actions',
+            'nb_uniq_visitors',
+            'nb_users'
+        );
+        if ($this->config->show_goals) {
+            $goalMetrics       = array('nb_conversions', 'revenue');
+            $defaultColumns    = array_merge($defaultColumns, $goalMetrics);
+        }
+
+        // Use the subset of default columns that are actually present in the datatable
+        $allColumns = $this->getDataTable()->getColumns();
+        $selectableColumns = array_intersect($defaultColumns, $allColumns);
+
+        // If there are no default columns, just strip out the 'label' column and use all the others
+        if (empty($selectableColumns)) {
+            $selectableColumns = $this->removeLabelFromArray($allColumns);
+        }
+
+        $this->config->selectable_columns = $selectableColumns;
+    }
+
+    private function removeLabelFromArray($theArray)
+    {
+        if (!empty($theArray) && is_array($theArray)) {
+            $key = array_search('label', $theArray);
+            if ($key !== false) {
+                unset($theArray[$key]);
+                $theArray = array_values($theArray);
+            }
+        }
+
+        return $theArray;
+    }
+
+    protected function ensureValidColumnsToDisplay()
+    {
+        $columnsToDisplay = $this->config->columns_to_display;
+
+        // Remove 'label' from columns to display if present
+        $columnsToDisplay = $this->removeLabelFromArray($columnsToDisplay);
+
+        // Strip out any columns_to_display that are not in the dataset
+        $allColumns = $this->getDataTable()->getColumns();
+        $this->config->columns_to_display = array_intersect($columnsToDisplay, $allColumns);
+    }
 }
diff --git a/plugins/CoreVisualizations/Visualizations/JqplotGraph/Bar.php b/plugins/CoreVisualizations/Visualizations/JqplotGraph/Bar.php
index a31a88d190..1204ae8a09 100644
--- a/plugins/CoreVisualizations/Visualizations/JqplotGraph/Bar.php
+++ b/plugins/CoreVisualizations/Visualizations/JqplotGraph/Bar.php
@@ -36,6 +36,16 @@ class Bar extends JqplotGraph
         return $config;
     }
 
+    protected function ensureValidColumnsToDisplay()
+    {
+        parent::ensureValidColumnsToDisplay();
+
+        $columnsToDisplay = $this->config->columns_to_display;
+
+        // Use a sensible default if the columns_to_display is empty
+        $this->config->columns_to_display = $columnsToDisplay ? : array('nb_visits');
+    }
+
     protected function makeDataGenerator($properties)
     {
         return JqplotDataGenerator::factory('bar', $properties);
diff --git a/plugins/CoreVisualizations/Visualizations/JqplotGraph/Pie.php b/plugins/CoreVisualizations/Visualizations/JqplotGraph/Pie.php
index 3c43bb31a1..42ba69ef3f 100644
--- a/plugins/CoreVisualizations/Visualizations/JqplotGraph/Pie.php
+++ b/plugins/CoreVisualizations/Visualizations/JqplotGraph/Pie.php
@@ -38,16 +38,16 @@ class Pie extends JqplotGraph
         $this->config->datatable_js_type = 'JqplotPieGraphDataTable';
     }
 
-    public function afterAllFiltersAreApplied()
+    protected function ensureValidColumnsToDisplay()
     {
-        parent::afterAllFiltersAreApplied();
+        parent::ensureValidColumnsToDisplay();
 
-        $metricColumn = reset($this->config->columns_to_display);
+        $columnsToDisplay = $this->config->columns_to_display;
 
-        if ($metricColumn == 'label') {
-            $metricColumn = next($this->config->columns_to_display);
-        }
+        // Ensure only one column_to_display - it is a pie graph after all!
+        $metricColumn = reset($columnsToDisplay);
 
+        // Set to a sensible default if no suitable value was found
         $this->config->columns_to_display = array($metricColumn ? : 'nb_visits');
     }
 
diff --git a/plugins/CoreVisualizations/tests/Unit/GraphTest.php b/plugins/CoreVisualizations/tests/Unit/GraphTest.php
new file mode 100644
index 0000000000..285960ae61
--- /dev/null
+++ b/plugins/CoreVisualizations/tests/Unit/GraphTest.php
@@ -0,0 +1,152 @@
+<?php
+/**
+ * Matomo - free/libre analytics platform
+ *
+ * @link http://matomo.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Plugins\CoreVisualizations\tests\Unit;
+
+use Piwik\DataTable;
+
+/**
+ * @group CoreVisualizations
+ * @group SparklinesConfigTest
+ * @group Sparklines
+ * @group Plugins
+ */
+class GraphTest extends \PHPUnit_Framework_TestCase
+{
+    public function testSelectableColumnsAlreadySet()
+    {
+        $bar = $this->getMockGraph(array());
+        $bar->config->selectable_columns = array('nb_abc', 'nb_xyz');
+        $bar->afterAllFiltersAreApplied();
+
+        $columns = array_column($bar->config->selectable_columns, 'column');
+        $this->assertEquals(array('nb_abc', 'nb_xyz'), $columns);
+    }
+
+    public function testSelectableColumnsNbVisitsOnlyPresent()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'nb_visits' => 0,
+        ));
+
+        $bar->afterAllFiltersAreApplied();
+
+        $columns = array_column($bar->config->selectable_columns, 'column');
+        $this->assertEquals(array('nb_visits'), $columns);
+    }
+
+    public function testSelectableColumnsUniqueVisitorsPresent()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'nb_visits' => 5,
+            'nb_uniq_visitors' => 2,
+            'nb_something_else' => 1
+        ));
+
+        $bar->afterAllFiltersAreApplied();
+
+        $columns = array_column($bar->config->selectable_columns, 'column');
+        $this->assertEquals(array('nb_visits', 'nb_uniq_visitors'), $columns);
+    }
+
+    public function testSelectableColumnsNoDefaultColumnsPresent()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'foo' => 5,
+            'bar' => 2
+        ));
+
+        $bar->afterAllFiltersAreApplied();
+
+        $columns = array_column($bar->config->selectable_columns, 'column');
+        $this->assertEquals(array('foo', 'bar'), $columns);
+    }
+
+    public function testSelectableColumnsNoDefaultColumnsPresentLabelNotPresent()
+    {
+        $bar = $this->getMockGraph(array(
+            'foo' => 5,
+            'bar' => 2
+        ));
+
+        $bar->afterAllFiltersAreApplied();
+
+        $columns = array_column($bar->config->selectable_columns, 'column');
+        $this->assertEquals(array('foo', 'bar'), $columns);
+    }
+
+    public function testSelectableColumnsAlreadySetToEmptyArray()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'foo' => 5,
+            'bar' => 2
+        ));
+        $bar->config->selectable_columns = array();
+
+        $bar->afterAllFiltersAreApplied();
+
+        $this->assertEmpty($bar->config->selectable_columns);
+    }
+
+    public function testColumnsToDisplayNotInDataset()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'nb_visits' => 25,
+            'nb_actions' => 80
+        ));
+        // Not present in the dataset = should be thrown out and replaced with first value from dataset
+        $bar->config->columns_to_display = array('nb_uniq_visitors');
+
+        $bar->afterAllFiltersAreApplied();
+
+        $this->assertEquals(array('nb_visits'), $bar->config->columns_to_display);
+    }
+
+    public function testColumnsToDisplayNotInSelectableColumns()
+    {
+        $bar = $this->getMockGraph(array(
+            'label' => 'abc',
+            'nb_visits' => 25,
+            'nb_actions' => 80
+        ));
+        // columns_to_display is not present in selectable_columbs, but as long as it's in the data set that's OK
+        $bar->config->selectable_columns = array('nb_visits');
+        $bar->config->columns_to_display = array('nb_actions');
+
+        $bar->afterAllFiltersAreApplied();
+
+        $this->assertEquals(array('nb_actions'), $bar->config->columns_to_display);
+    }
+
+    /**
+     * @return \Piwik\Plugins\CoreVisualizations\Visualizations\JqplotGraph\Bar
+     */
+    private function getMockGraph(array $firstDataRow)
+    {
+        $row = new DataTable\Row(array(
+            DataTable\Row::COLUMNS => $firstDataRow
+        ));
+        $dataTable = new DataTable();
+        $dataTable->setRows(array($row));
+
+        $bar = $this->getMock('Piwik\Plugins\CoreVisualizations\Visualizations\JqplotGraph\Bar',
+            array('getDataTable'),
+            array('', '')
+        );
+        $bar->expects($this->any())
+            ->method('getDataTable')
+            ->will($this->returnValue($dataTable));
+
+        return $bar;
+    }
+}