[Bug 30904] New: (bug 24387 follow-up) Modifying library in news (additional contents) causes inconsistencies
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Bug ID: 30904 Summary: (bug 24387 follow-up) Modifying library in news (additional contents) causes inconsistencies Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: minor Priority: P5 - low Component: Tools Assignee: koha-bugs@lists.koha-community.org Reporter: januszop@gmail.com QA Contact: testopia@bugs.koha-community.org Modifying an existing element of additional content (News) by changing the library causes a duplication of the entry on the Additional contents (News) list. A subsequent attempt of deleting one of the entries results in deleting both of them. This is due to the fact that after the unwanted duplication all the entries share the same additional_contents.code. A patch will follow. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Janusz Kaczmarek <januszop@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |januszop@gmail.com |ity.org | --- Comment #1 from Janusz Kaczmarek <januszop@gmail.com> --- Created attachment 135701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135701&action=edit Bug 30904 - (bug 24387 follow-up) Modifying library in news (additional contents) causes inconsistencies Modifying an existing element of additional content (News) by changing the library causes a duplication of the entry on the Additional contents (News) list. A subsequent attempt of deleting one of the entries results in deleting both of them. This is due to the fact that after the unwanted duplication all the entries share the same additional_contents.code. Test plan ========= 1. Create a piece of news under Tools > News. 2. After having saved it make a modification by changing the library. It should result in two rows in the table. 3. Delete one of the lines with Delete button. Both lines will disappear. 4. Apply the patch. 5. Repeat the steps 1 and 2. Only the one (the modified) entry should be present in the table. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Janusz Kaczmarek <januszop@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #2 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- I am not expecting this patch to work correctly. You are removing branchcode from ->find, in my understanding ->find is expecting only 1 result to be returned and for that you have to pass either the PK or the unique key. branchcode is part of the unique key UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`branchcode`,`lang`), -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #3 from Janusz Kaczmarek <januszop@gmail.com> --- (In reply to Jonathan Druart from comment #2)
I am not expecting this patch to work correctly. You are removing branchcode from ->find, in my understanding ->find is expecting only 1 result to be returned and for that you have to pass either the PK or the unique key. branchcode is part of the unique key
UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`branchcode`,`lang`),
Thank you, Jonathan, for your comment. IMVHO, from the logical point of view, if the change of the additional_contents.branchcode is possible by the design (exactly equal as it is in the case of Display location = additional_contents.location), additional_contents.branchcode should not be part of the unique key. For unambiguous identification of the row in question, category + code + lang is enough (in fact, probably even code + lang would be enough, keeping in mind how the data is generated). If I understand well, DBIx::Class::ResultSet->find would currently issue a warning in case when the find query based on supplied criteria produced more than one row. This will never happen since—I think, you would agree–category + code + lang would identify exactly one row (or no row) under each condition. Supplying four criteria to this find, i.e. category + code + lang + branchcode, will result with empty result in case of a change made to the library. This will make $additional_content undef (line 116) and so we jump to the line 147, with $code defined. So we create a new set of news rows, sharing the old code. This results in severe data inconsistency. Hence, the four columns as unique key should be considered superfluous and incorrect. To sum up—I would suggest considering the patch OK, but probably data definition introduced by you with the commit 8df3116760bdf1b889dc6f78e4605e231c4d7d6d requires correction as well. IMO, UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`lang`) would be perfect. What do you think? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #4 from Janusz Kaczmarek <januszop@gmail.com> --- Another possibility would be issuing search with three criteria instead of find. Maybe this would be simpler and still correct? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 135792 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135792&action=edit [ALTERNATIVE-PATCH] Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents Modifying an existing element of additional content (News) by changing the library causes a duplication of the entry on the Additional contents (News) list. A subsequent attempt of deleting one of the entries results in deleting both of them. This is due to the fact that after the unwanted duplication all the entries share the same additional_contents.code. Test plan ========= 1. Create a piece of news under Tools > News. 2. After having saved it make a modification by changing the library. It should result in two rows in the table. 3. Delete one of the lines with Delete button. Both lines will disappear. 4. Apply the patch. 5. Repeat the steps 1 and 2. Only the one (the modified) entry should be present in the table. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #6 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- What about this patch? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #7 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Janusz Kaczmarek from comment #3)
To sum up—I would suggest considering the patch OK, but probably data definition introduced by you with the commit 8df3116760bdf1b889dc6f78e4605e231c4d7d6d requires correction as well. IMO, UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`lang`) would be perfect.
Which means you couldn't have 2 identical contents for different libraries? Sounds wrong to me. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #8 from Janusz Kaczmarek <januszop@gmail.com> --- (In reply to Jonathan Druart from comment #7)
(In reply to Janusz Kaczmarek from comment #3)
To sum up—I would suggest considering the patch OK, but probably data definition introduced by you with the commit 8df3116760bdf1b889dc6f78e4605e231c4d7d6d requires correction as well. IMO, UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`lang`) would be perfect.
Which means you couldn't have 2 identical contents for different libraries? Sounds wrong to me.
Why not? In every case you have to create them separately, which means they would get a different additional_contents.code. The presence or absence of branchcode in unique key does not change anything here, IMHO. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Lucas Gass <lucas@bywatersolutions.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=30904 --- Comment #9 from Lucas Gass <lucas@bywatersolutions.com> --- Created attachment 135849 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135849&action=edit Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents Modifying an existing element of additional content (News) by changing the library causes a duplication of the entry on the Additional contents (News) list. A subsequent attempt of deleting one of the entries results in deleting both of them. This is due to the fact that after the unwanted duplication all the entries share the same additional_contents.code. Test plan ========= 1. Create a piece of news under Tools > News. 2. After having saved it make a modification by changing the library. It should result in two rows in the table. 3. Delete one of the lines with Delete button. Both lines will disappear. 4. Apply the patch. 5. Repeat the steps 1 and 2. Only the one (the modified) entry should be present in the table. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com Attachment #135701|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135792|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #10 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to Janusz Kaczmarek from comment #8)
(In reply to Jonathan Druart from comment #7)
(In reply to Janusz Kaczmarek from comment #3)
To sum up—I would suggest considering the patch OK, but probably data definition introduced by you with the commit 8df3116760bdf1b889dc6f78e4605e231c4d7d6d requires correction as well. IMO, UNIQUE KEY `additional_contents_uniq` (`category`,`code`,`lang`) would be perfect.
Which means you couldn't have 2 identical contents for different libraries? Sounds wrong to me.
Why not? In every case you have to create them separately, which means they would get a different additional_contents.code. The presence or absence of branchcode in unique key does not change anything here, IMHO.
You are right. I would like to not modify the DB structure and keep the patch as simple as possible, for backporting purposes. When I rewrote this module I wanted to stick to the same structure as the 'letter' one. I should not have done that, and preferred a better architecture. This table should have been split into two. Unfortunately it's now too much work and I won't have time for that soon. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #135849|0 |1 is obsolete| | --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 135965 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=135965&action=edit Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents Modifying an existing element of additional content (News) by changing the library causes a duplication of the entry on the Additional contents (News) list. A subsequent attempt of deleting one of the entries results in deleting both of them. This is due to the fact that after the unwanted duplication all the entries share the same additional_contents.code. Test plan ========= 1. Create a piece of news under Tools > News. 2. After having saved it make a modification by changing the library. It should result in two rows in the table. 3. Delete one of the lines with Delete button. Both lines will disappear. 4. Apply the patch. 5. Repeat the steps 1 and 2. Only the one (the modified) entry should be present in the table. Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|minor |normal -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |katrin.fischer@bsz-bw.de |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|januszop@gmail.com |jonathan.druart+koha@gmail. | |com CC| |tomascohen@gmail.com Keywords| |rel_22_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|22.11.00 |22.11.00, 22.05.03 released in| | --- Comment #13 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 22.05.x for 22.05.03 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30904 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_22_05_candidate | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org