Bug 12583

Summary: DelItem takes $dbh in parameter
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: kyle, mathsabypro, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12555    
Attachments: Bug 12583: DelItem prototype - Remove $dbh
Bug 12583: DelItem prototype - prefer hashref
Bug 12583: DelItem prototype - Add unit tests
Bug 12583: DelItem prototype - Remove $dbh
Bug 12583: DelItem prototype - prefer hashref
Bug 12583: DelItem prototype - Add unit tests
[PASSED QA] Bug 12583: DelItem prototype - Remove $dbh
[PASSED QA] Bug 12583: DelItem prototype - prefer hashref
[PASSED QA] Bug 12583: DelItem prototype - Add unit tests

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