https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Regression in when |Regression in when |reservation button appears |holdbutton appears -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #5 from 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #6 from Fridolin Somers <fridolin.somers@biblibre.com> --- We should also fix Bug 34836 right ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156150|0 |1 is obsolete| | --- Comment #9 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 156151 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156151&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #11 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 156153 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156153&action=edit 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" -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|martin.renvoize@ptfs-europe |pedro.amorim@ptfs-europe.co |.com |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156151|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156277&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156153|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 156279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156277|0 |1 is obsolete| | --- Comment #18 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 156293 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156293&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156279|0 |1 is obsolete| | --- Comment #20 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 156295 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156295&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 156326 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156326&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #24 from Nick Clemens <nick@bywatersolutions.com> --- 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 :-) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|katrin.fischer@bsz-bw.de |testopia@bugs.koha-communit | |y.org -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #25 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156326|0 |1 is obsolete| | Attachment #156327|0 |1 is obsolete| | --- Comment #27 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 156980 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=156980&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #29 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Eric Phetteplace <ephetteplace@cca.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ephetteplace@cca.edu -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156981|0 |1 is obsolete| | --- Comment #31 from David Nind <david@davidnind.com> --- Created attachment 159679 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159679&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #33 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Pedro, Nick, which version do we keep? If Nick's, Pedro can you QA it please (and change assignee)? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|pedro.amorim@ptfs-europe.co |nick@bywatersolutions.com |m | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156293|0 |1 is obsolete| | Attachment #156294|0 |1 is obsolete| | Attachment #156295|0 |1 is obsolete| | Attachment #159678|0 |1 is obsolete| | Attachment #159679|0 |1 is obsolete| | --- Comment #35 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 161633 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161633&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156293|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156295|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #156294|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #37 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 161981 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=161981&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #39 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #40 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- We have just been hit by this one in production, could we get QA please? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #42 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 163674 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163674&action=edit Bug 34886: (QA follow-up) chmod, remove POD -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #46 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161633|0 |1 is obsolete| | --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 164106 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164106&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #161981|0 |1 is obsolete| | --- Comment #49 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 164108 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164108&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163674|0 |1 is obsolete| | --- Comment #51 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 164110 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164110&action=edit Bug 34886: (QA follow-up) chmod, remove POD Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #53 from Pedro Amorim <pedro.amorim@ptfs-europe.com> --- Created attachment 164159 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164159&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Pedro Amorim <pedro.amorim@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164109|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #56 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Really glad to see this fixed! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #57 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #59 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- We have quite a bit of complaints about this one in 22.11. Please backport! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.05 |24.05.00,23.11.05,23.05.12 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 Ewa Gózd <ewa.gozd@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ewa.gozd@gmail.com --- Comment #62 from Ewa Gózd <ewa.gozd@gmail.com> --- 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 ;) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 --- Comment #63 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- When testing on 22.11, the following is not true: 4 - Search opac, not logged in, and verify neither the results page or details page shows the place hold button I am still able to see the place hold button -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34886 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Keywords|rel_22_11_candidate | Text to go in the| |This fixes a regression release notes| |when placing holds in the | |OPAC - the "Place hold" | |button for a record didn't | |appear unless you were | |logged in. Previously, you | |could place a hold and | |would then be prompted to | |log in to complete the | |holds process. Status|Needs documenting |RESOLVED --- Comment #65 from David Nind <david@davidnind.com> --- Bug fix, no updates required to the manual as far as I can see (for the page about the OPAC). -- You are receiving this mail because: You are watching all bug changes.