Bug 21674 - Data integrity not enforced for library groups
Summary: Data integrity not enforced for library groups
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: additional_work_needed
Depends on: 15707
Blocks: 24741
  Show dependency treegraph
 
Reported: 2018-10-25 20:34 UTC by Jonathan Druart
Modified: 2020-03-02 10:02 UTC (History)
2 users (show)

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


Attachments
Bug 21674: DB changes (1.84 KB, patch)
2019-12-11 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21674: Prevent to insert twice the same library in a group (4.21 KB, patch)
2019-12-11 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21674: Prevent to insert twice the same library in a group (4.34 KB, patch)
2020-01-21 12:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint (1.53 KB, patch)
2020-01-21 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint (1.38 KB, patch)
2020-01-23 12:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21674: DB changes (1.89 KB, patch)
2020-01-23 18:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21674: Prevent to insert twice the same library in a group (4.39 KB, patch)
2020-01-23 18:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint (1.43 KB, patch)
2020-01-23 18:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21674: DB changes (1.96 KB, patch)
2020-01-30 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21674: Prevent to insert twice the same library in a group (4.44 KB, patch)
2020-01-30 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint (1.48 KB, patch)
2020-01-30 15:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21674: (RM follow-up) Fix updatedatabase error (930 bytes, patch)
2020-01-31 11:40 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-10-25 20:34:21 UTC
You can create hierarchical groups of libraries you are not allowed to add several times the same library to a given group/subgroup. The libraries that are part of the group is not displayed on the interface, to prevent the user to select it.

However this restriction is only done template-side, the controller and module does not handle it.

To recreate the problem:
- Go to Home › Administration › Library groups
- Add a group
- Add a library
- reload the page several times
=> The same library appears several times in the group

Note that we should also replace the "Add" and "Remove" links with buttons.
Comment 1 Jonathan Druart 2019-12-11 11:53:49 UTC
Created attachment 96199 [details] [review]
Bug 21674: DB changes
Comment 2 Jonathan Druart 2019-12-11 11:53:52 UTC
Created attachment 96200 [details] [review]
Bug 21674: Prevent to insert twice the same library in a group

You can create hierarchical groups of libraries you are not allowed to add
several times the same library to a given group/subgroup. The libraries
that are part of the group is not displayed on the interface, to prevent
the user to select it.

However this restriction is only done template-side, the controller and
module does not handle it.

To prevent that this patchset enforces the constraint at DB level, and
display a message to the end user.

Test plan:
- Go to Home › Administration › Library groups
- Add a group
- Add a library
- reload the page several times
=> Without this patch the same library appears several times in the group
=> With this patch applied you will see a friendly error message
Comment 3 Nick Clemens 2020-01-15 18:49:36 UTC
Tested on a sandbox, did not work
Comment 4 Jonathan Druart 2020-01-21 12:25:37 UTC
Created attachment 97658 [details] [review]
Bug 21674: Prevent to insert twice the same library in a group

You can create hierarchical groups of libraries you are not allowed to add
several times the same library to a given group/subgroup. The libraries
that are part of the group is not displayed on the interface, to prevent
the user to select it.

However this restriction is only done template-side, the controller and
module does not handle it.

To prevent that this patchset enforces the constraint at DB level, and
display a message to the end user.

Test plan:
- Go to Home › Administration › Library groups
- Add a group
- Add a library
- reload the page several times
=> Without this patch the same library appears several times in the group
=> With this patch applied you will see a friendly error message
Comment 5 Jonathan Druart 2020-01-21 12:25:55 UTC
(In reply to Nick Clemens from comment #3)
> Tested on a sandbox, did not work

Works for me, please try again with this rebased patch.
Comment 6 Nick Clemens 2020-01-21 12:52:31 UTC
If a library is duplicated in the group before this patch then the DB update fails - we need to cleanup before adding the constraint
Comment 7 Jonathan Druart 2020-01-21 14:03:23 UTC
Created attachment 97667 [details] [review]
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint

To avoid to make it fails if duplicates existed prior to this change
Comment 8 Jonathan Druart 2020-01-21 14:04:46 UTC
(In reply to Jonathan Druart from comment #7)
> Created attachment 97667 [details] [review] [review]
> Bug 21674: Remove duplicates from library_groups before adding the uniq
> constraint
> 
> To avoid to make it fails if duplicates existed prior to this change

There is certainly a more efficient (SQL) way to do that.
Comment 9 Nick Clemens 2020-01-23 11:43:58 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > Created attachment 97667 [details] [review] [review] [review]
> > Bug 21674: Remove duplicates from library_groups before adding the uniq
> > constraint
> > 
> > To avoid to make it fails if duplicates existed prior to this change
> 
> There is certainly a more efficient (SQL) way to do that.

DELETE FROM library_groups WHERE id NOT IN (SELECT MIN(id) FROM library_groups WHERE parent_id IS NOT NULL GROUP BY parent_id,branchcode) AND parent_id IS NOT NULL;
Comment 10 Jonathan Druart 2020-01-23 12:21:13 UTC
Created attachment 97811 [details] [review]
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint

To avoid to make it fails if duplicates existed prior to this change

Credit for the SQL query goes to Nick, thanks!
Comment 11 Nick Clemens 2020-01-23 18:17:04 UTC
Created attachment 97837 [details] [review]
Bug 21674: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2020-01-23 18:17:06 UTC
Created attachment 97838 [details] [review]
Bug 21674: Prevent to insert twice the same library in a group

You can create hierarchical groups of libraries you are not allowed to add
several times the same library to a given group/subgroup. The libraries
that are part of the group is not displayed on the interface, to prevent
the user to select it.

However this restriction is only done template-side, the controller and
module does not handle it.

To prevent that this patchset enforces the constraint at DB level, and
display a message to the end user.

Test plan:
- Go to Home › Administration › Library groups
- Add a group
- Add a library
- reload the page several times
=> Without this patch the same library appears several times in the group
=> With this patch applied you will see a friendly error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2020-01-23 18:17:09 UTC
Created attachment 97839 [details] [review]
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint

To avoid to make it fails if duplicates existed prior to this change

Credit for the SQL query goes to Nick, thanks!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Martin Renvoize 2020-01-30 15:11:16 UTC
Created attachment 98170 [details] [review]
Bug 21674: DB changes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-01-30 15:11:19 UTC
Created attachment 98171 [details] [review]
Bug 21674: Prevent to insert twice the same library in a group

When you create hierarchical groups of libraries you are not allowed to add
several times the same library to a given group/subgroup. The libraries
that are part of the group is not displayed on the interface, to prevent
the user to select it.

However this restriction is only done template-side, the controller and
module does not handle it.

To prevent that this patchset enforces the constraint at DB level, and
display a message to the end user.

Test plan:
- Go to Home › Administration › Library groups
- Add a group
- Add a library
- reload the page several times
=> Without this patch the same library appears several times in the group
=> With this patch applied you will see a friendly error message

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-01-30 15:11:23 UTC
Created attachment 98172 [details] [review]
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint

To avoid to make it fails if duplicates existed prior to this change

Credit for the SQL query goes to Nick, thanks!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2020-01-30 15:11:48 UTC
Nice teamwork guys.. all works as expected.

Passing QA
Comment 18 Martin Renvoize 2020-01-30 15:45:53 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 19 Martin Renvoize 2020-01-31 09:46:51 UTC
I'm getting a wierd DB error on this one: 

DBD::mysql::db do failed: Table 'library_groups' is specified twice, both as a target for 'DELETE' and as a separate source for data [for Statement "
            DELETE FROM library_groups
            WHERE id NOT IN (
                SELECT MIN(id)
                FROM library_groups
                WHERE parent_id IS NOT NULL
                GROUP BY parent_id, branchcode
            )
            AND parent_id IS NOT NULL;
        "] at ./installer/data/mysql/updatedatabase.pl line 20666.
Comment 20 Martin Renvoize 2020-01-31 11:40:25 UTC
Created attachment 98224 [details] [review]
Bug 21674: (RM follow-up) Fix updatedatabase error