From 64780478120bb8f5336459865eae698ec1710495 Mon Sep 17 00:00:00 2001 From: Thomas Steur Date: Sun, 15 Feb 2015 22:50:17 +0000 Subject: [PATCH] refs #4633 a couple of code improvements eg we do not need to run a filter within a filter and run them recursive. not sure if recursive is a good idea though --- core/DataTable/Filter/AddSegmentByLabel.php | 67 +++++++++---------- .../Filter/AddSegmentByLabelMapping.php | 21 +++--- .../Filter/AddSegmentBySegmentValue.php | 2 + .../Filter/ColumnCallbackDeleteMetadata.php | 4 ++ .../ColumnCallbackDeleteMetadataTest.php | 27 +++++++- 5 files changed, 72 insertions(+), 49 deletions(-) diff --git a/core/DataTable/Filter/AddSegmentByLabel.php b/core/DataTable/Filter/AddSegmentByLabel.php index a841f18c9a9..1b2f5bc9ea6 100644 --- 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 index 95d6e05c792..89c48a5741f 100644 --- 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 index 89c3b6a59b3..ba5ed048b70 100644 --- 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 index dc37f960166..bb8618a6cb7 100644 --- 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 index 2bd33cf39f0..778125487e6 100644 --- 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); + } }