Bug 12801

Summary: Koha::Calendar is caching holidays for the wrong branch!
Product: Koha Reporter: Kyle M Hall <kyle>
Component: HolidaysAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: critical    
Priority: P5 - low CC: brendan, jonathan.druart, katrin.fischer, olli-antti.kivilahti, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11112
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 12803    
Attachments: Bug 12801 - Unit Test
Bug 12801 - Remove caching
[ALT] Bug 12801: Use the branchcode to cache holidays
[SIGNED-OFF] Bug 12801: Use the branchcode to cache holidays
Bug 10126: (qa followup) fix tests
Bug 12801 - Unit Test
Bug 12801: Use the branchcode to cache holidays

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!