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

* ENH: Update reorderItems() to use ORM where possible * Increase test coverage of orderable rows
2022-08-04 10:51:40 +12:00 · 2022-08-04 10:51:40 +12:00 · 23a5154c97
parent 7a8f244df0
commit 23a5154c97
8 changed files with 182 additions and 41 deletions
--- a/src/GridFieldOrderableRows.php
+++ b/src/GridFieldOrderableRows.php
@ -587,9 +587,10 @@ class GridFieldOrderableRows extends RequestHandler implements
     */
    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 @@ class GridFieldOrderableRows extends RequestHandler implements
        // 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 @@ class GridFieldOrderableRows extends RequestHandler implements
                // 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();
--- a/tests/GridFieldOrderableRowsTest.php
+++ b/tests/GridFieldOrderableRowsTest.php
@ -8,7 +8,6 @@ use SilverStripe\Forms\GridField\GridField;
 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\StubParent;
 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;
+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\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']
        ];
    }
--- a/tests/GridFieldOrderableRowsTest.yml
+++ b/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
--- a/tests/OrderableRowsThroughTest.yml
+++ b/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
--- a/tests/Stub/StubParent.php
+++ b/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';
--- a/tests/Stub/ThroughBelongsVersioned.php
+++ b/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,
+    ];
+}
--- a/tests/Stub/ThroughDefinerVersioned.php
+++ b/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,
+    ];
+}
--- a/tests/Stub/ThroughIntermediaryVersioned.php
+++ b/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,
+    ];
+}