Merge pull request #272 from creative-commoners/pulls/3.2/correct-sort-orders-for-mmtl

FIX Orderable rows now respects actual MMTL sort orders instead of incrementing from SiteTree

.travis.yml

@@ -9,9 +9,11 @@ matrix:
     - php: 5.6
       env: DB=MYSQL RECIPE_VERSION=1.0.x-dev PHPCS_TEST=1 PHPUNIT_TEST=1
     - php: 7.0
-      env: DB=PGSQL RECIPE_VERSION=1.1.x-dev PHPUNIT_TEST=1
+      env: DB=MYSQL RECIPE_VERSION=1.1.x-dev PHPUNIT_TEST=1
     - php: 7.1
-      env: DB=MYSQL RECIPE_VERSION=4.2.x-dev PHPUNIT_COVERAGE_TEST=1
+      env: DB=PGSQL RECIPE_VERSION=4.2.x-dev PHPUNIT_COVERAGE_TEST=1
+    - php: 7.2
+      env: DB=MYSQL RECIPE_VERSION=4.3.x-dev PHPUNIT_TEST=1
     - php: 7.2
       env: DB=MYSQL RECIPE_VERSION=4.x-dev PHPUNIT_TEST=1
 
@@ -21,7 +23,7 @@ before_script:
 
   - composer validate
   - composer require silverstripe/recipe-core "$RECIPE_VERSION"  --no-update
-  - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.0.x-dev --no-update; fi
+  - if [[ $DB == PGSQL ]]; then composer require silverstripe/postgresql:2.1.x-dev --no-update; fi
   - composer install --prefer-dist --no-interaction --no-progress --no-suggest --optimize-autoloader --verbose --profile
 
 script:

src/GridFieldOrderableRows.php

@@ -4,6 +4,8 @@ namespace Symbiote\GridFieldExtensions;
 
 use Exception;
 use SilverStripe\Control\Controller;
+use SilverStripe\Control\HTTPRequest;
+use SilverStripe\Control\HTTPResponse_Exception;
 use SilverStripe\Control\RequestHandler;
 use SilverStripe\Core\ClassInfo;
 use SilverStripe\Forms\GridField\GridField;
@@ -20,11 +22,11 @@ use SilverStripe\ORM\DataObject;
 use SilverStripe\ORM\DataObjectInterface;
 use SilverStripe\ORM\DataObjectSchema;
 use SilverStripe\ORM\DB;
+use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\ORM\ManyManyList;
 use SilverStripe\ORM\ManyManyThroughList;
 use SilverStripe\ORM\ManyManyThroughQueryManipulator;
 use SilverStripe\ORM\SS_List;
-use SilverStripe\ORM\FieldType\DBDatetime;
 use SilverStripe\Versioned\Versioned;
 use SilverStripe\View\ViewableData;
 
@@ -295,7 +297,22 @@ class GridFieldOrderableRows extends RequestHandler implements
             $record->ID,
             $this->getSortField()
         );
-        $sortField = new HiddenField($sortFieldName, false, $record->getField($this->getSortField()));
+
+        // Default: Get the sort field directly from the current record
+        $currentSortValue = $record->getField($this->getSortField());
+
+        $list = $grid->getList();
+        if ($list instanceof ManyManyThroughList) {
+            // In a many many through list we should get the current sort order from the relationship
+            // if it exists, not directly from the record
+            $throughListSorts = $this->getSortValuesFromManyManyThroughList($list, $this->getSortField());
+
+            if (array_key_exists($record->ID, $throughListSorts)) {
+                $currentSortValue = $throughListSorts[$record->ID];
+            }
+        }
+
+        $sortField = HiddenField::create($sortFieldName, false, $currentSortValue);
         $sortField->addExtraClass('ss-orderable-hidden-sort');
         $sortField->setForm($grid->getForm());
 
@@ -345,17 +362,18 @@ class GridFieldOrderableRows extends RequestHandler implements
                 $sortterm .= '"'.$this->getSortTable($list).'"."'.$this->getSortField().'"';
             }
             return $list->sort($sortterm);
-        } else {
-            return $list;
         }
+
+        return $list;
     }
 
     /**
      * Handles requests to reorder a set of IDs in a specific order.
      *
      * @param GridField $grid
-     * @param SS_HTTPRequest $request
+     * @param HTTPRequest $request
-     * @return SS_HTTPResponse
+     * @return string
+     * @throws HTTPResponse_Exception
      */
     public function handleReorder($grid, $request)
     {
@@ -536,16 +554,7 @@ class GridFieldOrderableRows extends RequestHandler implements
                 }
             }
         } elseif ($items instanceof ManyManyThroughList) {
-            $manipulator = $this->getManyManyInspector($list);
+            $current = $this->getSortValuesFromManyManyThroughList($list, $sortField);
-            $joinClass = $manipulator->getJoinClass();
-            $fromRelationName = $manipulator->getForeignKey();
-            $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();
         } else {
             $current = $items->map('ID', $sortField)->toArray();
         }
@@ -759,4 +768,30 @@ class GridFieldOrderableRows extends RequestHandler implements
         }
         return $inspector;
     }
+
+    /**
+     * Used to get sort orders from a many many through list relationship record, rather than the current
+     * record itself.
+     *
+     * @param ManyManyList|ManyManyThroughList $list
+     * @return int[] Sort orders for the
+     */
+    protected function getSortValuesFromManyManyThroughList($list, $sortField)
+    {
+        $manipulator = $this->getManyManyInspector($list);
+
+        // Find the foreign key name, ID and class to look up
+        $joinClass = $manipulator->getJoinClass();
+        $fromRelationName = $manipulator->getForeignKey();
+        $toRelationName = $manipulator->getLocalKey();
+
+        // Create a list of the MMTL relations
+        $sortlist = DataList::create($joinClass)->filter([
+            $toRelationName => $list->column('ID'),
+            // first() is safe as there are earlier checks to ensure our list to sort is valid
+            $fromRelationName => $list->first()->getJoin()->$fromRelationName,
+        ]);
+
+        return $sortlist->map($toRelationName, $sortField)->toArray();
+    }
 }

tests/GridFieldOrderableRowsTest.php

@@ -88,6 +88,40 @@ class GridFieldOrderableRowsTest extends SapphireTest
         $this->assertEquals($desiredOrder, $newOrder);
     }
 
+    public function testManyManyThroughListSortOrdersAreUsedForInitialRender()
+    {
+        /** @var ThroughDefiner $record */
+        $record = $this->objFromFixture(ThroughDefiner::class, 'DefinerOne');
+
+        $orderable = new GridFieldOrderableRows('Sort');
+        $config = new GridFieldConfig_RelationEditor();
+        $config->addComponent($orderable);
+
+        $grid = new GridField(
+            'Belongings',
+            'Testing Many Many',
+            $record->Belongings()->sort('Sort'),
+            $config
+        );
+
+        // Get the first record, which would be the first one to have column contents generated
+        /** @var ThroughIntermediary $expected */
+        $intermediary = $this->objFromFixture(ThroughIntermediary::class, 'One');
+
+        $result = $orderable->getColumnContent($grid, $record, 'irrelevant');
+
+        $this->assertContains(
+            'Belongings[GridFieldEditableColumns][' . $record->ID . '][Sort]',
+            $result,
+            'The field name is indexed under the record\'s ID'
+        );
+        $this->assertContains(
+            'value="' . $intermediary->Sort . '"',
+            $result,
+            'The value comes from the MMTL intermediary Sort value'
+        );
+    }
+
     public function testSortableChildClass()
     {
         $orderable = new GridFieldOrderableRows('Sort');

tests/OrderableRowsThroughVersionedTest.php

@@ -12,7 +12,7 @@ use Symbiote\GridFieldExtensions\Tests\Stub\ThroughIntermediary;
 use Symbiote\GridFieldExtensions\Tests\Stub\ThroughBelongs;
 use Symbiote\GridFieldExtensions\GridFieldOrderableRows;
 
-class OrderableRowsThroughTest extends SapphireTest
+class OrderableRowsThroughVersionedTest extends SapphireTest
 {
     protected static $fixture_file = 'OrderableRowsThroughTest.yml';
 
@@ -105,7 +105,7 @@ class OrderableRowsThroughTest extends SapphireTest
             }
         }
         $this->assertTrue($differenceFound, 'All records should have changes in draft');
-        
+
         // Verify live stage has NOT reordered
         Versioned::set_stage(Versioned::LIVE);
         $sameOrder = $parent->$relationName()->sort($sortName)->column('ID');

tests/Stub/ThroughIntermediary.php

@@ -2,18 +2,17 @@
 
 namespace Symbiote\GridFieldExtensions\Tests\Stub;
 
-use SilverStripe\ORM\DataObject;
-use SilverStripe\ORM\FieldType\DBInt;
 use SilverStripe\Dev\TestOnly;
+use SilverStripe\ORM\DataObject;
 
 class ThroughIntermediary extends DataObject implements TestOnly
 {
     private static $table_name = 'IntermediaryThrough';
 
     private static $db = [
-        'Sort' => DBInt::class,
+        'Sort' => 'Int',
     ];
-    
+
     private static $has_one = [
         'Defining' => ThroughDefiner::class,
         'Belonging' => ThroughBelongs::class,