Bug 25723

Summary: Improve efficiency of holiday calculation
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Joonas Kylmälä <joonas.kylmala>
Severity: enhancement    
Priority: P5 - low CC: aleisha, barton, dcook, emma.perks, jonathan.druart, jonathan.field, joonas.kylmala, kyle, lucas, mtj, nick, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25850
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26000
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03
Bug Depends on:    
Bug Blocks: 25781    
Attachments: Bug 25723: Drop DateTime::Set dependancy
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: [ALTERNATIVE] Use the same code for single and exception holidays
Bug 25723: [ALTERNATIVE] Use the same code for single and exception holidays
Bug 25723: Use the same code for single and exception holidays
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: (QA follow-up) Silent POD warning
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: (QA follow-up) Silent POD warning
Bug 25723: (QA follow-up) Handle holiday and exception on same day
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: (QA follow-up) Silent POD warning
Bug 25723: (QA follow-up) Handle holiday and exception on same day
Bug 25723: [DO NOT PUSH] Benchmark Script
Bug 25723: Drop DateTime::Set dependancy
Bug 25723: (follow-up) Fix Unit Test
Bug 25723: Drop DateTime::Set from cpanfile
Bug 25723: Use a hasref for lookup
Bug 25723: Use the same code for single and exception holidays
Bug 25723: Update cache flushing calls
Bug 25723: Remove tests for removed method
Bug 25723: (QA follow-up) Silent POD warning
Bug 25723: (QA follow-up) Handle holiday and exception on same day
Bug 25723: (QA follow-up) Remove unused module import
Bug 25723: (QA follow-up) Remove libdatetime-set-perl dependency

Description Martin Renvoize 2020-06-12 10:13:06 UTC
DateTime::Set is used in one place and one place only, and we only utilise a single method from it.

We should replace that occurrence, exception_holidays, with logic matching that from 'single_holidays' which achieves the same end.

I will attempt to do that and profile the difference too.
Comment 1 Martin Renvoize 2020-06-12 10:55:49 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
Comment 2 Martin Renvoize 2020-06-12 11:41:53 UTC
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
Comment 3 Martin Renvoize 2020-06-12 11:41:56 UTC
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
Comment 4 Jonathan Druart 2020-06-12 11:54:42 UTC
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.
Comment 5 Martin Renvoize 2020-06-12 14:56:31 UTC
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
Comment 6 Martin Renvoize 2020-06-12 14:56:34 UTC
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
Comment 7 Martin Renvoize 2020-06-12 14:56:38 UTC
Created attachment 105832 [details] [review]
Bug 25723: (follow-up) Fix Unit Test

The unit test previously relied on the internals of exception_holidays.
Comment 8 Martin Renvoize 2020-06-12 15:03:20 UTC
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
Comment 9 Martin Renvoize 2020-06-12 15:03:24 UTC
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
Comment 10 Martin Renvoize 2020-06-12 15:03:27 UTC
Created attachment 105835 [details] [review]
Bug 25723: (follow-up) Fix Unit Test

The unit test previously relied on the internals of exception_holidays.
Comment 11 Martin Renvoize 2020-06-12 15:03:30 UTC
Created attachment 105836 [details] [review]
Bug 25723: Drop DateTime::Set from cpanfile
Comment 12 Jonathan Druart 2020-06-16 09:45:41 UTC
Should not we use the same code for single_holidays?
Comment 13 Martin Renvoize 2020-06-16 14:41:33 UTC
(In reply to Jonathan Druart from comment #12)
> Should not we use the same code for single_holidays?

See bug 18519 😉 where I convert single holidays to use the hashref logic I settled on here.
Comment 14 David Cook 2020-06-17 04:25:18 UTC
Hurray for getting rid of unnecessary dependencies :D
Comment 15 Martin Renvoize 2020-06-17 06:58:59 UTC
*** Bug 18519 has been marked as a duplicate of this bug. ***
Comment 16 Martin Renvoize 2020-06-17 06:59:18 UTC
*** Bug 14315 has been marked as a duplicate of this bug. ***
Comment 17 Jonathan Druart 2020-06-17 08:11:38 UTC
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
Comment 18 Jonathan Druart 2020-06-17 08:11:42 UTC
Created attachment 105937 [details] [review]
Bug 25723: Use the same code for single and exception holidays
Comment 19 Jonathan Druart 2020-06-17 08:12:18 UTC
(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.
Comment 20 Martin Renvoize 2020-06-17 10:18:40 UTC
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.
Comment 21 Martin Renvoize 2020-06-17 10:20:11 UTC
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.
Comment 22 Martin Renvoize 2020-06-17 10:44:39 UTC
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.
Comment 23 Jonathan Druart 2020-06-17 11:43:53 UTC
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.
Comment 24 Martin Renvoize 2020-06-17 12:48:29 UTC
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
Comment 25 Martin Renvoize 2020-06-17 12:48:33 UTC
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
Comment 26 Martin Renvoize 2020-06-17 12:48:37 UTC
Created attachment 105956 [details] [review]
Bug 25723: (follow-up) Fix Unit Test

The unit test previously relied on the internals of exception_holidays.
Comment 27 Martin Renvoize 2020-06-17 12:48:41 UTC
Created attachment 105957 [details] [review]
Bug 25723: Drop DateTime::Set from cpanfile
Comment 28 Martin Renvoize 2020-06-17 12:48:44 UTC
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
Comment 29 Martin Renvoize 2020-06-17 12:48:50 UTC
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.
Comment 30 Martin Renvoize 2020-06-17 12:48:54 UTC
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.
Comment 31 Martin Renvoize 2020-06-17 12:48:57 UTC
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 ;)
Comment 32 Martin Renvoize 2020-06-17 13:07:22 UTC
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
Comment 33 Martin Renvoize 2020-06-17 13:07:25 UTC
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
Comment 34 Martin Renvoize 2020-06-17 13:07:29 UTC
Created attachment 105964 [details] [review]
Bug 25723: (follow-up) Fix Unit Test

The unit test previously relied on the internals of exception_holidays.
Comment 35 Martin Renvoize 2020-06-17 13:07:33 UTC
Created attachment 105965 [details] [review]
Bug 25723: Drop DateTime::Set from cpanfile
Comment 36 Martin Renvoize 2020-06-17 13:07:36 UTC
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
Comment 37 Martin Renvoize 2020-06-17 13:07:40 UTC
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.
Comment 38 Martin Renvoize 2020-06-17 13:07:43 UTC
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.
Comment 39 Martin Renvoize 2020-06-17 13:07:47 UTC
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 ;)
Comment 40 Martin Renvoize 2020-06-17 13:07:50 UTC
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.
Comment 41 Martin Renvoize 2020-07-02 11:49:27 UTC
We have this work deployed at a couple of sites now and hope to provide a signoff soon.
Comment 42 Martin Renvoize 2020-07-09 12:21:40 UTC
Tested and signed off by Heart of England NHS Trust.
Comment 43 Martin Renvoize 2020-07-17 07:16:30 UTC
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)
Comment 44 Nick Clemens 2020-07-17 12:57:31 UTC
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>
Comment 45 Nick Clemens 2020-07-17 12:57:35 UTC
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>
Comment 46 Nick Clemens 2020-07-17 12:57:38 UTC
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>
Comment 47 Nick Clemens 2020-07-17 12:57:41 UTC
Created attachment 106994 [details] [review]
Bug 25723: Drop DateTime::Set from cpanfile

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 48 Nick Clemens 2020-07-17 12:57:45 UTC
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>
Comment 49 Nick Clemens 2020-07-17 12:57:49 UTC
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>
Comment 50 Nick Clemens 2020-07-17 12:57:52 UTC
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>
Comment 51 Nick Clemens 2020-07-17 12:57:56 UTC
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>
Comment 52 Nick Clemens 2020-07-17 12:57:59 UTC
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>
Comment 53 Nick Clemens 2020-07-17 12:58:03 UTC
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>
Comment 54 Martin Renvoize 2020-07-17 13:20:03 UTC
(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 :)
Comment 55 Martin Renvoize 2020-07-17 13:27:47 UTC
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>
Comment 56 Martin Renvoize 2020-07-17 13:27:51 UTC
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>
Comment 57 Martin Renvoize 2020-07-17 13:27:55 UTC
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>
Comment 58 Martin Renvoize 2020-07-17 13:27:59 UTC
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>
Comment 59 Martin Renvoize 2020-07-17 13:28:03 UTC
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>
Comment 60 Martin Renvoize 2020-07-17 13:28:08 UTC
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>
Comment 61 Martin Renvoize 2020-07-17 13:28:12 UTC
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>
Comment 62 Martin Renvoize 2020-07-17 13:28:16 UTC
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>
Comment 63 Martin Renvoize 2020-07-17 13:28:20 UTC
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>
Comment 64 Martin Renvoize 2020-07-17 13:28:25 UTC
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>
Comment 65 Martin Renvoize 2020-07-17 13:29:20 UTC
Restored SO lines for Emma and added my own to Nicks followup.. thanks Nick.
Comment 66 Joonas Kylmälä 2020-07-17 14:42:13 UTC
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>
Comment 67 Joonas Kylmälä 2020-07-17 14:42:18 UTC
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>
Comment 68 Joonas Kylmälä 2020-07-17 14:42:22 UTC
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>
Comment 69 Joonas Kylmälä 2020-07-17 14:42:26 UTC
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>
Comment 70 Joonas Kylmälä 2020-07-17 14:42:30 UTC
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>
Comment 71 Joonas Kylmälä 2020-07-17 14:42:34 UTC
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>
Comment 72 Joonas Kylmälä 2020-07-17 14:42:38 UTC
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>
Comment 73 Joonas Kylmälä 2020-07-17 14:42:42 UTC
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>
Comment 74 Joonas Kylmälä 2020-07-17 14:42:46 UTC
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>
Comment 75 Joonas Kylmälä 2020-07-17 14:42:50 UTC
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>
Comment 76 Joonas Kylmälä 2020-07-17 14:42:54 UTC
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.
Comment 77 Joonas Kylmälä 2020-07-17 14:57:28 UTC
Created attachment 107025 [details] [review]
Bug 25723: (QA follow-up) Remove libdatetime-set-perl dependency

All usage to this module was removed.
Comment 78 Joonas Kylmälä 2020-07-17 14:59:15 UTC
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.
Comment 79 Martin Renvoize 2020-07-17 15:23:27 UTC
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 :)
Comment 80 Joonas Kylmälä 2020-07-20 06:37:59 UTC
(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.
Comment 81 Jonathan Druart 2020-07-20 15:49:04 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 82 Lucas Gass 2020-07-27 17:47:46 UTC
backported to 20.05.x for 20.05.03
Comment 83 Aleisha Amohia 2020-08-03 23:26:40 UTC
enhancement, not backported to 19.11.x
Comment 84 Nick Clemens 2020-08-04 02:05:07 UTC
(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