Bug 25039 - Move new due calculation to Koha::Checkout
Summary: Move new due calculation to Koha::Checkout
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 25020
Blocks: 26552
  Show dependency treegraph
 
Reported: 2020-04-02 08:24 UTC by Jonathan Druart
Modified: 2020-09-29 12:21 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25039: Move new due date calculation to Koha::Checkout (9.12 KB, patch)
2020-04-02 08:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25039: Move new due date calculation to Koha::Checkout (13.51 KB, patch)
2020-09-14 17:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25039: (follow-up) Don't forget to store the updated checkout (1.36 KB, patch)
2020-09-21 12:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25039: Move new due date calculation to Koha::Checkout (13.59 KB, patch)
2020-09-21 19:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 25039: (follow-up) Don't forget to store the updated checkout (1.41 KB, patch)
2020-09-21 19:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 25039: Move new due date calculation to Koha::Checkout (13.65 KB, patch)
2020-09-26 19:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25039: (follow-up) Don't forget to store the updated checkout (1.46 KB, patch)
2020-09-26 19:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25039: (QA follow-up) Restore date display (1.72 KB, patch)
2020-09-29 12:07 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 Jonathan Druart 2020-04-02 08:24:50 UTC

    
Comment 1 Jonathan Druart 2020-04-02 08:25:50 UTC
Created attachment 102273 [details] [review]
Bug 25039: Move new due date calculation to Koha::Checkout

This is a follow-up patch of bug 25020.

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 2 Jonathan Druart 2020-04-02 08:26:53 UTC
From bug 25020 comment 6:

"""
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 3 Tomás Cohen Arazi 2020-04-02 12:19:04 UTC
(In reply to Jonathan Druart from comment #2)
> From bug 25020 comment 6:
> 
> """
> 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');

I disagree. If we require a specific data type on the 'hard_due_date' param, we need to check it, otherwise we could have bad code working correctly in 'some' circumstances but failing of edge cases.

> +        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

I agree here, but we try to translate those exceptions into Koha::Exceptions, right?
Comment 4 Jonathan Druart 2020-04-03 09:11:55 UTC
I don't think we should start testing all types of all our parameters, that will be a pain.
We shoult assume $patron is a Koha::Patron, and *date* is a DateTime.
Comment 5 Tomás Cohen Arazi 2020-04-03 13:35:13 UTC
(In reply to Jonathan Druart from comment #4)
> I don't think we should start testing all types of all our parameters, that
> will be a pain.
> We shoult assume $patron is a Koha::Patron, and *date* is a DateTime.

I you are implying that this shouldn't be enforced in a coding guideline and QA, I *might* agree. But if a bug implements that, I wouldn't reject it because of that.

That said, we've been doing this for a while when we thought it was relevant or a good way to notify the caller so they take actions depending on the failure.
Comment 6 Tomás Cohen Arazi 2020-04-03 13:38:01 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > I don't think we should start testing all types of all our parameters, that
> > will be a pain.
> > We shoult assume $patron is a Koha::Patron, and *date* is a DateTime.
> 
> I you are implying that this shouldn't be enforced in a coding guideline and
> QA, I *might* agree. But if a bug implements that, I wouldn't reject it
> because of that.
> 
> That said, we've been doing this for a while when we thought it was relevant
> or a good way to notify the caller so they take actions depending on the
> failure.

In this specific case we could just make DateTime explode in a try/catch block inside this method and raise the same exceptions.

Generally speaking, it is debatable if we should catch them locally or let them through.  I think it is saner for writing the controllers if we raise our appropriate exceptions.
Comment 7 Tomás Cohen Arazi 2020-04-07 21:29:59 UTC
(In reply to Jonathan Druart from comment #2)
>
> +    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.

I tried removing the checks, and the results are not cool:

- Removing the check for DateTime object => found: Can't locate object method "clone" via package "cat" (perhaps you forgot to load "cat"?) at /kohadevbox/koha/Koha/Checkout.pm line 190.

- Removing the check for number => found: normal exit, and a simple warning: Argument "dog" isn't numeric in addition (+) at /usr/lib/x86_64-linux-gnu/perl5/5.24/DateTime/Duration.pm line 74.

So, from the developer POV, in this particular cases, I still think my patch is better with the checks than not having them. I agree on something: It is boring to code this much for a small feature. But I would do it like this anyway.
Comment 8 Jonathan Druart 2020-04-08 05:40:35 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> (In reply to Jonathan Druart from comment #2)
> >
> > +    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.
> 
> I tried removing the checks, and the results are not cool:
> 
> - Removing the check for DateTime object => found: Can't locate object
> method "clone" via package "cat" (perhaps you forgot to load "cat"?) at
> /kohadevbox/koha/Koha/Checkout.pm line 190.

Yes, that's the default perl error, everybody knows what it means :)

> - Removing the check for number => found: normal exit, and a simple warning:
> Argument "dog" isn't numeric in addition (+) at
> /usr/lib/x86_64-linux-gnu/perl5/5.24/DateTime/Duration.pm line 74.
> 
> So, from the developer POV, in this particular cases, I still think my patch
> is better with the checks than not having them. I agree on something: It is
> boring to code this much for a small feature. But I would do it like this
> anyway.

There is a philosophic change we are making in these patches, and if you want to generalize them I think the discussion should be brought to the dev team.
I guess there are hundreds of places where this check would be needed if such idea is adopted.

What about splitting the patches in 2 parts, one for the move and the tests, another one moved to its own bug report for the data type check?
Comment 9 Jonathan Druart 2020-04-08 13:12:31 UTC
Forget that last comment and move on this.
Comment 10 David Nind 2020-05-01 11:34:54 UTC
Applying this patch seems to stop the 'Batch extend due dates' from working as expected (for me anyway):

- Before applying the patch: issued some items, batch extend due dates comes up with items to extend the due date.

- After applying the patch: Using exactly the same criteria the batch extend due dates shows 'No checkouts for the selected filters.'

The tests still pass though!
Comment 11 Tomás Cohen Arazi 2020-09-14 17:54:41 UTC
Created attachment 110073 [details] [review]
Bug 25039: Move new due date calculation to Koha::Checkout

This is a follow-up patch of bug 25020.

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 12 Tomás Cohen Arazi 2020-09-14 17:57:31 UTC
(In reply to David Nind from comment #10)
> Applying this patch seems to stop the 'Batch extend due dates' from working
> as expected (for me anyway):
> 
> - Before applying the patch: issued some items, batch extend due dates comes
> up with items to extend the due date.
> 
> - After applying the patch: Using exactly the same criteria the batch extend
> due dates shows 'No checkouts for the selected filters.'

Sorry for taking so long! That's correct, I used .count instead of .size, which would be the correct way of doing it as I changed into using an array.
Comment 13 David Nind 2020-09-15 11:13:17 UTC
Sorry Tomás, I can't seem to get this to work.

- Before applying the patch the batch extend due date works as expected.

- After applying the patch it no longer works:
  . check out some items, use batch extend due dates, set date in future
  . it looks like everything has worked - the preview and then the results list shows the new due dates
  . when looking at individual items, the due dates have not changed

Hopefully it is something I'm doing wrong!

The tests pass though...
Comment 14 Tomás Cohen Arazi 2020-09-21 12:54:42 UTC
Created attachment 110460 [details] [review]
Bug 25039: (follow-up) Don't forget to store the updated checkout

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-09-21 13:20:31 UTC
(In reply to David Nind from comment #13)
> Sorry Tomás, I can't seem to get this to work.
> 
> - Before applying the patch the batch extend due date works as expected.
> 
> - After applying the patch it no longer works:
>   . check out some items, use batch extend due dates, set date in future
>   . it looks like everything has worked - the preview and then the results
> list shows the new due dates
>   . when looking at individual items, the due dates have not changed
> 
> Hopefully it is something I'm doing wrong!

Naw, it was just I forgot to commit the updated dates to the DB.
Comment 16 David Nind 2020-09-21 19:03:52 UTC
Created attachment 110483 [details] [review]
Bug 25039: Move new due date calculation to Koha::Checkout

This is a follow-up patch of bug 25020.

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>

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 David Nind 2020-09-21 19:03:56 UTC
Created attachment 110484 [details] [review]
Bug 25039: (follow-up) Don't forget to store the updated checkout

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2020-09-21 19:08:47 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> Naw, it was just I forgot to commit the updated dates to the DB.

It all works now and have signed off!

One minor thing I noticed was that in the preview table listing changes to be made it shows the new due date as (for example) 09/21/2020 00:00

When applied the new due date is shown as 09/21/2020 23:59 on the item record and in the patron's circulation history.

This may or may not be an issue...
Comment 19 Katrin Fischer 2020-09-26 19:11:37 UTC
(In reply to David Nind from comment #18)
> (In reply to Tomás Cohen Arazi from comment #15)
> > Naw, it was just I forgot to commit the updated dates to the DB.
> 
> It all works now and have signed off!
> 
> One minor thing I noticed was that in the preview table listing changes to
> be made it shows the new due date as (for example) 09/21/2020 00:00
> 
> When applied the new due date is shown as 09/21/2020 23:59 on the item
> record and in the patron's circulation history.
> 
> This may or may not be an issue...

Hi David, I've added a new bug for this:

Bug 26552 - Batch extend due date previews new due date as 00:00, but actually it's 23:59
Comment 20 Katrin Fischer 2020-09-26 19:12:52 UTC
Created attachment 110795 [details] [review]
Bug 25039: Move new due date calculation to Koha::Checkout

This is a follow-up patch of bug 25020.

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>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2020-09-26 19:12:57 UTC
Created attachment 110796 [details] [review]
Bug 25039: (follow-up) Don't forget to store the updated checkout

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Jonathan Druart 2020-09-29 07:31:38 UTC
(In reply to Katrin Fischer from comment #19)
> (In reply to David Nind from comment #18)
> > (In reply to Tomás Cohen Arazi from comment #15)
> > > Naw, it was just I forgot to commit the updated dates to the DB.
> > 
> > It all works now and have signed off!
> > 
> > One minor thing I noticed was that in the preview table listing changes to
> > be made it shows the new due date as (for example) 09/21/2020 00:00
> > 
> > When applied the new due date is shown as 09/21/2020 23:59 on the item
> > record and in the patron's circulation history.
> > 
> > This may or may not be an issue...
> 
> Hi David, I've added a new bug for this:
> 
> Bug 26552 - Batch extend due date previews new due date as 00:00, but
> actually it's 23:59

This is the behaviour with this patch set, not master.
Comment 23 Katrin Fischer 2020-09-29 07:50:44 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Katrin Fischer from comment #19)
> > (In reply to David Nind from comment #18)
> > > (In reply to Tomás Cohen Arazi from comment #15)
> > > > Naw, it was just I forgot to commit the updated dates to the DB.
> > > 
> > > It all works now and have signed off!
> > > 
> > > One minor thing I noticed was that in the preview table listing changes to
> > > be made it shows the new due date as (for example) 09/21/2020 00:00
> > > 
> > > When applied the new due date is shown as 09/21/2020 23:59 on the item
> > > record and in the patron's circulation history.
> > > 
> > > This may or may not be an issue...
> > 
> > Hi David, I've added a new bug for this:
> > 
> > Bug 26552 - Batch extend due date previews new due date as 00:00, but
> > actually it's 23:59
> 
> This is the behaviour with this patch set, not master.

Thx Joubu, sorry for missing the connection
Comment 24 Tomás Cohen Arazi 2020-09-29 11:04:01 UTC
(In reply to Jonathan Druart from comment #22)
> (In reply to Katrin Fischer from comment #19)
> > (In reply to David Nind from comment #18)
> > > (In reply to Tomás Cohen Arazi from comment #15)
> > > > Naw, it was just I forgot to commit the updated dates to the DB.
> > > 
> > > It all works now and have signed off!
> > > 
> > > One minor thing I noticed was that in the preview table listing changes to
> > > be made it shows the new due date as (for example) 09/21/2020 00:00
> > > 
> > > When applied the new due date is shown as 09/21/2020 23:59 on the item
> > > record and in the patron's circulation history.
> > > 
> > > This may or may not be an issue...
> > 
> > Hi David, I've added a new bug for this:
> > 
> > Bug 26552 - Batch extend due date previews new due date as 00:00, but
> > actually it's 23:59
> 
> This is the behaviour with this patch set, not master.

Are you sure? Date arithmetic is exactly the same. Isn't this highlighting some other bug?
Comment 25 Tomás Cohen Arazi 2020-09-29 12:07:09 UTC
Created attachment 110905 [details] [review]
Bug 25039: (QA follow-up) Restore date display

The original implementation didn't display the time portion of the
datetime object (00:00) as it used output_pref( ..., 'iso' ).

This patch makes it do the same.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Jonathan Druart 2020-09-29 12:15:52 UTC
The current implementation displays the time part if needed:

140                                                 <td>[% checkout.date_due | $KohaDates as_due_date => 1 %]</td>

147                                                     [% new_due_dates.shift | $KohaDates as_due_date => 1 %]