Bug 26000 - Holiday exceptions are incorrectly cached for an individual branch
Summary: Holiday exceptions are incorrectly cached for an individual branch
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 19.11
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
: 19426 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-07-16 18:14 UTC by Nick Clemens
Modified: 2020-09-20 14:57 UTC (History)
8 users (show)

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


Attachments
Bug 26000: Unit test (2.27 KB, patch)
2020-07-16 19:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26000: Cache all branches for exception_holidays (5.41 KB, patch)
2020-07-16 19:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26000: [alternate] Make exception_holidays cache with branch in key (5.46 KB, patch)
2020-07-17 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26000: Unit test (2.36 KB, patch)
2020-08-04 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26000: Make exception_holidays cache with branch in key (4.89 KB, patch)
2020-08-04 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26000: Unit test (2.42 KB, patch)
2020-08-04 13:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26000: Make exception_holidays cache with branch in key (4.96 KB, patch)
2020-08-04 13:13 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 Nick Clemens 2020-07-16 18:14:02 UTC
When updating exception holidays they are cleared from the cache until Koha::Calendar->exception_holidays is called

That routine checks the cache, and then generates the holidays for a single branch and sets them in the cache. This is incorrect.

single_holidays handles the situation correctly and exception_holidays should do the same.
Comment 1 Nick Clemens 2020-07-16 19:37:33 UTC
Created attachment 106968 [details] [review]
Bug 26000: Unit test
Comment 2 Nick Clemens 2020-07-16 19:37:35 UTC
Created attachment 106969 [details] [review]
Bug 26000: Cache all branches for exception_holidays

To test:
0 - Have an itemtype rule shared for branch A and B, or same period
1 - Set some holidays for branch A within circ period for an itemtype
2 - Set same holidays, but make some an exception, for branch b
3 - Set useDaysMode to 'Use the calendar to skip days the library is closed'
4 - Checkout an item for branch A, date is pushed forward
5 - Checkout an item for branch B, date is pushed forward?
    It hsould not be the same as A because fo holiday exception
6 - Apply patch
7 - Restart all
8 - Checkout an item for branch A, date is pushed forward
9 - Checkout an item for branch B, date is not pushed forward
Comment 3 Martin Renvoize 2020-07-16 21:04:16 UTC
Lots of overlap here with bug 25723.. might be worth a look 😉
Comment 4 Nick Clemens 2020-07-17 11:33:05 UTC
Created attachment 106987 [details] [review]
Bug 26000: [alternate] Make exception_holidays cache with branch in key

In light of 25723 offering a simpler solution here.
This would just let us avoid backporting larger changes, but this can be
marked as a duplicate if 25723 is preferred to this change
Comment 5 Nick Clemens 2020-07-17 14:30:49 UTC
*** Bug 19426 has been marked as a duplicate of this bug. ***
Comment 6 Jonathan Druart 2020-07-21 11:23:48 UTC
So this is for stable branches only?
Comment 7 Martin Renvoize 2020-07-21 13:02:04 UTC
I think bug 25723 is a trivial backport for 20.05, but this patch is still useful for 19.11 and beyond.. I'd be interested to see what the remains do.. if they all choose to backport 25723 then this is entirely a duplicate and can be closed.

I'll touch base with them.
Comment 8 Aleisha Amohia 2020-08-04 02:15:30 UTC
I did not backport Bug 25723 to 19.11.x because it was framed as an enhancement. If this patch could be reworked on top of that for 19.11.x, I'd be happy to backport this fix as it sorts a bug.
Comment 9 Nick Clemens 2020-08-04 11:54:38 UTC
Created attachment 107746 [details] [review]
Bug 26000: Unit test
Comment 10 Nick Clemens 2020-08-04 11:54:41 UTC
Created attachment 107747 [details] [review]
Bug 26000: Make exception_holidays cache with branch in key

In light of 25723 offering a simpler solution here.
This would just let us avoid backporting larger changes, but this can be
marked as a duplicate if 25723 is preferred to this change
Comment 11 Nick Clemens 2020-08-04 11:56:08 UTC
(In reply to Aleisha Amohia from comment #8)
> I did not backport Bug 25723 to 19.11.x because it was framed as an
> enhancement. If this patch could be reworked on top of that for 19.11.x, I'd
> be happy to backport this fix as it sorts a bug.

These apply cleanly to 19.11.x and tests pass - you only need this or 25723, not both. It would be nice to get an SO on this one. Martin, could you check as you wrote the other fix? :-)
Comment 12 Martin Renvoize 2020-08-04 13:13:18 UTC
Created attachment 107762 [details] [review]
Bug 26000: Unit test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2020-08-04 13:13:21 UTC
Created attachment 107763 [details] [review]
Bug 26000: Make exception_holidays cache with branch in key

In light of 25723 offering a simpler solution here.
This would just let us avoid backporting larger changes, but this can be
marked as a duplicate if 25723 is preferred to this change

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2020-08-04 13:14:07 UTC
Passing QA

Works well and is basically a simplified version of but 25723 for backport.
Comment 15 Jonathan Druart 2020-08-07 07:24:43 UTC
Sending to oldstable RM queue.
Comment 16 Aleisha Amohia 2020-08-19 05:49:45 UTC
backported to 19.11.x for 19.11.09
Comment 17 Victor Grousset/tuxayo 2020-09-20 14:57:24 UTC
Backported to 19.05.x branch for 19.05.15