ENH: Update reorderItems() to use ORM where possible (#336)

* ENH: Update reorderItems() to use ORM where possible

* Increase test coverage of orderable rows

src/GridFieldOrderableRows.php

@@ -587,9 +587,10 @@ protected function executeReorder(GridField $grid, $sortedIDs)
      */
     protected function reorderItems($list, array $values, array $sortedIDs)
     {
+        $this->extend('onBeforeReorderItems', $list, $values, $sortedIDs);
+
         // setup
         $sortField = $this->getSortField();
-        $class = $list->dataClass();
         // The problem is that $sortedIDs is a list of the _related_ item IDs, which causes trouble
         // with ManyManyThrough, where we need the ID of the _join_ item in order to set the value.
         $itemToSortReference = ($list instanceof ManyManyThroughList) ? 'getJoin' : 'Me';
@@ -598,53 +599,21 @@ protected function reorderItems($list, array $values, array $sortedIDs)
         // sanity check.
         $this->validateSortField($list);
 
-        $isVersioned = false;
-        // check if sort column is present on the model provided by dataClass() and if it's versioned
-        // cases:
-        // Model has sort column and is versioned - handle as versioned
-        // Model has sort column and is NOT versioned - handle as NOT versioned
-        // Model doesn't have sort column because sort column is on ManyManyList - handle as NOT versioned
-        // Model doesn't have sort column because sort column is on ManyManyThroughList - inspect through object
-        if ($list instanceof ManyManyThroughList) {
-            // We'll be updating the join class, not the relation class.
-            $class = $this->getManyManyInspector($list)->getJoinClass();
-            $isVersioned = $class::create()->hasExtension(Versioned::class);
-        } elseif (!$this->isManyMany($list)) {
-            $isVersioned = $class::create()->hasExtension(Versioned::class);
-        }
-
-        // Loop through each item, and update the sort values which do not
-        // match to order the objects.
-        if (!$isVersioned) {
+        // ManyManyList extra fields aren't easily updated via the ORM, and so they need to be updated through an SQL
+        // Query
+        if ($list instanceof ManyManyList) {
             $sortTable = $this->getSortTable($list);
-            $now = DBDatetime::now()->Rfc2822();
-            $additionalSQL = '';
-            $baseTable = DataObject::getSchema()->baseDataTable($class);
-
-            $isBaseTable = ($baseTable == $sortTable);
-            if (!$list instanceof ManyManyList && $isBaseTable) {
-                $additionalSQL = ", \"LastEdited\" = '$now'";
-            }
 
+            // Loop through each item, and update the sort values which do not match to order the objects.
             foreach ($sortedIDs as $newSortValue => $targetRecordID) {
                 if ($currentSortList[$targetRecordID]->$sortField != $newSortValue) {
                     DB::query(sprintf(
-                        'UPDATE "%s" SET "%s" = %d%s WHERE %s',
+                        'UPDATE "%s" SET "%s" = %d WHERE %s',
                         $sortTable,
                         $sortField,
                         $newSortValue,
-                        $additionalSQL,
                         $this->getSortTableClauseForIds($list, $targetRecordID)
                     ));
-
-                    if (!$isBaseTable && !$list instanceof ManyManyList) {
-                        DB::query(sprintf(
-                            'UPDATE "%s" SET "LastEdited" = \'%s\' WHERE %s',
-                            $baseTable,
-                            $now,
-                            $this->getSortTableClauseForIds($list, $targetRecordID)
-                        ));
-                    }
                 }
             }
         } else {
@@ -656,6 +625,7 @@ protected function reorderItems($list, array $values, array $sortedIDs)
                 // either the list data class (has_many, (belongs_)many_many)
                 // or the intermediary join class (many_many through)
                 $record = $currentSortList[$targetRecordID];
+
                 if ($record->$sortField != $newSortValue) {
                     $record->$sortField = $newSortValue;
                     $record->write();

tests/GridFieldOrderableRowsTest.php

@@ -8,7 +8,6 @@
 use SilverStripe\Forms\GridField\GridFieldConfig_RelationEditor;
 use SilverStripe\ORM\DataList;
 use Symbiote\GridFieldExtensions\GridFieldOrderableRows;
-use Symbiote\GridFieldExtensions\Tests\Stub\PolymorphM2MChild;
 use Symbiote\GridFieldExtensions\Tests\Stub\PolymorphM2MMapper;
 use Symbiote\GridFieldExtensions\Tests\Stub\PolymorphM2MParent;
 use Symbiote\GridFieldExtensions\Tests\Stub\StubOrderableChild;
@@ -18,11 +17,14 @@
 use Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass;
 use Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned;
 use Symbiote\GridFieldExtensions\Tests\Stub\StubUnorderable;
+use Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs;
+use Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned;
 use Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner;
+use Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned;
 use Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediary;
-use Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs;
 use Symbiote\GridFieldExtensions\Tests\Stub\TitleObject;
 use Symbiote\GridFieldExtensions\Tests\Stub\TitleSortedObject;
+use Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediaryVersioned;
 
 /**
  * Tests for the {@link GridFieldOrderableRows} component.
@@ -51,13 +53,21 @@ class GridFieldOrderableRowsTest extends SapphireTest
         ThroughBelongs::class,
         TitleObject::class,
         TitleSortedObject::class,
+        ThroughDefinerVersioned::class,
+        ThroughIntermediaryVersioned::class,
+        ThroughBelongsVersioned::class,
     ];
 
     public function reorderItemsProvider()
     {
         return [
+            [StubParent::class . '.parent', 'MyHasMany', 'Sort'],
+            [StubParent::class . '.parent', 'MyHasManySubclass', 'Sort'],
+            [StubParent::class . '.parent-subclass-ordered-versioned', 'MyHasManySubclassOrderedVersioned', 'Sort'],
             [StubParent::class . '.parent', 'MyManyMany', 'ManyManySort'],
+            [StubParent::class . '.parent', 'MyManyManyVersioned', 'ManyManySort'],
             [ThroughDefiner::class . '.DefinerOne', 'Belongings', 'Sort'],
+            [ThroughDefinerVersioned::class . '.DefinerOne', 'Belongings', 'Sort'],
             // [PolymorphM2MParent::class . '.ParentOne', 'Children', 'Sort']
         ];
     }

tests/GridFieldOrderableRowsTest.yml

@@ -7,6 +7,7 @@ Symbiote\GridFieldExtensions\Tests\Stub\StubOrderableChild:
     Sort: 3
   item4:
     Sort: 4
+
 Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered:
   item1:
     Sort: 1
@@ -27,6 +28,20 @@ Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered:
       - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrderableChild.item3
       - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrderableChild.item4
 
+Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass:
+  item1:
+    Sort: 1
+  item2:
+    Sort: 2
+  item3:
+    Sort: 3
+  item4:
+    Sort: 4
+  item5:
+    Sort: 5
+  item6:
+    Sort: 6
+
 Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned:
   item1:
     ExtraField: 1
@@ -56,6 +71,29 @@ Symbiote\GridFieldExtensions\Tests\Stub\StubParent:
         ManyManySort: 108
       - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item6:
         ManyManySort: 108
+    MyManyManyVersioned:
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned.item1:
+        ManyManySort: 1
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned.item2:
+        ManyManySort: 1
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned.item3:
+        ManyManySort: 108
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned.item4:
+        ManyManySort: 108
+    MyHasMany:
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item1
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item2
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item3
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item4
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item5
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubOrdered.item6
+    MyHasManySubclass:
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item1
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item2
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item3
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item4
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item5
+      - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclass.item6
   parent-subclass-ordered-versioned:
     MyHasManySubclassOrderedVersioned:
       - =>Symbiote\GridFieldExtensions\Tests\Stub\StubSubclassOrderedVersioned.item1

tests/OrderableRowsThroughTest.yml

@@ -28,3 +28,34 @@ Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediary:
     Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefiner.DefinerTwo
     Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs.BelongsThree
     Sort: 2
+
+Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned:
+  DefinerOne:
+  DefinerTwo:
+
+Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned:
+  BelongsOne:
+  BelongsTwo:
+  BelongsThree:
+
+Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediaryVersioned:
+  One:
+    Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned.DefinerOne
+    Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned.BelongsOne
+    Sort: 3
+  Two:
+    Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned.DefinerOne
+    Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned.BelongsTwo
+    Sort: 2
+  Three:
+    Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned.DefinerOne
+    Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned.BelongsThree
+    Sort: 1
+  Four:
+    Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned.DefinerTwo
+    Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned.BelongsTwo
+    Sort: 1
+  Five:
+    Defining: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughDefinerVersioned.DefinerTwo
+    Belonging: =>Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongsVersioned.BelongsThree
+    Sort: 2

tests/Stub/StubParent.php

@@ -15,10 +15,12 @@ class StubParent extends DataObject implements TestOnly
 
     private static $many_many = [
         'MyManyMany' => StubOrdered::class,
+        'MyManyManyVersioned' => StubSubclassOrderedVersioned::class,
     ];
 
     private static $many_many_extraFields = [
         'MyManyMany' => ['ManyManySort' => 'Int'],
+        'MyManyManyVersioned' => ['ManyManySort' => 'Int'],
     ];
 
     private static $table_name = 'StubParent';

tests/Stub/ThroughBelongsVersioned.php

@@ -0,0 +1,25 @@
+<?php
+
+namespace Symbiote\GridFieldExtensions\Tests\Stub;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\ManyManyList;
+use SilverStripe\Versioned\Versioned;
+
+/**
+ * @method ManyManyList|ThroughDefinerVersioned[] Definers()
+ * @mixin Versioned
+ */
+class ThroughBelongsVersioned extends DataObject implements TestOnly
+{
+    private static string $table_name = 'ThroughBelongsVersioned';
+
+    private static array $belongs_many_many = [
+        'Definers' => ThroughDefinerVersioned::class,
+    ];
+
+    private static array $extensions = [
+        Versioned::class,
+    ];
+}

tests/Stub/ThroughDefinerVersioned.php

@@ -0,0 +1,33 @@
+<?php
+
+namespace Symbiote\GridFieldExtensions\Tests\Stub;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\ManyManyThroughList;
+use SilverStripe\Versioned\Versioned;
+
+/**
+ * @method ManyManyThroughList|ThroughIntermediaryVersioned Belongings()
+ * @mixin Versioned
+ */
+class ThroughDefinerVersioned extends DataObject implements TestOnly
+{
+    private static string $table_name = 'ThroughDefinerVersioned';
+
+    private static array $many_many = [
+        'Belongings' => [
+            'through' => ThroughIntermediaryVersioned::class,
+            'from' => 'Defining',
+            'to' => 'Belonging',
+        ]
+    ];
+
+    private static array $owns = [
+        'Belongings'
+    ];
+
+    private static array $extensions = [
+        Versioned::class,
+    ];
+}

tests/Stub/ThroughIntermediaryVersioned.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace Symbiote\GridFieldExtensions\Tests\Stub;
+
+use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
+use SilverStripe\Versioned\Versioned;
+
+/**
+ * @property int $DefiningID
+ * @property int $BelongingID
+ * @method ThroughDefinerVersioned Defining()
+ * @method ThroughBelongsVersioned Belonging()
+ * @mixin Versioned
+ */
+class ThroughIntermediaryVersioned extends DataObject implements TestOnly
+{
+    private static string $table_name = 'ThroughIntermediaryVersioned';
+
+    private static array $db = [
+        'Sort' => 'Int',
+    ];
+
+    private static array $has_one = [
+        'Defining' => ThroughDefinerVersioned::class,
+        'Belonging' => ThroughBelongsVersioned::class,
+    ];
+
+    private static array $extensions = [
+        Versioned::class,
+    ];
+}