Bug 21832

Summary: Restore is_expired in ILS-DI GetPatronInfo service
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: Web servicesAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, lucas, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17578    
Bug Blocks:    
Attachments: Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: add unit test
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: add unit test
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service
Bug 21832: add unit test
Bug 21832: add is_expired to ILS-DI example
Bug 21832: add is_expired to ILS-DI example

Description Fridolin Somers 2018-11-14 09:57:21 UTC
Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restaure in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.
Comment 1 Fridolin Somers 2018-11-14 10:53:57 UTC
Created attachment 82323 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>
Comment 2 Jonathan Druart 2018-11-14 14:43:50 UTC
call Koha::Patron->is_expired instead.
Comment 3 Fridolin Somers 2018-11-15 14:46:48 UTC
Created attachment 82363 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>
Comment 4 Fridolin Somers 2018-11-15 14:49:46 UTC
(In reply to Jonathan Druart from comment #2)
> call Koha::Patron->is_expired instead.

Indeed, its much better
Comment 5 Maryse Simard 2019-02-08 19:41:54 UTC
Created attachment 84894 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 6 Katrin Fischer 2019-02-09 20:00:23 UTC
Created attachment 84909 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Katrin Fischer 2019-02-09 20:01:47 UTC
This works nicely and is an easy fix which is unlikely to have any side effects. It would probably still be nice to have a regression test to avoid future breakage.

Leaving final decision to Nick.
Comment 8 Nick Clemens 2019-02-11 13:28:04 UTC
Agreed, should be a simple enough test, please add a followup patch
Comment 9 Fridolin Somers 2019-02-12 14:54:17 UTC
Created attachment 85022 [details] [review]
Bug 21832: add unit test

Adds a check that is_expired exists in service GetPatronInfo.

Test plan :
Run : prove t/db_dependent/ILSDI_Services.t
Comment 10 Fridolin Somers 2019-02-12 14:55:06 UTC
(In reply to Nick Clemens from comment #8)
> Agreed, should be a simple enough test, please add a followup patch

Here it is.
Viva unit tests \o/
Comment 11 Josef Moravec 2019-02-27 20:49:05 UTC
Created attachment 85800 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2019-02-27 20:49:10 UTC
Created attachment 85801 [details] [review]
Bug 21832: add unit test

Adds a check that is_expired exists in service GetPatronInfo.

Test plan :
Run : prove t/db_dependent/ILSDI_Services.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Jonathan Druart 2019-03-05 13:10:07 UTC
Created attachment 86063 [details] [review]
Bug 21832: Restore is_expired in ILS-DI GetPatronInfo service

Since Bug 17578 removed C4::Member::GetMemberDetails, the information "is_expired" disappeared from ILS-DI service GetPatronInfo.

I propose to restore in by adding the code that exisited in C4::Member::GetMemberDetails directly in C4::ILSDI::Services.

Test plan :
1) Enable ILS-DI webservice
2) Choose a patron not expired
3) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
4) You see : <is_expired>0</is_expired>
5) Choose a patron expired
6) Call web-service (replace X with patron's borrowernumber) :
   /cgi-bin/koha/ilsdi.pl?service=GetPatronInfo&patron_id=X
7) You see : <is_expired>1</is_expired>

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2019-03-05 13:10:15 UTC
Created attachment 86064 [details] [review]
Bug 21832: add unit test

Adds a check that is_expired exists in service GetPatronInfo.

Test plan :
Run : prove t/db_dependent/ILSDI_Services.t

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended patch: remove ref to bug number, we have git for that
Comment 15 Jonathan Druart 2019-03-05 13:14:31 UTC
one sec.

I had (and still have) issues when I refactored C4 code called from the ILSDI module.
The problem is: what is really expected and what was there by 'accident'?

IMO is_expired was in the response by accident, proof of that: it is not in the "Example response" on http://catalogue.kohadev.org/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPatronInfo

Moreover we have dateexpiry, so client can guess if the patron's account has expired or not :)

Swithing to In Discussion.
Comment 16 Fridolin Somers 2019-03-06 15:20:25 UTC
But several portal use it already with this tag.
If it does no harm why not keeping it ?
Comment 17 Jonathan Druart 2019-03-06 15:45:55 UTC
(In reply to Fridolin SOMERS from comment #16)
> But several portal use it already with this tag.
> If it does no harm why not keeping it ?

Maybe adding it to the doc? :)
Comment 18 Fridolin Somers 2019-03-07 08:58:08 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Fridolin SOMERS from comment #16)
> > But several portal use it already with this tag.
> > If it does no harm why not keeping it ?
> 
> Maybe adding it to the doc? :)

Very good idea, I'm on it.
Comment 19 Fridolin Somers 2019-03-07 13:48:14 UTC
Created attachment 86243 [details] [review]
Bug 21832: add is_expired to ILS-DI example

Test plan :
1) Apply patch
2) Enable ILS-DI
3) Go to <opac URL>/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPatronInfo
4) Check you see is_expired in example response
Comment 20 Jonathan Druart 2019-03-16 00:32:50 UTC
Created attachment 86701 [details] [review]
Bug 21832: add is_expired to ILS-DI example

Test plan :
1) Apply patch
2) Enable ILS-DI
3) Go to <opac URL>/cgi-bin/koha/ilsdi.pl?service=Describe&verb=GetPatronInfo
4) Check you see is_expired in example response

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Nick Clemens 2019-03-16 13:54:16 UTC
Awesome work all!

Pushed to master for 19.05
Comment 22 Martin Renvoize 2019-03-19 11:05:14 UTC
Pushed to 18.11.x for 18.11.04
Comment 23 Lucas Gass 2019-03-26 23:55:49 UTC
backported to 18.05.x for 18.05.12
Comment 24 Fridolin Somers 2019-04-24 07:51:53 UTC
Pushed to 17.11.x for 17.11.18