Bug 17526 - OPAC lists sortfield breaks with a (
Summary: OPAC lists sortfield breaks with a (
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-31 19:40 UTC by Blou
Modified: 2019-06-20 13:00 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.01


Attachments
Bug 17526 - OPAC lists require a sortfield (1.80 KB, patch)
2016-10-31 19:44 UTC, Blou
Details | Diff | Splinter Review
Bug 17526 - OPAC lists require a sortfield (1.97 KB, patch)
2016-11-02 14:25 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17526: Change grep to deal with malformed sortfield (1.39 KB, patch)
2019-06-13 19:32 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 17526: Change grep to deal with malformed sortfield (1.46 KB, patch)
2019-06-17 08:03 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17526: Change grep to deal with malformed sortfield (1.52 KB, patch)
2019-06-17 20:22 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2016-10-31 19:40:11 UTC
Using the OPAC, going into the public lists, I click one of them and get

DBIx::Class::Storage::DBI::_dbh_execute(): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ASC LIMIT 10) `me`  JOIN `biblio` `biblionumber` ON `biblionumber`.`biblionumber'

THis is because the sortfield is empty and creates a SQL like GROUP BY ASC LIMIT 10.  Missing the group by.

In the intranet list, there's a default usage of 'title' that makes it all work.

TESTING:
1) just create a public list with no sortfield.  
   - virtualshelves.category = 2 
   - virtualshelves.sortfield = ''
2) click on the list in the OPAC's public list.  Boom!
Comment 1 Blou 2016-10-31 19:41:29 UTC
This also happens in 3.22, btw.
Comment 2 Blou 2016-10-31 19:44:36 UTC
Created attachment 57010 [details] [review]
Bug 17526 - OPAC lists require a sortfield

Using the OPAC, going into the public lists, we click one of them and get

DBIx::Class::Storage::DBI::_dbh_execute(): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ASC LIMIT 10) `me`  JOIN `biblio` `biblionumber` ON `biblionumber`.`biblionumber'

THis is because the sortfield is empty and creates a SQL like GROUP BY ASC LIMIT 10.  Missing the group by <what?>.
In the intranet list, there's a default usage of 'title' that makes it all work.

TESTING:
1) just create a public list with no sortfield.  In the DB:
   - virtualshelves.category = 2
   - virtualshelves.sortfield = ''
2) click on the list in the OPAC's public list.  Boom!

This simple patch uses 'title' if nothing else is specified.
Comment 3 Marc Véron 2016-11-02 14:25:19 UTC
Created attachment 57104 [details] [review]
Bug 17526 - OPAC lists require a sortfield

Using the OPAC, going into the public lists, we click one of them and get

DBIx::Class::Storage::DBI::_dbh_execute(): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ASC LIMIT 10) `me`  JOIN `biblio` `biblionumber` ON `biblionumber`.`biblionumber'

THis is because the sortfield is empty and creates a SQL like GROUP BY ASC LIMIT 10.  Missing the group by <what?>.
In the intranet list, there's a default usage of 'title' that makes it all work.

TESTING:
1) just create a public list with no sortfield.  In the DB:
   - virtualshelves.category = 2
   - virtualshelves.sortfield = ''
2) click on the list in the OPAC's public list.  Boom!

This simple patch uses 'title' if nothing else is specified.

Reproduced. Patch fixes issue as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 4 Jonathan Druart 2016-11-02 14:58:22 UTC
This should no longer be possible from the interface, so what about updating the bad data as well?
Comment 5 Marcel de Rooy 2016-11-18 13:30:17 UTC
Cannot reproduce this from the interface.
The request for a dbrev repairing bad data is a valid one.

Changing status
Comment 6 Blou 2016-11-18 14:05:43 UTC
But the data is there, the user doesn't really care that it won't happen anymore with the new interface.  It seems that a simple default value is a very valid programming.  In this case and in most others.

This is not a failed QA.  This is a "In Discussion" death sentence...
Comment 7 Jonathan Druart 2016-11-18 14:18:40 UTC
(In reply to Blou from comment #6)
> But the data is there, the user doesn't really care that it won't happen
> anymore with the new interface.  It seems that a simple default value is a
> very valid programming.  In this case and in most others.
> 
> This is not a failed QA.  This is a "In Discussion" death sentence...

Hum? Marcel is just asking the same thing as me: update the existing bad data.
so set searchfield="title" where searchfield is null or "" and we are done.
Comment 8 Marcel de Rooy 2016-11-18 16:21:08 UTC
(In reply to Blou from comment #6)
> But the data is there, the user doesn't really care that it won't happen
> anymore with the new interface.  It seems that a simple default value is a
> very valid programming.  In this case and in most others.
> 
> This is not a failed QA.  This is a "In Discussion" death sentence...

This is not a death sentence. Just add a small atomicupdate dbrev. I do not have a strong opinion about the line in opac-shelves. It should not be necessary, but it would not hurt either.

Although Failed QA or In Discussion may look like the final destination of some patches, this generally speaking does not have to be so. (This one is not in that category at all.)
Comment 9 Jonathan Druart 2017-10-10 16:36:57 UTC
This patch is no longer relevant, it has been fixed by
  commit 45cffd874c62c7b090390c5fb3c955c31f524608
    Bug 17901: Fix possible SQL injection in shelf editing

However we could update the existing bad data anyway.
Comment 10 M. Tompsett 2019-06-13 19:24:57 UTC
Repurposing... See also bug 22941 and bug 23084.
Comment 11 M. Tompsett 2019-06-13 19:32:54 UTC
Created attachment 90584 [details] [review]
Bug 17526: Change grep to deal with malformed sortfield

Giving sortfield a malformed value when viewing lists results in
Internal Server Error.

eg.
http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=view&shelfnumber=1&sortfield=title(

Note the trailing (

Before Patch: kaboom
apply patch
restart
After Patch: No kaboom
Comment 12 Martin Renvoize 2019-06-17 08:03:07 UTC
Created attachment 90641 [details] [review]
Bug 17526: Change grep to deal with malformed sortfield

Giving sortfield a malformed value when viewing lists results in
Internal Server Error.

eg.
http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=view&shelfnumber=1&sortfield=title(

Note the trailing (

Before Patch: kaboom
apply patch
restart
After Patch: No kaboom

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-06-17 08:03:28 UTC
Simple fix which works well for me, Signing Off
Comment 14 Katrin Fischer 2019-06-17 20:22:41 UTC
Created attachment 90695 [details] [review]
Bug 17526: Change grep to deal with malformed sortfield

Giving sortfield a malformed value when viewing lists results in
Internal Server Error.

eg.
http://localhost:8080/cgi-bin/koha/opac-shelves.pl?op=view&shelfnumber=1&sortfield=title(

Note the trailing (

Before Patch: kaboom
apply patch
restart
After Patch: No kaboom

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Martin Renvoize 2019-06-18 09:07:38 UTC
Nice work!

Pushed to master for 19.11.00
Comment 16 Fridolin SOMERS 2019-06-20 12:59:41 UTC
Pushed to 19.05.x for 19.05.01
Comment 17 Fridolin SOMERS 2019-06-20 13:00:58 UTC
Same fix as Bug 22941, nice catch