diff --git a/src/GridFieldOrderableRows.php b/src/GridFieldOrderableRows.php index dc92ee3b..492d96f6 100755 --- a/src/GridFieldOrderableRows.php +++ b/src/GridFieldOrderableRows.php @@ -148,6 +148,8 @@ public function getExtraSortFields() * Checks to see if the relationship list is for a type of many_many * * @param SS_List $list + * + * @return bool */ protected function isManyMany(SS_List $list) { @@ -493,7 +495,7 @@ public function handleSave(GridField $grid, DataObjectInterface $record) */ protected function executeReorder(GridField $grid, $sortedIDs) { - if (!is_array($sortedIDs)) { + if (!is_array($sortedIDs) || empty($sortedIDs)) { return false; } $sortField = $this->getSortField(); @@ -540,6 +542,7 @@ protected function executeReorder(GridField $grid, $sortedIDs) $toRelationName = $manipulator->getLocalKey(); $sortlist = DataList::create($joinClass)->filter([ $toRelationName => $items->column('ID'), + // first() is safe as there are earlier checks to ensure our list to sort is valid $fromRelationName => $items->first()->getJoin()->$fromRelationName, ]); $current = $sortlist->map($toRelationName, $sortField)->toArray(); @@ -583,19 +586,19 @@ protected function reorderItems($list, array $values, array $sortedIDs) // - Related item is versioned... // - Through object is versioned: handle as versioned // - Through object is NOT versioned: THROW an error. - $isManyMany = $this->isManyMany($list); - if ($isManyMany && $list instanceof ManyManyThroughList) { + if ($list instanceof ManyManyThroughList) { $listClassVersioned = $class::create()->hasExtension(Versioned::class); // We'll be updating the join class, not the relation class. $class = $this->getManyManyInspector($list)->getJoinClass(); $isVersioned = $class::create()->hasExtension(Versioned::class); + // If one side of the relationship is versioned and the other is not, throw an error. if ($listClassVersioned xor $isVersioned) { throw new Exception( 'ManyManyThrough cannot mismatch Versioning between join class and related class' ); } - } elseif (!$isManyMany) { + } elseif (!$this->isManyMany($list)) { $isVersioned = $class::create()->hasExtension(Versioned::class); } @@ -712,12 +715,12 @@ protected function getSortTableClauseForIds(DataList $list, $ids) } if ($this->isManyMany($list)) { - $intropector = $this->getManyManyInspector($list); + $introspector = $this->getManyManyInspector($list); $extra = $list instanceof ManyManyList ? - $intropector->getExtraFields() : - DataObjectSchema::create()->fieldSpecs($intropector->getJoinClass(), DataObjectSchema::DB_ONLY); - $key = $intropector->getLocalKey(); - $foreignKey = $intropector->getForeignKey(); + $introspector->getExtraFields() : + DataObjectSchema::create()->fieldSpecs($introspector->getJoinClass(), DataObjectSchema::DB_ONLY); + $key = $introspector->getLocalKey(); + $foreignKey = $introspector->getForeignKey(); $foreignID = (int) $list->getForeignID(); if ($extra && array_key_exists($this->getSortField(), $extra)) { @@ -739,7 +742,7 @@ protected function getSortTableClauseForIds(DataList $list, $ids) * these functions are moved to ManyManyThroughQueryManipulator, but otherwise retain * the same signature. * - * @param ManyManyList|ManyManyThroughList + * @param ManyManyList|ManyManyThroughList $list * * @return ManyManyList|ManyManyThroughQueryManipulator */ diff --git a/tests/GridFieldOrderableRowsTest.php b/tests/GridFieldOrderableRowsTest.php index 487d601f..024e7914 100644 --- a/tests/GridFieldOrderableRowsTest.php +++ b/tests/GridFieldOrderableRowsTest.php @@ -14,6 +14,9 @@ use Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass; use Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned; use Symbiote\GridFieldExtensions\Tests\Stub\StubUnorderable; +use Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner; +use Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediary; +use Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs; /** * Tests for the {@link GridFieldOrderableRows} component. @@ -23,7 +26,10 @@ class GridFieldOrderableRowsTest extends SapphireTest /** * @var string */ - protected static $fixture_file = 'GridFieldOrderableRowsTest.yml'; + protected static $fixture_file = [ + 'GridFieldOrderableRowsTest.yml', + 'OrderableRowsThroughTest.yml' + ]; /** * @var array @@ -36,27 +42,42 @@ class GridFieldOrderableRowsTest extends SapphireTest StubOrderableChild::class, StubOrderedVersioned::class, StubSubclassOrderedVersioned::class, + ThroughDefiner::class, + ThroughIntermediary::class, + ThroughBelongs::class, ]; - public function testReorderItems() + public function reorderItemsProvider() + { + return [ + [StubParent::class . '.parent', 'MyManyMany', 'ManyManySort'], + [ThroughDefiner::class . '.DefinerOne', 'Belongings', 'Sort'], + ]; + } + + /** + * @dataProvider reorderItemsProvider + */ + public function testReorderItems($fixtureID, $relationName, $sortName) { - $orderable = new GridFieldOrderableRows('ManyManySort'); + $orderable = new GridFieldOrderableRows($sortName); $reflection = new ReflectionMethod($orderable, 'executeReorder'); $reflection->setAccessible(true); - $parent = $this->objFromFixture(StubParent::class, 'parent'); - $config = new GridFieldConfig_RelationEditor(); $config->addComponent($orderable); + list($parentClass, $parentInstanceID) = explode('.', $fixtureID); + $parent = $this->objFromFixture($parentClass, $parentInstanceID); + $grid = new GridField( - 'MyManyMany', - 'My Many Many', - $parent->MyManyMany()->sort('ManyManySort'), + $relationName, + 'Testing Many Many', + $parent->$relationName()->sort($sortName), $config ); - $originalOrder = $parent->MyManyMany()->sort('ManyManySort')->column('ID'); + $originalOrder = $parent->$relationName()->sort($sortName)->column('ID'); $desiredOrder = []; // Make order non-contiguous, and 1-based @@ -68,7 +89,7 @@ public function testReorderItems() $reflection->invoke($orderable, $grid, $desiredOrder); - $newOrder = $parent->MyManyMany()->sort('ManyManySort')->map('ManyManySort', 'ID')->toArray(); + $newOrder = $parent->$relationName()->sort($sortName)->map($sortName, 'ID')->toArray(); $this->assertEquals($desiredOrder, $newOrder); } @@ -157,7 +178,7 @@ public function testReorderItemsSubclassVersioned() foreach ($parent->MyHasManySubclassOrderedVersioned() as $item) { /** @var StubSubclassOrderedVersioned|Versioned $item */ if ($item->stagesDiffer()) { - $this->fail('Unexpected diference found on stages'); + $this->fail('Unexpected difference found on stages'); } } diff --git a/tests/OrderableRowsThroughTest.yml b/tests/OrderableRowsThroughTest.yml new file mode 100644 index 00000000..a0e11ea1 --- /dev/null +++ b/tests/OrderableRowsThroughTest.yml @@ -0,0 +1,30 @@ +Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner: + DefinerOne: + DefinerTwo: + +Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs: + BelongsOne: + BelongsTwo: + BelongsThree: + +Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediary: + One: + Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerOne + Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsOne + Sort: 3 + Two: + Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerOne + Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsTwo + Sort: 2 + Three: + Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerOne + Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsThree + Sort: 1 + Four: + Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerTwo + Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsTwo + Sort: 1 + Five: + Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerTwo + Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsThree + Sort: 2 diff --git a/tests/OrderableRowsThroughVersionedTest.php b/tests/OrderableRowsThroughVersionedTest.php new file mode 100644 index 00000000..1ebeeab5 --- /dev/null +++ b/tests/OrderableRowsThroughVersionedTest.php @@ -0,0 +1,109 @@ + [Versioned::class], + ThroughIntermediary::class => [Versioned::class], + ThroughBelongs::class => [Versioned::class], + ]; + + /** + * Mostly the same as GridFieldOrderableRowsTest::testReorderItems + * but with some Versioned calls & checks mixed in. + */ + public function testReorderingSaves() + { + $parent = $this->objFromFixture(ThroughDefiner::class, 'DefinerOne'); + $sortName = 'Sort'; + + $orderable = new GridFieldOrderableRows($sortName); + $reflection = new ReflectionMethod($orderable, 'executeReorder'); + $reflection->setAccessible(true); + + $config = new GridFieldConfig_RelationEditor(); + $config->addComponent($orderable); + + // This test data is versioned - ensure we're all published + $parent->publishRecursive(); + // there should be no difference between stages at this point + foreach ($parent->Belongings() as $item) { + $this->assertFalse( + $item->stagesDiffer(), + 'No records should be different from their published versions' + ); + } + + $grid = new GridField( + 'Belongings', + 'Testing Many Many', + $parent->Belongings()->sort($sortName), + $config + ); + + $originalOrder = $parent->Belongings()->sort($sortName)->column('ID'); + $desiredOrder = []; + + // Make order non-contiguous, and 1-based + foreach (array_reverse($originalOrder) as $index => $id) { + $desiredOrder[$index * 2 + 1] = $id; + } + + $this->assertNotEquals($originalOrder, $desiredOrder); + + $reflection->invoke($orderable, $grid, $desiredOrder); + + $newOrder = $parent->Belongings()->sort($sortName)->map($sortName, 'ID')->toArray(); + + $this->assertEquals($desiredOrder, $newOrder); + + // pass forward to testReorderedPublish + return $parent; + } + + /** + * @depends testReorderingSaves + * @param ThroughDefiner $parent passed through from testReorderingSaves + */ + public function testReorderedPublish($parent) + { + // reorder should have been handled as versioned - there should be a difference between stages now + $differenceFound = false; + foreach ($parent->Belongings() as $item) { + if ($item->stagesDiffer()) { + $differenceFound = true; + break; + } + } + $this->assertTrue($differenceFound, 'At least one record should be changed'); + + $parent->publishRecursive(); + + foreach ($parent->Belongings() as $item) { + $this->assertFalse( + $item->stagesDiffer(), + 'No records should be different from their published versions anymore' + ); + } + } +} diff --git a/tests/Stub/StubParent.php b/tests/Stub/StubParent.php index 4193c1cd..89043188 100644 --- a/tests/Stub/StubParent.php +++ b/tests/Stub/StubParent.php @@ -7,19 +7,19 @@ class StubParent extends DataObject implements TestOnly { - private static $has_many = array( + private static $has_many = [ 'MyHasMany' => StubOrdered::class, 'MyHasManySubclass' => StubSubclass::class, 'MyHasManySubclassOrderedVersioned' => StubSubclassOrderedVersioned::class, - ); + ]; - private static $many_many = array( - 'MyManyMany' => StubOrdered::class - ); + private static $many_many = [ + 'MyManyMany' => StubOrdered::class, + ]; - private static $many_many_extraFields = array( - 'MyManyMany' => array('ManyManySort' => 'Int') - ); + private static $many_many_extraFields = [ + 'MyManyMany' => ['ManyManySort' => 'Int'], + ]; private static $table_name = 'StubParent'; } diff --git a/tests/Stub/ThroughBelongs.php b/tests/Stub/ThroughBelongs.php new file mode 100644 index 00000000..bbccee8a --- /dev/null +++ b/tests/Stub/ThroughBelongs.php @@ -0,0 +1,17 @@ + ThroughDefiner::class, + ]; +} diff --git a/tests/Stub/ThroughDefiner.php b/tests/Stub/ThroughDefiner.php new file mode 100644 index 00000000..3b567166 --- /dev/null +++ b/tests/Stub/ThroughDefiner.php @@ -0,0 +1,25 @@ + [ + 'through' => ThroughIntermediary::class, + 'from' => 'Defining', + 'to' => 'Belonging', + ] + ]; + + private static $owns = [ + 'Belongings' + ]; +} diff --git a/tests/Stub/ThroughIntermediary.php b/tests/Stub/ThroughIntermediary.php new file mode 100644 index 00000000..22ec3e20 --- /dev/null +++ b/tests/Stub/ThroughIntermediary.php @@ -0,0 +1,23 @@ + DBInt::class, + ]; + + private static $default_sort = 'Sort ASC'; + + private static $has_one = [ + 'Defining' => ThroughDefiner::class, + 'Belonging' => ThroughBelongs::class, + ]; +}