Bug 28125

Summary: All OPAC holds blocked when OPACHiddenItems contains incorrect values
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27662
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28359
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06
Bug Depends on: 15448    
Bug Blocks: 31473    
Attachments: Bug 28125: Display a warning on the about page for OPACHiddenItems is needed
Message larger than table
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed
Bug 28125: (QA follow-up) Fix missing filter

Description Andrew Fuerste-Henry 2021-04-09 14:53:10 UTC
From bug 15448, we're now checking OPACHiddenItems to see if something is hidden before we allow a hold on it. However, the manner in which that check is performed causes a problem if the syspref contains incorrect values.

To recreate:
- make sure OPACHiddenItems is empty
- find a holdable item on the OPAC, confirm you can place a hold on it
- enter "itemlost: [AB]" in OPACHiddenItems (an incorrect value because itemlost can only contain 1 bit)
- confirm your item is not hidden from the OPAC but you are told it is not available for holds

You'll see the same behavior if you use an incorrect field name, like "lost" or "itemtype."
Comment 1 Jonathan Druart 2021-04-13 08:57:20 UTC
Created attachment 119513 [details] [review]
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed

If OPACHiddenItems is containing badly construct it will lead to
inconsistent behaviours. For instance the holds view will not display
any items, when the detail page will display all of them (because we are
not using the same code, we are in transition to merge it).

This patch suggests to add a warning to the about page in case the query
generated by Koha::Items->filter_by_visible_in_opac produces a warning
Comment 2 Jonathan Druart 2021-04-13 08:58:22 UTC
The only thing I can think of is to display a warning to the about page. Would that work for you, Andrew?
Comment 3 Andrew Fuerste-Henry 2021-04-13 12:17:19 UTC
Created attachment 119534 [details]
Message larger than table

Seems reasonable! The real pain here is that it took a while to figure out that this system preference was the cause of the error, so a warning is definitely helpful. 
I've got a really picky display complaint, though. On Chrome, the warning message ends up a little longer than the table itself and doesn't look good. Can you make the table resize to fit the message?
Comment 4 Jonathan Druart 2021-04-13 14:21:31 UTC
Created attachment 119539 [details] [review]
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed

If OPACHiddenItems is containing badly construct it will lead to
inconsistent behaviours. For instance the holds view will not display
any items, when the detail page will display all of them (because we are
not using the same code, we are in transition to merge it).

This patch suggests to add a warning to the about page in case the query
generated by Koha::Items->filter_by_visible_in_opac produces a warning
Comment 5 Jonathan Druart 2021-04-13 14:21:58 UTC
(In reply to Andrew Fuerste-Henry from comment #3)
> I've got a really picky display complaint, though. On Chrome, the warning
> message ends up a little longer than the table itself and doesn't look good.
> Can you make the table resize to fit the message?

There was a td instead of a th, should be fixed now!
Comment 6 Andrew Fuerste-Henry 2021-04-13 14:33:00 UTC
Created attachment 119540 [details] [review]
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed

If OPACHiddenItems is containing badly construct it will lead to
inconsistent behaviours. For instance the holds view will not display
any items, when the detail page will display all of them (because we are
not using the same code, we are in transition to merge it).

This patch suggests to add a warning to the about page in case the query
generated by Koha::Items->filter_by_visible_in_opac produces a warning

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Andrew Fuerste-Henry 2021-04-13 14:33:18 UTC
Thank you!
Comment 8 Nick Clemens 2021-04-15 19:19:48 UTC
Created attachment 119682 [details] [review]
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed

If OPACHiddenItems is containing badly construct it will lead to
inconsistent behaviours. For instance the holds view will not display
any items, when the detail page will display all of them (because we are
not using the same code, we are in transition to merge it).

This patch suggests to add a warning to the about page in case the query
generated by Koha::Items->filter_by_visible_in_opac produces a warning

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2021-04-15 19:19:51 UTC
Created attachment 119683 [details] [review]
Bug 28125: (QA follow-up) Fix missing filter

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2021-04-16 10:30:24 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-04-22 14:15:13 UTC
Patches have been squashed is master
Comment 12 Fridolin Somers 2021-04-29 09:19:34 UTC
Pushed to 20.11.x for 20.11.06