Bug 34155

Summary:	OPAC item level holds "force" option broken
Product:	Koha	Reporter:	Owen Leonard <oleonard>
Component:	OPAC	Assignee:	Kyle M Hall (khall) <kyle>
Status:	RESOLVED FIXED	QA Contact:	Katrin Fischer <katrin.fischer>
Severity:	major
Priority:	P5 - low	CC:	fridolin.somers, jonathan.druart, julian.maurice, kyle, pierre.genty, tomascohen
Version:	Main	Keywords:	regression
Hardware:	All
OS:	All
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:
Bug Depends on:	24860
Bug Blocks:
Attachments:	Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced Bug 34155: Hide 'Next available' at OPAC when item-level hold is forced

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