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 <tomascohen@theke.io>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
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.
# 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);
$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 );
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
<p>This item must be checked in at following library: <strong>[% Branches.GetName( rightbranch ) %]</strong></p>
</div>
[% END %]
+
<!-- case of a mistake in transfer loop -->
[% IF ( WrongTransfer ) %]
<div id="return2" class="dialog message">
[% IF ( errmsgloo.foreverdebarred ) %]
<p class="problem"><b>Reminder: </b>Patron has an indefinite restriction.</p>
[% END %]
+
+ [% IF errmsgloo.data_corrupted %]
+ <p class="problem">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 <a href="/cgi-bin/koha/about.pl#sysinfo">about page</a> and search for the "data problems" section</p>
+ [% END %]
[% END %]
</div>
[% END %]
};
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 );
$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;
projects / koha-equinox.git / commitdiff