Summary: | Koha::Calendar is caching holidays for the wrong branch! | ||
---|---|---|---|
Product: | Koha | Reporter: | Kyle M Hall (khall) <kyle> |
Component: | Holidays | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | brendan, jonathan.druart, olli-antti.kivilahti, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11112 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
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 (khall)
2014-08-21 13:46:51 UTC
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? (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! |