Bug 34609 - Holds history errors 500 if old_reserves.biblionumber is NULL
Summary: Holds history errors 500 if old_reserves.biblionumber is NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P2 major
Assignee: Pedro Amorim
QA Contact: Testopia
URL:
Keywords: rel_22_11_candidate, rel_23_05_candidate
: 34603 (view as bug list)
Depends on: 32894
Blocks:
  Show dependency tree / graph
 
Reported: 2023-08-24 13:52 UTC by Pedro Amorim
Modified: 2023-12-28 20:47 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.10
Circulation function:

Attachments
Bug 34609: Return empty if no biblionumber (1.33 KB, patch)
2023-08-24 13:54 UTC, Pedro Amorim 		Details | Diff | Splinter Review
Bug 34609: Return empty if no biblionumber (1.38 KB, patch)
2023-08-29 13:45 UTC, Pedro Amorim 		Details | Diff | Splinter Review
Bug 34609: Add biblio method to Old::Hold (835 bytes, patch)
2023-08-30 13:13 UTC, Pedro Amorim 		Details | Diff | Splinter Review
Bug 34609: Add tests (1.74 KB, patch)
2023-08-30 13:13 UTC, Pedro Amorim 		Details | Diff | Splinter Review
Bug 34609: Add biblio method to Old::Hold (1.58 KB, patch)
2023-08-30 13:14 UTC, Pedro Amorim 		Details | Diff | Splinter Review
Bug 34609: Add tests (1.81 KB, patch)
2023-08-31 07:10 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 34609: Add biblio method to Old::Hold (1.65 KB, patch)
2023-08-31 07:10 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 34609: Add missing test for Koha::Hold->biblio (1.36 KB, patch)
2023-08-31 07:10 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-08-24 13:52:36 UTC 
To reproduce, clean k-t-d:

1) Add a reserve to koha/koha user, mysql run:
insert into reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, biblionumber) 
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 76);

2) Add an old_reserve to koha/koha user, mysql run:
insert into old_reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, reserve_id) 
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 1);

3) Visit holdshistory on either OPAC (requires OPACHoldsHistory sys pref) or STAFF 
STAFF/cgi-bin/koha/members/holdshistory.pl?borrowernumber=1
OPAC//cgi-bin/koha/opac-holdshistory.pl

4) Notice it blows up with a 500 error
Comment 1 Pedro Amorim 2023-08-24 13:54:37 UTC 
Created attachment 154738 [details] [review]
Bug 34609: Return empty if no biblionumber

Test plan, clean k-t-d:

1) Add a reserve to koha/koha user, mysql run:
insert into reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, biblionumber)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 76);

2) Add an old_reserve to koha/koha user, mysql run:
insert into old_reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, reserve_id)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 1);

3) Visit holdshistory on either OPAC (requires OPACHoldsHistory sys pref) or STAFF
STAFF/cgi-bin/koha/members/holdshistory.pl?borrowernumber=1
OPAC//cgi-bin/koha/opac-holdshistory.pl

4) Notice it blows up with a 500 error
5) Repeat step 3
Comment 2 Lucas Gass (lukeg) 2023-08-25 17:10:12 UTC 
*** Bug 34603 has been marked as a duplicate of this bug. ***
Comment 3 Lucas Gass (lukeg) 2023-08-25 17:11:27 UTC 
Thanks Pedro, this works! Do you think we need unit tests for this change?
Comment 4 Martin Renvoize (ashimema) 2023-08-25 18:02:32 UTC 
There's some prior art for tests for this ﷐[U+1F642]﷑.  I hate writing them as they feel like over repetition... but they would have caught this regression.. so...
Comment 5 Pedro Amorim 2023-08-28 09:02:18 UTC 
(In reply to Lucas Gass from comment #3)
> Thanks Pedro, this works! Do you think we need unit tests for this change?

Thanks Lucas, I had not realized there was already a bug filed for this.

What I think happened is, the biblio method in Hold.pm returned undef if it did not have a biblionumber, but after this patch:
https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=32894&attachment=152086
It started returning an empty DBix::ResultSource(?) instead of undef leading to holdhistory.tt to explode.

A test making sure biblio in Hold.pm returns either a biblio object or undef would certainly been helpful before bug 32894, and certainly does not hurt now either!
Comment 6 Pedro Amorim 2023-08-29 13:45:27 UTC 
Created attachment 154901 [details] [review]
Bug 34609: Return empty if no biblionumber

Test plan, clean k-t-d:

1) Add a reserve to koha/koha user, mysql run:
insert into reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, biblionumber)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 76);

2) Add an old_reserve to koha/koha user, mysql run:
insert into old_reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, reserve_id)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 1);

3) Visit holdshistory on either OPAC (requires OPACHoldsHistory sys pref) or STAFF
STAFF/cgi-bin/koha/members/holdshistory.pl?borrowernumber=1
OPAC//cgi-bin/koha/opac-holdshistory.pl

4) Notice it blows up with a 500 error
5) Repeat step 3

Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk>
Comment 7 Pedro Amorim 2023-08-30 09:33:30 UTC 
Raising this to major given the nature of the issue and attention it's gotten.
Comment 8 Jonathan Druart 2023-08-30 12:33:35 UTC 
No, this must be in Koha::Old::Hold, Koha::Hold reflect the 'reserves' table, and we have a NOT NULL there

`biblionumber` int(11) NOT NULL 

Also, tests are missing.
Comment 9 Pedro Amorim 2023-08-30 13:13:22 UTC 
Created attachment 154952 [details] [review]
Bug 34609: Add biblio method to Old::Hold
Comment 10 Pedro Amorim 2023-08-30 13:13:25 UTC 
Created attachment 154953 [details] [review]
Bug 34609: Add tests
Comment 11 Pedro Amorim 2023-08-30 13:14:21 UTC 
Created attachment 154954 [details] [review]
Bug 34609: Add biblio method to Old::Hold

Test plan, clean k-t-d:

1) Add a reserve to koha/koha user, mysql run:
insert into reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, biblionumber)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 76);

2) Add an old_reserve to koha/koha user, mysql run:
insert into old_reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, reserve_id)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 1);

3) Visit holdshistory on either OPAC (requires OPACHoldsHistory sys pref) or STAFF
STAFF/cgi-bin/koha/members/holdshistory.pl?borrowernumber=1
OPAC//cgi-bin/koha/opac-holdshistory.pl

4) Notice it blows up with a 500 error
5) Repeat step 3

Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk>
Comment 12 Jonathan Druart 2023-08-31 07:10:07 UTC 
Created attachment 154993 [details] [review]
Bug 34609: Add tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 13 Jonathan Druart 2023-08-31 07:10:10 UTC 
Created attachment 154994 [details] [review]
Bug 34609: Add biblio method to Old::Hold

Test plan, clean k-t-d:

1) Add a reserve to koha/koha user, mysql run:
insert into reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, biblionumber)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 76);

2) Add an old_reserve to koha/koha user, mysql run:
insert into old_reserves(borrowernumber, reservedate, branchcode, cancellationdate, timestamp, reserve_id)
VALUES (51, '2022-09-23', 'CPL', '2022-09-23', '2022-09-23 15:46:21', 1);

3) Visit holdshistory on either OPAC (requires OPACHoldsHistory sys pref) or STAFF
STAFF/cgi-bin/koha/members/holdshistory.pl?borrowernumber=1
OPAC//cgi-bin/koha/opac-holdshistory.pl

4) Notice it blows up with a 500 error
5) Repeat step 3

Signed-off-by: Ray Delahunty <r.delahunty@arts.ac.uk>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2023-08-31 07:10:13 UTC 
Created attachment 154995 [details] [review]
Bug 34609: Add missing test for Koha::Hold->biblio

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Tomás Cohen Arazi (tcohen) 2023-09-01 14:10:53 UTC 
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 16 Liz Rea 2023-09-05 14:59:20 UTC 
This can also cause a 500 error when partners go to their Cataloging > Export page. Log will say something like Template process failed: undef error - DBIC result _type isn't of the _type Biblio at /usr/share/koha/intranet/htdocs/intranet-tmpl/prog/en/modules/members/holdshistory.tt line 73
Comment 17 Lucas Gass (lukeg) 2023-09-05 15:10:58 UTC 
(In reply to Liz Rea from comment #16)
> This can also cause a 500 error when partners go to their Cataloging >
> Export page. Log will say something like Template process failed: undef
> error - DBIC result _type isn't of the _type Biblio at
> /usr/share/koha/intranet/htdocs/intranet-tmpl/prog/en/modules/members/
> holdshistory.tt line 73

With this patchset applied that error is also fixed.
Comment 18 Liz Rea 2023-09-07 13:48:21 UTC 
Hi may we have a backport to oldstable and oldoldstable please?
Comment 19 George Williams (NEKLS) 2023-09-07 13:53:24 UTC 
(In reply to Liz Rea from comment #18)
> Hi may we have a backport to oldstable and oldoldstable please?

+1
Comment 20 Rhonda Kuiper 2023-09-07 14:10:44 UTC 
Yes, backport please!
Comment 21 Fridolin Somers 2023-09-08 00:36:21 UTC 
Pushed to 23.05.x for 23.05.04
Comment 22 Matt Blenkinsop 2023-09-08 09:47:57 UTC 
Nice work everyone!

Pushed to oldstable for 22.11.x