Bug 12801 - Koha::Calendar is caching holidays for the wrong branch!
Summary: Koha::Calendar is caching holidays for the wrong branch!
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Holidays (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 12803
  Show dependency treegraph
 
Reported: 2014-08-21 13:46 UTC by Kyle M Hall
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12801 - Unit Test (1.39 KB, patch)
2014-08-21 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12801 - Remove caching (1.05 KB, patch)
2014-08-21 13:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
[ALT] Bug 12801: Use the branchcode to cache holidays (2.33 KB, patch)
2014-08-21 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12801: Use the branchcode to cache holidays (2.40 KB, patch)
2014-08-22 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10126: (qa followup) fix tests (1.04 KB, patch)
2014-10-17 15:48 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12801 - Unit Test (1.45 KB, patch)
2014-10-17 15:48 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 12801: Use the branchcode to cache holidays (2.45 KB, patch)
2014-10-17 15:48 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-08-21 13:46:51 UTC
If you try to use Koha::Calendar twice, the second instance will use the first instances special holidays!
Comment 1 Kyle M Hall 2014-08-21 13:47:41 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2014-08-21 13:49:19 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-08-21 14:00:21 UTC
Is there a place where 2 instances are created with different branches?
Comment 4 Jonathan Druart 2014-08-21 14:01:41 UTC
The single_holidays has been cached by bug 11112.
Comment 5 Kyle M Hall 2014-08-21 14:32:52 UTC
(In reply to Jonathan Druart from comment #3)
> Is there a place where 2 instances are created with different branches?

I discovered this while attempting to use is_holiday in a new feature which would use an instance for each branch.
Comment 6 Jonathan Druart 2014-08-21 14:45:27 UTC
I am convinced we should keep the cache. You will get performance issues if you remove it.
Maybe another way to fix this could be to cache using a hash:
branchcode_1 => $single_holidays_for_branch_1
branchcode_2 => $single_holidays_for_branch_2
Comment 7 Kyle M Hall 2014-08-21 15:05:59 UTC
Would you be able to submit a replacement for my second patch?

(In reply to Jonathan Druart from comment #6)
> I am convinced we should keep the cache. You will get performance issues if
> you remove it.
> Maybe another way to fix this could be to cache using a hash:
> branchcode_1 => $single_holidays_for_branch_1
> branchcode_2 => $single_holidays_for_branch_2
Comment 8 Jonathan Druart 2014-08-21 15:24:21 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-08-21 15:24:54 UTC
(In reply to Kyle M Hall from comment #7)
> Would you be able to submit a replacement for my second patch?

Something like that?
Note: I didn't test this patch!
Comment 10 Kyle M Hall 2014-08-22 11:14:06 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2014-08-22 11:15:11 UTC
Looks good. Thanks!

(In reply to Jonathan Druart from comment #9)
> (In reply to Kyle M Hall from comment #7)
> > Would you be able to submit a replacement for my second patch?
> 
> Something like that?
> Note: I didn't test this patch!
Comment 12 Brendan Gallagher 2014-10-17 15:48:25 UTC Comment hidden (obsolete)
Comment 13 Brendan Gallagher 2014-10-17 15:48:37 UTC
Created attachment 32515 [details] [review]
Bug 12801 - Unit Test

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 14 Brendan Gallagher 2014-10-17 15:48:43 UTC
Created attachment 32516 [details] [review]
Bug 12801: Use the branchcode to cache holidays

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 15 Tomás Cohen Arazi 2014-10-18 13:59:06 UTC
Patches pushed to master.

Thanks Kyle and Jonathan!
Comment 16 Olli-Antti Kivilahti 2014-12-09 09:53:13 UTC
Thanks for fixing this!