Bug 34886 - Regression in when hold button appears
Summary: Regression in when hold button appears
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major with 14 votes (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 34694 34836
Blocks:
  Show dependency treegraph
 
Reported: 2023-09-22 13:46 UTC by Martin Renvoize
Modified: 2024-04-25 15:09 UTC (History)
15 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:
24.05.00,23.11.05


Attachments
Bug 34886: Restore 'Place reserve' prior to session (1.53 KB, patch)
2023-09-22 15:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34886: Restore 'Place reserve' prior to session (1.58 KB, patch)
2023-09-22 16:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 34886: Restore 'Place reserve' prior to session (1.58 KB, patch)
2023-09-25 12:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Restore 'Place reserve' prior to session (1.58 KB, patch)
2023-09-25 12:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Rename can_item_be_reserved variable (3.67 KB, patch)
2023-09-25 12:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Move patron check onto C4/Reserves.pm (3.15 KB, patch)
2023-09-25 12:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Restore 'Place reserve' prior to session (1.64 KB, patch)
2023-09-27 10:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34886: Rename can_item_be_reserved variable (3.74 KB, patch)
2023-09-27 10:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34886: Move patron check onto C4/Reserves.pm (3.22 KB, patch)
2023-09-27 10:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34886: Restore 'Place reserve' prior to session (1.64 KB, patch)
2023-09-27 14:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Rename can_item_be_reserved variable (3.76 KB, patch)
2023-09-27 14:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Move patron check onto C4/Reserves.pm (2.60 KB, patch)
2023-09-27 14:14 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: [alternate] Adjust holdability checks on opac details page (4.84 KB, patch)
2023-09-28 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: [alternate] adjust other opac detail scripts (7.00 KB, patch)
2023-09-28 13:16 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: [alternate] Adjust holdability checks on opac details page (4.84 KB, patch)
2023-10-12 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: [alternate] adjust other opac detail scripts (7.02 KB, patch)
2023-10-12 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: [alternate] Adjust holdability checks on opac details page (4.89 KB, patch)
2023-12-08 20:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 34886: [alternate] adjust other opac detail scripts (7.07 KB, patch)
2023-12-08 20:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 34886: Adjust holdability checks on opac details page (4.88 KB, patch)
2024-01-30 12:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: Adjust other opac detail scripts (7.07 KB, patch)
2024-01-30 12:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 34886: Add selenium tests (17.00 KB, patch)
2024-02-09 16:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: [DO NOT PUSH] use case (1.34 KB, patch)
2024-02-09 16:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: (QA follow-up) chmod, remove POD (2.97 KB, patch)
2024-03-22 10:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: Adjust holdability checks on opac details page (4.97 KB, patch)
2024-03-29 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: Adjust other opac detail scripts (7.15 KB, patch)
2024-03-29 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: Add selenium tests (17.09 KB, patch)
2024-03-29 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: [DO NOT PUSH] use case (1.43 KB, patch)
2024-03-29 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: (QA follow-up) chmod, remove POD (3.02 KB, patch)
2024-03-29 07:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34886: Comment failing tests (2.66 KB, patch)
2024-04-01 08:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Comment failing tests (2.70 KB, patch)
2024-04-01 08:27 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34886: Comment failing tests (2.49 KB, patch)
2024-04-01 08:29 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-09-22 13:46:07 UTC
Prior to bug 34694 one could start the reservation process from the OPAC prior to login. This was useful and would prompt a login as the first step.  We should restore this functionality.
Comment 1 Martin Renvoize 2023-09-22 15:18:25 UTC
Created attachment 156107 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.
Comment 2 Martin Renvoize 2023-09-22 15:23:02 UTC
I'm not entirely happy with this one.. but it feels pragmatic.. I'd love to see a slightly deeper dive on it but don't really have the time to take that on myself.
Comment 3 Katrin Fischer 2023-09-22 15:56:14 UTC
Please add a test plan, I am stuck on how this is supposed to work.

* Logged in
* item type is not for loan
* item type is set to 'no holds allowed' in circ rules
* circ rule says: 0 holds allowed
* When I click 'place hold' the hold is not allowed

I still get a button... what triggers the button to disappear if not any of that?

But: the button also remains visible when not logged in. So that is good.
Comment 4 David Nind 2023-09-22 16:35:13 UTC
Created attachment 156114 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2023-09-22 16:40:31 UTC
Testing notes:

1. Go to the OPAC, and without logging in, perform a search, for example: perl.
2. Note that there is a 'Place hold' link in the search results - if you click on this it prompts you to log in before continuing to place the hold.
3. Note as well, that if you go to the record details page, there is no 'Place hold' link.
4. Apply the patch.
5. Repeat steps 1-3 - there is now a 'Place hold' link, and you are prompted to log in to place the hold.
Comment 6 Fridolin Somers 2023-09-22 21:10:05 UTC
We should also fix Bug 34836 right ?
Comment 7 Martin Renvoize 2023-09-22 21:42:48 UTC
Thanks for the interest here and the testing.

Bug 30846 explains when the button shouldn't appear I believe.

This bug is about resorting the behaviour prior to that one for just the not logged in case.

And yes, we should extend the bug to cover the cases you mention Fridolin.
Comment 8 Pedro Amorim 2023-09-25 12:50:08 UTC
Created attachment 156150 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Pedro Amorim 2023-09-25 12:50:31 UTC
Created attachment 156151 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Pedro Amorim 2023-09-25 12:50:33 UTC
Created attachment 156152 [details] [review]
Bug 34886: Rename can_item_be_reserved variable

This variable is named like a function that returns a boolean, but is actually just a flag.
Comment 11 Pedro Amorim 2023-09-25 12:50:36 UTC
Created attachment 156153 [details] [review]
Bug 34886: Move patron check onto C4/Reserves.pm

Rollback changes in opac-detail.pl

Run tests:
prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
prove t/db_dependent/Reserves.t

Bug 15534 changed onshelfholds going from 0/1 circ rule to 0/1/2
But the check in opac-detail.pl still remained checking for just a positive value.
Currently, onshelfholds is as follows:
1 - Yes
0 - If any unavailable
2 - If all unavailable

I think the check needs to be updated to only when onshelfholds = 1 (Yes), which is effectively done in IsAvailableForItemLevelRequest in C4/Reserves.pm, with the caveat that when onshelfholds = 2, it requires the $patron to exist to do the full check.

This patch fixes the issue. But also makes it so that the "Place hold" link only shows for the unauthenticated user if onshelfholds = "Yes", whereas before it showed for the unauthenticated user if onshelfholds = "Yes" OR if onshelfholds = "If all unavailable"
Comment 12 Martin Renvoize 2023-09-27 09:44:51 UTC
Your follow-up here is much better than my original approach, Pedro, thanks for jumping in.

I'm just reviewing it and will also get a few others to take a look :)
Comment 13 Martin Renvoize 2023-09-27 10:20:49 UTC
Created attachment 156277 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2023-09-27 10:20:53 UTC
Created attachment 156278 [details] [review]
Bug 34886: Rename can_item_be_reserved variable

This variable is named like a function that returns a boolean, but is actually just a flag.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2023-09-27 10:20:56 UTC
Created attachment 156279 [details] [review]
Bug 34886: Move patron check onto C4/Reserves.pm

Rollback changes in opac-detail.pl

Run tests:
prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
prove t/db_dependent/Reserves.t

Bug 15534 changed onshelfholds going from 0/1 circ rule to 0/1/2
But the check in opac-detail.pl still remained checking for just a positive value.
Currently, onshelfholds is as follows:
1 - Yes
0 - If any unavailable
2 - If all unavailable

I think the check needs to be updated to only when onshelfholds = 1 (Yes), which is effectively done in IsAvailableForItemLevelRequest in C4/Reserves.pm, with the caveat that when onshelfholds = 2, it requires the $patron to exist to do the full check.

This patch fixes the issue. But also makes it so that the "Place hold" link only shows for the unauthenticated user if onshelfholds = "Yes", whereas before it showed for the unauthenticated user if onshelfholds = "Yes" OR if onshelfholds = "If all unavailable"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2023-09-27 10:23:00 UTC
This is certainly an improvement, but there is still a regression in functionality.. especially noticeable if 'ReservesControlBranch' is set to 'ItemHomeLibrary' as we actually may not even need 'Patron' for the calculations in that case at all.

It's really hard.. this code is all so intertwined all over the place.. I'm not sure how deep down the rabbit hole we should be going.
Comment 17 Nick Clemens 2023-09-27 11:44:24 UTC
On the search results screen, we determine whether or not to show the hold button based on 'norequests' variable - this is set in Search.pm as the result of either:
Biblio itemtype holdability if using bib level itemtypes
Items filtered by filter_by_for_holds which basically just checks holdallowed rules and a few statuses

opac-detail / opac-ISBDdetail / opac-MARCdetail all have this parameter set in current code

I feel like using this when there is not a patron is generally close enough? It's consistent at least.
Comment 18 Pedro Amorim 2023-09-27 14:14:15 UTC
Created attachment 156293 [details] [review]
Bug 34886: Restore 'Place reserve' prior to session

This is a pragmatic patch to restore the display of 'Place hold' prior
to patron login. When we have no Patron, we fall back to only checkout
get_onshelfholds_policy without a patron. When we have a patron, we use
IsAvailableForItemLevelRequest instead to get the more detailed
response.

This isn't perfect, but I can't see an easy way to restore this
functionality otherwise.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Pedro Amorim 2023-09-27 14:14:19 UTC
Created attachment 156294 [details] [review]
Bug 34886: Rename can_item_be_reserved variable

This variable is named like a function that returns a boolean, but is actually just a flag.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Pedro Amorim 2023-09-27 14:14:22 UTC
Created attachment 156295 [details] [review]
Bug 34886: Move patron check onto C4/Reserves.pm

Rollback changes in opac-detail.pl

Run tests:
prove t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t
prove t/db_dependent/Reserves.t

Bug 15534 changed onshelfholds going from 0/1 circ rule to 0/1/2
But the check in opac-detail.pl still remained checking for just a positive value.
Currently, onshelfholds is as follows:
1 - Yes
0 - If any unavailable
2 - If all unavailable

I think the check needs to be updated to only when onshelfholds = 1 (Yes), which is effectively done in IsAvailableForItemLevelRequest in C4/Reserves.pm, with the caveat that when onshelfholds = 2, it requires the $patron to exist to do the full check.

This patch fixes the issue. But also makes it so that the "Place hold" link only shows for the unauthenticated user if onshelfholds = "Yes", whereas before it showed for the unauthenticated user if onshelfholds = "Yes" OR if onshelfholds = "If all unavailable"

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Pedro Amorim 2023-09-27 14:14:42 UTC
Rebased on top of bug 34836
Comment 22 Nick Clemens 2023-09-28 13:16:26 UTC
Created attachment 156326 [details] [review]
Bug 34886: [alternate] Adjust holdability checks on opac details page

This patch tries to simplify some of the logic here to match that on the search results. When we don't have a patron, we fallback to determining if an item can be held buy determining whether there are any items that don't have holds disallowed at the all libraries level. We also remove items with non-holdable statuses like withdrawn etc (and check some system preferences)

If we don't have a patron, then we are done, however, if we do, then we need to check each item against the policies related to that patron.

This patch also removes two checks at the end:
CountItemsIssued($biblionumber)
$biblio->has_items_waiting_or_intransit

These seem to be from bug 4319 - however, those rules are checked by IsAvailableForItemLevelRequest and are only relevant when we have a patron. These checks essentially assumed 'onshelfholds' policy of 'If any unavailable' For consistency sake I think we should follow the same logic as the results page.

To test:
1 - Find a record with two items, of different types, set a 'Default checkout, hold and return policy' of 'No holds allowed'
2 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
3 - Delete that rule, make both items withdrawn
4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
5 - Mark one item as not withdrawn
6 - Search opac, not logged in, and verify both the results page or details page shows the place hold button
7 - Log in to opac
8 - Search opac, logged in, and verify both the results page or details page shows the place hold button
9 - Place an 'On shelf holds policy' rule for that patron category of 'If any unavailable'
10 - Search opac, logged in, and verify the results and details page shows the place hold button
11 - Set the other item to not withdrawn
12 - Search opac, logged in, and verify the results page shows the place hold button, but details does not
13 - Try various other scenarios - details page should be more correct, results page is always an approximation
Comment 23 Nick Clemens 2023-09-28 13:16:29 UTC
Created attachment 156327 [details] [review]
Bug 34886: [alternate] adjust other opac detail scripts

Same chanegs as before, but for MARC and ISBD details pages
Comment 24 Nick Clemens 2023-09-28 13:19:58 UTC
I tried my hand, I didn't see the 34836 dependency until after, but addressed both scripts here as well.

This code is gnarly and weird - I think Pedro's change to IsAvailableForItemLevel request makes sense, but should possibly be its own bug.

We turn out to have strange assumptions here, so I tried to simplify as best as possible, I am not certain my test plan is all correct. I am fine if Pedro wants to simply obsolete my patches as well :-)
Comment 25 Pedro Amorim 2023-10-10 12:34:31 UTC
Tested Nick's patches.
Test plan works until step 10 where it fails for me.

Here's where it failed for me, k-t-d, On shelf holds policy = If any unavailable:

- biblio 180 - 3 items withdrawn. 1 item not withdrawn:
OPAC logged out:
- detail page 'place hold' link: yes
- results page 'place hold' link: yes

OPAC logged in:
- detail page 'place hold' link: no  (step 10 fail)
- results page 'place hold' link: yes

I also tested with "If all unavailable" with 4 items withdrawn and, being that all are unavailable, I would expect to be able to place holds, but the link doesn't show for detail or results, logged in or not. Unless I'm misunderstanding the intended behavior here (likely).

- Record 180 - 4 items withdrawn
OPAC logged out:
- detail page 'place hold' link: no
- results page 'place hold' link: no

OPAC logged in:
- detail page 'place hold' link: no
- results page 'place hold' link: no

Another funny thing I discovered is that, regardless of all of this, there is a "Place hold" link on the top of the search results that always shows, and always allows you to click it if you check at least one biblio which leads me to think it's not considering any rule value at all.
Comment 26 Pedro Amorim 2023-10-10 13:03:36 UTC
(In reply to Pedro Amorim from comment #25)
> 
> I also tested with "If all unavailable" with 4 items withdrawn and, being
> that all are unavailable, I would expect to be able to place holds, but the
> link doesn't show for detail or results, logged in or not. Unless I'm
> misunderstanding the intended behavior here (likely).
>

This is wrong, 'withdrawn' is a special case where IsAvailableForItemLevelRequest returns 0 as for other conditions (itemlost, notforloan, damaged) before onshelfholds policy is even checked.
Comment 27 Nick Clemens 2023-10-12 15:22:28 UTC
Created attachment 156980 [details] [review]
Bug 34886: [alternate] Adjust holdability checks on opac details page

This patch tries to simplify some of the logic here to match that on the search results. When we don't have a patron, we fallback to determining if an item can be held buy determining whether there are any items that don't have holds disallowed at the all libraries level. We also remove items with non-holdable statuses like withdrawn etc (and check some system preferences)

If we don't have a patron, then we are done, however, if we do, then we need to check each item against the policies related to that patron.

This patch also removes two checks at the end:
CountItemsIssued($biblionumber)
$biblio->has_items_waiting_or_intransit

These seem to be from bug 4319 - however, those rules are checked by IsAvailableForItemLevelRequest and are only relevant when we have a patron. These checks essentially assumed 'onshelfholds' policy of 'If any unavailable' For consistency sake I think we should follow the same logic as the results page.

To test:
1 - Find a record with two items, of different types, set a 'Default checkout, hold and return policy' of 'No holds allowed'
2 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
3 - Delete that rule, make both items withdrawn
4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
5 - Mark one item as not withdrawn
6 - Search opac, not logged in, and verify both the results page or details page shows the place hold button
7 - Log in to opac
8 - Search opac, logged in, and verify both the results page or details page shows the place hold button
9 - Place an 'On shelf holds policy' rule for that patron category of 'If any unavailable'
10 - Search opac, logged in, and verify the results and details page shows the place hold button
11 - Set the other item to not withdrawn
12 - Search opac, logged in, and verify the results page shows the place hold button, but details does not
13 - Try various other scenarios - details page should be more correct, results page is always an approximation
Comment 28 Nick Clemens 2023-10-12 15:22:31 UTC
Created attachment 156981 [details] [review]
Bug 34886: [alternate] adjust other opac detail scripts

Same chanegs as before, but for MARC and ISBD details pages
Comment 29 Nick Clemens 2023-10-12 15:24:13 UTC
(In reply to Pedro Amorim from comment #26)
> (In reply to Pedro Amorim from comment #25)
> > 
> > I also tested with "If all unavailable" with 4 items withdrawn and, being
> > that all are unavailable, I would expect to be able to place holds, but the
> > link doesn't show for detail or results, logged in or not. Unless I'm
> > misunderstanding the intended behavior here (likely).
> >
> 
> This is wrong, 'withdrawn' is a special case where
> IsAvailableForItemLevelRequest returns 0 as for other conditions (itemlost,
> notforloan, damaged) before onshelfholds policy is even checked.

That's why my plan fails at step 10, I missed this too - if you unmark on item as withdrawn, and then check it out then the patron can place hold

This matches behavior on master, so I think the issue with withdrawn (and other statuses) not counting towards 'any unavailable' is another bug

Rebased on top of 34836
Comment 30 David Nind 2023-12-08 20:07:06 UTC
Created attachment 159678 [details] [review]
Bug 34886: [alternate] Adjust holdability checks on opac details page

This patch tries to simplify some of the logic here to match that on the search results. When we don't have a patron, we fallback to determining if an item can be held buy determining whether there are any items that don't have holds disallowed at the all libraries level. We also remove items with non-holdable statuses like withdrawn etc (and check some system preferences)

If we don't have a patron, then we are done, however, if we do, then we need to check each item against the policies related to that patron.

This patch also removes two checks at the end:
CountItemsIssued($biblionumber)
$biblio->has_items_waiting_or_intransit

These seem to be from bug 4319 - however, those rules are checked by IsAvailableForItemLevelRequest and are only relevant when we have a patron. These checks essentially assumed 'onshelfholds' policy of 'If any unavailable' For consistency sake I think we should follow the same logic as the results page.

To test:
1 - Find a record with two items, of different types, set a 'Default checkout, hold and return policy' of 'No holds allowed'
2 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
3 - Delete that rule, make both items withdrawn
4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
5 - Mark one item as not withdrawn
6 - Search opac, not logged in, and verify both the results page or details page shows the place hold button
7 - Log in to opac
8 - Search opac, logged in, and verify both the results page or details page shows the place hold button
9 - Place an 'On shelf holds policy' rule for that patron category of 'If any unavailable'
10 - Search opac, logged in, and verify the results and details page shows the place hold button
11 - Set the other item to not withdrawn
12 - Search opac, logged in, and verify the results page shows the place hold button, but details does not
13 - Try various other scenarios - details page should be more correct, results page is always an approximation

Signed-off-by: David Nind <david@davidnind.com>
Comment 31 David Nind 2023-12-08 20:07:10 UTC
Created attachment 159679 [details] [review]
Bug 34886: [alternate] adjust other opac detail scripts

Same chanegs as before, but for MARC and ISBD details pages

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2023-12-08 20:17:11 UTC
Testing notes (using KTD):

1. Tested alternate patches.

2. Step 10 fails for me, but as per comment #29 I've signed off anyway (hopefully I understand that correctly). Results for me:
   . Logged in, one item withdrawn and other available, "If any unavailable" set as rule
   . Search results: has place hold option, if you go to place a hold - "There are no items that can be placed on hold"
   . Details page (Normal, MARC and ISBD views): no place hold option

3. I've changed the importance to Major as it is a regression, feel free to change it back to Normal.
Comment 33 Jonathan Druart 2024-01-12 09:57:18 UTC
Pedro, Nick, which version do we keep? If Nick's, Pedro can you QA it please (and change assignee)?
Comment 34 Marcel de Rooy 2024-01-26 07:28:38 UTC
Multiple choice report?
Moving to In Discussion
Comment 35 Nick Clemens 2024-01-30 12:53:16 UTC
Created attachment 161633 [details] [review]
Bug 34886: Adjust holdability checks on opac details page

This patch tries to simplify some of the logic here to match that on the search results. When we don't have a patron, we fallback to determining if an item can be held buy determining whether there are any items that don't have holds disallowed at the all libraries level. We also remove items with non-holdable statuses like withdrawn etc (and check some system preferences)

If we don't have a patron, then we are done, however, if we do, then we need to check each item against the policies related to that patron.

This patch also removes two checks at the end:
CountItemsIssued($biblionumber)
$biblio->has_items_waiting_or_intransit

These seem to be from bug 4319 - however, those rules are checked by IsAvailableForItemLevelRequest and are only relevant when we have a patron. These checks essentially assumed 'onshelfholds' policy of 'If any unavailable' For consistency sake I think we should follow the same logic as the results page.

To test:
1 - Find a record with two items, of different types, set a 'Default checkout, hold and return policy' of 'No holds allowed'
2 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
3 - Delete that rule, make both items withdrawn
4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
5 - Mark one item as not withdrawn
6 - Search opac, not logged in, and verify both the results page or details page shows the place hold button
7 - Log in to opac
8 - Search opac, logged in, and verify both the results page or details page shows the place hold button
9 - Place an 'On shelf holds policy' rule for that patron category of 'If any unavailable'
10 - Search opac, logged in, and verify the results and details page shows the place hold button
11 - Set the other item to not withdrawn
12 - Search opac, logged in, and verify the results page shows the place hold button, but details does not
13 - Try various other scenarios - details page should be more correct, results page is always an approximation

Signed-off-by: David Nind <david@davidnind.com>
Comment 36 Nick Clemens 2024-01-30 12:53:19 UTC
Created attachment 161634 [details] [review]
Bug 34886: Adjust other opac detail scripts

Same chanegs as before, but for MARC and ISBD details pages

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 Pedro Amorim 2024-02-09 16:20:30 UTC
Created attachment 161981 [details] [review]
Bug 34886: Add selenium tests

Tests added to cover different use cases and combinations of circulation rules values for authenticated and unauthenticated users
At the moment, 2 tests are failing, documented on the [DO NOT PUSH] commit.
These 2 failing tests fail for the search results page but pass on the detail page counterpart. Ideally they should match, for consistency sake.
But this may be the use case "details page should be more correct, results page is always an approximation" mentioned by Nick.

More test combinations may be added in the future.
Comment 38 Pedro Amorim 2024-02-09 16:20:34 UTC
Created attachment 161982 [details] [review]
Bug 34886: [DO NOT PUSH] use case
Comment 39 Pedro Amorim 2024-02-09 16:26:09 UTC
Hey,

I've submitted a tests patch to cover the use cases mentioned (authenticated vs unauthenticated, search results page vs detail page, and so on.

@Nick can you please take a look at the tests and make sure they're valid, I'm not sure about the onshelfholds = "If any unavaiable" and onshelfholds = "If all unavailable" functionality so I went with my current understanding of it.

There are 2 tests currently failing - documented in the commit message - , they shed light on an obvious inconsistency between the detail page vs search results page, but if others are okay with this inconsistency for performance sake, then so be it.
Comment 40 Katrin Fischer 2024-03-19 08:30:49 UTC
We have just been hit by this one in production, could we get QA please?
Comment 41 Nick Clemens 2024-03-19 17:46:45 UTC
I think Pedro's tests all make sense, and I agree we should preserve the two inconsistent cases (on this bug at least) and possibly improve in the future

If Pedro agrees and updates that and other QA issues I think we can call it PQA if both of us attach our lines

- remove POD from tests file routines
- add exec flag to tests
Comment 42 Marcel de Rooy 2024-03-22 10:24:31 UTC
Created attachment 163674 [details] [review]
Bug 34886: (QA follow-up) chmod, remove POD
Comment 43 Marcel de Rooy 2024-03-22 10:24:53 UTC
Looking here
Comment 44 Marcel de Rooy 2024-03-22 10:42:48 UTC
On 34886 we did:
-        $can_item_be_reserved = $can_item_be_reserved || IsAvailableForItemLevelRequest($item, $patron, undef);
+        $can_item_be_reserved = $can_item_be_reserved || $patron && IsAvailableForItemLevelRequest($item, $patron, undef);

And there was noted that this causes the regression repeated here in description? Could you explain that? This is just a very logical check before calling IsAvailableForItemLevelRequest? Which you should not for anonymous..

Note that this test
$can_item_be_reserved || CountItemsIssued($biblionumber) || $biblio->has_items_waiting_or_intransit
determines the Place Hold button visibility. And without a patron it will always be just an educated guess if some patron can reserve or not. This test was not adjusted on 34694?
Comment 45 Marcel de Rooy 2024-03-22 10:48:48 UTC
Now we remove it:
-if( $can_item_be_reserved || CountItemsIssued($biblionumber) || $biblio->has_items_waiting_or_intransit ) {
-    $template->param( ReservableItems => 1 );
-}

And actually replace it by $holdable_items for anonymous. This is just another guess. What makes it better than what we had ?
Comment 46 Nick Clemens 2024-03-22 11:05:01 UTC
(In reply to Marcel de Rooy from comment #44)
> On 34886 we did:
> -        $can_item_be_reserved = $can_item_be_reserved ||
> IsAvailableForItemLevelRequest($item, $patron, undef);
> +        $can_item_be_reserved = $can_item_be_reserved || $patron &&
> IsAvailableForItemLevelRequest($item, $patron, undef);
> 
> And there was noted that this causes the regression repeated here in
> description? Could you explain that? This is just a very logical check
> before calling IsAvailableForItemLevelRequest? Which you should not for
> anonymous..

Go back to 30486 - then we only called:
Koha::CirculationRules->get_onshelfholds_policy({ item => $item, patron => $patron } )

Which could operate with or without a patron, so sometimes returned 1

We changed it to:
IsAvailableForItemLevelRequest

which sometimes requires a patron or crashes, so we then only called it if we had a patron, which meant when not logged in $can_item_be_reserved was always set to 0



(In reply to Marcel de Rooy from comment #45)
> Now we remove it:
> -if( $can_item_be_reserved || CountItemsIssued($biblionumber) ||
> $biblio->has_items_waiting_or_intransit ) {
> -    $template->param( ReservableItems => 1 );
> -}
> 
> And actually replace it by $holdable_items for anonymous. This is just
> another guess. What makes it better than what we had ?

Because currently - without a patron, we are never setting can_item_be_reserved
to 1 - it is always 0
CountItemsIssued  is a meaningless check on its own without checking on-shelf hold policies
Same for has_items_waiting_or_intransit

Read the code comments for what the checks do now, I think they make more sense
Comment 47 Marcel de Rooy 2024-03-29 07:43:29 UTC
Created attachment 164106 [details] [review]
Bug 34886: Adjust holdability checks on opac details page

This patch tries to simplify some of the logic here to match that on the search results. When we don't have a patron, we fallback to determining if an item can be held buy determining whether there are any items that don't have holds disallowed at the all libraries level. We also remove items with non-holdable statuses like withdrawn etc (and check some system preferences)

If we don't have a patron, then we are done, however, if we do, then we need to check each item against the policies related to that patron.

This patch also removes two checks at the end:
CountItemsIssued($biblionumber)
$biblio->has_items_waiting_or_intransit

These seem to be from bug 4319 - however, those rules are checked by IsAvailableForItemLevelRequest and are only relevant when we have a patron. These checks essentially assumed 'onshelfholds' policy of 'If any unavailable' For consistency sake I think we should follow the same logic as the results page.

To test:
1 - Find a record with two items, of different types, set a 'Default checkout, hold and return policy' of 'No holds allowed'
2 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
3 - Delete that rule, make both items withdrawn
4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button
5 - Mark one item as not withdrawn
6 - Search opac, not logged in, and verify both the results page or details page shows the place hold button
7 - Log in to opac
8 - Search opac, logged in, and verify both the results page or details page shows the place hold button
9 - Place an 'On shelf holds policy' rule for that patron category of 'If any unavailable'
10 - Search opac, logged in, and verify the results and details page shows the place hold button
11 - Set the other item to not withdrawn
12 - Search opac, logged in, and verify the results page shows the place hold button, but details does not
13 - Try various other scenarios - details page should be more correct, results page is always an approximation

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Marcel de Rooy 2024-03-29 07:43:32 UTC
Created attachment 164107 [details] [review]
Bug 34886: Adjust other opac detail scripts

Same chanegs as before, but for MARC and ISBD details pages

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2024-03-29 07:43:36 UTC
Created attachment 164108 [details] [review]
Bug 34886: Add selenium tests

Tests added to cover different use cases and combinations of circulation rules values for authenticated and unauthenticated users
At the moment, 2 tests are failing, documented on the [DO NOT PUSH] commit.
These 2 failing tests fail for the search results page but pass on the detail page counterpart. Ideally they should match, for consistency sake.
But this may be the use case "details page should be more correct, results page is always an approximation" mentioned by Nick.

More test combinations may be added in the future.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2024-03-29 07:43:39 UTC
Created attachment 164109 [details] [review]
Bug 34886: [DO NOT PUSH] use case

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2024-03-29 07:43:43 UTC
Created attachment 164110 [details] [review]
Bug 34886: (QA follow-up) chmod, remove POD

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Marcel de Rooy 2024-03-29 07:44:17 UTC
(In reply to Nick Clemens from comment #46)
Thx for clarifying.
Passing QA now.
Comment 53 Pedro Amorim 2024-04-01 08:27:21 UTC Comment hidden (obsolete)
Comment 54 Pedro Amorim 2024-04-01 08:27:54 UTC Comment hidden (obsolete)
Comment 55 Pedro Amorim 2024-04-01 08:29:33 UTC
Created attachment 164161 [details] [review]
Bug 34886: Comment failing tests

These tests highlight the fact that the 'place hold' button visibility in the
search results page **does not** match the 'place hold' button visibility in
the detail page, given the same conditions.
Since that this is a known behavior, these tests should be commented out as they
are failing by design.

prove t/db_dependent/selenium/opac_holds.t
Comment 56 Katrin Fischer 2024-04-02 15:24:21 UTC
Really glad to see this fixed!
Comment 57 Katrin Fischer 2024-04-02 16:00:47 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 58 Fridolin Somers 2024-04-17 15:10:26 UTC
Pushed to 23.11.x for 23.11.05
Comment 59 Katrin Fischer 2024-04-25 15:09:19 UTC
We have quite a bit of complaints about this one in 22.11. Please backport!