Bug 9458 - Add sorting to lists
Summary: Add sorting to lists
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 9661
  Show dependency treegraph
 
Reported: 2013-01-23 14:06 UTC by Kyle M Hall
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9458 - Add sorting to lists (5.30 KB, patch)
2013-01-23 14:07 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists (7.74 KB, patch)
2013-01-23 16:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists (7.82 KB, patch)
2013-01-23 16:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists (7.81 KB, patch)
2013-01-23 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists (7.87 KB, patch)
2013-02-15 15:14 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup (3.89 KB, patch)
2013-02-19 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup (5.39 KB, patch)
2013-02-19 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup (7.23 KB, patch)
2013-02-19 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists (7.94 KB, patch)
2013-02-26 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup (7.30 KB, patch)
2013-02-26 09:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup 2 (804 bytes, patch)
2013-02-26 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup 3 (1.01 KB, patch)
2013-03-05 17:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup 4 (3.99 KB, patch)
2013-03-07 18:30 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9458 - Add sorting to lists - QA Followup 4 (4.44 KB, patch)
2013-03-11 15:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-01-23 14:06:10 UTC
Add the ability to sort results on lists.
Comment 1 Kyle M Hall 2013-01-23 14:07:21 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-01-23 14:37:35 UTC
I have some issues with this:

In the staff client:

- Sortable columns do not have a visual indicator that they are sortable. They should have an icon like JavaScript-sorted table headers do.
- Only DESC sorting is implemented. This is contrary to user expectations based on how table sorting is handled everywhere else in Koha.
- How do you sort by call number if there can be any number of call numbers attached to a particular title?

In the OPAC:

- Sorting depends on JavaScript, but isn't hidden if JavaScript is disabled. Two options here:

1. Hide the sorting form by default and reveal it via JS.
2. Add a submit button to the sort form and hide it with JS.

- The call number sort here is just as problematic as in the staff client.

I assume that server-side sorting was chosen because these interfaces have server-side pagination. Although it depends on JavaScript, I like the DataTables option for sorting, particularly in the staff client, because it keeps things consistent. I wouldn't reject a patch for not using DataTables, but I think it should be considered.
Comment 3 Marcel de Rooy 2013-01-23 14:49:33 UTC
Just a remark about the multiple items: See bug report 8032. The sorting on call number works on the first item (just an arbitrary choice). But all call numbers are listed.
Comment 4 Kyle M Hall 2013-01-23 15:38:45 UTC
(In reply to comment #3)
> Just a remark about the multiple items: See bug report 8032. The sorting on
> call number works on the first item (just an arbitrary choice). But all call
> numbers are listed.

You are correct. However, the callnumber is what the sponsor specifically wants. I'd say it's better than nothing, and in many cases the items will all have the same callnumber which makes the issue moot.
Comment 5 Kyle M Hall 2013-01-23 15:49:35 UTC
> In the staff client:
> 
> - Sortable columns do not have a visual indicator that they are sortable.
> They should have an icon like JavaScript-sorted table headers do.
> - Only DESC sorting is implemented. This is contrary to user expectations
> based on how table sorting is handled everywhere else in Koha.

I'll amend the patch to make it follow the conventions more closely.

> - How do you sort by call number if there can be any number of call numbers
> attached to a particular title?

I believe it's sorted by the callnumber highest ranked for that sorting.

> In the OPAC:
> 
> - Sorting depends on JavaScript, but isn't hidden if JavaScript is disabled.
> Two options here:
> 
> 1. Hide the sorting form by default and reveal it via JS.
> 2. Add a submit button to the sort form and hide it with JS.

I'll go with two since it will keep the feature for non-js enabled browsers.

> I assume that server-side sorting was chosen because these interfaces have
> server-side pagination. Although it depends on JavaScript, I like the
> DataTables option for sorting, particularly in the staff client, because it
> keeps things consistent. I wouldn't reject a patch for not using DataTables,
> but I think it should be considered.

I wanted to use datatables, but when I discovered the server side paging I had to change my method. The best way to handle this would be to rip it out completely and rewrite is as an AJAX-calling DataTable, but that would require some serious development which is a bit out of line with just adding the ability to sort the results.
Comment 6 Kyle M Hall 2013-01-23 16:16:08 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-01-23 16:19:02 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-01-23 16:20:27 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2013-02-15 15:14:13 UTC
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Comment 10 Paul Poulain 2013-02-15 15:14:31 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-02-19 10:53:25 UTC
3 small remarks:
- At the OPAC, the user does not know what is the default sorting. At the intranet, same thing, maybe we would like to add the arrows (up and down) according to the default sorting.
- We cannot sort by copyright date.
- We cannot choose a direction (asc/desc) on editing a list.
Comment 12 Kyle M Hall 2013-02-19 13:42:30 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-02-19 13:54:19 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-02-19 13:55:30 UTC
(In reply to comment #11)
> 3 small remarks:
> - At the OPAC, the user does not know what is the default sorting. At the
> intranet, same thing, maybe we would like to add the arrows (up and down)

Fixed for Intranet. For the OPAC, the sorting is shown in the "Sort by:" pulldown.

> - We cannot sort by copyright date.

Fixed!

> - We cannot choose a direction (asc/desc) on editing a list.

This one is a bit out of scope for this bug. It would require a database change. I'm going to file a separate bug report for it and attach this one as a blocker.
Comment 15 Jonathan Druart 2013-02-19 14:30:07 UTC
(In reply to comment #14)
> (In reply to comment #11)
> > 3 small remarks:
> > - At the OPAC, the user does not know what is the default sorting. At the
> > intranet, same thing, maybe we would like to add the arrows (up and down)
> 
> Fixed for Intranet. For the OPAC, the sorting is shown in the "Sort by:"
> pulldown.

Thanks.
There is an issue about arrows:
 [% IF direction == 'asc' %][% direction = 'desc' %][% ELSE %][% direction = 'asc' %][% END %]

So there is an inversion on them. When the 'asc' is needed, the desc.gif is displayed.

> > - We cannot choose a direction (asc/desc) on editing a list.
> 
> This one is a bit out of scope for this bug. It would require a database
> change. I'm going to file a separate bug report for it and attach this one
> as a blocker.

Yep!
Comment 16 Kyle M Hall 2013-02-19 17:01:41 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-02-19 17:03:03 UTC
> There is an issue about arrows:
>  [% IF direction == 'asc' %][% direction = 'desc' %][% ELSE %][% direction =
> 'asc' %][% END %]
> 
> So there is an inversion on them. When the 'asc' is needed, the desc.gif is
> displayed.
> 

Good catch! Fixed in this new replacement followup!
Comment 18 Jonathan Druart 2013-02-26 09:47:31 UTC
QA comment:

Works fine.

This patch adds sorting to lists at the OPAC and the intranet.
A great improvement would be to add links on arrows.

Marked as Passed QA.
Comment 19 Jonathan Druart 2013-02-26 09:47:55 UTC
Created attachment 15674 [details] [review]
Bug 9458 - Add sorting to lists

Test Plan:
1) Apply patch
2) View list in staff interface, try sorting by title, then author
3) Repeat 2 with same list in OPAC

Signed-off-by: Delaye Stephane <stephane.delaye@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 20 Jonathan Druart 2013-02-26 09:47:59 UTC
Created attachment 15675 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 21 Jared Camins-Esakov 2013-02-26 13:29:47 UTC
I am concerned about the way $sortfield is included directly in the query. Does it provide an SQL injection vector?
Comment 22 Kyle M Hall 2013-02-26 15:34:36 UTC
Created attachment 15694 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 2
Comment 23 Kyle M Hall 2013-02-26 15:36:08 UTC
Good catch! We cannot use a placeholder for ORDER BY fields, but we *can* escape it using quote_identifier to ensure it cannot be used for SQL injection attacks. I've attached a second followup to add this.

(In reply to comment #21)
> I am concerned about the way $sortfield is included directly in the query.
> Does it provide an SQL injection vector?
Comment 24 Jared Camins-Esakov 2013-03-01 00:56:15 UTC
Comment on attachment 15675 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup

Review of attachment 15675 [details] [review]:
-----------------------------------------------------------------

::: C4/VirtualShelves/Page.pm
@@ +249,4 @@
>                  my $items;
>                  my $tag_quantity;
>                  my $sortfield = ( $sorton ? $sorton : 'title' );
> +                $sortfield = $query->param('sort') || $sortfield || 'title'; ## Passed in sorting overrides default sorting

I am a little bit perplexed by this line. Why did you add the || 'title' when $sortfield has already been set? Or, equivalently, why did you set $sortfield previously? I would have thought the simplest way to get the desired results would be:
  my $sortfield = $query->param('sort') || $sorton || 'title';

My chief concern is, is there some sort of side-effect in the code that you're trying to work around with this?
Comment 25 Kyle M Hall 2013-03-05 17:45:01 UTC
> I am a little bit perplexed by this line. Why did you add the || 'title'
> when $sortfield has already been set? Or, equivalently, why did you set
> $sortfield previously? I would have thought the simplest way to get the
> desired results would be:
>   my $sortfield = $query->param('sort') || $sorton || 'title';
> 
> My chief concern is, is there some sort of side-effect in the code that
> you're trying to work around with this?

Nope, that's just me being stupid ; ) 

I'll post a small followup to fix that.
Comment 26 Kyle M Hall 2013-03-05 17:47:00 UTC
Created attachment 15891 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 3
Comment 27 Jared Camins-Esakov 2013-03-07 15:36:13 UTC
I've noticed something very odd about sorting on the staff client: if I choose to sort by title (for example), it always defaults to sorting in *descending* order, from Z-A. Is there any way to fix that so that it sorts according to what we might expect?
Comment 28 Kyle M Hall 2013-03-07 18:30:47 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-03-07 18:32:13 UTC
Good catch! This latest followup fixes that. I've also simplified some of the presentation logic.

(In reply to comment #27)
> I've noticed something very odd about sorting on the staff client: if I
> choose to sort by title (for example), it always defaults to sorting in
> *descending* order, from Z-A. Is there any way to fix that so that it sorts
> according to what we might expect?
Comment 30 Jared Camins-Esakov 2013-03-11 12:42:11 UTC
(In reply to comment #29)
> Good catch! This latest followup fixes that. I've also simplified some of
> the presentation logic.

There's a problem with your latest follow-up: it inserts unsanitized user input into the page. I checked and it seems that most browsers catch the XSS if you try to insert Javascript, but I'd rather not depend on that if we don't have to. To see what I mean, try a link like: /cgi-bin/koha/virtualshelves/shelves.pl?viewshelf=55&sort=title&direction=%22%3Ehere's%20some%20text%3Cspan%20style=%22
Comment 31 Kyle M Hall 2013-03-11 15:14:50 UTC
Created attachment 16031 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 4
Comment 32 Kyle M Hall 2013-03-11 15:15:33 UTC
> There's a problem with your latest follow-up: it inserts unsanitized user
> input into the page. I checked and it seems that most browsers catch the XSS
> if you try to insert Javascript, but I'd rather not depend on that if we
> don't have to. To see what I mean, try a link like:
> /cgi-bin/koha/virtualshelves/shelves.
> pl?viewshelf=55&sort=title&direction=%22%3Ehere's%20some%20text%3Cspan%20styl
> e=%22

Fixed!
Comment 33 Jared Camins-Esakov 2013-03-13 13:07:40 UTC
This patch has been pushed to master.
Comment 34 Bernardo Gonzalez Kriegel 2013-07-14 21:16:27 UTC
On request, a slightly modified version of this is included in 3.10.x, it changes a little, only add sort capability to lists by Author/Title.

Pushed to 3.10.x, will be in 3.10.8