Bug 22008

Summary: accountlines.manager_id is missing a foreign key constraint
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: DatabaseAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: hayleypelham, jonathan.druart, josef.moravec, katrin.fischer, kyle, maryse.simard, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22368
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22577
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 21065    
Bug Blocks: 7811, 22593, 24658, 26332, 26506    
Attachments: Bug 22008: Add missing constraints to accountlines
Bug 22008: Add missing constraints to accountlines
Bug 22008: Add missing constraints to accountlines
Bug 22008: Add tests for new constraints
Bug 22008: DBIC Schema Updates
Bug 22008: Add missing constraints to accountlines
Bug 22008: Add tests for new constraints
Bug 22008: Add tests for new constraints
Bug 22008: Add missing constraints to accountlines
Bug 22008: Add tests for new constraints
Bug 22008: Add missing constraints
Bug 22008: Add tests for new constraints
Bug 22008: Add missing constraints
Bug 22008: Fixes to make unit tests pass
Bug 22008: Add tests for new constraints
Bug 22008: Add missing constraints
Bug 22008: Fixes to make unit tests pass
Bug 22008: Fix failing tests
Bug 22008: Fix failing tests
Bug 22008: (follow-up) Additional test fix and fix typo
Bug 22008: (follow-up) Additional fix for bad test

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 Pelham 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 (kidclamp) 2019-03-22 20:37:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 37 Nick Clemens (kidclamp) 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 (kidclamp) 2019-03-25 19:39:05 UTC
Created attachment 86994 [details] [review]
Bug 22008: Fix failing tests
Comment 39 Nick Clemens (kidclamp) 2019-03-25 19:41:31 UTC
Created attachment 86995 [details] [review]
Bug 22008: Fix failing tests
Comment 40 Nick Clemens (kidclamp) 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 (kidclamp) 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.