When using the tool 'batch extend due dates' the time portion of a checkout is lost when setting a specific date. To recreate: 1 - Checkout an item 2 - Note the date due has time of 23:59 3 - Checkout an item and specify the time in the checkout 4 - Note it displays 5 - Use Tools->Batch extend due date 6 - Enter params to capture the two issues you just made 7 - Extend to a specified date 8 - Note checkouts are now due at 00:00:00
Created attachment 102085 [details] [review] Bug 25020: Preserve time part when batch extending due dates When selecting a new hard due date, we should keep the time part of the original checkouts. Test plan: 1 - Checkout an item specifying a date due 2 - Checkout an item without specifying a date due 3 - Use Tools->Batch extend due date 4 - Select a hard due date 5 - On the confirmation screen you should see that the time part has been kept 6 - Confirm 7 - Make sure the correct values hava been inserted in DB 8 - Try now using the other option, give a number of days 9 - Repeat 4-7
Created attachment 102123 [details] [review] Bug 25020: Preserve time part when batch extending due dates When selecting a new hard due date, we should keep the time part of the original checkouts. Test plan: 1 - Checkout an item specifying a date due 2 - Checkout an item without specifying a date due 3 - Use Tools->Batch extend due date 4 - Select a hard due date 5 - On the confirmation screen you should see that the time part has been kept 6 - Confirm 7 - Make sure the correct values hava been inserted in DB 8 - Try now using the other option, give a number of days 9 - Repeat 4-7 Signed-off-by: David Nind <david@davidnind.com>
Created attachment 102250 [details] [review] Bug 25020: Preserve time part when batch extending due dates When selecting a new hard due date, we should keep the time part of the original checkouts. Test plan: 1 - Checkout an item specifying a date due 2 - Checkout an item without specifying a date due 3 - Use Tools->Batch extend due date 4 - Select a hard due date 5 - On the confirmation screen you should see that the time part has been kept 6 - Confirm 7 - Make sure the correct values hava been inserted in DB 8 - Try now using the other option, give a number of days 9 - Repeat 4-7 Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 102251 [details] [review] [OPTIONAL] Bug 25020: (QA follow-up) Add some tests This patch adds a new method Koha::Checkout->shift_due_date that accepts the same parameters we provide in the form. It catches bad scenarios (type errors, passing both parameters when only one is accepted, and so on). Date manipulation is tested so time is kept and resulting dates are correct. The controller script is cleaned a bit to use the introduced method. I do this because: - We really need tests for this and doing it with selenium is no-end - I see a use for this new method for encapsulating behaviours, for example we might want to add Calendar support for the 'days' use case, and having the method here assures we will have tests, etc. To test: 1. Apply this patches 2. Repeat the original test plan => SUCCESS: Everything works as expected 3. Run: $ kshell k$ prove t/db_dependent/Koha/Checkout.t => SUCCESS: Tests pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
@RM I've added my two cents there as I see this is going to get more 'features' sooner than later. Feel free to ditch the follow-up. The original patch is PQA even if I think we should go with something more like the follow-up. Cheers
Thanks for the follow-up Tomas. A quick note, I think this kind of tests is overkill and not needed: + Koha::Exceptions::WrongParameter->throw( + "'hard_due_date' must be a DateTime object" + ) unless $hard_due_date->isa('DateTime'); + + $due_date = $hard_due_date->clone->set( + hour => $due_date->hour, + minute => $due_date->minute, + second => $due_date->second + ); + } + else { + Koha::Exceptions::WrongParameter->throw( + "'days' must be an integer" + ) unless looks_like_number($days); + $due_date->add( days => $days ); Developers will get an error anyway if $days is not an integer (DateTime will raise its own meaningful exception), or $hard_due_date is not a DateTime.
Comment on attachment 102251 [details] [review] [OPTIONAL] Bug 25020: (QA follow-up) Add some tests Moved to bug 25039
Nice work everyone! Pushed to master for 20.05
Backported to 19.11.x branch for 19.11.05
backported to 19.05.c for 19.05.10
Backported to 18.11.x for 18.11.16