GridField should respect pagination and filtering on save

[chillu]

In many cases, you're either editing a record, or you're paging through related data on the same editing view. For example, on a page with a "related documents" GridField, you're either editing page content, or editing details on a related document (in the GridField detail form).

But: In some cases those two activities overlap, particularly when the GridFields have inline editable fields which are saved "through" the same record. In this case, it's important that the GridField view you're currently looking at is retained, incl. sorting, filtering and pagination. It doesn't inspire confidence in a system if the editable row you've just entered and saved just disappears because it happens to be on the second page.

You could call this an issue on the module providing GridFieldEditableColumns, but I think this should work for all GridField views.

This kind of state management complexity makes me want to rewrite the whole edit view as a client-side app where you just transfer data rather than view content, but that's ... slight scope creep.

An alternative way to solve this is by allowing per-row saving.

Related issues:

• Navigating back from a record in a GridField doesn't respect selected pagination or filters #9380
• [GRIDFIELD] GridFieldPaginator must be applied after GridFieldFilterHeader to work correctly. #8454
• [2012-07-10] Retain pages list view state (pagination, filters) when returning from detail view silverstripe-cms#503

Acceptance criteria

• When you save a record within the Gridfield, the state is remembered. e.g.
• You can use the pagination to go to the next record.
• If it's trivial to back port this to CMS4 and it doesn't require new API, fix it in CMS4

Notes

• There's a potential edge case where you are filtering or sorting on a specific value. Saving the record changes its order on the list or exclude from the results sets. We can live with this edge case, as long as you are still viewing the same record after your save.
• Related issue CMS 4.12: CSV export sometimes produces unexpected results silverstripe-admin#1454

[brynwhyman]

Another semi-related issue: Gridfield Better Buttons don't handle sorting and pagination

[sminnee]

FYI I've suggested building on #9190 to fix this by getting grid field state pushed into the URL via the history API, and have it be managed by GridField itself rather than selected components.

[purplespider]

With the latest improvements to GridField state in 4.12.0 this issue now sticks out like a sore thumb. As if you click the back arrow from a record, the pagination/filter is remembered, but if you save that record and then click the same back arrow the pagination/search is reset.

Presume this is on your radar @GuySartorelli 😊

[GuySartorelli]

It is now.

[lekoala]

I've been digging recently also in the grid state issues for multiple reasons (because my cms-actions module breaks the state, which is not great, but also because i'm building an alternative to the gridfield using tabulator)

I'm not sure what is the reasoning behind passing the state in post/get, i think this is really dangerous on multiple levels.
Maybe i'm missing something here, but for my part, I went for a much more simple solution for my tabulator grid module using plain sessions. I basically store everything in the session in a scoped key per controller on each load. As soon as I change from controller, I clear non relevant entries to avoid having a super large session store. The nice part is that it keeps everything in memory, even when navigating prev/next records, you can toggle between records and keep grid state for each record.
:-) just throwing the idea here, maybe it helps!

[xini]

Any progress on this? I'm happy to try to dog into this if someone can give me some pointers?

[sb-relaxt-at]

From my understanding, this line is wrong as it results in always adding an empty state to the form submission:

silverstripe-framework/src/Forms/GridField/GridFieldDetailForm_ItemRequest.php

Line 418 in ca542a3

$actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));

GridState itself is already a HiddenField, so simply adding the state solves the problem, at least partially:

$actions->push($gridState);

Only partially as this will only save the state of the current GridField, but none of the other (parent) GridFields. I am not sure how to best solve this. From my testing it seems to be most feasible to remove the hidden field at all and modify the form's action to include all current GridField states as query parameters. This seems to work pretty well:

@@ -291,6 +291,9 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
             $cb($form, $this);
         }
         $this->extend("updateItemEditForm", $form);
+
+        $form->setFormAction($this->getGridField()->addAllStateToUrl($form->FormAction()));
+
         return $form;
     }
 
@@ -415,7 +418,6 @@ class GridFieldDetailForm_ItemRequest extends RequestHandler
             }
 
             $gridState = $this->gridField->getState(false);
-            $actions->push(HiddenField::create($manager->getStateKey($this->gridField), null, $gridState));
 
             $actions->push($this->getRightGroupField());
         } else { // adding new record

Only "pretty well" as there is a different handling in escaping spaces (I assume depending on the kind of http method). Restoring the a grid filter may result in a wrong unescaping, changing "john doe" to "john+doe". Edit: One could just base64 encode/decode the state inGridFieldStateManager. This bypasses the problem, just in case someone needs a quick solution.

Edit2: P.S.: There is at least one issue with appending the state as query params to the form action in TagField due to a small bug in the JavaScript, see silverstripe/silverstripe-tagfield#246

[xini]

Two dumb questions:

Does PHP need to know grid state at all? Or it this all handled in react anyway? If we only need to know state in the front-end, how about storing it in e.g. local storage? If we need it in the backend, then the hidden field is a good solution, as long as the current state doesn't include any parent state data (see next question).

I thought grid state was saved in the session using a unique key per gridfield, e.g. 'gf_xyz'? If that's the case parent gridfield state shouldn't be affected by pushing the state of the current field, right?

The query string is not a good solution as its length is restricted to 1024 characters afaik.

[GuySartorelli]

Does PHP need to know grid state at all? Or it this all handled in react anyway?

The GridField is only partially implemented in react (it notably does not work in a pure react form) - and the navigation within a gridfield results in a fresh react component (local state and all) being completely reinstated. React does not and currently can not handle a gridfield state.

If we only need to know state in the front-end, how about storing it in e.g. local storage?

There are many parts of gridfield functionality that are operated on in a per-instance basis, such as pagination, which necessarily involved PHP having a full context of the gridfield's current state.

I thought grid state was saved in the session using a unique key per gridfield, e.g. 'gf_xyz'? If that's the case parent gridfield state shouldn't be affected by pushing the state of the current field, right?

There's a weird double state thing that gridfield has going on, because of course there is. For backwards compatability with the portion that was already stored in the URL, we ended up putting it all in the URL. In a future major release it will hopefully swap to being all in the session or stored in some other less obtrusive and less restrictive way (assuming we don't have some new fancy pure-front-end system by then, which is unlikely).

[sb-relaxt-at]

The query string is not a good solution as its length is restricted to 1024 characters afaik.

There is no theoretical limit to the URL or the query parameter in the standard (see RFC 2616/3.1.2) but some practical limits in browsers and webservers. In former times Internet Explorer (what else) forces a limit of 2083 characters (see in RedirectorPage).

I do agree that the query param is not a perfect solution. When using a different solution there a several aspects to consider from my point of view:

• It should be possible to use multiple windows of the same application with different grid field states (filtering one model admin by "x" and another one by "y"). Therefore local storage is not an option. I see a similar issue when using the session, we would need to namespace this per tab/window somehow.
• Using session storage can solve this issue, it needs to be considered whether opening an new tab/window should retain the current grid field state (which I would accept from a user's perspective).