|
Description
Arthur Suzuki
2019-09-03 08:35:19 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. 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 |