Pagination with 1 item per page breaks breaks when using prev/next

lekoala commented 4 months ago

Module version(s) affected

5.x

Description

Stumbled upon this recently when doing some tests

When having a nested gridfield in a model admin, if you have setItemsPerPage = 1 for the nested grid field, when using the next arrow, it stops working after three clicks, as if there was no record anymore

Eg: i have 9 pages

It stops on record number 3

It works just fine with 2 items per page Refreshing the page doesn't help I guess something is wrong with the +1/-1 offset logic

How to reproduce

With a dataobject with a many many relation to another object
Using modeladmin navigate to the dataobject, seeing the gridfield with the many many relation that contains 3+ object
Given that the pagination is 1 item per page (see snippet below)
Click on the first record in the grid, using the arrow navigation in the bottom right corner. At the third record, the next becomes disabled even if there are more records

 public function getCMSFields()
    {
        $fields = parent::getCMSFields();

        $ManyRelation = $fields->dataFieldByName('ManyRelation');
        if ($ManyRelation) {
            $config = $ManyRelation->getConfig();
            $paginator = $config->getComponentByType(GridFieldPaginator::class);
            $paginator->setItemsPerPage(2);
        }

        return $fields;
    }

Possible Solution

No response

Additional Context

No response

Validations

[X] Check that there isn't already an issue that reports the same bug
[X] Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

GuySartorelli commented 4 months ago

Just to clarify, when you say "prev/next", is that the "previous page" and "next page" buttons in the paginator when you're viewing the GridField as a whole? Or is that the "previous record" and "next record" buttons when accessing the edit form for a record managed by the GridField?

GuySartorelli commented 4 months ago

Also your "how to reproduce" is just some code without context. Please give some steps along with the code, so it's easy to reproduce by just following some steps. Especially double check the code since you mention 1 item per page, but the code shows 2.

lekoala commented 4 months ago

Updated the how to reproduce. It's when using the detail form using the prev/next arrow next to the delete button

emteknetnz commented 4 months ago

I cannot replicate on 5.2.x-dev or 5.x-dev

Please let me know exact version and exact code used

Here's the code I failed to replicate with:

// app/src/MyModelAdmin.php

<?php

use SilverStripe\Admin\ModelAdmin;

class MyModelAdmin extends ModelAdmin
{
    private static $url_segment = 'MyModelAdmin';

    private static $menu_title = 'My model admin';

    private static $managed_models = [
        MyDataObject::class,
    ];
}

// app/src/MyDataObject.php

<?php

use SilverStripe\Forms\GridField\GridField;
use SilverStripe\Forms\GridField\GridFieldConfig_RecordEditor;
use SilverStripe\ORM\DataObject;
use SilverStripe\Forms\GridField\GridFieldPaginator;

class MyDataObject extends DataObject
{
    private static $table_name = 'MyDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $many_many = [
        'MySubDataObjects' => MySubDataObject::class
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $gridField = new GridField('MySubDataObjects', 'MySubDataObjecs', $this->MySubDataObjects());
        $config = GridFieldConfig_RecordEditor::create();
        $gridField->setConfig($config);
        $paginator = $config->getComponentByType(GridFieldPaginator::class);
        $paginator->setItemsPerPage(1);
        $fields->insertAfter('Title', $gridField);
        return $fields;
    }
}

// app/src/MySubDataObject.php

<?php

use SilverStripe\ORM\DataObject;

class MySubDataObject extends DataObject
{
    private static $table_name = 'MySubDataObject';

    private static $db = [
        'Title' => 'Varchar'
    ];

    private static $belongs_many_many = [
        'MyDataObjects' => MyDataObject::class
    ];
}

lekoala commented 4 months ago

here is a video maybe it's better:

https://github.com/silverstripe/silverstripe-framework/assets/250762/6868089c-8aca-4147-9710-f18fd0682b3e

it's on silverstripe/framework 5.2.6 using chrome/windows

emteknetnz commented 4 months ago

OK thanks that video helps a lot

I've done some debugging, looks like in the GridFieldDetailForm_ItemRequest the $this->gridField is the top level DataObject. Rather than the one being edited. This means that "gridstate" which is ?gridState-MySubDataObjects-1=%7B%22GridFieldPaginator%22%3A%7B%22currentPage%22%3A5%7D%7D thing in the URL will not match because it's using the wrong key, in the case of the code sample I had on this issue it $gridField->name should be 'MySubDataObjects' but it's mistakenly 'MyDataObject' instead

The full url of my request is /admin/MyModelAdmin/MyDataObject/EditForm/field/MyDataObject/item/1/ItemEditForm/field/MySubDataObjects/item/5?gridState-MySubDataObjects-1=%7B%22GridFieldPaginator%22%3A%7B%22currentPage%22%3A5%7D%7D

lekoala commented 4 months ago

yes, so while my issue (who would be using 1 item per page :) ) is pretty minor, i'm sure it leads to a larger issue that could cause tricky bugs

emteknetnz commented 4 months ago

Apparently we might look at a new way of managing the gridstate, so in the mean time I might just park this one for a wee while since it seems like it's very hard to fix

silverstripe / silverstripe-framework