Bug 23015 - Create a "GetPickupLocation" endpoint for the ILS-DI Service
Summary: Create a "GetPickupLocation" endpoint for the ILS-DI Service
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P4 enhancement (vote)
Assignee: Arthur Suzuki
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-29 20:20 UTC by Arthur Suzuki
Modified: 2019-10-12 21:09 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23015 : Implements a new ILS-DI service to get available pickup libraries for an item or record (7.27 KB, patch)
2019-05-29 23:25 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23015 : Implements a new ILS-DI service to get available pickup libraries for an item or record (7.36 KB, patch)
2019-10-02 12:56 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 23015: Implements a new ILS-DI service to get available pickup libraries for an item or record (7.37 KB, patch)
2019-10-12 16:17 UTC, Arthur Suzuki
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Arthur Suzuki 2019-05-29 20:20:46 UTC
That endpoint may take 2 parameters:
 - a borrowernumber 
 - and a biblio or item number

And it would return a list of available pickup location given a user and a biblio or item.

On success would return the following example :

<?xml version="1.0" encoding="UTF-8"?>
            <pickup_locations>
              <pickup_location>
                <pickup_location_label>First library</pickup_location_label>
                <pickup_location_id>1</pickup_location_id>
              </pickup_location>
              <pickup_location>
                <pickup_location_label>Second library</pickup_location_label>
		<pickup_location_id>2</pickup_location_id>
              </pickup_location>
            </pickup_locations>

On error :
 - PatronNotFound in case borrowernumber is not found
 - RecordNotFound in case Biblio doesn't exist
 - ItemNotFound in case item doesn't exist
Comment 1 Arthur Suzuki 2019-05-29 23:25:31 UTC
Created attachment 90195 [details] [review]
Bug 23015 : Implements a new ILS-DI service to get available pickup libraries for an item or record

test plan :
1 / Set a library A as available pickup location in the intranet
2 / query ILS-DI service with any record which has at least an item in
A or an item which can be transfered to A => should send all
informations about A

3 / Remove A from available pickup locations from the item in the intranet then repeat 2 => should
give empty reply

4 / Try with unexisting patron, unexisting item or unexisting record,
should throw an explicit error
Comment 2 Christophe Croullebois 2019-10-02 12:56:16 UTC
Created attachment 93455 [details] [review]
Bug 23015 : Implements a new ILS-DI service to get available pickup libraries for an item or record

test plan :
1 / Set a library A as available pickup location in the intranet
2 / query ILS-DI service with any record which has at least an item in
A or an item which can be transfered to A => should send all
informations about A

3 / Remove A from available pickup locations from the item in the intranet then repeat 2 => should
give empty reply

4 / Try with unexisting patron, unexisting item or unexisting record,
should throw an explicit error

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>
Comment 3 Katrin Fischer 2019-10-12 10:49:02 UTC
Hi Arthur,

just some feedback on formal stuff:

1) Please always start your commit messages with Bug: xxxx (no extra space before the :). I usually fix those, but it gets boring fast :)

2) There is a lot of complaints about whitespace. Please make sure your editor is set to not use tabs but 4 spaces instead. 

 FAIL	C4/ILSDI/Services.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 644)
		forbidden pattern: tab char (line 646)
		forbidden pattern: tab char (line 647)
		forbidden pattern: tab char (line 651)
		forbidden pattern: tab char (line 653)
		forbidden pattern: tab char (line 654)
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   OK	  valid

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/ilsdi.tt
   OK	  filters
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 69)
		forbidden pattern: tab char (line 692)
		forbidden pattern: tab char (line 693)
		forbidden pattern: tab char (line 697)
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

 FAIL	opac/ilsdi.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 94)
		forbidden pattern: tab char (line 112)
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

It's also very good to get into the havig of running the QA test tools, so those can be caught and fixed early!
Comment 4 Katrin Fischer 2019-10-12 10:49:17 UTC
havig = habit!
Comment 5 Arthur Suzuki 2019-10-12 10:53:03 UTC
Thanks for your review, will fix that after lunch :)
Comment 6 Katrin Fischer 2019-10-12 10:54:44 UTC
3) We are missing tests for the change to Service.pm

4) I can't seem to make the documentation show up? Checking the OPAC side documentation here: /cgi-bin/koha/ilsdi.pl
Comment 7 Katrin Fischer 2019-10-12 10:55:42 UTC
Wondering: does this use the same logic that is used in the OPAC to build the pick-up location list?
Comment 8 Arthur Suzuki 2019-10-12 16:17:42 UTC
Created attachment 94039 [details] [review]
Bug 23015: Implements a new ILS-DI service to get available pickup libraries for an item or record

test plan :
1 / Set a library A as available pickup location in the intranet
2 / query ILS-DI service with any record which has at least an item in
A or an item which can be transfered to A => should send all
informations about A

3 / Remove A from available pickup locations from the item in the intranet then repeat 2 => should
give empty reply

4 / Try with unexisting patron, unexisting item or unexisting record,
should throw an explicit error

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>
Comment 9 Arthur Suzuki 2019-10-12 21:09:52 UTC
(In reply to Katrin Fischer from comment #7)
> Wondering: does this use the same logic that is used in the OPAC to build
> the pick-up location list?

Huuuum... Uses Koha::Library->pickup_location.
Opac goes through all libraries which are set as possible pickup location (site configuration) and check if desired book is available for transfer there.
This is basically what does Koha::Library->pickup_location but embed in a nice method.

Uses the same function (Koha::Library->pickup_location(In reply to Katrin Fischer from comment #6)
> 3) We are missing tests for the change to Service.pm

Okay, have to write some then...

> 4) I can't seem to make the documentation show up? Checking the OPAC side
> documentation here: /cgi-bin/koha/ilsdi.pl

Neither can I... although it's been written in the code, have to check what's wrong there.