The accountlines table optionally references a number of tables, but the foreign key fields are unconstrained. We should add the constraints for: * issue_id (referencing issues) * manager_id (referencing borrowers)
Created attachment 83278 [details] [review] Bug 22008: Add missing constraints to accountlines
Created attachment 83280 [details] [review] Bug 22008: Add missing constraints to accountlines Add constraints for manager_id referencing the borrowers table and issue_id referencing the issues table and update the itemnumber constraint from `ON UPDATE SET NULL` to `ON UPDATE CASCADE`
Could you provide tests?
Good point.. tests for this hadn't even crossed my mind. I'll have a ponder as to what they should be.
(In reply to Martin Renvoize from comment #4) > Good point.. tests for this hadn't even crossed my mind. I'll have a ponder > as to what they should be. See bug 21065 :)
Created attachment 83460 [details] [review] Bug 22008: Add missing constraints to accountlines Add constraints for manager_id referencing the borrowers table and issue_id referencing the issues table and update the itemnumber constraint from `ON UPDATE SET NULL` to `ON UPDATE CASCADE`
Created attachment 83461 [details] [review] Bug 22008: Add tests for new constraints
Created attachment 83462 [details] [review] Bug 22008: DBIC Schema Updates
Tests added, rebased now that 21065 is in master and DBIC Schema Updates patch added for easy testing. Man I wish we could just use the relations the dbic gives us for free once we have proper constraints :(
Oh.. I also made the DB update more reliably idempotent (so this can be easily backported if we want)
Patch doesn't apply Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 22008: Add missing constraints to accountlines Using index info to reconstruct a base tree... M installer/data/mysql/kohastructure.sql Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql error: Failed to merge in the changes. Patch failed at 0001 Bug 22008: Add missing constraints to accountlines
Created attachment 85267 [details] [review] Bug 22008: Add missing constraints to accountlines Add constraints for manager_id referencing the borrowers table and issue_id referencing the issues table and update the itemnumber constraint from `ON UPDATE SET NULL` to `ON UPDATE CASCADE`
Created attachment 85268 [details] [review] Bug 22008: Add tests for new constraints
To test this you'll need to run the dbic schema updates script.
Created attachment 85613 [details] [review] Bug 22008: Add tests for new constraints
Created attachment 85614 [details] [review] Bug 22008: Add missing constraints to accountlines Add constraints for manager_id referencing the borrowers table and issue_id referencing the issues table and update the itemnumber constraint from `ON UPDATE SET NULL` to `ON UPDATE CASCADE`
Rebased and switch to `foreign_key_exists` method.
Comment on attachment 85614 [details] [review] Bug 22008: Add missing constraints to accountlines Review of attachment 85614 [details] [review]: ----------------------------------------------------------------- Replace NOT IN with LEFT JOIN... NOT NULL? ::: installer/data/mysql/atomicupdate/bug_22008.perl @@ +14,5 @@ > + $dbh->do("ALTER TABLE accountlines ADD CONSTRAINT `accountlines_ibfk_issues` FOREIGN KEY (`issue_id`) REFERENCES `issues` (`issue_id`) ON DELETE SET NULL ON UPDATE CASCADE"); > + } > + > + # Rename accountlines_ibfk_2 to accountlines_ibfk_items > + if( foreign_key_exists( 'accountlines', 'accountlines_ibfk_2' ) ) { You should test for accountlines_ibfk_items as well.
Won't this cause all accountline issue_ids to be nullified on checkin? I think that's a showstopper for that constraint. One solution would be to have issue_id and old_issue_id columns in the accountlines, and move the issue_id between them at checkin time. Also, Bug 19489 relies on issue_id not being nullified at checkin time.
Hmm, you're right Kyle. I think the relationship should probably the in the opposite direction in that case.. with the issues and old_issues tables each having an accountline_id field referencing account lines rather than the current case.
ack.. ignore that.. we can't go that way around either as it's a one issue may refer to many accountlines... i
Created attachment 85767 [details] [review] Bug 22008: Add tests for new constraints
Created attachment 85768 [details] [review] Bug 22008: Add missing constraints
Reduced the scope here to just add the constraint for manager_id. Also converted from NOT IN to LEFT JOIN query. New bug created for the more complex case involving issue_id.
There is still a bit more to this: CONSTRAINT `accountlines_ibfk_items` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE CASCADE, I wonder if the same applies here as for issues. (items > deleteditems).
Erm, the item constraint isn't added here, it's one of the few that was already in place.
Argh, sory, I misread the diff.
Is there a test plan for this?
The test plan is basically to run the included unit tests. They test for adding a non-existant user as the manager for an accountline, and they then try deleting a user whose attached to an accountline as a manager and test that the accountline isn't deleted. You could replicate all that manually if you wanted to take the belt and braces approach.
Created attachment 86162 [details] [review] Bug 22008: Add tests for new constraints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86163 [details] [review] Bug 22008: Add missing constraints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86164 [details] [review] Bug 22008: Fixes to make unit tests pass Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 86659 [details] [review] Bug 22008: Add tests for new constraints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 86660 [details] [review] Bug 22008: Add missing constraints Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 86661 [details] [review] Bug 22008: Fixes to make unit tests pass Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Awesome work all! Pushed to master for 19.05
There is an issue here. Currently we use 0 to indicate an action performed by command line scripts (userenv is undef, default to 0) With this patch we must have a real borrowernumber. If we simply default to 'NULL' we will no longer be able to differentiate fines from command line from those of deleted users
Created attachment 86994 [details] [review] Bug 22008: Fix failing tests
Created attachment 86995 [details] [review] Bug 22008: Fix failing tests
(In reply to Nick Clemens from comment #39) > Created attachment 86995 [details] [review] [review] > Bug 22008: Fix failing tests Followup pushed to master for 19.05
Created attachment 87066 [details] [review] Bug 22008: (follow-up) Additional test fix and fix typo
Created attachment 87072 [details] [review] Bug 22008: (follow-up) Additional fix for bad test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Enhancement will not be backported to 18.11.x series.