Bug 11338 - items that are captured for holds can be deleted without warning
Summary: items that are captured for holds can be deleted without warning
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-04 16:03 UTC by Fridolin Somers
Modified: 2015-06-04 23:23 UTC (History)
2 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:


Attachments
Bug 11338 - wrong SQL in DelItemCheck (2.22 KB, patch)
2013-12-04 16:12 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 11338 - wrong SQL in DelItemCheck (2.33 KB, patch)
2013-12-22 21:43 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 11338 - wrong SQL in DelItemCheck (2.50 KB, patch)
2013-12-23 19:17 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.