refs #4633 a couple of code improvements eg we do not need to run a f…

…ilter within a filter and run them recursive. not sure if recursive is a good idea though
matomo-org · Feb 15, 2015 · 6478047 · 6478047
1 parent 7d97c52
commit 6478047
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 49 deletions.
diff --git a/core/DataTable/Filter/AddSegmentByLabel.php b/core/DataTable/Filter/AddSegmentByLabel.php
@@ -54,59 +54,54 @@ public function __construct($table, $segmentOrSegments, $delimiter = '')
      */
     public function filter($table)
     {
-        $delimiter = $this->delimiter;
-        $segments  = $this->segments;
-
-        if (empty($segments)) {
+        if (empty($this->segments)) {
             $msg = 'AddSegmentByLabel is called without having any segments defined';
             Development::error($msg);
             return;
         }
 
-        if (count($segments) === 1) {
-            $table->filter(function (DataTable $dataTable) use ($segments) {
-                $segment = array_shift($segments);
+        if (count($this->segments) === 1) {
+            $segment = reset($this->segments);
 
-                foreach ($dataTable->getRows() as $key => $row) {
-                    if ($key == DataTable::ID_SUMMARY_ROW) {
-                        continue;
-                    }
+            foreach ($table->getRows() as $key => $row) {
+                if ($key == DataTable::ID_SUMMARY_ROW) {
+                    continue;
+                }
 
-                    $label = $row->getColumn('label');
+                $label = $row->getColumn('label');
 
-                    if (!empty($label)) {
-                        $row->setMetadata('segment', $segment . '==' . urlencode($label));
-                    }
+                if (!empty($label)) {
+                    $row->setMetadata('segment', $segment . '==' . urlencode($label));
                 }
-            });
-        } else if (!empty($delimiter)) {
-            $table->filter(function (DataTable $dataTable) use ($segments, $delimiter) {
-                $numSegments  = count($segments);
-                $conditionAnd = ';';
 
-                foreach ($dataTable->getRows() as $key => $row) {
-                    if ($key == DataTable::ID_SUMMARY_ROW) {
-                        continue;
-                    }
+                $this->filterSubTable($row);
+            }
+        } else if (!empty($this->delimiter)) {
+            $numSegments  = count($this->segments);
+            $conditionAnd = ';';
+
+            foreach ($table->getRows() as $key => $row) {
+                if ($key == DataTable::ID_SUMMARY_ROW) {
+                    continue;
+                }
 
-                    $label = $row->getColumn('label');
-                    if (!empty($label)) {
-                        $parts = explode($delimiter, $label);
+                $label = $row->getColumn('label');
+                if (!empty($label)) {
+                    $parts = explode($this->delimiter, $label);
 
-                        if (count($parts) === $numSegments) {
-                            $filter = array();
-                            foreach ($segments as $index => $segment) {
-                                if (!empty($segment)) {
-                                    $filter[] = $segment . '==' . urlencode($parts[$index]);
-                                }
+                    if (count($parts) === $numSegments) {
+                        $filter = array();
+                        foreach ($this->segments as $index => $segment) {
+                            if (!empty($segment)) {
+                                $filter[] = $segment . '==' . urlencode($parts[$index]);
                             }
-                            $row->setMetadata('segment', implode($conditionAnd, $filter));
                         }
+                        $row->setMetadata('segment', implode($conditionAnd, $filter));
                     }
                 }
-            });
+            }
         } else {
-            $names = implode(', ', $segments);
+            $names = implode(', ', $this->segments);
             $msg   = 'Multiple segments are given but no delimiter defined. Segments: ' . $names;
             Development::error($msg);
         }

diff --git a/core/DataTable/Filter/AddSegmentByLabelMapping.php b/core/DataTable/Filter/AddSegmentByLabelMapping.php
@@ -47,22 +47,19 @@ public function __construct($table, $segment, $mapping)
      */
     public function filter($table)
     {
-        $mapping = $this->mapping;
-        $segment = $this->segment;
-
-        if (empty($segment) || empty($mapping)) {
+        if (empty($this->segment) || empty($this->mapping)) {
             return;
         }
 
-        $table->filter(function (DataTable $dataTable) use ($segment, $mapping) {
-            foreach ($dataTable->getRows() as $row) {
-                $label = $row->getColumn('label');
+        foreach ($table->getRows() as $row) {
+            $label = $row->getColumn('label');
 
-                if (!empty($mapping[$label])) {
-                    $label = $mapping[$label];
-                    $row->setMetadata('segment', $segment . '==' . urlencode($label));
-                }
+            if (!empty($this->mapping[$label])) {
+                $label = $this->mapping[$label];
+                $row->setMetadata('segment', $this->segment . '==' . urlencode($label));
             }
-        });
+
+            $this->filterSubTable($row);
+        }
     }
 }
diff --git a/core/DataTable/Filter/AddSegmentBySegmentValue.php b/core/DataTable/Filter/AddSegmentBySegmentValue.php
@@ -73,6 +73,8 @@ public function filter($table)
             if ($value !== false && $filter === false) {
                 $row->setMetadata('segment', sprintf('%s==%s', $segmentName, urlencode($value)));
             }
+
+            $this->filterSubTable($row);
         }
     }
 }
diff --git a/core/DataTable/Filter/ColumnCallbackDeleteMetadata.php b/core/DataTable/Filter/ColumnCallbackDeleteMetadata.php
@@ -44,8 +44,12 @@ public function __construct($table, $metadataToRemove)
      */
     public function filter($table)
     {
+        $this->enableRecursive(true);
+
         foreach ($table->getRows() as $row) {
             $row->deleteMetadata($this->metadataToRemove);
+
+            $this->filterSubTable($row);
         }
     }
 }
diff --git a/tests/PHPUnit/Unit/DataTable/Filter/ColumnCallbackDeleteMetadataTest.php b/tests/PHPUnit/Unit/DataTable/Filter/ColumnCallbackDeleteMetadataTest.php
@@ -38,13 +38,21 @@ public function setUp()
         $this->addRowWithMetadata(array('test' => '4'));
     }
 
-    private function addRowWithMetadata($metadata)
+    private function buildRowWithMetadata($metadata)
     {
         $row = new Row(array(Row::COLUMNS => array('label' => 'val1')));
         foreach ($metadata as $name => $value) {
             $row->setMetadata($name, $value);
         }
+        return $row;
+    }
+
+    private function addRowWithMetadata($metadata)
+    {
+        $row = $this->buildRowWithMetadata($metadata);
         $this->table->addRow($row);
+
+        return $row;
     }
 
     public function test_filter_shouldRemoveAllMetadataEntriesHavingTheGivenName()
@@ -70,4 +78,21 @@ public function test_filter_shouldRemoveAllMetadataEntriesHavingTheGivenName_Eve
         $expected = array('1', '2', '3', '1', '4');
         $this->assertSame($expected, $metadata);
     }
+
+    public function test_filter_shouldRemoveTheMetadataFromSubtables_IfOneIsSet()
+    {
+        $row   = $this->addRowWithMetadata(array('test' => '5', 'other' => 'value2'));
+        $table = new DataTable();
+        $table->addRow($this->buildRowWithMetadata(array('other' => 'value3')));
+        $table->addRow($this->buildRowWithMetadata(array('test' => '6')));
+        $table->addRow($this->buildRowWithMetadata(array('test' => '7', 'other' => 'value4')));
+        $row->setSubtable($table);
+
+        $this->table->filter($this->filter, array('other'));
+
+        $this->assertFalse($row->getMetadata('other'));
+
+        $metadata = $table->getRowsMetadata('other');
+        $this->assertSame(array(false, false, false), $metadata);
+    }
 }