Query errors when sorted field is on a relation and appending to top

[patricknelson]

Hi, I found that issue #35 cited this issue as well (specifically in #35 (comment)). I've deduced that his issue is the same as mine with the exception that I'm appending to the top on a has_many relation. So the conditions to reproduce this bug are:

• Have a base data object, e.g. BaseObject
• Descendant of that base object, e.g. ChildObject
• Have a has_many relation from another object (e.g. ParentObject) which points to ChildObject thus having a foreign key e.g. ParentID living on ChildObject
• Have a sortable field (e.g. Sort) on that descendant ChildObject
• Ensure you call ->setAppendToTop(true) on this component (GridFieldSortableRows).

It looks like in a44c66d you were ensuring to directly reference the ID of the affected DataObject's along with the separate tables. However, this line (linked below) is attempting to update the LastEdited column on the correct root table but incorrectly assumes the foreign key also exists on that root table:

https://github.com/UndefinedOffset/SortableGridField/blob/master/code/forms/GridFieldSortableRows.php#L284

Fix: A possible fix here may instead to get the ID's of all the affected DataObject's in that relation and then either apply the UPDATE in aggregate first on the sort column and then separately update each LastEdited based on the ID's originally retrieved in that first SELECT. I only say this instead of using a JOIN statement since that could get more complex (albeit a superior solution).

[UndefinedOffset]

Good find, I wonder if you could try applying this diff locally and see if it helps with your problem. I think it should.

 code/forms/GridFieldSortableRows.php | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/code/forms/GridFieldSortableRows.php b/code/forms/GridFieldSortableRows.php
index b1652e4..71aabed 100644
--- a/code/forms/GridFieldSortableRows.php
+++ b/code/forms/GridFieldSortableRows.php
@@ -122,7 +122,6 @@ class GridFieldSortableRows implements GridField_HTMLProvider, GridField_ActionP
 			return $dataList->sort($headerState->SortColumn, $headerState->SortDirection);
 		}
 		
- //var_dump($state->sortableToggle);exit;
 		if ($state->sortableToggle === true) {
 			$gridField->getConfig()->removeComponentsByType('GridFieldFilterHeader');
 			$gridField->getConfig()->removeComponentsByType('GridFieldSortableHeader');
@@ -280,21 +279,10 @@ class GridFieldSortableRows implements GridField_HTMLProvider, GridField_ActionP
 							. '" SET "' . $sortColumn . '" = "' . $sortColumn .'"+1'
 							. ' WHERE '.($list instanceof RelationList ? '"' . $list->foreignKey . '" = '. $owner->ID:$idCondition) . (!empty($topIncremented) ? ' AND "ID" NOT IN(\''.implode('\',\'', $topIncremented).'\')':''));
 					
-					//LastEdited
-					DB::query('UPDATE "' . $baseDataClass
-							. '" SET "LastEdited" = \'' . date('Y-m-d H:i:s') . '\''
-							. ' WHERE '.($list instanceof RelationList ? '"' . $list->foreignKey . '" = '. $owner->ID:$idCondition) . (!empty($topIncremented) ? ' AND "ID" NOT IN(\''.implode('\',\'', $topIncremented).'\')':''));
-					
 					if($this->update_versioned_stage && class_exists($table) && Object::has_extension($table, 'Versioned')) {
 						DB::query('UPDATE "' . $table . '_' . $this->update_versioned_stage
 								. '" SET "' . $sortColumn . '" = "' . $sortColumn .'"+1'
 								. ' WHERE '. ($list instanceof RelationList ? '"' . $list->foreignKey . '" = '. $owner->ID:$idCondition) . (!empty($topIncremented) ? ' AND "ID" NOT IN(\''.implode('\',\'', $topIncremented).'\')':''));
-						
-						if(Object::has_extension($baseDataClass, 'Versioned')) {
-							DB::query('UPDATE "' . $baseDataClass . '_' . $this->update_versioned_stage
-									. '" SET "LastEdited" = \'' . date('Y-m-d H:i:s') . '\''
-									. ' WHERE ' . ($list instanceof RelationList ? '"' . $list->foreignKey . '" = '. $owner->ID:$idCondition) . (!empty($topIncremented) ? ' AND "ID" NOT IN(\''.implode('\',\'', $topIncremented).'\')':''));
-						}
 					}
 					
 					$topIncremented[]=$obj->ID;
@@ -324,6 +312,19 @@ class GridFieldSortableRows implements GridField_HTMLProvider, GridField_ActionP
 				$i++;
 			}
 			
+			//Update LastEdited for affected records when using append to top on a many_many relationship
+			if(!$many_many && $this->append_to_top) {
+				DB::query('UPDATE "' . $baseDataClass
+						. '" SET "LastEdited" = \'' . date('Y-m-d H:i:s') . '\''
+						. ' WHERE "ID" IN(\''.implode('\',\'', $topIncremented).'\')');
+				
+				if($this->update_versioned_stage && class_exists($table) && Object::has_extension($table, 'Versioned') && Object::has_extension($baseDataClass, 'Versioned')) {
+					DB::query('UPDATE "' . $baseDataClass . '_' . $this->update_versioned_stage
+							. '" SET "LastEdited" = \'' . date('Y-m-d H:i:s') . '\''
+							. ' WHERE "ID" IN(\''.implode('\',\'', $topIncremented).'\')');
+				}
+			}
+			
 			
 			//End transaction if supported
 			if(DB::getConn()->supportsTransactions()) {

[patricknelson]

Thanks so much @UndefinedOffset! That works perfectly in my edge case. Note that I was able to workaround this myself by just ensuring that I never ->setAppendToTop() when handling a has_many, since I just stumbled upon it in my own edge case testing.

I can submit a PR for this using your code since I'd be happy to roll this out as soon as it's available to composer.

[UndefinedOffset]

Nice, I actually have a commit with tests for this case ready to go... But it's on my work machine and I forgot to push it before I left lol. I'll push it and tag a release in the am.

[patricknelson]

Ok -- what I would like to do instead is maybe submit a separate PR relating to code formatting (since I was going to incorporate this + the formatting tweaks into the same PR).

[UndefinedOffset]

Sure, probably will be easier after I push the changes tomorrow. That would probably make merging much easier.

[patricknelson]

👍 Thanks!