Bug 22008 - accountlines.manager_id is missing a foreign key constraint
Summary: accountlines.manager_id is missing a foreign key constraint
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 21065
Blocks: 7811 22593
  Show dependency treegraph
 
Reported: 2018-12-16 16:26 UTC by Martin Renvoize
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

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


Attachments
Bug 22008: Add missing constraints to accountlines (3.17 KB, patch)
2018-12-17 13:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add missing constraints to accountlines (3.62 KB, patch)
2018-12-17 13:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add missing constraints to accountlines (4.32 KB, patch)
2018-12-21 12:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.24 KB, patch)
2018-12-21 12:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: DBIC Schema Updates (4.96 KB, patch)
2018-12-21 12:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add missing constraints to accountlines (4.63 KB, patch)
2019-02-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.24 KB, patch)
2019-02-19 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.24 KB, patch)
2019-02-25 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add missing constraints to accountlines (4.31 KB, patch)
2019-02-25 13:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.06 KB, patch)
2019-02-27 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add missing constraints (3.64 KB, patch)
2019-02-27 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.13 KB, patch)
2019-03-06 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22008: Add missing constraints (3.71 KB, patch)
2019-03-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22008: Fixes to make unit tests pass (2.51 KB, patch)
2019-03-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22008: Add tests for new constraints (2.17 KB, patch)
2019-03-15 10:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22008: Add missing constraints (3.75 KB, patch)
2019-03-15 10:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22008: Fixes to make unit tests pass (2.55 KB, patch)
2019-03-15 10:08 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22008: Fix failing tests (3.18 KB, patch)
2019-03-25 19:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22008: Fix failing tests (2.59 KB, patch)
2019-03-25 19:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22008: (follow-up) Additional test fix and fix typo (1.58 KB, patch)
2019-03-27 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22008: (follow-up) Additional fix for bad test (1.44 KB, patch)
2019-03-27 14:48 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 Martin Renvoize 2018-12-16 16:26:34 UTC
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)
Comment 1 Martin Renvoize 2018-12-17 13:29:51 UTC
Created attachment 83278 [details] [review]
Bug 22008: Add missing constraints to accountlines
Comment 2 Martin Renvoize 2018-12-17 13:41:18 UTC
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`
Comment 3 Jonathan Druart 2018-12-19 13:03:43 UTC
Could you provide tests?
Comment 4 Martin Renvoize 2018-12-19 13:24:43 UTC
Good point.. tests for this hadn't even crossed my mind.  I'll have a ponder as to what they should be.
Comment 5 Jonathan Druart 2018-12-19 13:36:15 UTC
(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 :)
Comment 6 Martin Renvoize 2018-12-21 12:11:01 UTC
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`
Comment 7 Martin Renvoize 2018-12-21 12:11:05 UTC
Created attachment 83461 [details] [review]
Bug 22008: Add tests for new constraints
Comment 8 Martin Renvoize 2018-12-21 12:11:08 UTC
Created attachment 83462 [details] [review]
Bug 22008: DBIC Schema Updates
Comment 9 Martin Renvoize 2018-12-21 12:12:18 UTC
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 :(
Comment 10 Martin Renvoize 2018-12-21 12:13:43 UTC
Oh.. I also made the DB update more reliably idempotent (so this can be easily backported if we want)
Comment 11 Maryse Simard 2019-02-15 21:19:10 UTC
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
Comment 12 Martin Renvoize 2019-02-19 12:00:17 UTC
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`
Comment 13 Martin Renvoize 2019-02-19 12:00:21 UTC
Created attachment 85268 [details] [review]
Bug 22008: Add tests for new constraints
Comment 14 Martin Renvoize 2019-02-19 12:01:31 UTC
To test this you'll need to run the dbic schema updates script.
Comment 15 Martin Renvoize 2019-02-25 13:10:51 UTC
Created attachment 85613 [details] [review]
Bug 22008: Add tests for new constraints
Comment 16 Martin Renvoize 2019-02-25 13:10:55 UTC
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`
Comment 17 Martin Renvoize 2019-02-25 13:11:51 UTC
Rebased and switch to `foreign_key_exists` method.
Comment 18 Jonathan Druart 2019-02-26 19:21:06 UTC
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.
Comment 19 Kyle M Hall 2019-02-26 19:50:26 UTC
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.
Comment 20 Martin Renvoize 2019-02-27 10:54:34 UTC
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.
Comment 21 Martin Renvoize 2019-02-27 10:58:22 UTC
ack.. ignore that.. we can't go that way around either as it's a one issue may refer to many accountlines... i
Comment 22 Martin Renvoize 2019-02-27 12:26:31 UTC
Created attachment 85767 [details] [review]
Bug 22008: Add tests for new constraints
Comment 23 Martin Renvoize 2019-02-27 12:26:34 UTC
Created attachment 85768 [details] [review]
Bug 22008: Add missing constraints
Comment 24 Martin Renvoize 2019-02-27 12:28:23 UTC
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.
Comment 25 Katrin Fischer 2019-02-27 13:27:27 UTC
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).
Comment 26 Martin Renvoize 2019-02-27 22:35:34 UTC
Erm, the item constraint isn't added here, it's one of the few that was already in place.
Comment 27 Katrin Fischer 2019-02-28 06:12:40 UTC
Argh, sory, I misread the diff.
Comment 28 Hayley Mapley 2019-03-06 01:11:17 UTC
Is there a test plan for this?
Comment 29 Martin Renvoize 2019-03-06 06:39:50 UTC
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.
Comment 30 Kyle M Hall 2019-03-06 13:08:45 UTC
Created attachment 86162 [details] [review]
Bug 22008: Add tests for new constraints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2019-03-06 13:09:05 UTC
Created attachment 86163 [details] [review]
Bug 22008: Add missing constraints

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2019-03-06 13:09:08 UTC
Created attachment 86164 [details] [review]
Bug 22008: Fixes to make unit tests pass

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Josef Moravec 2019-03-15 10:08:19 UTC
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>
Comment 34 Josef Moravec 2019-03-15 10:08:23 UTC
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>
Comment 35 Josef Moravec 2019-03-15 10:08:27 UTC
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>
Comment 36 Nick Clemens 2019-03-22 20:37:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 37 Nick Clemens 2019-03-25 15:17:23 UTC
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
Comment 38 Nick Clemens 2019-03-25 19:39:05 UTC
Created attachment 86994 [details] [review]
Bug 22008: Fix failing tests
Comment 39 Nick Clemens 2019-03-25 19:41:31 UTC
Created attachment 86995 [details] [review]
Bug 22008: Fix failing tests
Comment 40 Nick Clemens 2019-03-27 12:37:34 UTC
(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
Comment 41 Nick Clemens 2019-03-27 13:14:10 UTC
Created attachment 87066 [details] [review]
Bug 22008: (follow-up) Additional test fix and fix typo
Comment 42 Martin Renvoize 2019-03-27 14:48:28 UTC
Created attachment 87072 [details] [review]
Bug 22008: (follow-up) Additional fix for bad test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2019-04-01 10:35:04 UTC
Enhancement will not be backported to 18.11.x series.