Bug 31395 - Checking in non-existent barcodes makes Koha explode
Summary: Checking in non-existent barcodes makes Koha explode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 22456
Blocks:
  Show dependency treegraph
 
Reported: 2022-08-18 15:06 UTC by Lucas Gass
Modified: 2023-06-08 22:26 UTC (History)
4 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:
22.11.00


Attachments
Bug 31395: Only try to cancel holds if item found (1.75 KB, patch)
2022-08-18 17:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31395: Only try to cancel holds if item found (1.80 KB, patch)
2022-08-18 19:55 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 31395: Only try to cancel holds if item found (1.86 KB, patch)
2022-08-19 11:56 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2022-08-18 15:06:26 UTC
To recreate:

-Checkin a barcode that doesn't exist. 
-Boom, explosion. 


Can't call method "id" on an undefined value at /kohadevbox/koha/circ/returns.pl line 351
Comment 1 Tomás Cohen Arazi 2022-08-18 17:19:39 UTC
Created attachment 139407 [details] [review]
Bug 31395: Only try to cancel holds if item found

This patch makes the code dealing with waiting holds with cancellation
requests be dependent on the fact an item has been found.

The returns.pl controller is a bit messy as the real return takes place
outside the main `if ($item)` block. This should be refactored and
probably run inside a transaction...

In the meantime this patch will make the job.

To test:
1. Try to return an invalid barcode (e.g. ASDQWE)
=> FAIL: Things explode
2. Apply this patch
3. Repeat 1
=> SUCCESS: Doesn't explode!
4. Verify that returning an item with a waiting hold with cancellation
   requests still cancells the hold.
=> SUCCESS: It does!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 ByWater Sandboxes 2022-08-18 19:55:29 UTC
Created attachment 139417 [details] [review]
Bug 31395: Only try to cancel holds if item found

This patch makes the code dealing with waiting holds with cancellation
requests be dependent on the fact an item has been found.

The returns.pl controller is a bit messy as the real return takes place
outside the main `if ($item)` block. This should be refactored and
probably run inside a transaction...

In the meantime this patch will make the job.

To test:
1. Try to return an invalid barcode (e.g. ASDQWE)
=> FAIL: Things explode
2. Apply this patch
3. Repeat 1
=> SUCCESS: Doesn't explode!
4. Verify that returning an item with a waiting hold with cancellation
   requests still cancells the hold.
=> SUCCESS: It does!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 3 Kyle M Hall 2022-08-19 11:56:46 UTC
Created attachment 139485 [details] [review]
Bug 31395: Only try to cancel holds if item found

This patch makes the code dealing with waiting holds with cancellation
requests be dependent on the fact an item has been found.

The returns.pl controller is a bit messy as the real return takes place
outside the main `if ($item)` block. This should be refactored and
probably run inside a transaction...

In the meantime this patch will make the job.

To test:
1. Try to return an invalid barcode (e.g. ASDQWE)
=> FAIL: Things explode
2. Apply this patch
3. Repeat 1
=> SUCCESS: Doesn't explode!
4. Verify that returning an item with a waiting hold with cancellation
   requests still cancells the hold.
=> SUCCESS: It does!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Tomás Cohen Arazi 2022-08-19 12:58:16 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 5 Lucas Gass 2022-10-14 19:20:13 UTC
Missing the dependency for 22.05.x, no backport