Bug 28125 - All OPAC holds blocked when OPACHiddenItems contains incorrect values
Summary: All OPAC holds blocked when OPACHiddenItems contains incorrect values
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15448
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-09 14:53 UTC by Andrew Fuerste-Henry
Modified: 2021-06-16 20:39 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06


Attachments
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed (7.35 KB, patch)
2021-04-13 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Message larger than table (29.69 KB, image/png)
2021-04-13 12:17 UTC, Andrew Fuerste-Henry
Details
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed (7.35 KB, patch)
2021-04-13 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed (7.41 KB, patch)
2021-04-13 14:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 28125: Display a warning on the about page for OPACHiddenItems is needed (7.46 KB, patch)
2021-04-15 19:19 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28125: (QA follow-up) Fix missing filter (1.46 KB, patch)
2021-04-15 19:19 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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