Bug 24658 - Deleting items with fines does not update itemnumber in accountlines to NULL causing ISE
Summary: Deleting items with fines does not update itemnumber in accountlines to NULL ...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 22008
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-13 17:17 UTC by Addie
Modified: 2021-06-14 21:35 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.01,20.05.07


Attachments
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031 (1.62 KB, patch)
2020-11-17 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031 (1.69 KB, patch)
2020-12-03 11:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031 (1.74 KB, patch)
2020-12-07 16:52 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Addie 2020-02-13 17:17:38 UTC
When batch deleting items through the staff client, the itemnumber in the accountlines table remains and does update to NULL. If you then go into the Accountlines tab of a borrower and into the Transactions you will get an internal server error if there is a fine in that patrons accountlines with an itemnumber in the database for the deleted item
Comment 1 Jonathan Druart 2020-02-14 09:36:16 UTC
Hi Addie, which error do you get?

I do not recreate (19.05.x and master), can you give more detail please?

What I did:
* Create manual invoice, barcode=39999000018714, amount=42
* Delete the item with barcode=39999000018714 (from cataloguing/additem.pl)
* Accounting > Transactions

I see: https://snipboard.io/8GNMjB.jpg
Comment 2 Addie 2020-02-18 14:19:43 UTC
The process was:

Using the batch item deletion tool, deleted items by barcode numbers.

Then if you go to a patron account where a fine was at any point associated with those items, you would get a white page with black "internal server error" in the left corner of the page.

From the database side, the itemnumber of those items remained in the accountlines for associated fines.
Comment 3 Jonathan Druart 2020-02-18 14:33:44 UTC
Which exact version of Koha? Do you recreate the problem on the latest 19.05.x? On master?
Comment 4 Addie 2020-02-18 20:38:56 UTC
This happened either on 19.05.05 or 19.05.06
Comment 5 Jonathan Druart 2020-02-19 08:13:12 UTC
Tested on 19.05.06:

* Create manual invoice, barcode=39999000018714, amount=12
* Create manual invoice, barcode=39999000018653, amount=23. Then pay it
* Delete the 2 items
* Accounting > Transactions
=> No crash

You need to provide more detail on the error you have in the log, or what kind of transaction was associated to the item.
Comment 6 Cindy Murdock Ames 2020-10-06 19:55:29 UTC
I am also seeing this on 19.11.  My plack-error.log shows the error below when it occurs, and if I change the itemnumber to NULL for the deleted item's fine in accountlines the ISE doesn't happen:  

Template process failed: undef error - DBIC result _type  isn't of the _type Biblio at /usr/share/koha/intranet/htdocs/intranet-tmpl/prog/en/modules/members/pay.tt line 96.

This is line 96 from pay.tt:
        [% IF line.itemnumber %]
            <a href="/cgi-bin/koha/catalogue/moredetail.pl?itemnumber=[% line.itemnumber | uri %]&amp;biblionumber=[% line.item.biblionumber | uri %]#item[% line.itemnumber | uri %]">[% line.item.barcode | html %]</a>


I am not sure what method staff used when deleting the item, but since it was a magazine I suspect they used the batch deletion tool.

Wouldn't it be better to add some error handling in pay.tt so it doesn't fail if it can't find the record it's trying to link to?
Comment 7 Cindy Murdock Ames 2020-10-06 20:06:11 UTC
Note however that I'm not sure when this item was deleted, and we upgraded to 19.11 from 3.20 in April, and the fine was generated in December 2019.  So I'm not sure this is still problematic for newly deleted items.  If you'd like me to do further testing on 19.11 I'd be happy to.
Comment 8 Katrin Fischer 2020-10-06 20:46:17 UTC
Hm, we'd need to do some testing here, but I'd argue we should not set it to NULL but instead keep it and make it not explode (the deleteditems information might still be useful)
Comment 9 Jonathan Druart 2020-10-13 10:16:39 UTC
There is a FK on accountlines.itemnumber, and apparently it is there for a long time now.

Can you confirm that this constraint does not exist in your database?

"show create table accountlines;" will tell you if there is a CONSTRAINT  on items.itemnumber
Comment 10 Nick Clemens 2020-10-13 11:17:08 UTC
(In reply to Jonathan Druart from comment #9)
> There is a FK on accountlines.itemnumber, and apparently it is there for a
> long time now.
> 
> Can you confirm that this constraint does not exist in your database?
> 
> "show create table accountlines;" will tell you if there is a CONSTRAINT  on
> items.itemnumber

The update that added this constraint:
if ( foreign_key_exists( 'accountlines', 'accountlines_ibfk_2' ) && !foreign_key_exists( 'accountlines', 'accountlines_ibfk_items' ) ) {
    $dbh->do("ALTER TABLE accountlines DROP FOREIGN KEY accountlines_ibfk_2");
    $dbh->do("ALTER TABLE accountlines ADD CONSTRAINT `accountlines_ibfk_items` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE CASCADE");
}

It looks like if you don't have the original constraint you don't get the new one, perhaps we could do:
if ( foreign_key_exists( 'accountlines', 'accountlines_ibfk_2' ) {
    $dbh->do("ALTER TABLE accountlines DROP FOREIGN KEY accountlines_ibfk_2");
}
unless( foreign_key_exists( 'accountlines', 'accountlines_ibfk_items' ) ){
    $dbh->do("ALTER TABLE accountlines ADD CONSTRAINT `accountlines_ibfk_items` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE CASCADE");
}
Comment 11 Cindy Murdock Ames 2020-10-13 15:53:06 UTC
(In reply to Jonathan Druart from comment #9)
> There is a FK on accountlines.itemnumber, and apparently it is there for a
> long time now.
> 
> Can you confirm that this constraint does not exist in your database?
> 
> "show create table accountlines;" will tell you if there is a CONSTRAINT  on
> items.itemnumber

It looks to me like mine doesn't have that constraint.  Here's my output:

accountlines | CREATE TABLE `accountlines` (
  `accountlines_id` int(11) NOT NULL AUTO_INCREMENT,
  `issue_id` int(11) DEFAULT NULL,
  `borrowernumber` int(11) DEFAULT NULL,
  `itemnumber` int(11) DEFAULT NULL,
  `date` date DEFAULT NULL,
  `amount` decimal(28,6) DEFAULT NULL,
  `description` longtext COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `credit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `debit_type_code` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `status` varchar(16) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `payment_type` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `amountoutstanding` decimal(28,6) DEFAULT NULL,
  `timestamp` timestamp NOT NULL DEFAULT current_timestamp() ON UPDATE current_timestamp(),
  `note` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `manager_id` int(11) DEFAULT NULL,
  `register_id` int(11) DEFAULT NULL,
  `interface` varchar(16) COLLATE utf8mb4_unicode_ci NOT NULL,
  `branchcode` varchar(10) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  PRIMARY KEY (`accountlines_id`),
  KEY `acctsborridx` (`borrowernumber`),
  KEY `timeidx` (`timestamp`),
  KEY `itemnumber` (`itemnumber`),
  KEY `accountlines_ibfk_branches` (`branchcode`),
  KEY `accountlines_ibfk_borrowers_2` (`manager_id`),
  KEY `accountlines_ibfk_registers` (`register_id`),
  KEY `accountlines_ibfk_debit_type` (`debit_type_code`),
  KEY `accountlines_ibfk_credit_type` (`credit_type_code`),
  CONSTRAINT `accountlines_ibfk_borrowers_2` FOREIGN KEY (`manager_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE,
  CONSTRAINT `accountlines_ibfk_branches` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE,
  CONSTRAINT `accountlines_ibfk_credit_type` FOREIGN KEY (`credit_type_code`) REFERENCES `account_credit_types` (`code`) ON UPDATE CASCADE,
  CONSTRAINT `accountlines_ibfk_debit_type` FOREIGN KEY (`debit_type_code`) REFERENCES `account_debit_types` (`code`) ON UPDATE CASCADE,
  CONSTRAINT `accountlines_ibfk_registers` FOREIGN KEY (`register_id`) REFERENCES `cash_registers` (`id`) ON DELETE SET NULL ON UPDATE CASCADE
) ENGINE=InnoDB AUTO_INCREMENT=469046 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci |
Comment 12 Jonathan Druart 2020-11-17 14:17:45 UTC
Created attachment 113735 [details] [review]
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031

If the original constraint does not exist, the second one is not added.

Test plan:
1. checkout dbrev 18.12.00.030
2. Remove the constraint: ALTER TABLE accountlines DROP FOREIGN KEY accountlines_ibfk_2
3. checkout master
4.
% updatedatabase
=> The constraint accountlines_ibfk_items is not added
5. Repeat 1 to 4
=> The constraint is added correctly
Comment 13 Kyle M Hall 2020-12-03 11:20:26 UTC
Created attachment 114139 [details] [review]
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031

If the original constraint does not exist, the second one is not added.

Test plan:
1. checkout dbrev 18.12.00.030
2. Remove the constraint: ALTER TABLE accountlines DROP FOREIGN KEY accountlines_ibfk_2
3. checkout master
4.
% updatedatabase
=> The constraint accountlines_ibfk_items is not added
5. Repeat 1 to 4
=> The constraint is added correctly

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2020-12-04 13:29:33 UTC
Bumping the severity. This bug do to the nature of the bug causing ISEs that make some actions impossible. No data loss so not a blocker.
Comment 15 Martin Renvoize 2020-12-07 16:52:49 UTC
Created attachment 114237 [details] [review]
Bug 24658: (bug 22008 follow-up) Correct update DB entry 18.12.00.031

If the original constraint does not exist, the second one is not added.

Test plan:
1. checkout dbrev 18.12.00.030
2. Remove the constraint: ALTER TABLE accountlines DROP FOREIGN KEY accountlines_ibfk_2
3. checkout master
4.
% updatedatabase
=> The constraint accountlines_ibfk_items is not added
5. Repeat 1 to 4
=> The constraint is added correctly

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-12-07 16:54:12 UTC
This is a good retrospective fix.. I'm wondering if we ought to also add a further DB update to catch cases where it wasn't caught but their already past that original upgrade?

Passing QA and leaving the RM to decide whether that followup is needed.
Comment 17 Jonathan Druart 2020-12-09 09:01:13 UTC
(In reply to Martin Renvoize from comment #16)
> This is a good retrospective fix.. I'm wondering if we ought to also add a
> further DB update to catch cases where it wasn't caught but their already
> past that original upgrade?
> 
> Passing QA and leaving the RM to decide whether that followup is needed.

I am not sure we need it. If people get the 500 they will find this bug report with the SQL query that is needed to add the missing constraint.
I can still add it later if too many people is facing it.
Comment 18 Jonathan Druart 2020-12-09 10:53:12 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 19 Fridolin Somers 2020-12-10 10:16:57 UTC
Pushed to 20.11.x for 20.11.01
Comment 20 Andrew Fuerste-Henry 2020-12-18 20:29:00 UTC
Pushed to 20.05.x for 20.05.07