Bug 23015 - Create a "GetPickupLocation" endpoint for the ILS-DI Service
Summary: Create a "GetPickupLocation" endpoint for the ILS-DI Service
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
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: 2024-04-04 09:18 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
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
Bug 23015: Implements a new ILS-DI service to get available pickup libraries for an item or record (7.42 KB, patch)
2020-01-04 20:26 UTC, Arthur Suzuki
Details | Diff | Splinter Review
Bug 23015: Adding tests for GetPickupLocation method (2.59 KB, patch)
2020-01-04 20:26 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.
Comment 10 Arthur Suzuki 2020-01-04 20:26:08 UTC
Created attachment 96815 [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 11 Arthur Suzuki 2020-01-04 20:26:12 UTC
Created attachment 96816 [details] [review]
Bug 23015: Adding tests for GetPickupLocation method
Comment 12 David Nind 2020-03-01 07:59:02 UTC
Have tested, but a couple of questions:

1) Details for the service don't display on http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl but can be accessed from http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPickupLocation

2) The result returned doesn't seem to match up with what is shown in the describe text, for example for http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPickupLocation&patron_id=19&id=23&id_type=item I get: 

<GetPickupLocation>
<library>
<anon>
<branchname>Centerville</branchname>
<branchcode>CPL</branchcode>
<branchaddress1>Jefferson Summit</branchaddress1>
<pickup_location>1</pickup_location>
</anon>
<anon>
<pickup_location>1</pickup_location>
<branchaddress1>River Station</branchaddress1>
<branchcode>FFL</branchcode>
<branchname>Fairfield</branchname>
</anon>
<anon>
<pickup_location>1</pickup_location>
<branchaddress1>Hickory Squere</branchaddress1>
<branchcode>FPL</branchcode>
<branchname>Fairview</branchname>.....

Apart from the above it seems to work fine and the tests pass.
Comment 13 David Nind 2020-04-01 07:47:13 UTC
1) Details for the service don't display on http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl but can be accessed from http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPickupLocation

2) The result returned doesn't seem to match up with what is shown in the describe text, for example for http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=GetPickupLocation&patron_id=19&id=23&id_type=item I get: 

<GetPickupLocation>
<library>
<anon>
<branchname>Centerville</branchname>
<branchcode>CPL</branchcode>
<branchaddress1>Jefferson Summit</branchaddress1>
<pickup_location>1</pickup_location>
</anon>
<anon>
<pickup_location>1</pickup_location>
<branchaddress1>River Station</branchaddress1>
<branchcode>FFL</branchcode>
<branchname>Fairfield</branchname>
</anon>
<anon>
<pickup_location>1</pickup_location>
<branchaddress1>Hickory Squere</branchaddress1>
<branchcode>FPL</branchcode>
<branchname>Fairview</branchname>.....

3) Test patch no longer applies:

root@5d9aac93ee4e:koha(bz23015)$ git bz apply 23015
Bug 23015 - Create a "GetPickupLocation" endpoint for the ILS-DI Service

96815 - Bug 23015: Implements a new ILS-DI service to get available pickup libraries for an item or record
96816 - Bug 23015: Adding tests for GetPickupLocation method

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23015: Implements a new ILS-DI service to get available pickup libraries for an item or record
Using index info to reconstruct a base tree...
M	C4/ILSDI/Services.pm
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/ilsdi.tt
M	opac/ilsdi.pl
Falling back to patching base and 3-way merge...
Auto-merging opac/ilsdi.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/ilsdi.tt
Auto-merging C4/ILSDI/Services.pm
Applying: Bug 23015: Adding tests for GetPickupLocation method
Using index info to reconstruct a base tree...
M	t/db_dependent/ILSDI_Services.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/ILSDI_Services.t
CONFLICT (content): Merge conflict in t/db_dependent/ILSDI_Services.t
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23015: Adding tests for GetPickupLocation method
Comment 14 Katrin Fischer 2023-10-08 10:22:45 UTC
Should we abandon this for the REST API routes? We already have routes for items and biblio:

https://api.koha-community.org/#tag/biblios/operation/getBiblioPickupLocations
https://api.koha-community.org/#tag/items/operation/getItemPickupLocations
Comment 15 Arthur Suzuki 2024-04-04 09:18:37 UTC
Hi Katrin,
Indeed this bug is not needed anymore given those routes have been implemented into the Rest API.
Best,
Arthur