Summary: | OPAC bib record blows up with error 500 | ||
---|---|---|---|
Product: | Koha | Reporter: | Pedro Amorim <pedro.amorim> |
Component: | OPAC | Assignee: | Pedro Amorim <pedro.amorim> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | aleisha, clemens.tubach, emily.lamancusa, fridolin.somers, jonathan.druart, lucas, m.de.rooy, magnus, martin.renvoize, michaela.sieber, nick, tomascohen |
Version: | Main | Keywords: | rel_22_11_candidate, rel_23_05_candidate |
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
23.11.00,23.05.04,22.11.11
|
|
Circulation function: | |||
Bug Depends on: | 30846 | ||
Bug Blocks: | 34886, 34836 | ||
Attachments: |
Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated
Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated |
Description
Pedro Amorim
2023-09-01 16:04:37 UTC
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 |