FIX Don't update sort order in live until a block has been published

src/Models/BaseElement.php

@@ -5,6 +5,7 @@
 use DNADesign\Elemental\Controllers\ElementController;
 use DNADesign\Elemental\Forms\TextCheckboxGroupField;
 use DNADesign\Elemental\ORM\FieldType\DBObjectType;
+use DNADesign\Elemental\Services\ReorderElements;
 use DNADesign\Elemental\TopPage\DataExtension;
 use Exception;
 use SilverStripe\CMS\Controllers\CMSPageEditController;
@@ -1289,4 +1290,10 @@ public static function getGraphQLTypeName(): string
         }
         return str_replace('\\', '_', static::class);
     }
+
+    public function onAfterPublish()
+    {
+        $reorderer = Injector::inst()->create(ReorderElements::class, $this);
+        $reorderer->publishSortOrder();
+    }
 }

src/Services/ReorderElements.php

@@ -6,6 +6,7 @@
 use InvalidArgumentException;
 use SilverStripe\Core\Convert;
 use SilverStripe\ORM\DataObject;
+use SilverStripe\ORM\DB;
 use SilverStripe\ORM\Queries\SQLUpdate;
 use SilverStripe\Versioned\Versioned;
 
@@ -58,7 +59,8 @@ public function setElement(BaseElement $element)
     }
 
     /**
-     * Set the ordering of Elements in relation to sibling Elements in the parent {@see ElementalArea}
+     * Set the ordering of Elements in relation to sibling Elements in the parent {@see ElementalArea}.
+     * This only affects the sort order in draft.
      *
      * @param int $elementToBeAfterID ID of the Element to be ordered after
      */
@@ -94,39 +96,45 @@ public function reorder($elementToBeAfterID = 0)
         // We are updating records with SQL queries to avoid the ORM triggering the creation of new versions
         // for each element that is affected by this reordering.
         $baseTableName = Convert::raw2sql(DataObject::getSchema()->tableName(BaseElement::class));
-
-        // Update both the draft and live versions of the records
-        $tableNames = [$baseTableName];
-        if (BaseElement::has_extension(Versioned::class)) {
-            /** @var BaseElement&Versioned $element */
-            $tableNames[] = $element->stageTable($baseTableName, Versioned::LIVE);
+        $tableName = sprintf('"%s"', $baseTableName);
+
+        if ($sortAfterPosition < $currentPosition) {
+            $operator = '+';
+            $filter = "$tableName.\"Sort\" > $sortAfterPosition AND $tableName.\"Sort\" < $currentPosition";
+            $newBlockPosition = $sortAfterPosition + 1;
+        } else {
+            $operator = '-';
+            $filter = "$tableName.\"Sort\" <= $sortAfterPosition AND $tableName.\"Sort\" > $currentPosition";
+            $newBlockPosition = $sortAfterPosition;
         }
 
-        foreach ($tableNames as $tableName) {
-            $tableName = sprintf('"%s"', $tableName);
-
-            if ($sortAfterPosition < $currentPosition) {
-                $operator = '+';
-                $filter = "$tableName.\"Sort\" > $sortAfterPosition AND $tableName.\"Sort\" < $currentPosition";
-                $newBlockPosition = $sortAfterPosition + 1;
-            } else {
-                $operator = '-';
-                $filter = "$tableName.\"Sort\" <= $sortAfterPosition AND $tableName.\"Sort\" > $currentPosition";
-                $newBlockPosition = $sortAfterPosition;
-            }
-
-            $query = SQLUpdate::create()
-                ->setTable("$tableName")
-                ->assignSQL('"Sort"', "$tableName.\"Sort\" $operator 1")
-                ->addWhere([$filter, "$tableName.\"ParentID\"" => $parentId]);
-
-            $query->execute();
-        }
+        $query = SQLUpdate::create()
+            ->setTable("$tableName")
+            ->assignSQL('"Sort"', "$tableName.\"Sort\" $operator 1")
+            ->addWhere([$filter, "$tableName.\"ParentID\"" => $parentId]);
+        $query->execute();
 
         // Now use the ORM to write a new version of the record that we are directly reordering
         $element->Sort = $newBlockPosition;
         $element->write();
 
         return $element;
     }
+
+    /**
+     * Force live sort order to match stage sort order
+     */
+    public function publishSortOrder()
+    {
+        $baseTableName = Convert::raw2sql(DataObject::getSchema()->tableName(BaseElement::class));
+        $live = Versioned::LIVE;
+        $sql = sprintf(
+            'UPDATE "%2$s"
+			SET "Sort" = (SELECT "%1$s"."Sort" FROM "%1$s" WHERE "%2$s"."ID" = "%1$s"."ID")
+			WHERE EXISTS (SELECT "%1$s"."Sort" FROM "%1$s" WHERE "%2$s"."ID" = "%1$s"."ID") AND "ParentID" = ?',
+            $baseTableName,
+            "{$baseTableName}_{$live}"
+        );
+        DB::prepared_query($sql, [$this->getElement()->ParentID]);
+    }
 }

tests/Services/ReorderElementsTest.php

@@ -2,9 +2,11 @@
 
 namespace DNADesign\Elemental\Tests\Services;
 
+use DNADesign\Elemental\Models\BaseElement;
 use DNADesign\Elemental\Services\ReorderElements;
 use DNADesign\Elemental\Tests\Src\TestElement;
 use SilverStripe\Dev\SapphireTest;
+use SilverStripe\Versioned\Versioned;
 
 class ReorderElementsTest extends SapphireTest
 {
@@ -19,28 +21,49 @@ class ReorderElementsTest extends SapphireTest
      */
     public function testReorder()
     {
+        foreach (BaseElement::get() as $toPublish) {
+            $toPublish->publishSingle();
+        }
         $element = $this->objFromFixture(TestElement::class, 'element1');
         $reorderService = new ReorderElements($element);
 
         $reorderService->reorder(4);
-        $this->assertIdsInOrder([2, 3, 4, 1], 'The Element should be last in the list');
+        $this->assertIdsInOrder([2, 3, 4, 1], [1, 2, 3, 4], 'The Element should be last in the list (draft only)');
+        $element->publishSingle();
+        $this->assertIdsInOrder([2, 3, 4, 1], [2, 3, 4, 1], 'The sort order should be published correctly');
 
         $reorderService->reorder(0);
-        $this->assertIdsInOrder([1, 2, 3, 4], 'The Element should be first in the list');
+        $this->assertIdsInOrder([1, 2, 3, 4], [2, 3, 4, 1], 'The Element should be first in the list (draft only)');
+        $element->publishSingle();
+        $this->assertIdsInOrder([1, 2, 3, 4], [1, 2, 3, 4], 'The sort order should be published correctly');
 
         $reorderService->reorder(2);
-        $this->assertIdsInOrder([2, 1, 3, 4], 'The Element should be second in the list');
+        $this->assertIdsInOrder([2, 1, 3, 4], [1, 2, 3, 4], 'The Element should be second in the list (draft only)');
+        $element->publishSingle();
+        $this->assertIdsInOrder([2, 1, 3, 4], [2, 1, 3, 4], 'The sort order should be published correctly');
 
         $reorderService->reorder();
-        $this->assertIdsInOrder([1, 2, 3, 4], 'The Element should be first in the list');
+        $this->assertIdsInOrder([1, 2, 3, 4], [2, 1, 3, 4], 'The Element should be first in the list (draft only)');
+        $element->publishSingle();
+        $this->assertIdsInOrder([1, 2, 3, 4], [1, 2, 3, 4], 'The sort order should be published correctly');
     }
 
-    protected function assertIdsInOrder($ids, $message = null)
+    protected function assertIdsInOrder($draftIds, $publishedIDs, $message = null)
     {
-        $actualIDs = TestElement::get()->sort('Sort')->column('ID');
+        // Check draft
+        Versioned::withVersionedMode(function () use ($draftIds, $message) {
+            Versioned::set_stage(Versioned::DRAFT);
+            $actualIDs = TestElement::get()->sort('Sort')->column('ID');
 
-        // Ideally this should be assertSame, but sometimes the database
-        // returns IDs as strings instead of integers (e.g. "1" instead of 1).
-        $this->assertEquals($ids, $actualIDs, $message);
+            // Ideally this should be assertSame, but sometimes the database
+            // returns IDs as strings instead of integers (e.g. "1" instead of 1).
+            $this->assertEquals($draftIds, $actualIDs, $message);
+        });
+        // Check published
+        Versioned::withVersionedMode(function () use ($publishedIDs, $message) {
+            Versioned::set_stage(Versioned::LIVE);
+            $actualIDs = TestElement::get()->sort('Sort')->column('ID');
+            $this->assertEquals($publishedIDs, $actualIDs, $message);
+        });
     }
 }