[Bug 19475] New: Calendar copy creates duplicates
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Bug ID: 19475 Summary: Calendar copy creates duplicates Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Tools Assignee: koha-bugs@lists.koha-community.org Reporter: fridolin.somers@biblibre.com QA Contact: testopia@bugs.koha-community.org Calendar of current branch can be copied to another branch. This may be done several times creating duplicates. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Emmi Takkinen <emmi.takkinen@outlook.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |emmi.takkinen@outlook.com --- Comment #1 from Emmi Takkinen <emmi.takkinen@outlook.com> --- 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Emmi Takkinen <emmi.takkinen@outlook.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |emmi.takkinen@outlook.com |ity.org | Status|NEW |Needs Signoff --- Comment #2 from Emmi Takkinen <emmi.takkinen@outlook.com> --- Created attachment 100214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100214&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |black23@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Michal Denar <black23@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100214|0 |1 is obsolete| | --- Comment #3 from Michal Denar <black23@gmail.com> --- Created attachment 100483 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100483&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Can you provide tests to cover this change please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #5 from Emmi Takkinen <emmi.takkinen@outlook.com> --- Created attachment 100668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=100668&action=edit Bug 19475: Add unit tests To test prove: t/db_dependent/Holidays.t Sponsored-by: Koha-Suomi Oy -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Emmi Takkinen <emmi.takkinen@outlook.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100668|0 |1 is obsolete| | --- Comment #6 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 101846 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101846&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #100483|0 |1 is obsolete| | --- Comment #7 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Created attachment 101847 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=101847&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #8 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Nice work, thanks a lot 🤗 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Emmi, can you explain the changes a bit please? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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 :) -- You are receiving this mail because: You are watching all bug changes.
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
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #11 from Emmi Takkinen <emmi.takkinen@outlook.com> --- (In reply to Jonathan Druart from comment #9) 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
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #12 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #14 from Emmi Takkinen <emmi.takkinen@outlook.com> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- And "Unique holiday" are not copied! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #16 from Emmi Takkinen <emmi.takkinen@outlook.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Ok, will retest later then. I might have clicked on March dates, so past! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #101846|0 |1 is obsolete| | Attachment #101847|0 |1 is obsolete| | --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102072 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102072&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102073 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102073&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 102074 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=102074&action=edit Bug 19475: Clear the caches at the end of the tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #21 from Emmi Takkinen <emmi.takkinen@outlook.com> --- Thanks for adding caches Jonathan! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |20.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Nice work everyone! Pushed to master for 20.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Joy Nelson <joy@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|20.05.00 |20.05.00,19.11.06 released in| | Status|Pushed to master |Pushed to stable CC| |joy@bywatersolutions.com --- Comment #23 from Joy Nelson <joy@bywatersolutions.com> --- Backported to 19.11.x branch for 19.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #24 from Lucas Gass <lucas@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19475 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org