Bug 34694 - OPAC bib record blows up with error 500
Summary: OPAC bib record blows up with error 500
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Pedro Amorim
QA Contact: Testopia
URL:
Keywords: release-notes-needed, rel_22_11_candidate, rel_23_05_candidate
Depends on: 30846
Blocks: 34836 34886
  Show dependency treegraph
 
Reported: 2023-09-01 16:04 UTC by Pedro Amorim
Modified: 2023-10-12 16:00 UTC (History)
13 users (show)

See Also:
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


Attachments
Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated (1.24 KB, patch)
2023-09-01 16:13 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated (1.29 KB, patch)
2023-09-01 17:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34694: Only check for IsAvailableForItemLevelRequest if is authenticated (1.34 KB, patch)
2023-09-01 18:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-09-01 16:04:37 UTC
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.
Comment 1 Pedro Amorim 2023-09-01 16:13:16 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
Comment 2 Owen Leonard 2023-09-01 17:28:03 UTC
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>
Comment 3 Tomás Cohen Arazi 2023-09-01 18:49:37 UTC
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>
Comment 4 Tomás Cohen Arazi 2023-09-05 14:53:55 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 5 Fridolin Somers 2023-09-08 06:29:51 UTC
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.
Comment 6 Emily Lamancusa 2023-09-11 16:24:00 UTC
(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.
Comment 7 Aleisha Amohia 2023-09-14 01:27:38 UTC
(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 :)
Comment 8 Aleisha Amohia 2023-09-14 01:29:49 UTC
(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
Comment 9 Tomás Cohen Arazi 2023-09-19 18:54:09 UTC
(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!
Comment 10 Fridolin Somers 2023-09-20 07:46:19 UTC
(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.
Comment 11 Fridolin Somers 2023-09-20 07:51:30 UTC
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
Comment 12 Fridolin Somers 2023-09-20 07:54:15 UTC
Pushed to 23.05.x for 23.05.04
Comment 13 Martin Renvoize 2023-09-22 12:46:13 UTC
This causes a regression actually.. you can no longer start a reservation prior to login.
Comment 14 Katrin Fischer 2023-10-11 06:58:11 UTC
(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.
Comment 15 Magnus Enger 2023-10-11 12:55:38 UTC
The bug is present in 22.11.10 and I can confirm this patch fixes it.
Comment 16 Matt Blenkinsop 2023-10-12 16:00:56 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x