If you try to use Koha::Calendar twice, the second instance will use the first instances special holidays!
Created attachment 31052 [details] [review] Bug 12801 - Unit Test
Created attachment 31053 [details] [review] Bug 12801 - Remove caching This patch removes the special holiday caching which fixes the bug, but causes a different unit test to fail. I do not no if the failing unit test is valid. If it is, this patch will cause a regression. If it is not valid, the test should be removed.
Is there a place where 2 instances are created with different branches?
The single_holidays has been cached by bug 11112.
(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.
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
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
Created attachment 31056 [details] [review] [ALT] Bug 12801: Use the branchcode to cache holidays
(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!
Created attachment 31075 [details] [review] [SIGNED-OFF] Bug 12801: Use the branchcode to cache holidays Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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!
Created attachment 32514 [details] [review] Bug 10126: (qa followup) fix tests It seems the removal of global variables changes the behaviour of Test::MockModule, and it now expects the namespace in front of the statically called method. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Created attachment 32515 [details] [review] Bug 12801 - Unit Test Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
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>
Patches pushed to master. Thanks Kyle and Jonathan!
Thanks for fixing this!