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;
*** Bug 19699 has been marked as a duplicate of this bug. ***
Upping severity a bit - keeping potential sensitive data about patrons with no way to delete them aside from SQL is not good.
Created attachment 83367 [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
This will need tests.
Created attachment 83463 [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: Devinim <kohadevinim@devinim.com.tr>
Created attachment 83508 [details] [review] Bug 13515: Add unit tests
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>
Created attachment 83510 [details] [review] Bug 13515: Add unit tests
QA is ok for the patch. I'd like a second QA point of view for the unit tests i wrote.
The update doe snot appear to be idempotent - can you check the condition? The tests seem to cover the basic case
(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
Created attachment 85138 [details] [review] Bug 13515: Fix contrainst name before deleting it
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>
Created attachment 85156 [details] [review] Bug 13515: Add unit tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 85157 [details] [review] Bug 13515: Fix constraint name before deleting it Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.04.
Pushed to 18.05.x for 18.05.10
I prefer not to impact 17.11.x database structure.