|
Description
Jonathan Druart
2020-04-02 08:24: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> 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. """ (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? 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. (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 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. (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. (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? Forget that last comment and move on this. 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! 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> (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. 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... 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> (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. 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> 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> (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... (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 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> 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> (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. (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 (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? 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> 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 %] |