|
Description
Martin Renvoize (ashimema)
2020-06-12 10:13:06 UTC
Created attachment 105789 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Created attachment 105792 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Created attachment 105793 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation t/db_dependent/Holidays.t is failing # Failed test 'No tests run for subtest "exception_holidays() tests"' # at t/db_dependent/Holidays.t line 79. Can't locate object method "iterator" via package "0" (perhaps you forgot to load "0"?) at t/db_dependent/Holidays.t line 67. Created attachment 105830 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Created attachment 105831 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Created attachment 105832 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Created attachment 105833 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Created attachment 105834 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Created attachment 105835 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Created attachment 105836 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Should not we use the same code for single_holidays? (In reply to Jonathan Druart from comment #12) > Should not we use the same code for single_holidays? See bug 18519 [U+1F609] where I convert single holidays to use the hashref logic I settled on here. Hurray for getting rid of unnecessary dependencies :D *** Bug 18519 has been marked as a duplicate of this bug. *** *** Bug 14315 has been marked as a duplicate of this bug. *** Created attachment 105936 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Created attachment 105937 [details] [review] Bug 25723: Use the same code for single and exception holidays (In reply to Jonathan Druart from comment #17) > Created attachment 105936 [details] [review] [review] > Bug 25723: Use a hasref for lookup > > Rather than using an array of dates and iterating them, use a hashref > and do a direct lookup Patch moved from bug 18519. Created attachment 105939 [details] [review] Bug 25723: [ALTERNATIVE] Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. If you like the alternative implementation of _holidays in my final patch, I will continue on it and update the cache flushing mechanisms to reflect the updated cache key and logic. Created attachment 105941 [details] [review] Bug 25723: [ALTERNATIVE] Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Created attachment 105947 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Created attachment 105954 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Created attachment 105955 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Created attachment 105956 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Created attachment 105957 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Created attachment 105958 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Created attachment 105959 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Created attachment 105960 [details] [review] Bug 25723: Update cache flushing calls This patch updates the previous single_holidays and exeption_holidays cache flushing calls to match the new cache key structure of the updated routines. Created attachment 105961 [details] [review] Bug 25723: Remove tests for removed method We removed the exception_holidays (and single_holidays) methods as part of this patchset. As such, we should no longer test them ;) Created attachment 105962 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Created attachment 105963 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Created attachment 105964 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Created attachment 105965 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Created attachment 105966 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Created attachment 105967 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Created attachment 105968 [details] [review] Bug 25723: Update cache flushing calls This patch updates the previous single_holidays and exeption_holidays cache flushing calls to match the new cache key structure of the updated routines. Created attachment 105969 [details] [review] Bug 25723: Remove tests for removed method We removed the exception_holidays (and single_holidays) methods as part of this patchset. As such, we should no longer test them ;) Created attachment 105970 [details] [review] Bug 25723: (QA follow-up) Silent POD warning I didn't introduce the method that is warned about, but adding POD for it seemed reasonable and will silence the QA Script warning. We have this work deployed at a couple of sites now and hope to provide a signoff soon. Tested and signed off by Heart of England NHS Trust. At Nicks request I checked the backportability of this patchset (in relation to bug 26000 being identified as a bug whilst this patchset fixes that bug but also enhances the performance too). The patchset applies without issue to 20.05 and applies with minor changes to 19.11 and 19.05 branches (cpanfile doesn't exist yet in those branches) Created attachment 106991 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106992 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106993 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106994 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106995 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106996 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106997 [details] [review] Bug 25723: Update cache flushing calls This patch updates the previous single_holidays and exeption_holidays cache flushing calls to match the new cache key structure of the updated routines. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106998 [details] [review] Bug 25723: Remove tests for removed method We removed the exception_holidays (and single_holidays) methods as part of this patchset. As such, we should no longer test them ;) Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 106999 [details] [review] Bug 25723: (QA follow-up) Silent POD warning I didn't introduce the method that is warned about, but adding POD for it seemed reasonable and will silence the QA Script warning. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107000 [details] [review] Bug 25723: (QA follow-up) Handle holiday and exception on same day When a holiday is entered, then exceptions generated on a range, there exists both a holiday and exception in the special holidays table. We should cache the exception over the holiday instead of both Also, !1 in perl returns '' rather than 0, so we should explicitly set the value Add blank line to clear pod error from qa tools Signed-off-by: Nick Clemens <nick@bywatersolutions.com> (In reply to Nick Clemens from comment #53) > Created attachment 107000 [details] [review] [review] > Bug 25723: (QA follow-up) Handle holiday and exception on same day > > When a holiday is entered, then exceptions generated on a range, there > exists both a holiday and exception in > the special holidays table. We should cache the exception over the holiday > instead of both > > Also, !1 in perl returns '' rather than 0, so we should explicitly set the > value > > Add blank line to clear pod error from qa tools > > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Good catch and an elegant solution.. thanks :) Created attachment 107001 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107002 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107003 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107004 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107005 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107006 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107007 [details] [review] Bug 25723: Update cache flushing calls This patch updates the previous single_holidays and exeption_holidays cache flushing calls to match the new cache key structure of the updated routines. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107008 [details] [review] Bug 25723: Remove tests for removed method We removed the exception_holidays (and single_holidays) methods as part of this patchset. As such, we should no longer test them ;) Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107009 [details] [review] Bug 25723: (QA follow-up) Silent POD warning I didn't introduce the method that is warned about, but adding POD for it seemed reasonable and will silence the QA Script warning. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107010 [details] [review] Bug 25723: (QA follow-up) Handle holiday and exception on same day When a holiday is entered, then exceptions generated on a range, there exists both a holiday and exception in the special holidays table. We should cache the exception over the holiday instead of both Also, !1 in perl returns '' rather than 0, so we should explicitly set the value Add blank line to clear pod error from qa tools Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Restored SO lines for Emma and added my own to Nicks followup.. thanks Nick. Created attachment 107013 [details] [review] Bug 25723: [DO NOT PUSH] Benchmark Script A quick benchmark to compare DateTime::Set->contains to a simple hashref lookup Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 107014 [details] [review] Bug 25723: Drop DateTime::Set dependancy This patch removes our use of DateTime::Set and replaces it with a quick hashref implimentation Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107015 [details] [review] Bug 25723: (follow-up) Fix Unit Test The unit test previously relied on the internals of exception_holidays. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107016 [details] [review] Bug 25723: Drop DateTime::Set from cpanfile Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107017 [details] [review] Bug 25723: Use a hasref for lookup Rather than using an array of dates and iterating them, use a hashref and do a direct lookup Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107018 [details] [review] Bug 25723: Use the same code for single and exception holidays This patch refactors is_exception_holiday and is_single_holiday to utilise a single _holidays method which combines the logic of the previous single_holidays and exception_holidays methods. As Koha::Calendar is instantiated at a branch level, we also move split the cache into branches too. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107019 [details] [review] Bug 25723: Update cache flushing calls This patch updates the previous single_holidays and exeption_holidays cache flushing calls to match the new cache key structure of the updated routines. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107020 [details] [review] Bug 25723: Remove tests for removed method We removed the exception_holidays (and single_holidays) methods as part of this patchset. As such, we should no longer test them ;) Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107021 [details] [review] Bug 25723: (QA follow-up) Silent POD warning I didn't introduce the method that is warned about, but adding POD for it seemed reasonable and will silence the QA Script warning. Signed-off-by: Emma Perks <Emma.Perks2@uhb.nhs.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107022 [details] [review] Bug 25723: (QA follow-up) Handle holiday and exception on same day When a holiday is entered, then exceptions generated on a range, there exists both a holiday and exception in the special holidays table. We should cache the exception over the holiday instead of both Also, !1 in perl returns '' rather than 0, so we should explicitly set the value Add blank line to clear pod error from qa tools Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Re-introduce the blank line mentioned in the commit message, it was accidentally removed by automatic formatting Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 107023 [details] [review] Bug 25723: (QA follow-up) Remove unused module import The last usage of this module was removed in the commit "Bug 25723: Update cache flushing calls" but the import was not removed along with it. Created attachment 107025 [details] [review] Bug 25723: (QA follow-up) Remove libdatetime-set-perl dependency All usage to this module was removed. I added the two follow-up qa commits, would still like to someone go over them also just to make sure it's all good, although they are pretty trivial. Adding Mason to the cc list.. am I right in thinking the control file change gets taken care of as part of packaging/ci processes and is built from the cpanfile update I included here.. or should we now be looking for and submitting control file updates inline with bugs? Thanks so much for testing and QAing everyone :) Those followups look great to me Joonas, good catch on the no longer required import :) (In reply to Martin Renvoize from comment #79) > Adding Mason to the cc list.. am I right in thinking the control file change > gets taken care of as part of packaging/ci processes and is built from the > cpanfile update I included here.. or should we now be looking for and > submitting control file updates inline with bugs? You are right, missed the comment on top of the control file. I obsoleted that patch. Pushed to master for 20.11, thanks to everybody involved! backported to 20.05.x for 20.05.03 enhancement, not backported to 19.11.x (In reply to Aleisha Amohia from comment #83) > enhancement, not backported to 19.11.x This also fixes a bug, please consider backporting, or picking bug 26000 as an alternative |