@@ -, +, @@ checkin - batch checkins may fail - issue_id will always stay out of sync (between issues and old_issues) --- C4/Circulation.pm | 42 ++++++++++------------ circ/returns.pl | 3 ++ .../intranet-tmpl/prog/en/modules/circ/returns.tt | 6 +++- t/db_dependent/Circulation/Returns.t | 42 ++++++++++++++++++---- 4 files changed, 62 insertions(+), 31 deletions(-) --- a/C4/Circulation.pm +++ a/C4/Circulation.pm @@ -53,6 +53,8 @@ use Koha::Patron::Debarments; use Koha::Database; use Koha::Libraries; use Koha::Holds; +use Koha::Issues; +use Koha::OldIssues; use Carp; use List::MoreUtils qw( uniq ); use Scalar::Util qw( looks_like_number ); @@ -1920,21 +1922,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. @@ -2147,21 +2146,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::Issues->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); @@ -2170,7 +2163,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 ); --- a/circ/returns.pl +++ a/circ/returns.pl @@ -546,6 +546,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 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt +++ a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt @@ -205,6 +205,7 @@ $(document).ready(function () {

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

[% END %] + [% IF ( WrongTransfer ) %]
@@ -514,7 +515,10 @@ $(document).ready(function () { [% END %] [% END %] - [% ELSE %] + + [% 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 %]
--- a/t/db_dependent/Circulation/Returns.t +++ a/t/db_dependent/Circulation/Returns.t @@ -1,6 +1,7 @@ use Modern::Perl; use Test::More tests => 3; +use Test::Warn; use t::lib::Mocks; use C4::Biblio; @@ -9,6 +10,7 @@ use C4::Items; use C4::Members; use Koha::Database; use Koha::DateUtils; +use Koha::Issues; use Koha::OldIssues; use t::lib::TestBuilder; @@ -60,7 +62,13 @@ sub Mock_userenv { } 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' ); + my $dbh = C4::Context->dbh; + $dbh->do(q|UPDATE issuingrules SET chargeperiod = 1, fine = 1, firstremind = 1|); my $biblio = $builder->build( { source => 'Biblio' } ); my $item = $builder->build( @@ -76,13 +84,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::Issues->find( $issue_id )->issue_id, $issue_id, 'The issues entry should not have been removed' ); }; 1; --