Bug 13990 - ILS-DI LookupPatron Requries ID Type
Summary: ILS-DI LookupPatron Requries ID Type
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Jon Knight
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-13 20:45 UTC by Tom Misilo
Modified: 2019-10-14 19:56 UTC (History)
5 users (show)

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


Attachments
Bug 13990 - ILS-DI LookupPatron Requries ID Type (3.97 KB, patch)
2017-11-07 18:20 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 13990 - ILS-DI LookupPatron Requries ID Type (4.08 KB, patch)
2018-01-26 16:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990: Followup - fix various whitespace errors (10.08 KB, patch)
2018-01-26 16:33 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990 - ILS-DI LookupPatron Requries ID Type (4.03 KB, patch)
2018-01-28 06:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990: Followup - fix various whitespace errors (10.08 KB, patch)
2018-01-28 06:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990: QA Followup - Add unit test coverage (2.68 KB, patch)
2018-01-28 06:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990: Followup: silence warnings on testing (1.65 KB, patch)
2018-01-28 06:45 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13990 - ILS-DI LookupPatron Requries ID Type (4.08 KB, patch)
2018-01-28 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13990: Followup - fix various whitespace errors (10.14 KB, patch)
2018-01-28 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13990: QA Followup - Add unit test coverage (2.74 KB, patch)
2018-01-28 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13990: Followup: silence warnings on testing (1.71 KB, patch)
2018-01-28 12:27 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Misilo 2015-04-13 20:45:27 UTC
For the LookupPatron the id_type argument seems to be required, even though it says it is optional.

For example: https://site/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=4456

I get the following error: 
<h1>Software error:</h1>
<pre>Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054.
</pre>

However, if I add &id_type=borrowernumber

I get the correct output.

<LookupPatron>
  <id>4456</id>
</LookupPatron>
Comment 1 Owen Leonard 2016-08-10 14:45:48 UTC
Although this doesn't seem to generate an error anymore, it is still the case that id_type is required.

/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=4456

...returns:

<LookupPatron>
  <message>PatronNotFound</message>
</LookupPatron>

/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=4456&id_type=borrowernumber

...returns:

<LookupPatron>
  <id>4456</id>
</LookupPatron>
Comment 2 Jon Knight 2017-11-06 17:54:20 UTC
I assume the obvious fix to this is to say that the id_type is required rather than optional in the POD?  Alternative there could be a set of well defined id_types that are cycled through to find matches (that would be a pretty easy bit of code).

On a similar note, we've noticed that "email" also appears to be a valid id_type but isn't in the POD list. A quick look in the code (hoorah for open source!) shows that any field in the borrower table can be looked up with this call.  For example "phone", "phonepro" or "mobile" let me find myself using my primary, secondary and "other" phone number. 

This begs the question: should Koha be constraining the list of id_type's that can be passed into this routine from the client? Or is being able to search on any column in the borrowers table regarded as a useful feature (we've found "email" to be handy for sure).
Comment 3 Jon Knight 2017-11-07 18:20:28 UTC
Created attachment 69004 [details] [review]
Bug 13990 - ILS-DI LookupPatron Requries ID Type

Patch provides a constrained set of borrower attributes to try in turn if the
ID type is not specified.  Also added "email" to the POD documented list of
id types seeing as its quite a useful one for integration developers.

Test plan:

1) without the patch applied, turn on ils-di interface in your admin sysprefs
   and then try accessing the URL:

   https://your-server/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=Mabel

   replacing "your-server" your Koha dev test domain name (localhost:8080 if
   you're using a kohadevbox VM) and "Mabel" with the first name of a patron
   (Mabel is in the test database already if you're using that).

   You should get back an error message in the XML.

2) Apply the patch and repeat. This time you should get an <id> element in
   the XML with Mabel's patron ID in it.
Comment 4 M. Tompsett 2018-01-26 16:15:22 UTC
Comment on attachment 69004 [details] [review]
Bug 13990 - ILS-DI LookupPatron Requries ID Type

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

::: C4/ILSDI/Services.pm
@@ +303,3 @@
>  	- borrowernumber
> +	- firstname
> +        - surname

I would have put these in the same order as the loop, but it isn't really a big deal.
Comment 5 M. Tompsett 2018-01-26 16:18:39 UTC
(In reply to Jon Knight from comment #2)
> This begs the question: should Koha be constraining the list of id_type's
> that can be passed into this routine from the client? Or is being able to
> search on any column in the borrowers table regarded as a useful feature
> (we've found "email" to be handy for sure).

I think searching on any valid field is handy. Though, some fields are more useful than others. No error checking on valid fields is in this patch, but that isn't the tight scope of this bug.

However, if you list it, you should search it. And this patch makes the list and what is searched matched nicely. I think email was a reasonable addition to the searchable list that is stated.
Comment 6 M. Tompsett 2018-01-26 16:33:25 UTC
Created attachment 70965 [details] [review]
Bug 13990 - ILS-DI LookupPatron Requries ID Type

Patch provides a constrained set of borrower attributes to try in turn if the
ID type is not specified.  Also added "email" to the POD documented list of
id types seeing as its quite a useful one for integration developers.

Test plan:

1) without the patch applied, turn on ils-di interface in your admin sysprefs
   and then try accessing the URL:

   https://your-server/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=Mabel

   replacing "your-server" your Koha dev test domain name (localhost:8080 if
   you're using a kohadevbox VM) and "Mabel" with the first name of a patron
   (Mabel is in the test database already if you're using that).

   You should get back an error message in the XML.

2) Apply the patch and repeat. This time you should get an <id> element in
   the XML with Mabel's patron ID in it.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 M. Tompsett 2018-01-26 16:33:28 UTC
Created attachment 70966 [details] [review]
Bug 13990: Followup - fix various whitespace errors

- tabs to 4 spaces
- trailing whitespace
- missing vertical whitespace before =head3

Without this patch, this bug fails qa test tools.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 8 Nick Clemens 2018-01-26 20:06:03 UTC
Need unit tests to cover changes
Comment 9 M. Tompsett 2018-01-28 06:45:40 UTC
Created attachment 70997 [details] [review]
Bug 13990 - ILS-DI LookupPatron Requries ID Type

Patch provides a constrained set of borrower attributes to try in turn if the
ID type is not specified.  Also added "email" to the POD documented list of
id types seeing as its quite a useful one for integration developers.

Test plan:

1) without the patch applied, turn on ils-di interface in your admin sysprefs
   and then try accessing the URL:

   https://your-server/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=Mabel

   replacing "your-server" your Koha dev test domain name (localhost:8080 if
   you're using a kohadevbox VM) and "Mabel" with the first name of a patron
   (Mabel is in the test database already if you're using that).

   You should get back an error message in the XML.

2) Apply the patch and repeat. This time you should get an <id> element in
   the XML with Mabel's patron ID in it.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 10 M. Tompsett 2018-01-28 06:45:46 UTC
Created attachment 70998 [details] [review]
Bug 13990: Followup - fix various whitespace errors

- tabs to 4 spaces
- trailing whitespace
- missing vertical whitespace before =head3

Without this patch, this bug fails qa test tools.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 11 M. Tompsett 2018-01-28 06:45:51 UTC
Created attachment 70999 [details] [review]
Bug 13990: QA Followup - Add unit test coverage

prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 12 M. Tompsett 2018-01-28 06:45:59 UTC
Created attachment 71000 [details] [review]
Bug 13990: Followup: silence warnings on testing

prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 13 M. Tompsett 2018-01-28 06:47:48 UTC
Moved back to signed off, as full case unit testing was added for the affected function: C4::ILSDI::Services::LookupPatron.
Comment 14 Nick Clemens 2018-01-28 12:27:35 UTC
Created attachment 71001 [details] [review]
Bug 13990 - ILS-DI LookupPatron Requries ID Type

Patch provides a constrained set of borrower attributes to try in turn if the
ID type is not specified.  Also added "email" to the POD documented list of
id types seeing as its quite a useful one for integration developers.

Test plan:

1) without the patch applied, turn on ils-di interface in your admin sysprefs
   and then try accessing the URL:

   https://your-server/cgi-bin/koha/ilsdi.pl?service=LookupPatron&id=Mabel

   replacing "your-server" your Koha dev test domain name (localhost:8080 if
   you're using a kohadevbox VM) and "Mabel" with the first name of a patron
   (Mabel is in the test database already if you're using that).

   You should get back an error message in the XML.

2) Apply the patch and repeat. This time you should get an <id> element in
   the XML with Mabel's patron ID in it.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2018-01-28 12:27:38 UTC
Created attachment 71002 [details] [review]
Bug 13990: Followup - fix various whitespace errors

- tabs to 4 spaces
- trailing whitespace
- missing vertical whitespace before =head3

Without this patch, this bug fails qa test tools.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2018-01-28 12:27:41 UTC
Created attachment 71003 [details] [review]
Bug 13990: QA Followup - Add unit test coverage

prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2018-01-28 12:27:45 UTC
Created attachment 71004 [details] [review]
Bug 13990: Followup: silence warnings on testing

prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Jonathan Druart 2018-01-30 17:19:24 UTC
Please make sure the commit messages start with "Bug 12345: "

Please make sure the "follow-up" and " (QA follow-up) " flags are correct

https://wiki.koha-community.org/wiki/Commit_messages#Examples
Comment 19 Jonathan Druart 2018-01-30 17:25:39 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 20 Nick Clemens 2018-02-12 20:56:56 UTC
Awesome work all!

This patch has been pushed to 17.11.x and will be in 17.11.03