From a9a500e81d5d90c7a83a785c48014fce8bc17eec Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 20 Jul 2017 13:39:43 -0300 Subject: [PATCH] Bug 18966: Do not deal with duplicate issue_id on checkin Koha suffers of big bugs due to its history: When data are deleted, they are moved to another tables. For instance issues and old_issues: when a checkin is done, it is moved to the old_issues table. That leads to a main problem that is described on https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix However we tried first to fix the problem (for issues/old_issues) at code level on bug 18242. The goal was to prevent data lost. Data lost may happens in this case: Check an item out (issue_id = 1) Check an item in (issue_id = 1) Restart MySQL (reset auto increment for issue_id to 1) Check an item out (issue_id = 1) Check an item in => BOOM, the issue_id is a PK in old_issues and the move fails. Before bug 18242 the data were lost, we inserted the value into old_issues, which fails silently (because of RaiseError set to 0 in Koha::Database), then delete the row from issues. That has been fixed using a transaction. This patch introduced a regression we tried to fix on bug 18651 comment 0, the patron was charged even if the checkin was rejected. A good way to fix that would have been to LOCK the tables: 1- Start a transaction 2- LOCK the table to make sure nobody will read id and avoid race conditions 3- Move the content from one table to the other, dealing with ids 4- UNLOCK the table 5- Commit the transaction But there were problems using LOCK and DBIx::Class (See commit 905572910b3a - Do no LOCK/UNLOCK the table). Finally the solution implemented is not acceptable for several reasons: - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) See 18651 comment 66. Since the next stable releases are very soon, and we absolutely need to fix this problem, I am suggesting to: 1- Execute the move in a transaction to avoid data lost and reject the checkin if we face IDs dup => It will only reject 1 checkin (max is 1 * MySQL restart), no need to deal with race conditions, 2- Display a warning on the checkin page and link to a solution/explanation 3- Communicate as much as we can on the proper fix: Update auto increment values when the DBMS is restarted - https://wiki.koha-community.org/wiki/DBMS_auto_increment_fix 4- Display a warning on the about page for corrupted data (see bug 18931) 5- Write and make available a maintenance script to fix corrupted data (TODO LATER) Signed-off-by: Tomas Cohen Arazi Signed-off-by: Julian Maurice Signed-off-by: Jonathan Druart --- C4/Circulation.pm | 29 +++++----------- circ/returns.pl | 3 ++ .../prog/en/modules/circ/returns.tt | 5 +++ t/db_dependent/Circulation/Returns.t | 33 ++++++++++++------- 4 files changed, 38 insertions(+), 32 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index ead7e2b4c9..fba723f7f8 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -1928,21 +1928,17 @@ sub AddReturn { if ($patron) { eval { - my $issue_id = MarkIssueReturned( $borrowernumber, $item->{'itemnumber'}, + MarkIssueReturned( $borrowernumber, $item->{'itemnumber'}, $circControlBranch, $return_date, $patron->privacy ); - $issue->issue_id($issue_id); }; unless ( $@ ) { if ( ( C4::Context->preference('CalculateFinesOnReturn') && $is_overdue ) || $return_date ) { _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $patron_unblessed, return_date => $return_date } ); } } else { - $messages->{'Wrongbranch'} = { - Wrongbranch => $branch, - Rightbranch => $message - }; - carp $@; - return ( 0, { WasReturned => 0 }, $issue, $patron_unblessed ); + carp "The checkin for the following issue failed, Please go to the about page, section 'data corrupted' to know how to fix this problem ($@)" . Dumper( $issue->unblessed ); + + return ( 0, { WasReturned => 0, DataCorrupted => 1 }, $issue, $patron_unblessed ); } # FIXME is the "= 1" right? This could be the borrower hash. @@ -2169,23 +2165,14 @@ sub MarkIssueReturned { # FIXME Improve the return value and handle it from callers $schema->txn_do(sub { + # Update the returndate $dbh->do( $query, undef, @bind ); + # Retrieve the issue my $issue = Koha::Checkouts->find( $issue_id ); # FIXME should be fetched earlier # Create the old_issues entry - my $old_checkout_data = $issue->unblessed; - - if ( Koha::Old::Checkouts->find( $issue_id ) ) { - my $new_issue_id = ( Koha::Old::Checkouts->search( - {}, - { columns => [ { max_issue_id => { max => 'issue_id' } } ] } - )->get_column('max_issue_id') )[0]; - $new_issue_id++; - $issue_id = $new_issue_id; - } - $old_checkout_data->{issue_id} = $issue_id; - my $old_checkout = Koha::Old::Checkout->new($old_checkout_data)->store; + my $old_checkout = Koha::Old::Checkout->new($issue->unblessed)->store; # Update the fines $dbh->do(q|UPDATE accountlines SET issue_id = ? WHERE issue_id = ?|, undef, $old_checkout->issue_id, $issue->issue_id); @@ -2195,7 +2182,7 @@ sub MarkIssueReturned { $dbh->do(q|UPDATE old_issues SET borrowernumber=? WHERE issue_id = ?|, undef, $anonymouspatron, $old_checkout->issue_id); } - # Delete the issue + # And finally delete the issue $issue->delete; ModItem( { 'onloan' => undef }, undef, $itemnumber ); diff --git a/circ/returns.pl b/circ/returns.pl index 26286e8fc2..5fe3c70c17 100755 --- a/circ/returns.pl +++ b/circ/returns.pl @@ -549,6 +549,9 @@ foreach my $code ( keys %$messages ) { elsif ( $code eq 'NotForLoanStatusUpdated' ) { $err{NotForLoanStatusUpdated} = $messages->{NotForLoanStatusUpdated}; } + elsif ( $code eq 'DataCorrupted' ) { + $err{data_corrupted} = 1; + } else { die "Unknown error code $code"; # note we need all the (empty) elsif's above, or we die. # This forces the issue of staying in sync w/ Circulation.pm diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt index 56bfb6a0d0..b3b5910342 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -237,6 +237,7 @@ $(document).ready(function () {

This item must be checked in at following library: [% Branches.GetName( rightbranch ) %]

[% END %] + [% IF ( WrongTransfer ) %]
@@ -640,6 +641,10 @@ $(document).ready(function () { [% IF ( errmsgloo.foreverdebarred ) %]

Reminder: Patron has an indefinite restriction.

[% END %] + + [% IF errmsgloo.data_corrupted %] +

The item has not been checked in due to a configuration issue in your system. You must ask an administrator to take a look at the about page and search for the "data problems" section

+ [% END %] [% END %]
[% END %] diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t index 3910951bc1..4135d66fa2 100644 --- a/t/db_dependent/Circulation/Returns.t +++ b/t/db_dependent/Circulation/Returns.t @@ -277,7 +277,7 @@ subtest "AddReturn logging on statistics table (item-level_itypes=0)" => sub { }; subtest 'Handle ids duplication' => sub { - plan tests => 4; + plan tests => 6; t::lib::Mocks::mock_preference( 'item-level_itypes', 1 ); t::lib::Mocks::mock_preference( 'CalculateFinesOnReturn', 1 ); @@ -302,21 +302,32 @@ subtest 'Handle ids duplication' => sub { $patron = Koha::Patrons->find( $patron->{borrowernumber} ); my $original_checkout = AddIssue( $patron->unblessed, $item->{barcode}, dt_from_string->subtract( days => 50 ) ); - $builder->build({ source => 'OldIssue', value => { issue_id => $original_checkout->issue_id } }); - my $old_checkout = Koha::Old::Checkouts->find( $original_checkout->issue_id ); - AddRenewal( $patron->borrowernumber, $item->{itemnumber} ); + my $issue_id = $original_checkout->issue_id; + # Create an existing entry in old_issue + $builder->build({ source => 'OldIssue', value => { issue_id => $issue_id } }); + + my $old_checkout = Koha::Old::Checkouts->find( $issue_id ); - my ($doreturn, $messages, $new_checkout, $borrower) = AddReturn( $item->{barcode}, undef, undef, undef, dt_from_string ); + my ($doreturn, $messages, $new_checkout, $borrower); + warning_like { + ( $doreturn, $messages, $new_checkout, $borrower ) = + AddReturn( $item->{barcode}, undef, undef, undef, dt_from_string ); + } + [ + qr{.*DBD::mysql::st execute failed: Duplicate entry.*}, + { carped => qr{The checkin for the following issue failed.*DBIx::Class::Storage::DBI::_dbh_execute.*} } + ], + 'DBD should have raised an error about dup primary key'; - my $account_lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber, issue_id => $original_checkout->issue_id }); - is( $account_lines->count, 0, 'No account lines should exist on old issue_id' ); + is( $doreturn, 0, 'Return should not have been done' ); + is( $messages->{WasReturned}, 0, 'messages should have the WasReturned flag set to 0' ); + is( $messages->{DataCorrupted}, 1, 'messages should have the DataCorrupted flag set to 1' ); - $account_lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber, issue_id => $new_checkout->{issue_id} }); - is( $account_lines->count, 2, 'Two account lines should exist on new issue_id' ); + my $account_lines = Koha::Account::Lines->search({ borrowernumber => $patron->borrowernumber, issue_id => $issue_id }); + is( $account_lines->count, 0, 'No account lines should exist for this issue_id, patron should not have been charged' ); - isnt( $original_checkout->issue_id, $new_checkout->{issue_id}, 'AddReturn should return the issue with the new issue_id' ); - isnt( $old_checkout->itemnumber, $item->{itemnumber}, 'If an item is checked-in, it should be moved to old_issues even if the issue_id already existed in the table' ); + is( Koha::Checkouts->find( $issue_id )->issue_id, $issue_id, 'The issues entry should not have been removed' ); }; 1; -- 2.39.5