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.

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.

(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.

> 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.

Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>

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.

(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.

(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!

> 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!

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.

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>

Created attachment 15675 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

I am concerned about the way $sortfield is included directly in the query. Does it provide an SQL injection vector?

Created attachment 15694 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 2

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 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?

> 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.

Created attachment 15891 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 3

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?

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?

(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

Created attachment 16031 [details] [review]
Bug 9458 - Add sorting to lists - QA Followup 4

> 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!

This patch has been pushed to master.

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