Bug 23974 - hours_between and days_between lack tests
Summary: hours_between and days_between lack tests
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: rel_20_05_target
Depends on:
Blocks:
 
Reported: 2019-11-05 16:41 UTC by Jonathan Druart
Modified: 2019-11-19 09:49 UTC (History)
1 user (show)

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


Attachments
Bug 23974: Improve readability (4.58 KB, patch)
2019-11-05 16:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23974: Add new tests for hours_between (5.94 KB, patch)
2019-11-05 16:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23974: Add tests for days_between (7.27 KB, patch)
2019-11-05 16:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23974: both methods should have the same logic/construction (2.04 KB, patch)
2019-11-05 16:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23974: Improve readability (4.63 KB, patch)
2019-11-08 13:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add new tests for hours_between (6.00 KB, patch)
2019-11-08 13:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add tests for days_between (7.33 KB, patch)
2019-11-08 13:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: both methods should have the same logic/construction (2.10 KB, patch)
2019-11-08 13:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: (follow-up) Add same day loan tests (1.87 KB, patch)
2019-11-08 13:49 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2019-11-05 16:41:42 UTC
Those two methods are not covered by tests, which makes difficult changes like:
  commit a693c7243c23f888e2fad38a4fff9f37ff4a9301
  Bug 23382: hours_between should match the logic of days_between


We need to add more test coverage in order to improve them. For example we would like to improve the calculation for hourly loans (due date or suspension).
Comment 1 Jonathan Druart 2019-11-05 16:44:59 UTC
Created attachment 95062 [details] [review]
Bug 23974: Improve readability

This patch does not change anything, just a bit of cleaning
Comment 2 Jonathan Druart 2019-11-05 16:45:02 UTC
Created attachment 95063 [details] [review]
Bug 23974: Add new tests for hours_between
Comment 3 Jonathan Druart 2019-11-05 16:45:06 UTC
Created attachment 95064 [details] [review]
Bug 23974: Add tests for days_between
Comment 4 Jonathan Druart 2019-11-05 16:45:09 UTC
Created attachment 95065 [details] [review]
Bug 23974: both methods should have the same logic/construction
Comment 5 Jonathan Druart 2019-11-05 16:45:48 UTC
Now that we have a base for tests we can discuss the expected behaviors/values :)
Comment 6 Martin Renvoize 2019-11-08 13:49:28 UTC
Created attachment 95210 [details] [review]
Bug 23974: Improve readability

This patch does not change anything, just a bit of cleaning

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-11-08 13:49:31 UTC
Created attachment 95211 [details] [review]
Bug 23974: Add new tests for hours_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-11-08 13:49:34 UTC
Created attachment 95212 [details] [review]
Bug 23974: Add tests for days_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2019-11-08 13:49:37 UTC
Created attachment 95213 [details] [review]
Bug 23974: both methods should have the same logic/construction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-11-08 13:49:41 UTC
Created attachment 95214 [details] [review]
Bug 23974: (follow-up) Add same day loan tests

Hourly loans can be short enough to not cross a day boundary and as such
we should test for that scenario in the hour_between tests too.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-11-08 13:50:42 UTC
I'm really happy to see these tests and agree with the logic in them... Signing off on yours and have added one minor addition to ensure we're checking same day hourly loans.
Comment 12 Martin Renvoize 2019-11-12 20:06:15 UTC
I think we're safe to push these additional tests as they are. Not sure what more needs discussing at the minute?  Can we set to NSO ?
Comment 13 Martin Renvoize 2019-11-12 20:08:50 UTC
Brain fuzz, already at signed off.. I meant I think these are safe to push and just improve our test coverage at this point.  Would you just check my final followup looks ok Jonathan?
Comment 14 Jonathan Druart 2019-11-12 20:33:09 UTC
What about the FIXMEs?