Make the previous and next buttons always work #9827
Conversation
bergice
changed the title
bergice
force-pushed
the
pulls/4.7/fix-better-button-pagination
branch
2 times, most recently
from
6efef0d
to
1e8d7b7
Compare
bergice
force-pushed
the
pulls/4.7/fix-better-button-pagination
branch
from
1e8d7b7
to
a66e441
Compare
bergice
changed the title
Spent some time trying to get a unit test done but it's quite tricky because of the convoluted state logic and pagination that's required. A behat test would be more ideal but we don't have anything suited in I'd prefer just getting this merged as we're a lot better off with it than without, unless someone else wants to give it a go first. |
|
I don't feel confident merging this as is, or at least I don't feel confident that it won't break again sometime in the future. We've assigned 3 story points on the parent issue so we've got some time to fix this. Seems like we could add a corresponding behat test in admin to cover this functionality |
| ->sort(['ID' => 'ASC']); | ||
| } else { | ||
| $list = $list->where(["\"$table\".\"ID\" <= ?" => $this->record->ID]) | ||
| ->sort(['ID' => 'DESC']); |
There was a problem hiding this comment.
Doesn’t this change the sorting completely? Any sort order applied to the gridfield (or a default_sort) will be ignored, and the records will be sorted by ID. Is that intended?
There was a problem hiding this comment.
Doesn’t this change the sorting completely? Any sort order applied to the gridfield (or a
default_sort) will be ignored, and the records will be sorted by ID. Is that intended?
Hmm, I'm pretty sure $list->getManipulatedList() already applies all the filters.
There was a problem hiding this comment.
This approach seems like it won't fix the root cause of the issue
I've done some investigation into part of the issue
This pull-request just filtering and sorting $list, however $list is fundamentally incomplete, which is why the next button can only be clicked a couple of times. I did not investigate the other issue identified of results being in the incorrect order. There I'd say it's likely the $list is simply incorrect
A proper solution would be to get a correct $list
If you're able to update the PR to get a correct $list that would still work in a 100K database records scenario, then great, otherwise recommend this one be closed
|
Closing, while the filtering is still working as expected, sorting is broken and will need to be fixed. Can reopen later if we want. |
Resolves silverstripe/silverstripe-admin#886