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.

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>

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?

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).

(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.

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>

Created attachment 96564 [details] [review]
Bug 23531: Add some tests for HoldItem and HoldTitle with date params provided

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

Bug for DataTables error on patrons hold tab is bug 24769

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>

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

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>

Hi David,
tests are now green thanks to Julian :)
patch ready for testing!
(was previously signed off by christophe)
have a nice one,
Arthur

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

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

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>

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>

(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

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>

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>

(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.

In the test plan "the ISL-DI system preference" should be "the ILS-DI system preference"!

\o/
Many thanks for your help and fixing typo :)

<HoldItem><code>IllegalParameter</code><message>The parameter pickup_expiry_date is illegal.</message></HoldItem>

Why do I get that?

Created attachment 100895 [details] [review]
Bug 23531: Simplify tests using build_object

And build_sample_item.
Also remove a debug statement

(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.

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>

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>

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>

Not sure why we do not pick reserve_date here, but ok for me.

Nice work everyone!

Pushed to master for 20.05

Yipee! awesome, thanks joubu & martin \o/

Does not apply to 19.11.x branch.  not backported