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).
Created attachment 95062 [details] [review] Bug 23974: Improve readability This patch does not change anything, just a bit of cleaning
Created attachment 95063 [details] [review] Bug 23974: Add new tests for hours_between
Created attachment 95064 [details] [review] Bug 23974: Add tests for days_between
Created attachment 95065 [details] [review] Bug 23974: both methods should have the same logic/construction
Now that we have a base for tests we can discuss the expected behaviors/values :)
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>
Created attachment 95211 [details] [review] Bug 23974: Add new tests for hours_between Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 95212 [details] [review] Bug 23974: Add tests for days_between Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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.
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 ?
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?
What about the FIXMEs?
Minor issue with variable declared twice on the tests ('Different hours' subtest).
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>
Corrected the double variable.
Created attachment 95890 [details] [review] Bug 23974: Add POD for hours_between Add a bit of missing POD whilst we're working in here
Created attachment 95891 [details] [review] Bug 23974: Test Notes
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.
Created attachment 95893 [details] [review] Bug 23974: Add POD for hours_between Add a bit of missing POD whilst we're working in here
Created attachment 95894 [details] [review] Bug 23974: Test Notes
Created attachment 95895 [details] [review] Bug 23974: Add tests for issue on closed day
Created attachment 95896 [details] [review] Bug 23974: Add test descriptions
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.
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>
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>
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>
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>
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>
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>
Created attachment 96784 [details] [review] Bug 23974: Test Notes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 96785 [details] [review] Bug 23974: Add tests for issue on closed day Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 96786 [details] [review] Bug 23974: Add test descriptions Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
Nice work everyone! Pushed to master for 20.05
Pushed to 19.11.x branch for 19.11.03