Bugzilla – Attachment 65155 Details for
Bug 18966
Move of checkouts - Deal with duplicate IDs at DBMS level
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin
1611x-Bug-18966-Do-not-deal-with-duplicate-issueid.patch (text/plain), 10.84 KB, created by
Jonathan Druart
on 2017-07-20 17:57:13 UTC
(
hide
)
Description:
[16.11.x] Bug 18966: Do not deal with duplicate issue_id on checkin
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2017-07-20 17:57:13 UTC
Size:
10.84 KB
patch
obsolete
>From 60aa98078c4dc30facf4f8fa05ec64d4a659d4ba Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 20 Jul 2017 13:39:43 -0300 >Subject: [PATCH] [16.11.x] 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) >--- > C4/Circulation.pm | 42 ++++++++++------------ > circ/returns.pl | 3 ++ > .../intranet-tmpl/prog/en/modules/circ/returns.tt | 6 +++- > t/db_dependent/Circulation/Returns.t | 41 +++++++++++++++++---- > 4 files changed, 61 insertions(+), 31 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index b039830c1d..f618c04204 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -45,6 +45,8 @@ use Koha::DateUtils; > use Koha::Calendar; > use Koha::IssuingRules; > use Koha::Items; >+use Koha::Checkouts; >+use Koha::OldIssues; > use Koha::Patrons; > use Koha::Patron::Debarments; > use Koha::Database; >@@ -1929,21 +1931,18 @@ sub AddReturn { > } > > if ($borrowernumber) { >- if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) { >- _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } ); >- } >- > eval { > MarkIssueReturned( $borrowernumber, $item->{'itemnumber'}, > $circControlBranch, $return_date, $borrower->{'privacy'} ); > }; >- if ( $@ ) { >- $messages->{'Wrongbranch'} = { >- Wrongbranch => $branch, >- Rightbranch => $message >- }; >- carp $@; >- return ( 0, { WasReturned => 0 }, $issue, $borrower ); >+ unless ( $@ ) { >+ if ( ( C4::Context->preference('CalculateFinesOnReturn') && $issue->{'overdue'} ) || $return_date ) { >+ _CalculateAndUpdateFine( { issue => $issue, item => $item, borrower => $borrower, return_date => $return_date } ); >+ } >+ } else { >+ 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 ); >+ >+ return ( 0, { WasReturned => 0, DataCorrupted => 1 }, $issue, $borrower ); > } > > # FIXME is the "= 1" right? This could be the borrower hash. >@@ -2167,21 +2166,15 @@ sub MarkIssueReturned { > > # FIXME Improve the return value and handle it from callers > $schema->txn_do(sub { >+ >+ # Update the returndate > $dbh->do( $query, undef, @bind ); > >- my $id_already_exists = $dbh->selectrow_array( >- q|SELECT COUNT(*) FROM old_issues WHERE issue_id = ?|, >- undef, $issue_id >- ); >+ # Retrieve the issue >+ my $issue = Koha::Checkouts->find( $issue_id ); # FIXME should be fetched earlier > >- if ( $id_already_exists ) { >- my $new_issue_id = $dbh->selectrow_array(q|SELECT MAX(issue_id)+1 FROM old_issues|); >- $dbh->do( >- q|UPDATE issues SET issue_id = ? WHERE issue_id = ?|, >- undef, $new_issue_id, $issue_id >- ); >- $issue_id = $new_issue_id; >- } >+ # Create the old_issues entry >+ my $old_checkout = Koha::OldIssue->new($issue->unblessed)->store; > > $dbh->do(q|INSERT INTO old_issues SELECT * FROM issues WHERE issue_id = ?|, undef, $issue_id); > >@@ -2190,7 +2183,8 @@ sub MarkIssueReturned { > $dbh->do(q|UPDATE old_issues SET borrowernumber=? WHERE issue_id = ?|, undef, $anonymouspatron, $issue_id); > } > >- $dbh->do(q|DELETE FROM issues WHERE issue_id = ?|, undef, $issue_id); >+ # And finally delete the issue >+ $issue->delete; > > ModItem( { 'onloan' => undef }, undef, $itemnumber ); > >diff --git a/circ/returns.pl b/circ/returns.pl >index ab4ece82aa..f32cd9c928 100755 >--- a/circ/returns.pl >+++ b/circ/returns.pl >@@ -543,6 +543,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 e7483e71cc..4a6cdbf299 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt >@@ -227,6 +227,7 @@ $(document).ready(function () { > <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"> >@@ -624,7 +625,10 @@ $(document).ready(function () { > [% END %] > > [% END %] >- [% ELSE %] >+ >+ [% 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> > >diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t >index d1fb3f2724..37acd36814 100644 >--- a/t/db_dependent/Circulation/Returns.t >+++ b/t/db_dependent/Circulation/Returns.t >@@ -31,6 +31,8 @@ use C4::Members; > use Koha::Database; > use Koha::DateUtils; > use Koha::Items; >+use Koha::Checkouts; >+use Koha::OldIssues; > > use MARC::Record; > use MARC::Field; >@@ -263,7 +265,12 @@ subtest "AddReturn logging on statistics table (item-level_itypes=0)" => sub { > }; > > subtest 'Handle ids duplication' => sub { >- plan tests => 1; >+ plan tests => 6; >+ >+ t::lib::Mocks::mock_preference( 'item-level_itypes', 1 ); >+ t::lib::Mocks::mock_preference( 'CalculateFinesOnReturn', 1 ); >+ t::lib::Mocks::mock_preference( 'finesMode', 'production' ); >+ Koha::IssuingRules->search->update({ chargeperiod => 1, fine => 1, firstremind => 1, }); > > my $biblio = $builder->build( { source => 'Biblio' } ); > my $item = $builder->build( >@@ -279,13 +286,35 @@ subtest 'Handle ids duplication' => sub { > } > ); > my $patron = $builder->build({source => 'Borrower'}); >+ $patron = Koha::Patrons->find( $patron->{borrowernumber} ); >+ >+ my $original_checkout = AddIssue( $patron->unblessed, $item->{barcode}, dt_from_string->subtract( days => 50 ) ); >+ >+ 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::OldIssues->find( $issue_id ); >+ >+ 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'; >+ >+ 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' ); > >- my $checkout = AddIssue( $patron, $item->{barcode} ); >- $builder->build({ source => 'OldIssue', value => { issue_id => $checkout->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' ); > >- my @a = AddReturn( $item->{barcode} ); >- my $old_checkout = Koha::OldIssues->find( $checkout->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.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18966
:
65151
|
65154
|
65155
|
65156
|
65203
|
65204
|
65205
|
65206
|
65207
|
65272
|
65293
|
65294