Bug 13515

Summary: Table messages is missing FK constraints and is never cleaned up
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: DatabaseAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: alex.arnaud, axelle.clarisse, fridolin.somers, jonathan.druart, kohadevinim, m.de.rooy, martin.renvoize, nick, sophie.meynieux, tmisilo
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22368
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5334    
Attachments: Bug 13515: Add a foreign key constraint to messages.borrowernumber
Bug 13515: Add a foreign key constraint to messages.borrowernumber
Bug 13515: Add unit tests
Bug 13515: Add a foreign key constraint to messages.borrowernumber
Bug 13515: Add unit tests
Bug 13515: Fix contrainst name before deleting it
Bug 13515: Add a foreign key constraint to messages.borrowernumber
Bug 13515: Add unit tests
Bug 13515: Fix constraint name before deleting it

Description Katrin Fischer 2015-01-05 15:03:43 UTC
The table messages is used to store notes to the patron (visible in OPAC) and internal notes for circulation. 

There is no FK on the borrowernumber, so the data in this table is never cleaned up, not even on deleting the patron.

There is also no FK on the branchcode, although the comments state otherwise.

2723 -- Table structure for table `messages`
2724 --
2725 DROP TABLE IF EXISTS `messages`;
2726 CREATE TABLE `messages` ( -- circulation messages left via the patron's check out screen
2727   `message_id` int(11) NOT NULL auto_increment, -- unique identifier assigned by Koha
2728   `borrowernumber` int(11) NOT NULL, -- foreign key linking this message to the borrowers table
2729   `branchcode` varchar(10) default NULL, -- foreign key linking the message to the branches table
2730   `message_type` varchar(1) NOT NULL, -- whether the message is for the librarians (L) or the patron (B)
2731   `message` text NOT NULL, -- the text of the message
2732   `message_date` TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, -- the date and time the message was written
2733   PRIMARY KEY (`message_id`)
2734 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
Comment 1 Katrin Fischer 2017-11-27 23:22:28 UTC
*** Bug 19699 has been marked as a duplicate of this bug. ***
Comment 2 Katrin Fischer 2018-07-12 11:19:11 UTC
Upping severity a bit - keeping potential sensitive data about patrons with no way to delete them aside from SQL is not good.
Comment 3 Jonathan Druart 2018-12-18 15:47:45 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2018-12-19 13:36:06 UTC
This will need tests.
Comment 5 Devinim 2018-12-21 13:06:36 UTC Comment hidden (obsolete)
Comment 6 Alex Arnaud 2018-12-26 16:45:05 UTC Comment hidden (obsolete)
Comment 7 Alex Arnaud 2018-12-26 16:46:33 UTC
Created attachment 83509 [details] [review]
Bug 13515: Add a foreign key constraint to messages.borrowernumber

This patch adds a missing foreign key constraint to
mesages.borrowernumber.
To create it sucessfully the entries from the messages table that are
not linked with an existing patron will be removed.

Test plan:
0/ Do not apply the patch
1/ Add messages to different patrons
2/ Delete one of the patron's record
3/ Have a look at the messages table and notice that the messages for
the deleted patron's record still appear
4/ Apply the patch and execute the update DB entry
5/ Have a look again at the messages table and notice that the messages
for the deleted patron's record have been removed
6/ Delete a patron that have messages
7/ Notice that now the messages are deleted when the patron's record is
deleted

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 8 Alex Arnaud 2018-12-26 16:46:41 UTC
Created attachment 83510 [details] [review]
Bug 13515: Add unit tests
Comment 9 Alex Arnaud 2018-12-26 16:48:09 UTC
QA is ok for the patch.
I'd like a second QA point of view for the unit tests i wrote.
Comment 10 Nick Clemens 2019-01-02 20:35:04 UTC
The update doe snot appear to be idempotent - can you check the condition?

The tests seem to cover the basic case
Comment 11 Alex Arnaud 2019-01-07 09:02:13 UTC
(In reply to Nick Clemens from comment #10)
> The update doe snot appear to be idempotent - can you check the condition?
> 
Yep. Constraint nameis wrong in foreign_key_exists call. Need a follow-up
Comment 12 Jonathan Druart 2019-02-14 20:25:25 UTC
Created attachment 85138 [details] [review]
Bug 13515: Fix contrainst name before deleting it
Comment 13 Marcel de Rooy 2019-02-15 11:14:19 UTC
Created attachment 85155 [details] [review]
Bug 13515: Add a foreign key constraint to messages.borrowernumber

This patch adds a missing foreign key constraint to
mesages.borrowernumber.
To create it sucessfully the entries from the messages table that are
not linked with an existing patron will be removed.

Test plan:
0/ Do not apply the patch
1/ Add messages to different patrons
2/ Delete one of the patron's record
3/ Have a look at the messages table and notice that the messages for
the deleted patron's record still appear
4/ Apply the patch and execute the update DB entry
5/ Have a look again at the messages table and notice that the messages
for the deleted patron's record have been removed
6/ Delete a patron that have messages
7/ Notice that now the messages are deleted when the patron's record is
deleted

Signed-off-by: Nazlı Çetin <nazli@devinim.com.tr>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2019-02-15 11:14:24 UTC
Created attachment 85156 [details] [review]
Bug 13515: Add unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2019-02-15 11:14:29 UTC
Created attachment 85157 [details] [review]
Bug 13515: Fix constraint name before deleting it

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Nick Clemens 2019-02-15 18:46:05 UTC
Awesome work all!

Pushed to master for 19.05
Comment 17 Martin Renvoize 2019-02-25 12:28:16 UTC
Pushed to 18.11.x for 18.11.04.
Comment 18 Fridolin Somers 2019-03-12 08:07:11 UTC
Pushed to 18.05.x for 18.05.10
Comment 19 Fridolin Somers 2019-03-12 08:08:38 UTC
I prefer not to impact 17.11.x database structure.