Calendar of current branch can be copied to another branch. This may be done several times creating duplicates.
This is still valid. To test: 1. Add holidays for branch A 2. Copy branch A calendar to branch B 3. Repeat copy to branch B 4. Check tables special_holidays and repeatable_holidays from your database 5. Branch B has now duplicate holidays
Created attachment 100214 [details] [review] Bug 19475: Don't copy holiday if it already exists in target calendar Calendars copy tool created duplicate values to database. Holidays in target calendar weren't checked before inserting new holidays. This patch fixes this. To test: 1. Add holidays for branch A 2. Copy branch A calendar to branch B 3. Repeat copy to branch B => Check database, branch B has now duplicate holidays 4. Delete holidays from branches A and B 5. Apply patch 6. Repeat steps 1-3 => Check database, no duplicates Sponsored-by: Koha-Suomi Oy
Created attachment 100483 [details] [review] Bug 19475: Don't copy holiday if it already exists in target calendar Calendars copy tool created duplicate values to database. Holidays in target calendar weren't checked before inserting new holidays. This patch fixes this. To test: 1. Add holidays for branch A 2. Copy branch A calendar to branch B 3. Repeat copy to branch B => Check database, branch B has now duplicate holidays 4. Delete holidays from branches A and B 5. Apply patch 6. Repeat steps 1-3 => Check database, no duplicates Sponsored-by: Koha-Suomi Oy Signed-off-by: Michal Denar <black23@gmail.com>
Can you provide tests to cover this change please?
Created attachment 100668 [details] [review] Bug 19475: Add unit tests To test prove: t/db_dependent/Holidays.t Sponsored-by: Koha-Suomi Oy
Created attachment 101846 [details] [review] Bug 19475: Add unit tests To test prove: t/db_dependent/Holidays.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 101847 [details] [review] Bug 19475: Don't copy holiday if it already exists in target calendar Calendars copy tool created duplicate values to database. Holidays in target calendar weren't checked before inserting new holidays. This patch fixes this. To test: 1. Add holidays for branch A 2. Copy branch A calendar to branch B 3. Repeat copy to branch B => Check database, branch B has now duplicate holidays 4. Delete holidays from branches A and B 5. Apply patch 6. Repeat steps 1-3 => Check database, no duplicates Sponsored-by: Koha-Suomi Oy Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Nice work, thanks a lot [U+1F917]
Emmi, can you explain the changes a bit please?
(In reply to Jonathan Druart from comment #9) > Emmi, can you explain the changes a bit please? ie. it hurts my brain to make sure it will not introduce regression, it will help me to PQA if you could show me you know what you are doing :)
(In reply to Jonathan Druart from comment #9) > Emmi, can you explain the changes a bit please? Previously copying holidays just simply took holidays from copied branch and inserted them into target branch. This resulted as duplicated values in database. With this patch applied, before inserting holidays, we check which of them are already present in target calendar. First we get all e.g weekday holidays for both copied branch and target branch. Then, while looping holidays from copied branch, filter out each equally valued keys/values from them with grep and insert rest to target branch. Check for single and exception holidays works just like before and filters all holidays from past (all holidays past todays date). >ie. it hurts my brain to make sure it will not introduce regression, it will help me to PQA if you could show me you know what you are doing :) Uum, I'm not sure if above is what you need but I hope it was explanatory enough. And headache free :D
Ok so we don't handle collision (on title or description for instance), but I think that's correct as it. Thanks for the explanation, looks like I was feeling lazy yesterday!
2 things about the tests: * Why do you use raw SQL queries to retrieve holidays, who not using existing subroutines? * It's better to flush the cache at the end of the tests, holidays are set in cache. It would make sense to have a flush at the end of the test file.
(In reply to Jonathan Druart from comment #13) > 2 things about the tests: > * Why do you use raw SQL queries to retrieve holidays, who not using > existing subroutines? Simply because calendars subroutines hide this bug. If you look at the descriptions for them in C4::Calendar.pm it is stated that they return a hash reference which use holidays as keys. So all duplicate values are (naturally) ignored. I tried to use them but in the end only solution I could come up with was those queries :/ > * It's better to flush the cache at the end of the tests, holidays are set > in cache. It would make sense to have a flush at the end of the test file. You're right, missed that one. I'll add it at some point.
And "Unique holiday" are not copied!
(In reply to Jonathan Druart from comment #15) > And "Unique holiday" are not copied! If they're past dates then yes they're not copied. Or do you mean unique holidays aren't copied at all?
Ok, will retest later then. I might have clicked on March dates, so past!
Created attachment 102072 [details] [review] Bug 19475: Add unit tests To test prove: t/db_dependent/Holidays.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 102073 [details] [review] Bug 19475: Don't copy holiday if it already exists in target calendar Calendars copy tool created duplicate values to database. Holidays in target calendar weren't checked before inserting new holidays. This patch fixes this. To test: 1. Add holidays for branch A 2. Copy branch A calendar to branch B 3. Repeat copy to branch B => Check database, branch B has now duplicate holidays 4. Delete holidays from branches A and B 5. Apply patch 6. Repeat steps 1-3 => Check database, no duplicates Sponsored-by: Koha-Suomi Oy Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Amended patch: Remove trailing whitespaces and add missing semicolon
Created attachment 102074 [details] [review] Bug 19475: Clear the caches at the end of the tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Thanks for adding caches Jonathan!
Nice work everyone! Pushed to master for 20.05
Backported to 19.11.x branch for 19.11.06
This passes tests on 19.05.x and seems to work fine but the QA tool does not like it: FAIL C4/Calendar.pm OK critic OK forbidden patterns OK git manipulation FAIL pod *** ERROR: Spurious =cut command in file C4/Calendar.pm OK pod coverage OK spelling OK valid No backport
Lucas, I have applied the patch on 19.05.x and I cannot recreate the QA failure. You can certainly ignore it anyway, it's a POD structure issue.