Bug 25020 - Extending due dates to a specified date should preserve time portion of original due date
Summary: Extending due dates to a specified date should preserve time portion of origi...
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 24846
Blocks: 25039 25101
  Show dependency treegraph
 
Reported: 2020-03-30 13:06 UTC by Nick Clemens
Modified: 2020-04-15 01:20 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10, 18.11.16


Attachments
Bug 25020: Preserve time part when batch extending due dates (5.16 KB, patch)
2020-03-30 14:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25020: Preserve time part when batch extending due dates (5.22 KB, patch)
2020-03-31 11:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 25020: Preserve time part when batch extending due dates (5.26 KB, patch)
2020-04-01 19:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[OPTIONAL] Bug 25020: (QA follow-up) Add some tests (9.07 KB, patch)
2020-04-01 19:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-03-30 13:06:47 UTC
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
Comment 1 Jonathan Druart 2020-03-30 14:15:24 UTC
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
Comment 2 David Nind 2020-03-31 11:26:53 UTC
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>
Comment 3 Tomás Cohen Arazi 2020-04-01 19:56:58 UTC
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>
Comment 4 Tomás Cohen Arazi 2020-04-01 19:57:04 UTC
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>
Comment 5 Tomás Cohen Arazi 2020-04-01 19:58:56 UTC
@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
Comment 6 Jonathan Druart 2020-04-02 08:23:16 UTC
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 7 Jonathan Druart 2020-04-02 08:26:14 UTC
Comment on attachment 102251 [details] [review]
[OPTIONAL] Bug 25020: (QA follow-up) Add some tests

Moved to bug 25039
Comment 8 Martin Renvoize 2020-04-03 13:24:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 9 Joy Nelson 2020-04-03 19:44:55 UTC
Backported to 19.11.x branch for 19.11.05
Comment 10 Lucas Gass 2020-04-13 16:48:12 UTC
backported to 19.05.c for 19.05.10
Comment 11 Hayley Mapley 2020-04-15 01:20:04 UTC
Backported to 18.11.x for 18.11.16