Bug 23974 - hours_between and days_between lack tests
Summary: hours_between and days_between lack tests
Status: RESOLVED FIXED
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: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-05 16:41 UTC by Jonathan Druart
Modified: 2020-06-04 20:37 UTC (History)
4 users (show)

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


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
Bug 23974: (follow-up) Add same day loan tests (2.31 KB, patch)
2019-11-29 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add POD for hours_between (1.38 KB, patch)
2019-11-29 17:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Test Notes (1.76 KB, patch)
2019-11-29 17:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add POD for hours_between (1.38 KB, patch)
2019-12-02 09:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Test Notes (1.76 KB, patch)
2019-12-02 09:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add tests for issue on closed day (3.07 KB, patch)
2019-12-02 09:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Add test descriptions (11.71 KB, patch)
2019-12-02 09:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23974: Improve readability (4.70 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Add new tests for hours_between (6.06 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Add tests for days_between (7.39 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: both methods should have the same logic/construction (2.16 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: (follow-up) Add same day loan tests (2.37 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Add POD for hours_between (1.44 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Test Notes (1.82 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Add tests for issue on closed day (3.13 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Add test descriptions (11.77 KB, patch)
2020-01-03 06:52 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 23974: Improve readability (4.74 KB, patch)
2020-01-09 19:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Add new tests for hours_between (6.11 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Add tests for days_between (7.44 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: both methods should have the same logic/construction (2.21 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: (follow-up) Add same day loan tests (2.42 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Add POD for hours_between (1.49 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Test Notes (1.87 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Add tests for issue on closed day (3.18 KB, patch)
2020-01-09 19:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23974: Add test descriptions (11.82 KB, patch)
2020-01-09 19:05 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 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?
Comment 15 Tomás Cohen Arazi 2019-11-29 14:45:11 UTC
Minor issue with variable declared twice on the tests ('Different hours' subtest).
Comment 16 Martin Renvoize 2019-11-29 16:01:32 UTC
Created attachment 95889 [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 17 Martin Renvoize 2019-11-29 16:10:27 UTC
Corrected the double variable.
Comment 18 Martin Renvoize 2019-11-29 17:10:19 UTC
Created attachment 95890 [details] [review]
Bug 23974: Add POD for hours_between

Add a bit of missing POD whilst we're working in here
Comment 19 Martin Renvoize 2019-11-29 17:10:22 UTC
Created attachment 95891 [details] [review]
Bug 23974: Test Notes
Comment 20 Martin Renvoize 2019-11-29 17:11:47 UTC
We do not yet test for the behaviour when passing a start_dt which is a holiday.. I think to get full coverage we should test that case too.
Comment 21 Martin Renvoize 2019-12-02 09:23:22 UTC
Created attachment 95893 [details] [review]
Bug 23974: Add POD for hours_between

Add a bit of missing POD whilst we're working in here
Comment 22 Martin Renvoize 2019-12-02 09:23:25 UTC
Created attachment 95894 [details] [review]
Bug 23974: Test Notes
Comment 23 Martin Renvoize 2019-12-02 09:23:28 UTC
Created attachment 95895 [details] [review]
Bug 23974: Add tests for issue on closed day
Comment 24 Martin Renvoize 2019-12-02 09:23:31 UTC
Created attachment 95896 [details] [review]
Bug 23974: Add test descriptions
Comment 25 Martin Renvoize 2019-12-13 09:35:12 UTC
How are we doing here?  I think this is close to ready now.. we're well tested for the existing functionality and I think the notes are actually helpful as they for future reference and we can tidy them up as and when we come to needing to implement changes..  the pragmatic side of me wants to push this sooner rather than later.
Comment 26 Katrin Fischer 2020-01-03 06:52:04 UTC
Created attachment 96778 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2020-01-03 06:52:07 UTC
Created attachment 96779 [details] [review]
Bug 23974: Add new tests for hours_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Katrin Fischer 2020-01-03 06:52:10 UTC
Created attachment 96780 [details] [review]
Bug 23974: Add tests for days_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2020-01-03 06:52:14 UTC
Created attachment 96781 [details] [review]
Bug 23974: both methods should have the same logic/construction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2020-01-03 06:52:17 UTC
Created attachment 96782 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2020-01-03 06:52:20 UTC
Created attachment 96783 [details] [review]
Bug 23974: Add POD for hours_between

Add a bit of missing POD whilst we're working in here

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 32 Katrin Fischer 2020-01-03 06:52:24 UTC
Created attachment 96784 [details] [review]
Bug 23974: Test Notes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Katrin Fischer 2020-01-03 06:52:27 UTC
Created attachment 96785 [details] [review]
Bug 23974: Add tests for issue on closed day

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2020-01-03 06:52:31 UTC
Created attachment 96786 [details] [review]
Bug 23974: Add test descriptions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-01-03 06:55:02 UTC
The dangerous bits here are the changes to Calendar.pm and not the .t parts. 
Testing focused on tests passing - changes appear to make sense to me. Expecting QA to take another closer look.
Comment 36 Tomás Cohen Arazi 2020-01-09 19:04:59 UTC
Created attachment 97129 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Tomás Cohen Arazi 2020-01-09 19:05:03 UTC
Created attachment 97130 [details] [review]
Bug 23974: Add new tests for hours_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Tomás Cohen Arazi 2020-01-09 19:05:06 UTC
Created attachment 97131 [details] [review]
Bug 23974: Add tests for days_between

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2020-01-09 19:05:10 UTC
Created attachment 97132 [details] [review]
Bug 23974: both methods should have the same logic/construction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2020-01-09 19:05:13 UTC
Created attachment 97133 [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>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2020-01-09 19:05:17 UTC
Created attachment 97134 [details] [review]
Bug 23974: Add POD for hours_between

Add a bit of missing POD whilst we're working in here

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 42 Tomás Cohen Arazi 2020-01-09 19:05:21 UTC
Created attachment 97135 [details] [review]
Bug 23974: Test Notes

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Tomás Cohen Arazi 2020-01-09 19:05:24 UTC
Created attachment 97136 [details] [review]
Bug 23974: Add tests for issue on closed day

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Tomás Cohen Arazi 2020-01-09 19:05:28 UTC
Created attachment 97137 [details] [review]
Bug 23974: Add test descriptions

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Martin Renvoize 2020-01-10 08:39:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 46 Joy Nelson 2020-01-30 23:22:02 UTC
Pushed to 19.11.x branch for 19.11.03