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>
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>
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).
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 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.
(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.
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>
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>
Need unit tests to cover changes
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>
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>
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>
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>
Moved back to signed off, as full case unit testing was added for the affected function: C4::ILSDI::Services::LookupPatron.
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>
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>
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>
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>
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
Pushed to master for 18.05, thanks to everybody involved!
Awesome work all! This patch has been pushed to 17.11.x and will be in 17.11.03