Bug 30904 - (bug 24387 follow-up) Modifying library in news (additional contents) causes inconsistencies
Summary: (bug 24387 follow-up) Modifying library in news (additional contents) causes ...
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: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords: rel_22_05_candidate
Depends on:
Blocks:
 
Reported: 2022-06-05 16:10 UTC by Janusz Kaczmarek
Modified: 2022-07-13 19:38 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.03


Attachments
Bug 30904 - (bug 24387 follow-up) Modifying library in news (additional contents) causes inconsistencies (1.50 KB, patch)
2022-06-05 16:20 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents (3.15 KB, patch)
2022-06-08 09:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents (3.20 KB, patch)
2022-06-08 22:35 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 30904: (bug 24387 follow-up) Fix library modification of additional contents (3.26 KB, patch)
2022-06-12 10:50 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2022-06-05 16:10:11 UTC
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.
Comment 1 Janusz Kaczmarek 2022-06-05 16:20:54 UTC
Created attachment 135701 [details] [review]
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.
Comment 2 Jonathan Druart 2022-06-07 09:34:55 UTC
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`),
Comment 3 Janusz Kaczmarek 2022-06-07 21:49:44 UTC
(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?
Comment 4 Janusz Kaczmarek 2022-06-07 21:56:12 UTC
Another possibility would be issuing search with three criteria instead of find.  Maybe this would be simpler and still correct?
Comment 5 Jonathan Druart 2022-06-08 09:57:55 UTC
Created attachment 135792 [details] [review]
[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.
Comment 6 Jonathan Druart 2022-06-08 09:58:09 UTC
What about this patch?
Comment 7 Jonathan Druart 2022-06-08 10:00:01 UTC
(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.
Comment 8 Janusz Kaczmarek 2022-06-08 16:42:06 UTC
(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.
Comment 9 Lucas Gass 2022-06-08 22:35:50 UTC
Created attachment 135849 [details] [review]
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>
Comment 10 Jonathan Druart 2022-06-09 10:07:07 UTC
(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.
Comment 11 Katrin Fischer 2022-06-12 10:50:11 UTC
Created attachment 135965 [details] [review]
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>
Comment 12 Tomás Cohen Arazi 2022-06-13 14:10:04 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 13 Lucas Gass 2022-07-13 19:38:02 UTC
Backported to 22.05.x for 22.05.03