Bug 34155 - OPAC item level holds "force" option broken
Summary: OPAC item level holds "force" option broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Kyle M Hall (khall)
QA Contact: Katrin Fischer
URL:
Keywords: regression
Depends on: 24860
Blocks:
  Show dependency treegraph
 
Reported: 2023-06-29 12:47 UTC by Owen Leonard
Modified: 2023-12-28 20:47 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.03,22.11.09
Circulation function:


Attachments
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced (2.05 KB, patch)
2023-07-17 09:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced (2.10 KB, patch)
2023-07-17 10:45 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced (2.16 KB, patch)
2023-07-29 19:10 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 Owen Leonard 2023-06-29 12:47:11 UTC
If I set OPAC item level holds to "force" in Circulation and Fine rules, when I go to place a hold in the OPAC I'm still given the option of choosing "next available."

git bisect says the cause is Bug 24860: "Add ability to select an item group when placing a hold - OPAC"
Comment 1 Julian Maurice 2023-07-17 09:16:21 UTC
Created attachment 153532 [details] [review]
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced

This was a regression caused by bug 24860

Test plan:
1. Set up circulation rules so that OPAC users can place holds only on
   specific items ("OPAC item level holds" = "force")
2. Try to place a hold at OPAC. The "Next available item" option should
   not appear.
3. Set "OPAC item level holds" to "allow"
4. Try to place a hold at OPAC. The "Next available item" option should
   appear
Comment 2 Owen Leonard 2023-07-17 10:45:12 UTC
Created attachment 153537 [details] [review]
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced

This was a regression caused by bug 24860

Test plan:
1. Set up circulation rules so that OPAC users can place holds only on
   specific items ("OPAC item level holds" = "force")
2. Try to place a hold at OPAC. The "Next available item" option should
   not appear.
3. Set "OPAC item level holds" to "allow"
4. Try to place a hold at OPAC. The "Next available item" option should
   appear

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 3 Jonathan Druart 2023-07-17 12:27:23 UTC
Isn't there a flaw in the controller as well? See bug 24860 comment 231.
Comment 4 Fridolin Somers 2023-07-22 02:54:15 UTC
(In reply to Jonathan Druart from comment #3)
> Isn't there a flaw in the controller as well? See bug 24860 comment 231.

Indeed.

In my opinion we should use only 'bibitemloo.forced_hold_level' in TT if possible.
bibitemloo.force_hold is not very explicit.
Comment 5 Katrin Fischer 2023-07-29 19:10:37 UTC
Created attachment 154059 [details] [review]
Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced

This was a regression caused by bug 24860

Test plan:
1. Set up circulation rules so that OPAC users can place holds only on
   specific items ("OPAC item level holds" = "force")
2. Try to place a hold at OPAC. The "Next available item" option should
   not appear.
3. Set "OPAC item level holds" to "allow"
4. Try to place a hold at OPAC. The "Next available item" option should
   appear

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Katrin Fischer 2023-07-29 19:12:06 UTC
(In reply to Fridolin Somers from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > Isn't there a flaw in the controller as well? See bug 24860 comment 231.
> 
> Indeed.
> 
> In my opinion we should use only 'bibitemloo.forced_hold_level' in TT if
> possible.
> bibitemloo.force_hold is not very explicit.

This patch works for now, but I am all for fixing it up at a higher level too. Maybe we can compromise to do that on a separate bug? 

Unless someone wants to do a quick bugfix here? We left this unfixed for a month now, that's not good for this type of bug.
Comment 7 Jonathan Druart 2023-08-01 06:45:39 UTC
(In reply to Katrin Fischer from comment #6)
> Unless someone wants to do a quick bugfix here? We left this unfixed for a
> month now, that's not good for this type of bug.

Backporting bad bugfixes is worst. Code in the controller must be cleaned before push in my opinion.
Comment 8 Katrin Fischer 2023-08-01 08:16:16 UTC
(In reply to Jonathan Druart from comment #7)
> (In reply to Katrin Fischer from comment #6)
> > Unless someone wants to do a quick bugfix here? We left this unfixed for a
> > month now, that's not good for this type of bug.
> 
> Backporting bad bugfixes is worst. Code in the controller must be cleaned
> before push in my opinion.

So we just keep it broken, because noone is going to write that fix apparently?

This was filed a month ago, is a user facing issue in stable releases. The patch is really small, it will be quite easy to do a proper fix on top.
Comment 9 Tomás Cohen Arazi (tcohen) 2023-08-15 08:24:43 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 10 Fridolin Somers 2023-08-17 18:59:07 UTC
Pushed to 23.05.x for 23.05.03
Comment 11 Pedro Amorim 2023-08-18 13:46:27 UTC
Nice work everyone!

Pushed to 22.11.x for next release