Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX #355 Sorting in paginated lists #418

Open
wants to merge 1 commit into
base: 4.0
Choose a base branch
from

Conversation

johannesx75
Copy link

@johannesx75 johannesx75 commented Sep 21, 2024

Description

If a many_many relation with the sort in an many_many_extraField has an item dragged to another page the resulting sort is wrong. This was caused by the handleMoveToPage function not respecting the original sort of the list.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli
Copy link
Collaborator

GuySartorelli commented Sep 24, 2024

Thank you for submitting this change.
Can you please retarget this to the 4.0 branch? Otherwise it'll need to wait until the April 2025 minor release.

You may need to reset your commits after retargetting the PR.

If a many_many relation with the sort in an many_many_extraField has an item draged to another page the resulting sort
is wrong. This was caused by the handleMoveToPage function not respecting the original sort of the list.
@johannesx75 johannesx75 force-pushed the fix-sort-pageinated-lists branch from 8e070a1 to fbf7b86 Compare September 25, 2024 08:07
@johannesx75 johannesx75 changed the base branch from 4 to 4.0 September 25, 2024 08:08
@johannesx75
Copy link
Author

Ok, I've changed it to the 4.0 branch.

@GuySartorelli
Copy link
Collaborator

Thanks for doing that.
I've just tested this PR and it worked fine. But I then tried to reproduce the original bug, and I can't. The reproduction steps there aren't very detailed about how to set up the relations or how to create the records.

Here's my class for the "taxonomy group" and the through class that the reproduction steps say I need:

use SilverStripe\ORM\DataObject;

class TaxonomyGroup extends DataObject
{
    private static $many_many = [
        'Terms' => [
            'through' => TermThrough::class,
            'from' => 'Group',
            'to' => 'Term',
        ],
    ];
}
use SilverStripe\ORM\DataObject;
use SilverStripe\Taxonomy\TaxonomyTerm;

class TermThrough extends DataObject
{
    private static $db = [
        'Sort' => 'Int',
    ];

    private static $has_one = [
        'Group' => TaxonomyGroup::class,
        'Term' => TaxonomyTerm::class,
    ];
}

And here's how I generated the records:

use SilverStripe\Taxonomy\TaxonomyTerm;

if (TaxonomyGroup::get()->count() === 0) {
    $group = TaxonomyGroup::create();
    $group->write();

    for ($i = 0; $i < 200; $i++) {
        $term = TaxonomyTerm::create(['Name' => $i, 'Sort' => $i]);
        $term->write();
        $group->Terms()->add($term);
    }
}

With that setup I can't actually reproduce the original bug (with or without the PR).
Can you or @mfendeksilverstripe please let me know what I need to change to reproduce the original bug?

@mfendeksilverstripe can you please test this PR and check it resolves the problem for you in CMS 5.2+?

@mfendeksilverstripe
Copy link
Contributor

Hey @GuySartorelli I did test this change but it doesn't seem to be fixing the issue. I suspect that we might have two scenarios here to test: many_many_extra_field with sort and many_many_through with sort. I tested the latter.

This is my local test setup:

  • parent model < many many through with sort > child model
  • add GridField with the many many relation from parent model to child model
  • make sure you have plenty of child models so you need pagination with multiple pages
  • make sure that you have orderable rows component available in the GridField
  • then scroll to the bottom of the page 1 and click the last item orderable component widget and drag it to the next page icon on the paginator (it should highlight itself as you hover over it), finally drop the item which should trigger reorder action

For me it reorders different items compared to what I selected. For example, I attempted to move item 15 to the next page and instead it moved item 3 to the last page.

If your still having trouble with the setup please let me know and I can assist you further as we can use one of my projects as a testing ground.

@GuySartorelli
Copy link
Collaborator

@johannesx75 Sounds like there's still some work to don on this PR, please amend the PR as per the comment above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants