Bug 19475 - Calendar copy creates duplicates
Summary: Calendar copy creates duplicates
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-17 14:01 UTC by Fridolin SOMERS
Modified: 2020-05-12 08:35 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00,19.11.06


Attachments
Bug 19475: Don't copy holiday if it already exists in target calendar (2.96 KB, patch)
2020-03-06 08:23 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 19475: Don't copy holiday if it already exists in target calendar (2.98 KB, patch)
2020-03-10 21:05 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 19475: Add unit tests (5.26 KB, patch)
2020-03-13 06:48 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 19475: Add unit tests (5.32 KB, patch)
2020-03-26 13:50 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19475: Don't copy holiday if it already exists in target calendar (3.04 KB, patch)
2020-03-26 13:50 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 19475: Add unit tests (5.39 KB, patch)
2020-03-30 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19475: Don't copy holiday if it already exists in target calendar (5.63 KB, patch)
2020-03-30 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19475: Clear the caches at the end of the tests (720 bytes, patch)
2020-03-30 09:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2017-10-17 14:01:20 UTC
Calendar of current branch can be copied to another branch.
This may be done several times creating duplicates.
Comment 1 Emmi Takkinen 2020-03-03 12:39:18 UTC
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
Comment 2 Emmi Takkinen 2020-03-06 08:23:35 UTC
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
Comment 3 Michal Denar 2020-03-10 21:05:07 UTC
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>
Comment 4 Jonathan Druart 2020-03-11 15:32:49 UTC
Can you provide tests to cover this change please?
Comment 5 Emmi Takkinen 2020-03-13 06:48:57 UTC
Created attachment 100668 [details] [review]
Bug 19475: Add unit tests

To test prove:
t/db_dependent/Holidays.t

Sponsored-by: Koha-Suomi Oy
Comment 6 Fridolin SOMERS 2020-03-26 13:50:05 UTC
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>
Comment 7 Fridolin SOMERS 2020-03-26 13:50:34 UTC
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>
Comment 8 Fridolin SOMERS 2020-03-26 13:51:49 UTC
Nice work, thanks a lot 🤗
Comment 9 Jonathan Druart 2020-03-26 15:44:39 UTC
Emmi, can you explain the changes a bit please?
Comment 10 Jonathan Druart 2020-03-26 15:45:48 UTC
(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 :)
Comment 11 Emmi Takkinen 2020-03-27 09:48:58 UTC
(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
Comment 12 Jonathan Druart 2020-03-27 10:52:01 UTC
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!
Comment 13 Jonathan Druart 2020-03-27 10:57:11 UTC
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.
Comment 14 Emmi Takkinen 2020-03-27 11:44:35 UTC
(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.
Comment 15 Jonathan Druart 2020-03-30 07:23:21 UTC
And "Unique holiday" are not copied!
Comment 16 Emmi Takkinen 2020-03-30 08:28:22 UTC
(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?
Comment 17 Jonathan Druart 2020-03-30 09:18:29 UTC
Ok, will retest later then. I might have clicked on March dates, so past!
Comment 18 Jonathan Druart 2020-03-30 09:37:02 UTC
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>
Comment 19 Jonathan Druart 2020-03-30 09:37:06 UTC
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
Comment 20 Jonathan Druart 2020-03-30 09:37:09 UTC
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>
Comment 21 Emmi Takkinen 2020-03-30 09:59:12 UTC
Thanks for adding caches Jonathan!
Comment 22 Martin Renvoize 2020-04-03 13:24:05 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 23 Joy Nelson 2020-04-28 20:24:03 UTC
Backported to 19.11.x branch for 19.11.06
Comment 24 Lucas Gass 2020-05-11 19:25:32 UTC
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
Comment 25 Jonathan Druart 2020-05-12 08:35:12 UTC
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.