Bug 24658

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

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.

Which exact version of Koha? Do you recreate the problem on the latest 19.05.x? On master?

This happened either on 19.05.05 or 19.05.06

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.

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?

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.

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)

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

(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");
}

(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 |

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

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>

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.

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>

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.

(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.

Pushed to master for 21.05, thanks to everybody involved!

Pushed to 20.11.x for 20.11.01

Pushed to 20.05.x for 20.05.07