Bug 28764

Summary: Sorting not correct in pagination on OPAC lists
Product: Koha Reporter: Nick Clemens <nick>
Component: OPACAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, kyle, m.de.rooy, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03,20.11.09
Attachments: Bug 28764: Parse direction parameter in public list view
Bug 28764: Parse direction parameter in public list view
Bug 28764: Alter sort dropdown to use direction parameter
Bug 28764: Parse direction parameter in public list view
Bug 28764: Alter sort dropdown to use direction parameter
Bug 28764: (QA follow-up) Add comment, remove two defined's

Description Nick Clemens 2021-07-27 11:23:49 UTC
When viewing a list on the OPAC you can force a sorting direction by appending to the sortfield with a colon:
sortfield=dateadded:desc

When clicking on the next page, however, the direction is formed as:
direction=desc&sortfield=dateadded

The script doesn't read the 'direction' parameter, so sorting fails

To recreate:
1 - Create a public list
2 - Add enough items to generate two pages (40 worked for me)
3 - View the list and enforce sorting by date added descending:
    sortfield=dateadded:desc
4 - Note the titles shown
5 - Click to second page using the pagination bar
6 - Note the same titles are shown, in a reversed order
Comment 1 Nick Clemens 2021-07-27 11:31:23 UTC
Created attachment 123205 [details] [review]
Bug 28764: Parse direction parameter in public list view

This patch adds a check for a 'direction' paramter in list sorting.

It maintains the existing syntax for sorting 'sortfield=field:direction'
but adds support for the form used in paginations and more commonly thtoughout
Koha' sortfield=field&direction=direction'

To test:
1 - Create a public list
2 - Add enough items to generate two pages (40 worked for me)
3 - View the list and enforce sorting by date added descending:
    sortfield=dateadded:desc
4 - Note the titles shown
5 - Click to second page using the pagination bar
6 - Note the same titles are shown, in a reversed order
7 - Apply patch
8 - repeat 305
9 - Second page is sorted correctly!
Comment 2 Owen Leonard 2021-07-27 11:49:17 UTC
Created attachment 123207 [details] [review]
Bug 28764: Parse direction parameter in public list view

This patch adds a check for a 'direction' paramter in list sorting.

It maintains the existing syntax for sorting 'sortfield=field:direction'
but adds support for the form used in paginations and more commonly thtoughout
Koha' sortfield=field&direction=direction'

To test:
1 - Create a public list
2 - Add enough items to generate two pages (40 worked for me)
3 - View the list and enforce sorting by date added descending:
    sortfield=dateadded:desc
4 - Note the titles shown
5 - Click to second page using the pagination bar
6 - Note the same titles are shown, in a reversed order
7 - Apply patch
8 - repeat 305
9 - Second page is sorted correctly!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Marcel de Rooy 2021-07-30 09:46:06 UTC
This is a bit confusing. We overrule the sortfield parameter which is still used in Koha by direction.

When I am in Your Lists, I click Sort and get a URL with sortfield=title:desc

If direction is the way to go, please be more thorough.
Comment 4 Nick Clemens 2021-07-30 10:57:33 UTC
Created attachment 123297 [details] [review]
Bug 28764: Alter sort dropdown to use direction parameter

This code is odd -we build a select in the template, then turn it into a list of links using javascript
This patch adds a direction data element and uses it while building the links
Comment 5 Nick Clemens 2021-07-30 10:59:26 UTC
(In reply to Marcel de Rooy from comment #3)
> This is a bit confusing. We overrule the sortfield parameter which is still
> used in Koha by direction.
> 
> When I am in Your Lists, I click Sort and get a URL with sortfield=title:desc
> 
> If direction is the way to go, please be more thorough.

I think the follow up is something like you wanted?

We could remove the old style all together, but I felt libraries could have existing links that we should not break.

Rewriting the code here to build the dropdown sorting list more straightforward is beyond scope I think.
Comment 6 Marcel de Rooy 2021-08-09 13:54:09 UTC
Created attachment 123650 [details] [review]
Bug 28764: Parse direction parameter in public list view

This patch adds a check for a 'direction' paramter in list sorting.

It maintains the existing syntax for sorting 'sortfield=field:direction'
but adds support for the form used in paginations and more commonly thtoughout
Koha' sortfield=field&direction=direction'

To test:
1 - Create a public list
2 - Add enough items to generate two pages (40 worked for me)
3 - View the list and enforce sorting by date added descending:
    sortfield=dateadded:desc
4 - Note the titles shown
5 - Click to second page using the pagination bar
6 - Note the same titles are shown, in a reversed order
7 - Apply patch
8 - repeat 305
9 - Second page is sorted correctly!

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2021-08-09 13:54:12 UTC
Created attachment 123651 [details] [review]
Bug 28764: Alter sort dropdown to use direction parameter

This code is odd -we build a select in the template, then turn it into a list of links using javascript
This patch adds a direction data element and uses it while building the links

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2021-08-09 13:54:16 UTC
Created attachment 123652 [details] [review]
Bug 28764: (QA follow-up) Add comment, remove two defined's

If we no longer offer the sortfield with :asc actively, we should
probably write it somewhere too.
Replaced a few defined tests by empty test. No need to split empty
string or zero etc.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Jonathan Druart 2021-08-11 11:24:05 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 10 Kyle M Hall 2021-08-13 11:52:05 UTC
Pushed to 21.05.x for 21.05.03
Comment 11 Fridolin Somers 2021-08-13 20:16:58 UTC
Pushed to 20.11.x for 20.11.09
Comment 12 Victor Grousset/tuxayo 2021-08-16 21:09:57 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.