Bug 21674

Summary: Data integrity not enforced for library groups
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 15707    
Bug Blocks: 24741    
Attachments: Bug 21674: DB changes
Bug 21674: Prevent to insert twice the same library in a group
Bug 21674: Prevent to insert twice the same library in a group
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint
Bug 21674: DB changes
Bug 21674: Prevent to insert twice the same library in a group
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint
Bug 21674: DB changes
Bug 21674: Prevent to insert twice the same library in a group
Bug 21674: Remove duplicates from library_groups before adding the uniq constraint
Bug 21674: (RM follow-up) Fix updatedatabase error

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