Bug 12583 - DelItem takes $dbh in parameter
Summary: DelItem takes $dbh in parameter
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12555
  Show dependency treegraph
 
Reported: 2014-07-16 11:50 UTC by Jonathan Druart
Modified: 2015-06-04 23:33 UTC (History)
3 users (show)

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


Attachments
Bug 12583: DelItem prototype - Remove $dbh (2.96 KB, patch)
2014-07-16 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12583: DelItem prototype - prefer hashref (2.84 KB, patch)
2014-07-16 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12583: DelItem prototype - Add unit tests (4.06 KB, patch)
2014-07-16 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12583: DelItem prototype - Remove $dbh (3.01 KB, patch)
2014-07-25 00:43 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12583: DelItem prototype - prefer hashref (2.89 KB, patch)
2014-07-25 00:43 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12583: DelItem prototype - Add unit tests (4.11 KB, patch)
2014-07-25 00:44 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 12583: DelItem prototype - Remove $dbh (3.09 KB, patch)
2014-07-25 14:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12583: DelItem prototype - prefer hashref (2.96 KB, patch)
2014-07-25 14:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12583: DelItem prototype - Add unit tests (4.18 KB, patch)
2014-07-25 14:03 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 Jonathan Druart 2014-07-16 11:50:04 UTC
Since C4::Context->dbh shares the DB handler, it's useless to pass it to routines.

On the way, It would be great to pass the itemnumber for removing an item (the biblionumber should be optional).
Comment 1 Jonathan Druart 2014-07-16 12:09:04 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-07-16 12:09:08 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-07-16 12:09:11 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2014-07-25 00:43:42 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2014-07-25 00:43:55 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2014-07-25 00:44:07 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2014-07-25 14:02:47 UTC
Created attachment 30110 [details] [review]
[PASSED QA] Bug 12583: DelItem prototype - Remove $dbh

Since C4::Context->dbh shares the DB handler, it's useless to pass it to
routines.

Test plan:
Try to remove an item from the Koha interface.
Verify that unit tests pass.

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2014-07-25 14:03:04 UTC
Created attachment 30111 [details] [review]
[PASSED QA] Bug 12583: DelItem prototype - prefer hashref

To delete an item, only the itemnumber should be mandatory. The DelItem
routine can retrieve the biblionumber from the itemnumber.

Test plan:
Verify that t/db_dependent/Items/DelItem.t passes

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall 2014-07-25 14:03:07 UTC
Created attachment 30112 [details] [review]
[PASSED QA] Bug 12583: DelItem prototype - Add unit tests

DelItem should return 1 if the item has been deleted, otherwise 0.

Test plan:
Verify that t/db_dependent/Items/DelItem.t returns green

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2014-07-25 14:03:43 UTC
Looks great! We should do this for DelItemCheck as well.
Comment 11 Tomás Cohen Arazi 2014-07-26 13:23:25 UTC
Patches pushed to master.

Thanks Jonathan!

P.S. I renamed the unit tests file. I'll propose a discussion on naming schemas and arrangement.
Comment 12 Mathieu Saby 2014-08-06 10:07:52 UTC
See also  Bug 10869, in which I first removed the useless parameter in DelOrder as did Jonathan, but changed my mind after a comment by M de Rooy.
"Just a *dumb* question: Why remove the biblio parameter and look it up in the database if you already had it in the first place?"

Mathieu Saby
Comment 13 Mathieu Saby 2014-08-08 12:06:34 UTC
Sorry, I'm confused. Your patch was about biblionumber beeing passed to DelItem, mine was about biblionumber beeing passed to DelOrder...

Mathieu