Bug 11338

Summary: items that are captured for holds can be deleted without warning
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: CatalogingAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: gmcharlt, m.de.rooy
Version: Main   
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:
Attachments: Bug 11338 - wrong SQL in DelItemCheck
Bug 11338 - wrong SQL in DelItemCheck
[PASSED QA] Bug 11338 - wrong SQL in DelItemCheck

Description Fridolin Somers 2013-12-04 16:03:55 UTC
In C4::Items::DelItemCheck, there are two SQL queries : one to check if item is on loan, the other if item is reserved.
Those two queries use "SELECT * FROM table", fetch the datas with "$var = $sth->fetchrow", and use "$var" as a boolean condition.
This is not correct, SQL query should be "SELECT COUNT(*) FROM table".

Code actually works, but is dangerous.
Comment 1 Fridolin Somers 2013-12-04 16:12:49 UTC Comment hidden (obsolete)
Comment 2 Fridolin Somers 2013-12-04 16:13:43 UTC
(first time I use git bz, whouuu)
Comment 3 Chris Cormack 2013-12-22 21:43:43 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2013-12-23 19:17:47 UTC
Created attachment 23792 [details] [review]
[PASSED QA] Bug 11338 - wrong SQL in DelItemCheck

In C4::Items::DelItemCheck, there are two SQL queries : one to check if item is on loan, the other if item is reserved.
Those two queries use "SELECT * FROM table", fetch the datas with "$var = $sth->fetchrow", and use "$var" as a boolean condition.
This is not correct, SQL query should be "SELECT COUNT(*) FROM table".
Code actually works, but is dangerous.

This patch corrects the SQL queries and sets my ($var) to show that fetchrow returns an array.

Test plan :
- Set an item A onloan
- Set an item B reserved and the reserve waiting
- Go to items cataloguing : cgi-bin/koha/cataloguing/additem.pl?biblionumber=XXX
- Try to delete item A
=> You get an alert and item is not deleted
- Try to delete item B
=> You get an alert and item is not deleted

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Works, and has the added bonus of being a tiny bit faster.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes t, xt and QA script tests.
Also tried deleting via batch delete - correct warnings are displayed.
Comment 5 Galen Charlton 2013-12-25 17:19:38 UTC
Pushed to master, along with unit tests for DelItemCheck().  Thanks, Fridolin!

Note that I've amended the bug title and the patch description significantly to reflect the fact that this patch corrects a nasty, user-visible bug.  Suffice it to say that DelItemCheck() was *not* working properly prior to this patch, as it permitted deleting items that were captured for hold requests.
Comment 6 Fridolin Somers 2014-01-03 09:11:07 UTC
Patches pushed to 3.14.x, will be in 3.14.2.