Bug 12122

Summary: TransferSlip should accept both itemnumber and barcode
Product: Koha Reporter: Benjamin Rokseth <benjamin.rokseth>
Component: CirculationAssignee: Bugs List <koha-bugs>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: benjamin.rokseth, gmcharlt, katrin.fischer, kyle.m.hall, mtj, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: small patch to allow barcode as input in TransferSlip
[SIGNED-OFF] Bug 12122: TransferSlip should accept both itemnumber and barcode
[PASSED QA] Bug 12122: TransferSlip should accept both itemnumber and barcode
Bug 12122: TransferSlip should accept both itemnumber and barcode
[PASSED QA] Bug 12122: TransferSlip should accept both itemnumber and barcode

Description Benjamin Rokseth 2014-04-22 11:18:51 UTC
C4::Circulation::TransferSlip uses Getitem which takes either itemnumber or barcode. It should therefore accept both itemnumber and barcode as input. 

This would make it easy to generate print slip from barcode instead of doing item lookup (GetItemnumberfromBarcode) first.
Comment 1 Benjamin Rokseth 2014-04-22 12:20:25 UTC Comment hidden (obsolete)
Comment 2 Benjamin Rokseth 2014-04-22 12:20:58 UTC
Added small patch to allow barcode as input in TransferSlip routine, mostly
to allow generating transfer slips where only barcode is present (aka.
javascript).

Test plan:
1) find book with <barcode> and <itemnumber>
2) generate transferslips with both:
  transfer-slip.pl?transferitem=<itemnumber>3967925&amp;branchcode=MPL&amp;op=slip
  transfer-slip.pl?barcode=<barcode>&amp;branchcode=MPL&amp;op=slip
and verify that the generated slips match.
Comment 3 Owen Leonard 2014-04-22 13:04:36 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2014-04-25 12:24:17 UTC Comment hidden (obsolete)
Comment 5 Galen Charlton 2014-04-29 16:23:31 UTC
Changes to routines in C4 and Koha really ought to be accompanied by a test case.

However, of more import in this particular case, I am not seeing that the new barcode parameter to transfer-slip.pl is actually getting used.  Existing templates that link to transfer-slip.pl already have the itemnumber.

Please advise if there is specific code available, or soon to be available, that will actually use the new parameter.

Setting to in discussion.
Comment 6 Benjamin Rokseth 2014-05-04 17:28:07 UTC
Agreed to the neccessity of unit tests in this setting. I can add one when available. 

The reason for this patch is addressing the gap between SIP2 circulation and internal circulation in Koha. The itemnumber is an internal DB id for the item it is not accessible anywhere and not suitable for item lookup. Barcode is an actual ID for an item. 

To explain, we have the following case:

All circulation is handled by RFID and SIP2 transactions by an external RFIDhub (code will be added in Bug 11858 when tested enough).

To minimize Koha code replication/rework we inject by javascript the needed functionality (checkin/checkout/RFID tag write). All transactions then pass the RFIDhub and communication with browser goes via websockets.

As SIP transactions only deals with barcodes as IDs and we need to generate receipts I thought it best to use TransferSlip, better than duplicating the existing templates in Koha.

As GetItem was already used it was easier to add barcode as parameter. An alternative would be to expand the svc or ILS-DI api to allow item lookup.

Sorry for the long explanation, but probably someone else has some ideas to better solutions.
Comment 7 Benjamin Rokseth 2014-05-06 20:35:20 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2014-11-14 12:52:21 UTC
Resetting this to signed off and awaiting QA as the discussion never came and tests are still passing. It was already tagged for release candidate 3.16
Comment 9 Katrin Fischer 2014-11-16 09:20:22 UTC
Hi Benjamin, 

I think if we have a documented use case and unit tests this is ok for me. I think maybe it would be a little nicer to have a webservice for slip printing longer term, so we are even more flexible.
Comment 10 Katrin Fischer 2014-11-16 09:53:13 UTC
I have some concerns about this - there might be cases where we don't have a barcode (itemBarcodeFallbackSearch). I wanted to test, but the feature does not seem to work for me currently - filing a separate bug.
Comment 11 Katrin Fischer 2014-11-16 10:05:26 UTC
Hm, see bug 11599 - missing barcodes are probably not a problem at the moment.
Comment 12 Katrin Fischer 2014-11-16 10:10:29 UTC
Created attachment 33601 [details] [review]
[PASSED QA] Bug 12122: TransferSlip should accept both itemnumber and barcode

Added small patch to allow barcode as input in TransferSlip routine, mostly
to allow generating transfer slips where only barcode is present (aka.
javascript).

Test plan:
1) find book with <barcode> and <itemnumber>
2) generate transferslips with both:
  transfer-slip.pl?transferitem=<itemnumber>3967925&amp;branchcode=MPL&amp;op=slip
  transfer-slip.pl?barcode=<barcode>&amp;branchcode=MPL&amp;op=slip
and verify that the generated slips match.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Edit:
 - Added tests in t/db_dependent/Circulation_transfers.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Passes tests and QA script.
Works with both itemnumber or barcode as described.
Tested printing transfer slips with the URL examples given
and in the UI.
Comment 13 Katrin Fischer 2014-11-16 10:12:07 UTC
> I have some concerns about this - there might be cases where we don't have a
> barcode (itemBarcodeFallbackSearch). I wanted to test, but the feature does
> not seem to work for me currently - filing a separate bug.

Sorry for the initial confusion - all works like it should. My zebrasrv was not running, so itemBarcodeFallbackSearch didn't work at first.
Comment 14 Benjamin Rokseth 2015-02-12 14:51:59 UTC
any status on this bug? it was tagged for 3_16 but never pushed.
It still applies cleanly on master.
Comment 15 Tomás Cohen Arazi 2015-03-05 15:31:44 UTC
Patch pushed to master.

Skåle!
Comment 16 Mason James 2015-04-22 12:37:53 UTC
Pushed to 3.16.x, will be in 3.16.10