Bug 34886

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.

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.

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.

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>

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.

We should also fix Bug 34836 right ?

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.

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>

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>

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.

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"

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 :)

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>

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>

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>

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.

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.

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>

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>

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>

Rebased on top of bug 34836

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

Created attachment 156327 [details] [review]
Bug 34886: [alternate] adjust other opac detail scripts

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

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 :-)

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.

(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.

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

Created attachment 156981 [details] [review]
Bug 34886: [alternate] adjust other opac detail scripts

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

(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

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>

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>

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.

Pedro, Nick, which version do we keep? If Nick's, Pedro can you QA it please (and change assignee)?

Multiple choice report?
Moving to In Discussion

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>

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>

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.

Created attachment 161982 [details] [review]
Bug 34886: [DO NOT PUSH] use case

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.

We have just been hit by this one in production, could we get QA please?

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

Created attachment 163674 [details] [review]
Bug 34886: (QA follow-up) chmod, remove POD

Looking here

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?

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 ?

(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

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>

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>

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>

Created attachment 164109 [details] [review]
Bug 34886: [DO NOT PUSH] use case

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 164110 [details] [review]
Bug 34886: (QA follow-up) chmod, remove POD

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(In reply to Nick Clemens from comment #46)
Thx for clarifying.
Passing QA now.

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

Really glad to see this fixed!

Pushed for 24.05!

Well done everyone, thank you!

Pushed to 23.11.x for 23.11.05

We have quite a bit of complaints about this one in 22.11. Please backport!

Backported to 23.05.x for upcoming 23.05.12.

Please consider for 22.11 as this was a regression from previous versions.

Hi there,

Katrin is right. It's a regression to from previous versions. No one is happy about that. 
We (and many other libraries) get currently updated on 22.11. 
So it would be worth it ;)