To reproduce, on k-t-d: - Go to circulation rules and set On shelf holds allowed to If all unavailable - Log out - Visit a bib record on OPAC: http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=76 - Notice it blows up with error 500 Can't call method "id" on an undefined value at /kohadevbox/koha/C4/Reserves.pm line 1385 I believe this was caused by bug 30846. $patron is undefined at that point in the code.
Created attachment 155130 [details] [review] Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated - Go to circulation rules and set On shelf holds allowed to If all unavailable - Log out - Visit a bib record on OPAC: http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=76 - Notice it blows up with error 500 - Apply patch. Repeat
Created attachment 155141 [details] [review] Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated - Go to circulation rules and set On shelf holds allowed to If all unavailable - Log out - Visit a bib record on OPAC: http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=76 - Notice it blows up with error 500 - Apply patch. Repeat Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 155151 [details] [review] Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated - Go to circulation rules and set On shelf holds allowed to If all unavailable - Log out - Visit a bib record on OPAC: http://localhost:8080/cgi-bin/koha/opac-detail.pl?biblionumber=76 - Notice it blows up with error 500 - Apply patch. Repeat Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 23.11. Nice work everyone, thanks!
Ah, Bug 30846 also impacted opac/opac-ISBDdetail.pl and opac/opac-MARCdetail.pl And isn't is better to use 'and' instead of '&&' ? https://perldoc.perl.org/perlop#Logical-And Also even if not mandatory in my opinion we should add parenthesis to explain the logic.
(In reply to Fridolin Somers from comment #5) > Ah, Bug 30846 also impacted opac/opac-ISBDdetail.pl and > opac/opac-MARCdetail.pl > > And isn't is better to use 'and' instead of '&&' ? > https://perldoc.perl.org/perlop#Logical-And > > Also even if not mandatory in my opinion we should add parenthesis to > explain the logic. I think the parentheses WOULD be mandatory if it was switched to 'and', because 'and' has lower precedence than '||'. https://perldoc.perl.org/perlop#Operator-Precedence-and-Associativity Can you explain more about why 'and' would be better in this case? At first I was thinking it was because we want it to short-circuit if $patron is not defined, but looking at the section for '&&', '&&' will also short-circuit, so I'm not clear on what the advantage of 'and' would be.
(In reply to Tomás Cohen Arazi from comment #4) > Pushed to master for 23.11. > > Nice work everyone, thanks! Awesome! Can this please be backported to 23.05.x :)
(In reply to Aleisha Amohia from comment #7) > (In reply to Tomás Cohen Arazi from comment #4) > > Pushed to master for 23.11. > > > > Nice work everyone, thanks! > > Awesome! Can this please be backported to 23.05.x :) Sorry and 22.11! In time for the next release
(In reply to Fridolin Somers from comment #5) > Ah, Bug 30846 also impacted opac/opac-ISBDdetail.pl and > opac/opac-MARCdetail.pl This has got a bit confusing. Can you file a separate bug if you think there's a problem? Thanks!
(In reply to Emily Lamancusa from comment #6) > > Can you explain more about why 'and' would be better in this case? At first > I was thinking it was because we want it to short-circuit if $patron is not > defined, but looking at the section for '&&', '&&' will also short-circuit, > so I'm not clear on what the advantage of 'and' would be. Mmm forget that I've done some tests. Indeed && can be used here.
34836(In reply to Tomás Cohen Arazi from comment #9) > (In reply to Fridolin Somers from comment #5) > > Ah, Bug 30846 also impacted opac/opac-ISBDdetail.pl and > > opac/opac-MARCdetail.pl > > This has got a bit confusing. Can you file a separate bug if you think > there's a problem? Thanks! Opened Bug 34836
Pushed to 23.05.x for 23.05.04
This causes a regression actually.. you can no longer start a reservation prior to login.
(In reply to Martin Renvoize from comment #13) > This causes a regression actually.. you can no longer start a reservation > prior to login. We agree that this is a regression and a big behavior change for a bugfix release. We have worked on 2 alternative solutions: If the user is not logged in Option A: always. When the user clicks on the link they will be asked to log in and will receive feedback on the possibility of the action. Option B: show the link if there are items that are * not of an item type that is set to 'not for loan' in the itemtypes administration * don't have a positive not for loan status The second would be preferred, but I think the first would already be less of a regression than what we see now. Right now you can have a 'place hold' link in the results list, to find it gone in the detail page - that is a bad UX experience.
The bug is present in 22.11.10 and I can confirm this patch fixes it.
Nice work everyone! Pushed to oldstable for 22.11.x