Bug 23531

Summary: ILS-DI doesn't implement needed_before_date and pickup_expiry_date parameters (renamed start_date and expiry_date)
Product: Koha Reporter: Arthur Suzuki <arthur.suzuki>
Component: Web servicesAssignee: Arthur Suzuki <arthur.suzuki>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: 1joynelson, david, jonathan.druart, katrin.fischer, martin.renvoize
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29002
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch implements the documented "start_date" and "expiry_date" parameters for hold requests using Koha's ILS-DI service. Note: the previously documented but not implemented parameter names were changed: - needed_before_date => start_date - pickup_expiry_date => expiry_date
Version(s) released in:
20.05.00
Bug Depends on:    
Bug Blocks: 23669, 11641    
Attachments: bug23531 : fix ILSDI non-implemented but documented parameters
bug23531 : fix ILSDI non-implemented but documented parameters
Bug 23531: fix ILSDI non-implemented but documented parameters
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Bug 23531: fix ILSDI non-implemented but documented parameters
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Bug 23531: fix ILSDI non-implemented but documented parameters
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Bug 23531: Fix ILS-DI non-implemented but documented parameters
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Bug 23531: Simplify tests using build_object
Bug 23531: fix ILSDI non-implemented but documented parameters
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Bug 23531: Simplify tests using build_object

Description Arthur Suzuki 2019-09-03 08:35:19 UTC
tried to send the following request :
GET /cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=196&bib_id=96629&item_id=112194&needed_before_date=2020-08-18&pickup_expiry_date=2020-10-18&pickup_location=BDY

the reservation which is placed doesn't take the "needed_before_date" and pickup_expiry_date" parameter, although those are documented.
Arthur
Comment 1 Arthur Suzuki 2019-09-03 09:14:37 UTC
Created attachment 92538 [details] [review]
bug23531 : fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with needed_before_date and
pickup_expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.
Comment 2 Christophe Croullebois 2019-10-02 13:32:28 UTC
Created attachment 93475 [details] [review]
bug23531 : fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with needed_before_date and
pickup_expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>
Comment 3 Katrin Fischer 2019-10-12 11:06:55 UTC
1) Same formal issues than on bug 23015:
- commit message
- failing qa tools for tabs vs. spaces

 FAIL	C4/ILSDI/Services.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 730)
		forbidden pattern: tab char (line 735)
		forbidden pattern: tab char (line 816)
		forbidden pattern: tab char (line 821)
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   OK	  valid

2) Unit tests for change to Service.pm 

3) I am not sure about the patch 'content'.

needed_before_date = Visible on the Koha holds form as 'Hold not needed after'. Stored in the database in reserves.expirationdate. Correct?

pickup_expiry_date = This one confuses me. The expiration date for pick-up is usually calculated by Koha using different configuration parameters and not something that the user can 'pick' at the time of placing a hold.

But what makes it worse: Both go in the same column in the database atm (yes, bad). When a hold is set to waiting, the pick-up date is calculated and rebases the patron entered date in reserves.expirationdate. 

So... I think this cannot work?
Comment 4 Arthur Suzuki 2019-10-12 21:00:41 UTC
needed_before_date is actually stored in the "reservedate", this is just a way to place a reserve on a future date.

pickup_expiry_date is the date after which the reserve is automatically cancelled if not picked up by the user and this one is stored in the "expirydate"

Maybe the parameter "needed_before_date" shall be renamed another way.

The use case is for example for teachers/students who need to reserve a book for a course, they need it between "needed_before_date" and "pickup_expiry_date".
There is no way to provide either date from ILS-DI right now, but we have this need for bundled holds (for which Cecile showed the bundled loans counterpart at the Hackfest in Marseille).
Comment 5 Katrin Fischer 2019-10-13 06:49:17 UTC
(In reply to Arthur Suzuki from comment #4)
> needed_before_date is actually stored in the "reservedate", this is just a
> way to place a reserve on a future date.
> pickup_expiry_date is the date after which the reserve is automatically
> cancelled if not picked up by the user and this one is stored in the
> "expirydate"
> 
> Maybe the parameter "needed_before_date" shall be renamed another way.
> 
> The use case is for example for teachers/students who need to reserve a book
> for a course, they need it between "needed_before_date" and
> "pickup_expiry_date".
> There is no way to provide either date from ILS-DI right now, but we have
> this need for bundled holds (for which Cecile showed the bundled loans
> counterpart at the Hackfest in Marseille).

I think I would make it something like 'expiry_date' (doesn't matter if it has been waiting for pickup or not) and something like 'not_needed_before' or even 'start_date' to clear things up a bit.
Comment 6 Arthur Suzuki 2019-12-22 07:29:46 UTC
Created attachment 96563 [details] [review]
Bug 23531: fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with start_date and expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>
Comment 7 Arthur Suzuki 2019-12-22 07:29:49 UTC
Created attachment 96564 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided
Comment 8 David Nind 2020-03-01 03:53:20 UTC
Patch no longer applies:

Applying: Bug 23531: fix ILSDI non-implemented but documented parameters
Using index info to reconstruct a base tree...
M	C4/ILSDI/Services.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/ILSDI/Services.pm
CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23531: fix ILSDI non-implemented but documented parameters

Notes:
- Get DataTables error on patrons hold tab before patch is applied (bug 
- Before patch applied query for use in koha-testing-docker:
http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=19&bib_id=126&item_id=264&needed_before_date=2020-03-02&pickup_expiry_date=2020-03-31&pickup_location=CPL
Comment 9 David Nind 2020-03-01 03:55:28 UTC
Bug for DataTables error on patrons hold tab is bug 24769
Comment 10 Arthur Suzuki 2020-03-05 07:50:41 UTC
Created attachment 100165 [details] [review]
Bug 23531: fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with start_date and expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 11 Arthur Suzuki 2020-03-05 07:53:35 UTC
Hi David,
I've rebased the patch on master so patch should apply now.
I've also renamed the parameters according to comment 5 from Katrin.
The functionality works for real but not the automated tests, I'll check it out with a colleague today, probably it's just me not being good at writing tests scripts (ILSDI_Service.t)
Kr,
Arthur
Comment 12 Arthur Suzuki 2020-03-05 08:20:36 UTC
Created attachment 100166 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 13 Arthur Suzuki 2020-03-05 08:22:12 UTC
Hi David,
tests are now green thanks to Julian :)
patch ready for testing!
(was previously signed off by christophe)
have a nice one,
Arthur
Comment 14 David Nind 2020-03-05 18:26:34 UTC
Sorry Arthur, patch no longer applies!

Applying: Bug 23531: fix ILSDI non-implemented but documented parameters
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
CONFLICT (content): Merge conflict in C4/ILSDI/Services.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23531: fix ILSDI non-implemented but documented parameters
Comment 15 Arthur Suzuki 2020-03-06 07:52:09 UTC
Hi David,
last patch i can see on master is 22816, is it the same on your side?...
I use gitlab as my remote : https://gitlab.com/koha-community/Koha
I guess I'm doing smthg wrong but can't figure out what.
I'll try to check around with my colleagues.
Arthur
Comment 16 Arthur Suzuki 2020-03-06 09:12:27 UTC
Created attachment 100221 [details] [review]
Bug 23531: fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with start_date and expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 17 Arthur Suzuki 2020-03-06 09:12:50 UTC
Created attachment 100222 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>
Comment 18 Arthur Suzuki 2020-03-06 09:14:09 UTC
(In reply to David Nind from comment #14)
> Sorry Arthur, patch no longer applies!

Yup, had some issues with my remotes configuration, just got to learn that one shall not use gitlab anymore.
Patch should apply now.
Arthur
Comment 19 David Nind 2020-03-06 21:16:42 UTC
Created attachment 100294 [details] [review]
Bug 23531: Fix ILS-DI non-implemented but documented parameters

Koha's ILS-DI service does not implement the "needed_before_date" and "pickup_expiry_date" parameters for holds, although they are documented.

This patch changes the parameter names and implements them:
- needed_before_date => start_date
- pickup_expiry_date => expiry_date

Test plan:
1) Enable the ISL-DI system preference.
2) Submit a hold request using ILS-DI with "needed_before_date" and "pickp_expiry_date" parameters, for example (change the bib_id, item_id, and dates as appropriate):
http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=19&bib_id=126&item_id=264&needed_before_date=2020-03-12&pickup_expiry_date=2020-03-31&pickup_location=CPL
3) Check that the dates in the request are NOT shown for the hold (go to the item record and then the holds section):
- Date: will show the current date, rather than the needed_before_date entered
- Expiration: will be empty, rather than the pickup_expiry_date entered
4) Delete the hold.
5) Apply the patch.
6) Resubmit the hold request in step 2 using "start_date" and "expiry_date" parameters, for example:
http://127.0.0.1:8080/cgi-bin/koha/ilsdi.pl?service=HoldItem&patron_id=19&bib_id=126&item_id=264&start_date=2020-03-12&expiry_date=2020-03-31&pickup_location=CPL
7) Check that dates shown for the hold are now correct:
- Date: will show the start_date entered
- Expiration: will show the expiry_date entered
8) Check that the tests pass (prove t/db_dependent/ILSDI_Services.t)
9) Sign off!

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 David Nind 2020-03-06 21:16:45 UTC
Created attachment 100295 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2020-03-06 21:19:00 UTC
(In reply to Arthur Suzuki from comment #18)
> Yup, had some issues with my remotes configuration, just got to learn that
> one shall not use gitlab anymore.
> Patch should apply now.
> Arthur

Thanks Arthur for fixing _ I'm not sure I would have been much help working out what was wrong 8-) I also updated the test plan.
Comment 22 David Nind 2020-03-06 21:20:39 UTC
In the test plan "the ISL-DI system preference" should be "the ILS-DI system preference"!
Comment 23 Arthur Suzuki 2020-03-07 06:34:10 UTC
\o/
Many thanks for your help and fixing typo :)
Comment 24 Jonathan Druart 2020-03-17 14:17:25 UTC
<HoldItem><code>IllegalParameter</code><message>The parameter pickup_expiry_date is illegal.</message></HoldItem>

Why do I get that?
Comment 25 Jonathan Druart 2020-03-17 14:17:42 UTC
Created attachment 100895 [details] [review]
Bug 23531: Simplify tests using build_object

And build_sample_item.
Also remove a debug statement
Comment 26 Jonathan Druart 2020-03-17 14:20:27 UTC
(In reply to Jonathan Druart from comment #24)
> <HoldItem><code>IllegalParameter</code><message>The parameter
> pickup_expiry_date is illegal.</message></HoldItem>
> 
> Why do I get that?

Ok, forget that.
Comment 27 Jonathan Druart 2020-03-17 14:23:09 UTC
Created attachment 100896 [details] [review]
Bug 23531: fix ILSDI non-implemented but documented parameters

test plan :
1/ Submit a hold through ILSDI with start_date and expiry_date parameters
2/ Verify in Koha those parameters are not applied to the newly created
reserve. Delete reserve.
3/ Apply patch and repeat 1.
4/ Verify the reserve created by same request as 1 now has both dates
applied and visible in Koha.

Signed-off-by: Christophe Croullebois <christophe.croullebois@biblibre.com>

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2020-03-17 14:23:17 UTC
Created attachment 100897 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided

Signed-off-by: Arthur Suzuki <arthur.suzuki@biblibre.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2020-03-17 14:23:21 UTC
Created attachment 100898 [details] [review]
Bug 23531: Simplify tests using build_object

And build_sample_item.
Also remove a debug statement

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Jonathan Druart 2020-03-17 14:24:18 UTC
Not sure why we do not pick reserve_date here, but ok for me.
Comment 31 Martin Renvoize 2020-03-18 08:45:43 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 32 Arthur Suzuki 2020-03-18 08:59:00 UTC
Yipee! awesome, thanks joubu & martin \o/
Comment 33 Joy Nelson 2020-04-03 18:28:14 UTC
Does not apply to 19.11.x branch.  not backported