Bug 7811 - borrowers.borrowernumber should be a foreign key for 8 tables
Summary: borrowers.borrowernumber should be a foreign key for 8 tables
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P3 enhancement (vote)
Assignee: Martin Renvoize
QA Contact:
URL:
Keywords:
Depends on: 22008 22368
Blocks: 5334
  Show dependency treegraph
 
Reported: 2012-03-23 09:54 UTC by Christophe Croullebois
Modified: 2023-01-08 00:24 UTC (History)
3 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christophe Croullebois 2012-03-23 09:54:49 UTC
I just add constraints :
    alert.borrowernumber (with ON DELETE CASCADE)
    statistics.borrowernumber (with ON DELETE SET NULL)
    messages.borrowernumber (with ON DELETE SET NULL)
    reserveconstraints.borrowernumber (with ON DELETE CASCADE)
    reviews.borrowernumber (with ON DELETE CASCADE)
    saved_sql.borrowernumber (with ON DELETE SET NULL)
    search_history.borrowernumber (with ON DELETE CASCADE) -the borrowernumber field is called userid atm, see below-
    suggestions.suggestedby, suggestion.managedby, acceptedby, rejectedby (with ON DELETE SET NULL)
Comment 1 Owen Leonard 2012-03-23 10:03:02 UTC
(In reply to comment #0)
>     reviews.borrowernumber (with ON DELETE CASCADE)

Was this not already "ON DELETE SET NULL?" My fix for Bug 7493 should have set it as such so that reviews would be kept even if the patron had been deleted. Templates were modified so that display of the patron name (or lack thereof) would be handled gracefully.
Comment 2 Marc Véron 2016-10-02 16:46:22 UTC
Still valid?
Comment 3 Martin Renvoize 2019-02-19 11:42:38 UTC
I'm upset that this still appears to apply.. writing a patch to make these all foreign keys is pretty trivial.. but I'm not sure what side effects may ensue... personally, I feel we should enforce this at the database level.. especially as we're moving to enforce strict modes at the db and opting to fix bug highlighted by such modes.. these logical relationships should be enforced too to enable us to catch existing mishandling.
Comment 4 Martin Renvoize 2019-02-19 16:11:35 UTC
I've split some of these off into their own bugs as they're already covered or it's easier to test them per table.
Comment 5 Katrin Fischer 2023-01-08 00:24:20 UTC
>alert.borrowernumber (with ON DELETE CASCADE)
CONSTRAINT `alert_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE

>statistics.borrowernumber (with ON DELETE SET NULL)
I think this could be debatable.
I am pretty sure libraries look-up in deletedborrowers for statistical reports regularly and this is a statistics table. I consider it WONTFIX.

>messages.borrowernumber (with ON DELETE SET NULL)
CONSTRAINT `messages_borrowernumber` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,
I believe the ON DELETE CASCADE is correct, you don't want to keep those messages without the patrons as they might contain sensitive information.

>reserveconstraints.borrowernumber (with ON DELETE CASCADE)
Table no longer exists.

>reviews.borrowernumber (with ON DELETE CASCADE)
CONSTRAINT `reviews_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE,
I believe the ON DELETE SET NULL is what most libraries would want. 

>saved_sql.borrowernumber (with ON DELETE SET NULL)
NOT FIXED, but already filed separately and stuck in discussion:
Bug 13668 - Creator of an SQL report shoudl be NULL when the staff patron is deleted

>search_history.borrowernumber (with ON DELETE CASCADE) -the borrowernumber field is called userid atm, see below-
NOT FIXED
Now filed separately: 
Bug 32590 - Add FK constraint for search_history.userid

>suggestions.suggestedby, suggestion.managedby, acceptedby, rejectedby (with ON DELETE SET NULL)
CONSTRAINT `suggestions_ibfk_managedby` FOREIGN KEY (`managedby`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE,
CONSTRAINT `suggestions_ibfk_rejectedby` FOREIGN KEY (`rejectedby`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE,
CONSTRAINT `suggestions_ibfk_suggestedby` FOREIGN KEY (`suggestedby`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE
CONSTRAINT `suggestions_ibfk_acceptedby` FOREIGN KEY (`acceptedby`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE,