http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #3 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> ---

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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14790|0 |1 is obsolete| | --- Comment #7 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 14791 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14791&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #14791|0 |1 is obsolete| | --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 14792 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=14792&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Paul Poulain <paul.poulain@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |paul.poulain@biblibre.com --- Comment #9 from Paul Poulain <paul.poulain@biblibre.com> --- Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #10 from Paul Poulain <paul.poulain@biblibre.com> --- Created attachment 15363 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15363&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #12 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 15543 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15543&action=edit Bug 9458 - Add sorting to lists - QA Followup -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #15 from Jonathan Druart <jonathan.druart@biblibre.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> ---

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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15363|0 |1 is obsolete| | Attachment #15554|0 |1 is obsolete| | --- Comment #19 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 15674 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=15674&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #21 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- I am concerned about the way $sortfield is included directly in the query. Does it provide an SQL injection vector? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- 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?

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> ---

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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #27 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 --- Comment #29 from Kyle M Hall <kyle@bywatersolutions.com> --- 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?

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #15952|0 |1 is obsolete| | --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 16031 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=16031&action=edit Bug 9458 - Add sorting to lists - QA Followup 4 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9458 Jared Camins-Esakov <jcamins@cpbibliography.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #33 from Jared Camins-Esakov <jcamins@cpbibliography.com> --- This patch has been pushed to master. -- You are receiving this mail because: You are watching all bug changes.