Bug 26911 - Update for 18936 can cause data loss if constraints are violated
Summary: Update for 18936 can cause data loss if constraints are violated
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 18936
Blocks:
  Show dependency treegraph
 
Reported: 2020-11-03 15:33 UTC by Nick Clemens (kidclamp)
Modified: 2021-06-14 21:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Circulation function:


Attachments
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules (1.27 KB, patch)
2020-11-03 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules (1.70 KB, patch)
2020-11-03 18:39 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules (1.81 KB, patch)
2020-11-03 20:13 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules (1.91 KB, patch)
2020-11-04 00:25 UTC, David Nind
Details | Diff | Splinter Review
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules (1.99 KB, patch)
2020-11-05 12:51 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2020-11-03 15:33:46 UTC
In 19.11 and before you can create circ rules for an itemtype, delete the itemtype, and the circ rule remains. This also applies to category codes and branches

On upgrade from bug 18936 we insert from issuing into circulation rules, however, if rules exist that violate the new constraints on circulation_rules then the upgrade will fail and rules will not be inserted

To test:
1 - Checkout 19.11.x
2 - Add a new itemtype
3 - Define some circ rules for that type and some others
4 - Delete that itemtype
5 - Checkout 20.05.x
6 - updatedatabase
7 - See errors:
DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`circulation_rules`, CONSTRAINT `circ_rules_ibfk_3` FOREIGN KEY (`itemtype`) REFERENCES `itemtypes` (`itemtype`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "
                INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value )
                SELECT IF(categorycode='*', NULL, categorycode), IF(branchcode='*', NULL, branchcode), IF(itemtype='*', NULL, itemtype), 'restrictedtype', COALESCE( restrictedtype, '' )
                FROM issuingrules
            "] at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 20712.
Comment 1 Jonathan Druart 2020-11-03 15:46:30 UTC
Created attachment 112935 [details] [review]
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules
Comment 2 Jonathan Druart 2020-11-03 15:47:03 UTC
I did not test the patch but it should fix the issue you reported.
Comment 3 Nick Clemens (kidclamp) 2020-11-03 18:39:31 UTC
Created attachment 112949 [details] [review]
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules

To test:
 1 - Checkout 19.11.x and reset_all
 2 - Add a new patron category, itemtype, and branch to your system
 3 - Create issuing rules using thise 3 values, and others
 4 - Delete the new category, itemtype and branch
 5 - Checkout master
 6 - Restart_all
 7 - updatedatabase
 8 - you get errors
 9 - repeat 1-4
10 - apply patch
11 - restart_all
12 - updatedatabase
13 - no errors!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Andrew Fuerste-Henry 2020-11-03 19:26:59 UTC
These delete statements would also delete rules where the value is *. That'd be bad.
Comment 5 Andrew Fuerste-Henry 2020-11-03 20:13:28 UTC
Created attachment 112950 [details] [review]
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules

UPDATE: changed queries to not touch * values

To test:
 1 - Checkout 19.11.x and reset_all
 2 - Add a new patron category, itemtype, and branch to your system
 3 - Create issuing rules using thise 3 values, and others
 4 - Delete the new category, itemtype and branch
 5 - Checkout master
 6 - Restart_all
 7 - updatedatabase
 8 - you get errors
 9 - repeat 1-4
10 - apply patch
11 - restart_all
12 - updatedatabase
13 - no errors!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 6 David Nind 2020-11-04 00:25:38 UTC
Created attachment 112968 [details] [review]
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules

UPDATE: changed queries to not touch * values

To test:
 1 - Checkout 19.11.x and reset_all
 2 - Add a new patron category, itemtype, and branch to your system
 3 - Create issuing rules using thise 3 values, and others
 4 - Delete the new category, itemtype and branch
 5 - Checkout master
 6 - Restart_all
 7 - updatedatabase
 8 - you get errors
 9 - repeat 1-5
10 - apply patch
11 - restart_all
12 - updatedatabase
13 - no errors!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 Martin Renvoize (ashimema) 2020-11-05 12:51:52 UTC
Created attachment 113098 [details] [review]
Bug 26911: (bug 18936 follow-up) Remove invalid data before copying the circ rules

UPDATE: changed queries to not touch * values

To test:
 1 - Checkout 19.11.x and reset_all
 2 - Add a new patron category, itemtype, and branch to your system
 3 - Create issuing rules using thise 3 values, and others
 4 - Delete the new category, itemtype and branch
 5 - Checkout master
 6 - Restart_all
 7 - updatedatabase
 8 - you get errors
 9 - repeat 1-5
10 - apply patch
11 - restart_all
12 - updatedatabase
13 - no errors!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2020-11-05 12:52:26 UTC
Good catch guys, logic is sound and testing it locally passed.

Passing QA
Comment 9 Jonathan Druart 2020-11-05 14:25:41 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 10 Magnus Enger 2020-11-06 08:33:13 UTC
I was just hit by this problem on an upgrade from 19.11 to 20.05.05. So a backport to 20.05.x is definitely needed. Perhaps we could even consider an early release of 20.05.06 or issuing a warning against upgrading to 20.05.05?
Comment 11 Lucas Gass (lukeg) 2020-11-16 21:51:40 UTC
backported to 20.05.x for 20.05.06
Comment 12 Aleisha Amohia 2020-11-17 05:31:53 UTC
missing dependencies, not backported to 19.11.x