Bug 38357 - When adding new holidays Koha sometimes copies same holidays to other librarys
Summary: When adding new holidays Koha sometimes copies same holidays to other librarys
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Holidays (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Jonathan Druart
QA Contact: Emily Lamancusa (emlam)
URL:
Keywords: rel_22_11_candidate
Depends on:
Blocks:
 
Reported: 2024-11-05 13:47 UTC by Piia Semenoff
Modified: 2025-03-31 09:51 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.06,23.11.11
Circulation function:


Attachments
Holidays module (calendar) (79.56 KB, application/pdf)
2024-11-05 13:47 UTC, Piia Semenoff
Details
Bug 38357: Prevent holidays to be wrongly added (1.39 KB, patch)
2024-11-06 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 38357: Prevent holidays to be wrongly added (1.45 KB, patch)
2024-11-07 10:47 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 38357: Prevent holidays to be wrongly added (1.51 KB, patch)
2024-11-12 18:18 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Piia Semenoff 2024-11-05 13:47:26 UTC
Created attachment 173980 [details]
Holidays module (calendar)

I noticed that when I add unique holiday to a library and repeat adding different holidays to two other librarys, the third library will have random unique holidays even if it should't have.

It's easier to explain with pictures so I put them in an PDF and added it as an attachment. But here's what I did:

1. I added one week as unique holidays to Tyrnava library from 2025.03.03.to 2025.03.09. All went well.

2. I added one week as unique holidays to Tupos library from 2025.03.10 to 2025.03.16. All went well.

3. I added one week as unique holidays to Tuira library from 2025.03.17 to 2025.03.23. This didin't go so well: Tuira had unique holidays two weeks, but I added only one week.

4.  I added one week as unique holidays to Temmes library from 2025.03.03 to 2025.03.09. This didin't go so well either: After saving Temmes had unique holidays three weeks, but I added only one week.

5. I added one week as unique holidays to Taivalkoski library from 2025.03.24 to 2025.03.30. This time after saving Taivalkoski's holidays, library had unique holidays two weeks (in the beginning of March and in the end of March), but I added only one week.

These above are easy to fix because you see them at once, but we had also unique holidays that appeared on the other months (for example christmas-holidays appeared to librarys that are open on christmastime and don't need holiday-markings on their calendar.) so those extra-holidays could be hard to catch.

We don't have this problem in Koha version 23.11, but in the version 24.05 it is going to be a problem.
Comment 1 Emmi Takkinen 2024-11-06 07:30:29 UTC
Adding my test results to this. I strongly believe this has something to do with plack, since I'm unable to reproduce this on my own test environments where I don't have plack running. We tested this with Piia on our upcoming upgrades test environment and this seems to follow a certain pattern: adding holidays to first and second library works as expected. Then adding holidays to the third one and all the following libraries fail. After restarting plack everything works fine for the first two libraries and then it fails again. Piia also tested this on sandbox and was able reproduce this also there.

We had same kind of problem with version 17.05, but it didn't happen again after we upgraded to version 22.11. I'm not sure if this followed same pattern then but symptoms were the same.
Comment 2 Jonathan Druart 2024-11-06 08:43:07 UTC
Created attachment 174065 [details] [review]
Bug 38357: Prevent holidays to be wrongly added

This patch resets @holiday_list to an empty array, to prevent holidays
to be accumulated in worker's memory.

If you added single holidays to library A then others to library B,
holidays from A were also added to B.

Test plan:
Go to /cgi-bin/koha/tools/holidays.pl
Define the holidays for Centerville
 Select a date
 To: another date (pick only 2 days to ease testing)
 Set a title "cpl"
 Tick "Holidays on a range"
 Save
Define the holidays for Fairview
 Select *another* date
 To: another date (pick only 2 days to ease testing)
 Set a title "fpl"
 Tick "Holidays on a range"
 Save

=> Without this patch Fairview has 4 days of holidays instead of 2
=> With this patch applied the dates you selected are considered
holidays for Fairview
Comment 3 Jonathan Druart 2024-11-06 08:43:42 UTC
I haven't tried on 23.11 but I didn't find what could have caused this in the git log.
Comment 4 Jonathan Druart 2024-11-06 08:44:43 UTC
(In reply to Emmi Takkinen from comment #1)
> Adding my test results to this. I strongly believe this has something to do
> with plack, since I'm unable to reproduce this on my own test environments
> where I don't have plack running. We tested this with Piia on our upcoming
> upgrades test environment and this seems to follow a certain pattern: adding
> holidays to first and second library works as expected. Then adding holidays
> to the third one and all the following libraries fail. After restarting
> plack everything works fine for the first two libraries and then it fails
> again. Piia also tested this on sandbox and was able reproduce this also
> there.

I get the wrong behaviour for the second library. Can you please try the patch and see if it fixes the problem you described? Maybe there is still something else to fix.
Comment 5 Emmi Takkinen 2024-11-07 10:36:45 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Emmi Takkinen from comment #1)
> > Adding my test results to this. I strongly believe this has something to do
> > with plack, since I'm unable to reproduce this on my own test environments
> > where I don't have plack running. We tested this with Piia on our upcoming
> > upgrades test environment and this seems to follow a certain pattern: adding
> > holidays to first and second library works as expected. Then adding holidays
> > to the third one and all the following libraries fail. After restarting
> > plack everything works fine for the first two libraries and then it fails
> > again. Piia also tested this on sandbox and was able reproduce this also
> > there.
> 
> I get the wrong behaviour for the second library. Can you please try the
> patch and see if it fixes the problem you described? Maybe there is still
> something else to fix.

Piia tested this on 24.05.01 and it works! She didn't find any other problems so this probably can be signed off. Thank you for quick fix :)
Comment 6 Emmi Takkinen 2024-11-07 10:45:04 UTC
Tested this also in main and calendar works as expected.
Comment 7 Emmi Takkinen 2024-11-07 10:47:02 UTC
Created attachment 174110 [details] [review]
Bug 38357: Prevent holidays to be wrongly added

This patch resets @holiday_list to an empty array, to prevent holidays
to be accumulated in worker's memory.

If you added single holidays to library A then others to library B,
holidays from A were also added to B.

Test plan:
Go to /cgi-bin/koha/tools/holidays.pl
Define the holidays for Centerville
 Select a date
 To: another date (pick only 2 days to ease testing)
 Set a title "cpl"
 Tick "Holidays on a range"
 Save
Define the holidays for Fairview
 Select *another* date
 To: another date (pick only 2 days to ease testing)
 Set a title "fpl"
 Tick "Holidays on a range"
 Save

=> Without this patch Fairview has 4 days of holidays instead of 2
=> With this patch applied the dates you selected are considered
holidays for Fairview

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 8 Emily Lamancusa (emlam) 2024-11-12 18:18:48 UTC
Created attachment 174435 [details] [review]
Bug 38357: Prevent holidays to be wrongly added

This patch resets @holiday_list to an empty array, to prevent holidays
to be accumulated in worker's memory.

If you added single holidays to library A then others to library B,
holidays from A were also added to B.

Test plan:
Go to /cgi-bin/koha/tools/holidays.pl
Define the holidays for Centerville
 Select a date
 To: another date (pick only 2 days to ease testing)
 Set a title "cpl"
 Tick "Holidays on a range"
 Save
Define the holidays for Fairview
 Select *another* date
 To: another date (pick only 2 days to ease testing)
 Set a title "fpl"
 Tick "Holidays on a range"
 Save

=> Without this patch Fairview has 4 days of holidays instead of 2
=> With this patch applied the dates you selected are considered
holidays for Fairview

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 9 Katrin Fischer 2024-11-18 16:08:38 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 10 Lucas Gass (lukeg) 2024-12-05 22:46:37 UTC
Backported to 24.05.x for upcoming 24.05.06
Comment 11 Fridolin Somers 2024-12-18 13:27:42 UTC
Pushed to 23.11.x for 23.11.11
Comment 12 Jesse Maseto 2025-02-03 16:22:31 UTC
Not pushed to LTS. Marked Resolved.

If you feel this should be in LTS please reply with your reason.
Comment 13 Tomás Cohen Arazi (tcohen) 2025-02-18 11:14:45 UTC
Is there a reason not to get rid of all the `our` variable definitions? I'm not sure we know exactly why it is being used.

We only know those variables persist in memory in Plack mode (and other persistent runtime environments).
Comment 14 Tomás Cohen Arazi (tcohen) 2025-02-18 11:16:44 UTC
(In reply to Jesse Maseto from comment #12)
> Not pushed to LTS. Marked Resolved.
> 
> If you feel this should be in LTS please reply with your reason.

I feel this should be backported if possible.
Comment 15 Jonathan Druart 2025-03-10 11:51:24 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #13)
> Is there a reason not to get rid of all the `our` variable definitions? I'm
> not sure we know exactly why it is being used.
> 
> We only know those variables persist in memory in Plack mode (and other
> persistent runtime environments).

It has a global scope, and is available from the subroutines.
When switching to plack we (lazily) switched some "my" to "our" to just make things worked.

It can be fixed, but it's not simply a s/^our/my/g substitution.

But yes, it must be done.
Comment 16 Jonathan Druart 2025-03-10 11:52:37 UTC
```
my $global;
foo();
foo();
warn $global;

sub foo {
    $global++;
}
```

=> Variable "$global" is not available

Replace with `our`
=> 2
Comment 17 Tomás Cohen Arazi (tcohen) 2025-03-10 12:26:07 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Tomás Cohen Arazi (tcohen) from comment #13)
> > Is there a reason not to get rid of all the `our` variable definitions? I'm
> > not sure we know exactly why it is being used.
> > 
> > We only know those variables persist in memory in Plack mode (and other
> > persistent runtime environments).
> 
> It has a global scope, and is available from the subroutines.
> When switching to plack we (lazily) switched some "my" to "our" to just make
> things worked.
> 
> It can be fixed, but it's not simply a s/^our/my/g substitution.
> 
> But yes, it must be done.

Yeah, I know. I asked on Mattermost too, if someone remembered some caching mechanism would need to be re-done (i.e. if this was somehow a feature).

I think we all agree this needs to go away.
Comment 18 Jesse Maseto 2025-03-31 09:51:02 UTC
Pushed to 22.11.x. Will be in next point release. 22.11.25