Bug 21184

Summary: C4::Items - Remove GetBarcodeFromItemnumber
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: josef.moravec, m.de.rooy, martin.renvoize, nick
Version: unspecified   
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:
Bug Depends on: 21183    
Bug Blocks: 18252, 21201, 21206, 21471    
Attachments: Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls
Bug 21184: Remove GetBarcodeFromItemnumber
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls
Bug 21184: Remove GetBarcodeFromItemnumber
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls
Bug 21184: Remove GetBarcodeFromItemnumber
Bug 21184: (follow-up) Fix adding barcode to waiting hold item in SIP
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls
Bug 21184: Remove GetBarcodeFromItemnumber
Bug 21184: (follow-up) Fix adding barcode to waiting hold item in SIP

Description Jonathan Druart 2018-08-08 18:32:04 UTC
my $barcode = GetBarcodeFromItemnumber($itemnumber);

will become:

my $barcode = Koha::Items->find($itemnumber)->barcode;
Comment 1 Jonathan Druart 2018-08-08 18:56:29 UTC
Created attachment 77589 [details] [review]
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls

Those calls to C4::Items::GetBarcodeFromItemnumber can be replaced with
    my $barcode = Koha::Items->find($itemnumber)->barcode;
But if we are not sure that the item exists, we should test the return
of ->find before ->barcode

Test plan:
- Edit an item
- Check an item in

- Test SIP - I do not really know how to trigger that code, apparently
misc/sip_cli_emulator.pl does not deal with holds. Any ideas?
Comment 2 Jonathan Druart 2018-08-08 18:56:33 UTC
Created attachment 77590 [details] [review]
Bug 21184: Remove GetBarcodeFromItemnumber

Test plan:
  git grep GetBarcodeFromItemnumber
should not return anything
Comment 3 Josef Moravec 2018-08-16 16:05:11 UTC
Comment on attachment 77589 [details] [review]
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls

Review of attachment 77589 [details] [review]:
-----------------------------------------------------------------

::: C4/SIP/ILS/Patron.pm
@@ +316,5 @@
>      my $self = shift;
>      my $item_arr = $self->x_items('hold_items', @_);
>      foreach my $item (@{$item_arr}) {
> +        my $item = Koha::Items->find($item->{itemnumber});
> +        $item->{barcode} = $item ? $item->barcode : undef;

I think there is too many variables called $item ;)
Comment 4 Jonathan Druart 2018-08-16 19:45:48 UTC
Created attachment 77955 [details] [review]
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls

Those calls to C4::Items::GetBarcodeFromItemnumber can be replaced with
    my $barcode = Koha::Items->find($itemnumber)->barcode;
But if we are not sure that the item exists, we should test the return
of ->find before ->barcode

Test plan:
- Edit an item
- Check an item in

- Test SIP - I do not really know how to trigger that code, apparently
misc/sip_cli_emulator.pl does not deal with holds. Any ideas?
Comment 5 Jonathan Druart 2018-08-16 19:45:52 UTC
Created attachment 77956 [details] [review]
Bug 21184: Remove GetBarcodeFromItemnumber

Test plan:
  git grep GetBarcodeFromItemnumber
should not return anything
Comment 6 Jonathan Druart 2018-08-16 19:46:48 UTC
(In reply to Josef Moravec from comment #3)
> Comment on attachment 77589 [details] [review] [review]
> I think there is too many variables called $item ;)

Haha yes, good catch :)
Comment 7 Josef Moravec 2018-08-17 05:36:01 UTC
You could test this using something like this:

misc/sip_cli_emulator.pl -a localhost -p 6001 -su <login> -sp <passsword> -l <library_code> --patron <borrowernumber> -s "Y         " -m patron_information
Comment 8 Josef Moravec 2018-08-17 05:36:38 UTC
Created attachment 77961 [details] [review]
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls

Those calls to C4::Items::GetBarcodeFromItemnumber can be replaced with
    my $barcode = Koha::Items->find($itemnumber)->barcode;
But if we are not sure that the item exists, we should test the return
of ->find before ->barcode

Test plan:
- Edit an item
- Check an item in

- Test SIP - I do not really know how to trigger that code, apparently
misc/sip_cli_emulator.pl does not deal with holds. Any ideas?

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 9 Josef Moravec 2018-08-17 05:36:43 UTC
Created attachment 77962 [details] [review]
Bug 21184: Remove GetBarcodeFromItemnumber

Test plan:
  git grep GetBarcodeFromItemnumber
should not return anything

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2018-08-17 05:36:49 UTC
Created attachment 77963 [details] [review]
Bug 21184: (follow-up) Fix adding barcode to waiting hold item in SIP

Test plan:
1) Set up your SIP: https://wiki.koha-community.org/wiki/Koha_SIP2_server_setup
2) Run:
misc/sip_cli_emulator.pl -a localhost -p 6001 -su <login> -sp <passsword> -l <library_code> --patron <borrowernumber> -s "Y         " -m patron_information

the -s parameter says what kind of additional information you want to see

3) Check the response: if the requested patron has waiting hold, there
is field "AS" with the barcode of waiting item

The right response looks like this:
64              00120180817
052434000100000000000000000000AOCPL|AA1|AEKoha Admin|BLY|CC5|AS39999000004410|PCS|PIY|AFGreetings from Koha. |

More reading ;)
http://multimedia.3m.com/mws/media/355361O/sip2-protocol.pdf

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Marcel de Rooy 2018-08-24 08:10:11 UTC
QA: Looking here
Comment 12 Marcel de Rooy 2018-08-24 08:16:55 UTC
Created attachment 78111 [details] [review]
Bug 21184: Replace C4::Items::GetBarcodeFromItemnumber calls

Those calls to C4::Items::GetBarcodeFromItemnumber can be replaced with
    my $barcode = Koha::Items->find($itemnumber)->barcode;
But if we are not sure that the item exists, we should test the return
of ->find before ->barcode

Test plan:
- Edit an item
- Check an item in

- Test SIP - I do not really know how to trigger that code, apparently
misc/sip_cli_emulator.pl does not deal with holds. Any ideas?

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2018-08-24 08:16:59 UTC
Created attachment 78112 [details] [review]
Bug 21184: Remove GetBarcodeFromItemnumber

Test plan:
  git grep GetBarcodeFromItemnumber
should not return anything

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2018-08-24 08:17:04 UTC
Created attachment 78113 [details] [review]
Bug 21184: (follow-up) Fix adding barcode to waiting hold item in SIP

Test plan:
1) Set up your SIP: https://wiki.koha-community.org/wiki/Koha_SIP2_server_setup
2) Run:
misc/sip_cli_emulator.pl -a localhost -p 6001 -su <login> -sp <passsword> -l <library_code> --patron <borrowernumber> -s "Y         " -m patron_information

the -s parameter says what kind of additional information you want to see

3) Check the response: if the requested patron has waiting hold, there
is field "AS" with the barcode of waiting item

The right response looks like this:
64              00120180817
052434000100000000000000000000AOCPL|AA1|AEKoha Admin|BLY|CC5|AS39999000004410|PCS|PIY|AFGreetings from Koha. |

More reading ;)
http://multimedia.3m.com/mws/media/355361O/sip2-protocol.pdf

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Nick Clemens 2018-08-31 12:59:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-09-05 14:33:12 UTC
Enhancement, will not be backported to 18.05.x series.