Merge pull request #154 from dhensby/pulls/simple-reorder

Bug with reordering sortable grid when some sort IDs are duplicated

.travis.yml

@@ -1,25 +1,36 @@
-language: php
+# See https://github.com/silverstripe-labs/silverstripe-travis-support for setup details
 
 sudo: false
 
-php: 
+language: php
+
+php:
   - 5.3
+  - 5.4
+  - 5.5
+  - 5.6
 
 env:
-  - DB=MYSQL CORE_RELEASE=3.1
+  - DB=MYSQL CORE_RELEASE=3.2
 
 matrix:
   include:
-    - php: 5.5
+    - php: 5.6
       env: DB=MYSQL CORE_RELEASE=3
     - php: 5.6
-      env: DB=MYSQL CORE_RELEASE=3.2
-
+      env: DB=PGSQL CORE_RELEASE=3.1
+    - php: 5.6
+      env: DB=PGSQL CORE_RELEASE=3.3
+    - php: 5.6
+      env: DB=PGSQL CORE_RELEASE=3.4
+  fast_finish: true
+
 before_script:
-  - phpenv rehash
+  - composer self-update || true
   - git clone git://github.com/silverstripe-labs/silverstripe-travis-support.git ~/travis-support
   - php ~/travis-support/travis_setup.php --source `pwd` --target ~/builds/ss
   - cd ~/builds/ss
+  - composer install
 
 script:
-  - phpunit gridfieldextensions/tests
+  - vendor/bin/phpunit gridfieldextensions/tests

code/GridFieldOrderableRows.php

@@ -413,9 +413,13 @@ protected function executeReorder(GridField $grid, $ids) {
 	}
 
 	protected function reorderItems($list, array $values, array $order) {
-		// Get a list of sort values that can be used.
-		$pool = array_values($values);
-		sort($pool);
+		$sortField = $this->getSortField();
+		/** @var SS_List $map */
+		$map = $list->map('ID', $sortField);
+		//fix for versions of SS that return inconsistent types for `map` function
+		if ($map instanceof SS_Map) {
+			$map = $map->toArray();
+		}
 
 		// If not a ManyManyList and using versioning, detect it.
 		$isVersioned = false;
@@ -427,14 +431,15 @@ protected function reorderItems($list, array $values, array $order) {
 		// Loop through each item, and update the sort values which do not
 		// match to order the objects.
 		if (!$isVersioned) {
+			$sortTable = $this->getSortTable($list);
 			$additionalSQL = (!$list instanceof ManyManyList) ? ', "LastEdited" = NOW()' : '';
 			foreach(array_values($order) as $pos => $id) {
-				if($values[$id] != $pool[$pos]) {
+				if($map[$id] != $pos) {
 					DB::query(sprintf(
 						'UPDATE "%s" SET "%s" = %d%s WHERE %s',
-						$this->getSortTable($list),
-						$this->getSortField(),
-						$pool[$pos],
+						$sortTable,
+						$sortField,
+						$pos,
 						$additionalSQL,
 						$this->getSortTableClauseForIds($list, $id)
 					));
@@ -445,11 +450,10 @@ protected function reorderItems($list, array $values, array $order) {
 			// *_versions table is updated. This ensures re-ordering works
 			// similar to the SiteTree where you change the position, and then
 			// you go into the record and publish it.
-			$sortField = $this->getSortField();
 			foreach(array_values($order) as $pos => $id) {
-				if($values[$id] != $pool[$pos]) {
+				if($map[$id] != $pos) {
 					$record = $class::get()->byID($id);
-					$record->$sortField = $pool[$pos];
+					$record->$sortField = $pos;
 					$record->write();
 				}
 			}

tests/GridFieldAddNewMultiClassTest.php

@@ -41,7 +41,7 @@ public function testGetClasses() {
  * @ignore
  */
 
-class GridFieldAddNewMultiClassTest_A {
+class GridFieldAddNewMultiClassTest_A implements TestOnly {
 	public function i18n_singular_name() {
 		$class = get_class($this);
 		return substr($class, strpos($class, '_') + 1);
@@ -52,7 +52,7 @@ public function canCreate() {
 	}
 }
 
-class GridFieldAddNewMultiClassTest_B extends GridFieldAddNewMultiClassTest_A {}
-class GridFieldAddNewMultiClassTest_C extends GridFieldAddNewMultiClassTest_A {}
+class GridFieldAddNewMultiClassTest_B extends GridFieldAddNewMultiClassTest_A implements TestOnly {}
+class GridFieldAddNewMultiClassTest_C extends GridFieldAddNewMultiClassTest_A implements TestOnly {}
 
 /**#@-*/

tests/GridFieldOrderableRowsTest.php

@@ -6,6 +6,44 @@ class GridFieldOrderableRowsTest extends SapphireTest {
 
 	protected $usesDatabase = true;
 
+	protected static $fixture_file = 'GridFieldOrderableRowsTest.yml';
+
+	protected $extraDataObjects = array(
+		'GridFieldOrderableRowsTest_Parent',
+		'GridFieldOrderableRowsTest_Ordered',
+		'GridFieldOrderableRowsTest_Subclass',
+	);
+
+	public function testReorderItems() {
+		$orderable = new GridFieldOrderableRows('ManyManySort');
+		$reflection = new ReflectionMethod($orderable, 'executeReorder');
+		$reflection->setAccessible(true);
+
+		$parent = $this->objFromFixture('GridFieldOrderableRowsTest_Parent', 'parent');
+
+		$config = new GridFieldConfig_RelationEditor();
+		$config->addComponent($orderable);
+
+		$grid = new GridField(
+			'MyManyMany',
+			'My Many Many',
+			$parent->MyManyMany()->sort('ManyManySort'),
+			$config
+		);
+
+		$originalOrder = $parent->MyManyMany()->sort('ManyManySort')->column('ID');
+		$desiredOrder = array_reverse($originalOrder);
+
+		$this->assertNotEquals($originalOrder, $desiredOrder);
+
+		$reflection->invoke($orderable, $grid, $desiredOrder);
+
+		$newOrder = $parent->MyManyMany()->sort('ManyManySort')->column('ID');
+
+		$this->assertEquals($desiredOrder, $newOrder);
+
+	}
+
 	/**
 	 * @covers GridFieldOrderableRows::getSortTable
 	 */
@@ -42,7 +80,7 @@ public function testGetSortTable() {
  * @ignore
  */
 
-class GridFieldOrderableRowsTest_Parent extends DataObject {
+class GridFieldOrderableRowsTest_Parent extends DataObject implements TestOnly {
 
 	private static $has_many = array(
 		'MyHasMany' => 'GridFieldOrderableRowsTest_Ordered',
@@ -59,7 +97,7 @@ class GridFieldOrderableRowsTest_Parent extends DataObject {
 
 }
 
-class GridFieldOrderableRowsTest_Ordered extends DataObject {
+class GridFieldOrderableRowsTest_Ordered extends DataObject implements TestOnly {
 
 	private static $db = array(
 		'Sort' => 'Int'
@@ -69,9 +107,13 @@ class GridFieldOrderableRowsTest_Ordered extends DataObject {
 		'Parent' => 'GridFieldOrderableRowsTest_Parent'
 	);
 
+	private static $belongs_many_many =array(
+		'MyManyMany' => 'GridFieldOrderableRowsTest_Parent',
+	);
+
 }
 
-class GridFieldOrderableRowsTest_Subclass extends GridFieldOrderableRowsTest_Ordered {
+class GridFieldOrderableRowsTest_Subclass extends GridFieldOrderableRowsTest_Ordered implements TestOnly {
 }
 
 /**#@-*/

tests/GridFieldOrderableRowsTest.yml

@@ -0,0 +1,22 @@
+GridFieldOrderableRowsTest_Ordered:
+  item1:
+  item2:
+  item3:
+  item4:
+  item5:
+  item6:
+GridFieldOrderableRowsTest_Parent:
+  parent:
+    MyManyMany:
+      - 0: =>GridFieldOrderableRowsTest_Ordered.item1
+        ManyManySort: 1
+      - 1: =>GridFieldOrderableRowsTest_Ordered.item2
+        ManyManySort: 1
+      - 2: =>GridFieldOrderableRowsTest_Ordered.item3
+        ManyManySort: 2
+      - 3: =>GridFieldOrderableRowsTest_Ordered.item4
+        ManyManySort: 2
+      - 4: =>GridFieldOrderableRowsTest_Ordered.item5
+        ManyManySort: 108
+      - 5: =>GridFieldOrderableRowsTest_Ordered.item6
+        ManyManySort: 108