Bug 21065

Summary: Data in account_offsets and accountlines is deleted with the patron leaving gaps in financial reports
Product: Koha Reporter: Katrin Fischer <katrin.fischer>
Component: CirculationAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: blocker    
Priority: P5 - low CC: caroline.cyr-la-rose, cmurdock, fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22052
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: 22008    
Attachments: Bug 21065: Add tests
Bug 21065: Add koha_object(s)_class methods to accountline
Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber
Bug 21065: Update DBIC Schema change
Bug 21065: Update DBIC Schema change
Bug 21065: Add tests
Bug 21065: Add koha_object(s)_class methods to accountline
Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber
Bug 21065: Update DBIC Schema change
Bug 19850: (QA follow-up) Make the update idempotent

Description Katrin Fischer 2018-07-12 11:30:22 UTC
The tables accountoffsets and accountlines are both "cleaned up" when a patron is deleted by using FK constraints. 

This is a problem because 
1) The patrons have a "right to be forgotten" with GDPR and you can't refuse to delete them if their fines are paid etc.
2) Especially for partial payments and in other cases the data in accountlines and accountoffsets is neded to create reliable reports for statistics, but also for financial reports.

It's not possible to determine from other tables like statistics and action_logs what fines are linked to which payment which doesn't allow to create reports by fine/fee type and similar.

Instead of deleting the info, we should really just anonymize it.

--

CREATE TABLE `accountoffsets` (
  `borrowernumber` int(11) NOT NULL DEFAULT '0',
  `accountno` smallint(6) NOT NULL DEFAULT '0',
  `offsetaccount` smallint(6) NOT NULL DEFAULT '0',
  `offsetamount` decimal(28,6) DEFAULT NULL,
  `timestamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  KEY `accountoffsets_ibfk_1` (`borrowernumber`),
  CONSTRAINT `accountoffsets_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;

CREATE TABLE `accountlines` (
  `accountlines_id` int(11) NOT NULL AUTO_INCREMENT,
  `issue_id` int(11) DEFAULT NULL,
  `borrowernumber` int(11) NOT NULL DEFAULT '0',
  `accountno` smallint(6) NOT NULL DEFAULT '0',
  `itemnumber` int(11) DEFAULT NULL,
  `date` date DEFAULT NULL,
  `amount` decimal(28,6) DEFAULT NULL,
  `description` mediumtext COLLATE utf8_unicode_ci,
  `dispute` mediumtext COLLATE utf8_unicode_ci,
  `accounttype` varchar(5) COLLATE utf8_unicode_ci DEFAULT NULL,
  `amountoutstanding` decimal(28,6) DEFAULT NULL,
  `lastincrement` decimal(28,6) DEFAULT NULL,
  `timestamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  `notify_id` int(11) NOT NULL DEFAULT '0',
  `notify_level` int(2) NOT NULL DEFAULT '0',
  `note` text COLLATE utf8_unicode_ci,
  `manager_id` int(11) DEFAULT NULL,
  PRIMARY KEY (`accountlines_id`),
  KEY `acctsborridx` (`borrowernumber`),
  KEY `timeidx` (`timestamp`),
  KEY `itemnumber` (`itemnumber`),
  CONSTRAINT `accountlines_ibfk_1` FOREIGN KEY (`borrowernumber`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `accountlines_ibfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE SET NULL
) ENGINE=InnoDB AUTO_INCREMENT=4666 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci;
Comment 1 Jonathan Druart 2018-08-13 16:42:09 UTC
on delete set null?
Comment 2 Martin Renvoize 2018-12-14 11:40:40 UTC
or set to anon patron?
Comment 3 Katrin Fischer 2018-12-14 11:50:39 UTC
I think this is a real evil bug - can we please prioritize it a bit with the accounts rework?

It means you can't rely on reports that libraries need - like statistics on what was paid for certain fee types during a certain time period. There is no way to get this information from Koha in a reliable way, which is a real issue.
Comment 4 Katrin Fischer 2018-12-14 11:51:30 UTC
This is a data loss bug... I am upping severity to get more attention.
Comment 5 Jonathan Druart 2018-12-14 15:53:16 UTC
Created attachment 83220 [details] [review]
Bug 21065: Add tests
Comment 6 Jonathan Druart 2018-12-14 15:53:20 UTC
Created attachment 83221 [details] [review]
Bug 21065: Add koha_object(s)_class methods to accountline
Comment 7 Jonathan Druart 2018-12-14 15:53:24 UTC
Created attachment 83222 [details] [review]
Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber

Note: Why do we have ON UPDATE SET NULL on accountlines.itemnumber?
Comment 8 Jonathan Druart 2018-12-14 15:53:28 UTC
Created attachment 83223 [details] [review]
Bug 21065: Update DBIC Schema change
Comment 9 Jonathan Druart 2018-12-14 15:55:28 UTC
No test plan provided, on purpose.
Comment 10 Jonathan Druart 2018-12-14 16:04:17 UTC
Created attachment 83224 [details] [review]
Bug 21065: Update DBIC Schema change
Comment 11 Martin Renvoize 2018-12-14 16:29:40 UTC
I find the ON UPDATE SET NULL odd too.. think it's a mistake and should also be CASCADE for itemnumber.. would be happy to sneek that in here.

I'm currently running the entire test suit against this as if memory serves there's something odd going on with koha_object_class stuff in this area which fell out once pushed to Jenkins last time I pushed a similar change.
Comment 12 Martin Renvoize 2018-12-14 17:10:48 UTC
Created attachment 83235 [details] [review]
Bug 21065: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2018-12-14 17:10:52 UTC
Created attachment 83236 [details] [review]
Bug 21065: Add koha_object(s)_class methods to accountline

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2018-12-14 17:10:56 UTC
Created attachment 83237 [details] [review]
Bug 21065: Set ON DELETE SET NULL on accountlines.borrowernumber

Note: Why do we have ON UPDATE SET NULL on accountlines.itemnumber?
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2018-12-14 17:10:59 UTC
Created attachment 83238 [details] [review]
Bug 21065: Update DBIC Schema change

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2018-12-14 17:11:55 UTC
It works as described and I've run the entire test suit against it.  I believe this is a solid improvement to an important deficit.. Going straight for PQA
Comment 17 Martin Renvoize 2018-12-14 17:28:24 UTC
It should, however, be noted that this action, depending on the size of your accountlines table, may take some time to run and may get executed multiple times if it is backported... 

RMaints, RM.. perhaps we could come up with a way of preventing this re-execution as part of backporting?
Comment 18 Martin Renvoize 2018-12-17 09:51:25 UTC
I think we should start using a more descriptive naming scheme for foreign keys.. instead of table_ibfk_int (where int is a running number) we should make them more descriptive and have table1_fk_table2.. or {tablename}_{columnname(s)}_{suffix} (as per postgres defaults perhaps)..  that would mean in this case we could check for the old _1 key before deleting it and check for the new _table2 constraint before creating it.
Comment 19 Katrin Fischer 2018-12-17 09:52:53 UTC
(In reply to Martin Renvoize from comment #18)
> I think we should start using a more descriptive naming scheme for foreign
> keys.. instead of table_ibfk_int (where int is a running number) we should
> make them more descriptive and have table1_fk_table2.. or
> {tablename}_{columnname(s)}_{suffix} (as per postgres defaults perhaps).. 
> that would mean in this case we could check for the old _1 key before
> deleting it and check for the new _table2 constraint before creating it.

Makes sense for me.
Comment 20 Martin Renvoize 2018-12-18 14:01:35 UTC
Created attachment 83366 [details] [review]
Bug 19850: (QA follow-up) Make the update idempotent

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Jonathan Druart 2018-12-18 14:22:35 UTC
(In reply to Martin Renvoize from comment #20)
> Created attachment 83366 [details] [review] [review]
> Bug 19850: (QA follow-up) Make the update idempotent
> 
> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Not much important, but the existing occurrences are either the automatic one ${table}_ibfk_$i or ${table}_${column}, not ${table}_${related_table}

So I would have named it accountlines_borrowernumber.

Not considering this as blocker, just noting ;)
Comment 22 Nick Clemens 2018-12-20 00:55:29 UTC
Awesome work all!

Pushed ot master for 19.05
Comment 23 Martin Renvoize 2018-12-20 09:36:16 UTC
Pushed to 18.11.x for 18.11.01
Comment 24 Lucas Gass 2018-12-28 16:10:39 UTC
backported to 18.05 for 18.05.08
Comment 25 Katrin Fischer 2019-02-04 22:33:45 UTC
*** Bug 18323 has been marked as a duplicate of this bug. ***
Comment 26 Caroline Cyr La Rose 2019-02-05 22:31:23 UTC
Hello everyone!
I noticed this bug was pushed but the wrong one is mentioned in the release notes... 

https://gitlab.com/koha-community/koha-release-notes/blob/master/release_notes_18_12_00.md

Under Enhancements > Acquisitions it says that bug [19850] Enhance invoicing functionality for each line item is pushed, but there is nothing so far in this bug.
Comment 27 Katrin Fischer 2019-02-06 05:44:25 UTC
(In reply to Caroline Cyr La Rose from comment #26)
> Hello everyone!
> I noticed this bug was pushed but the wrong one is mentioned in the release
> notes... 
> 
> https://gitlab.com/koha-community/koha-release-notes/blob/master/
> release_notes_18_12_00.md
> 
> Under Enhancements > Acquisitions it says that bug [19850] Enhance invoicing
> functionality for each line item is pushed, but there is nothing so far in
> this bug.

I think it might have happened here:
http://git.koha-community.org/gitweb/?p=koha.git&a=search&h=6d44f9dac8235dfe0259ea5b935c5a8908d2e441&st=commit&s=21065
Comment 28 Fridolin Somers 2019-02-25 07:31:44 UTC
Sorry I missed this one for 17.11.x
I prefer not push now at branch end life.
Comment 29 Martin Renvoize 2019-02-26 10:53:21 UTC
Did I mess up here.. should we be replacing the query to INFORMATION SCHEMA with foreign_key_exists? 

Jonathan Druart, I'd appreciate your thoughts here... I'm happy to code it up as a followup or a new bug.
Comment 30 Jonathan Druart 2019-02-26 16:42:18 UTC
(In reply to Martin Renvoize from comment #29)
> Did I mess up here.. should we be replacing the query to INFORMATION SCHEMA
> with foreign_key_exists? 
> 
> Jonathan Druart, I'd appreciate your thoughts here... I'm happy to code it
> up as a followup or a new bug.

I do not think so.
We are dropping the FK to recreate it with a different ON DELETE clause value.

So we could have used foreign_key_exists but rename the FK.