From 4b0b273cb078c17b82818211576fb7b3b17c2703 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 16 Apr 2015 13:01:03 +0200 Subject: [PATCH] Bug 12847: Items issued today is considered as overdue The date comparisons in C4::Members::IssueSlip does not work as expected. Is an item is issue yesterday and due today (23:59), it should not be considered as an overdue yet. Test plan: Define a valid issue slip (code ISSUESLIP) Check 2 items out and update the issuedate value for one of them as yesterday (using the mariadb/mysql cli or similar) Print the slip Before this patch the item marked as issued yesterday is considered as overdue. Special cases: - hourly loans - Quick slip is impacted too Signed-off-by: Nick Clemens Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- C4/Members.pm | 32 ++- t/db_dependent/Members/IssueSlip.t | 394 +++++++++++++++++++++++++++++ 2 files changed, 414 insertions(+), 12 deletions(-) create mode 100644 t/db_dependent/Members/IssueSlip.t diff --git a/C4/Members.pm b/C4/Members.pm index 4ca83b9b2b..386fb5ce2e 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -1200,6 +1200,8 @@ sub GetPendingIssues { $_->{issuedate} = dt_from_string($_->{issuedate}, 'sql'); } $_->{date_due} or next; + $_->{date_due_sql} = $_->{date_due}; + # FIXME no need to have this value $_->{date_due} = dt_from_string($_->{date_due}, 'sql'); if ( DateTime->compare($_->{date_due}, $today) == -1 ) { $_->{overdue} = 1; @@ -2439,22 +2441,28 @@ sub DeleteMessage { sub IssueSlip { my ($branch, $borrowernumber, $quickslip) = @_; -# return unless ( C4::Context->boolean_preference('printcirculationslips') ); + # FIXME Check callers before removing this statement + #return unless $borrowernumber; - my $now = POSIX::strftime("%Y-%m-%d", localtime); + my @issues = @{ GetPendingIssues($borrowernumber) }; - my $issueslist = GetPendingIssues($borrowernumber); - foreach my $it (@$issueslist){ - if ((substr $it->{'issuedate'}, 0, 10) eq $now || (substr $it->{'lastreneweddate'}, 0, 10) eq $now) { - $it->{'now'} = 1; + for my $issue (@issues) { + $issue->{date_due} = $issue->{date_due_sql}; + if ($quickslip) { + my $today = output_pref({ dt => dt_from_string, dateformat => 'iso', dateonly => 1 }); + if ( substr( $issue->{issuedate}, 0, 10 ) eq $today + or substr( $issue->{lastreneweddate}, 0, 10 ) eq $today ) { + $issue->{now} = 1; + }; } - elsif ((substr $it->{'date_due'}, 0, 10) le $now) { - $it->{'overdue'} = 1; - } - my $dt = dt_from_string( $it->{'date_due'} ); - $it->{'date_due'} = output_pref( $dt );; } - my @issues = sort { $b->{'timestamp'} <=> $a->{'timestamp'} } @$issueslist; + + # Sort on timestamp then on issuedate (useful for tests and could be if modified in a batch + @issues = sort { + my $s = $b->{timestamp} <=> $a->{timestamp}; + $s == 0 ? + $b->{issuedate} <=> $a->{issuedate} : $s; + } @issues; my ($letter_code, %repeat); if ( $quickslip ) { diff --git a/t/db_dependent/Members/IssueSlip.t b/t/db_dependent/Members/IssueSlip.t new file mode 100644 index 0000000000..71abe67924 --- /dev/null +++ b/t/db_dependent/Members/IssueSlip.t @@ -0,0 +1,394 @@ +#!/usr/bin/perl + +use Modern::Perl; + +use Test::More tests => 3; +use Test::MockModule; + +use C4::Biblio; +use C4::Items; +use C4::Members; +use C4::Branch; +use C4::Category; +use C4::Circulation; + +use Koha::DateUtils qw( dt_from_string output_pref ); +use DateTime::Duration; + +use MARC::Record; + +my $dbh = C4::Context->dbh; +$dbh->{AutoCommit} = 0; +$dbh->{RaiseError} = 1; + +$dbh->do(q|DELETE FROM issues|); +$dbh->do(q|DELETE FROM borrowers|); +$dbh->do(q|DELETE FROM items|); +$dbh->do(q|DELETE FROM branches|); +$dbh->do(q|DELETE FROM biblio|); +$dbh->do(q|DELETE FROM categories|); +$dbh->do(q|DELETE FROM letter|); + +my $branchcode = 'B'; +ModBranch( { add => 1, branchcode => $branchcode, branchname => 'Branch' } ); + +my $categorycode = 'C'; +$dbh->do( "INSERT INTO categories(categorycode) VALUES(?)", + undef, $categorycode ); + +my %item_branch_infos = ( + homebranch => $branchcode, + holdingbranch => $branchcode, +); + +my $slip_content = < +Title: <> +Barcode: <> +Date due: <> + + +Overdues: + +Title: <> +Barcode: <> +Date due: <> + +EOS + +$dbh->do(q| + INSERT INTO letter( module, code, branchcode, name, is_html, title, content, message_transport_type) VALUES ('circulation', 'ISSUESLIP', '', 'Issue Slip', 0, 'Issue Slip', ?, 'email') +|, {}, $slip_content); + +my $quick_slip_content = < +Title: <> +Barcode: <> +Date due: <> + +EOS + +$dbh->do(q| + INSERT INTO letter( module, code, branchcode, name, is_html, title, content, message_transport_type) VALUES ('circulation', 'ISSUEQSLIP', '', 'Issue Quick Slip', 0, 'Issue Quick Slip', ?, 'email') +|, {}, $quick_slip_content); + +my ( $title1, $title2 ) = ( 'My title 1', 'My title 2' ); +my ( $barcode1, $barcode2 ) = ('BC0101', 'BC0202' ); +my $record = MARC::Record->new; +$record->append_fields( + MARC::Field->new( + 245, '1', '4', + a => $title1, + ), +); +my ($biblionumber1) = AddBiblio( $record, '' ); +my $itemnumber1 = + AddItem( { barcode => $barcode1, %item_branch_infos }, $biblionumber1 ); + +$record = MARC::Record->new; +$record->append_fields( + MARC::Field->new( + 245, '1', '4', + a => $title2, + ), +); +my ($biblionumber2) = AddBiblio( $record, '' ); +my $itemnumber2 = + AddItem( { barcode => $barcode2, %item_branch_infos }, $biblionumber2 ); + +my $borrowernumber = + AddMember( categorycode => $categorycode, branchcode => $branchcode ); +my $borrower = GetMember( borrowernumber => $borrowernumber ); + +my $module = new Test::MockModule('C4::Context'); +$module->mock( 'userenv', sub { { branch => $branchcode } } ); + +my $today = dt_from_string; +my $yesterday = dt_from_string->subtract_duration( DateTime::Duration->new( days => 1 ) ); + +subtest 'Issue slip' => sub { + plan tests => 3; + + subtest 'Empty slip' => sub { + plan tests => 1; + my $slip = IssueSlip( $branchcode, $borrowernumber ); + my $empty_slip = <{content}, $empty_slip, 'No checked out or overdues return an empty slip, it should return undef (FIXME)' ); + }; + + subtest 'Daily loans' => sub { + plan tests => 2; + # Test 1: No overdue + my $today_daily = $today->clone->set( hour => 23, minute => 59 ); + my $today_daily_as_formatted = output_pref( $today_daily ); + my $yesterday_daily = $yesterday->clone->set( hour => 23, minute => 59 ); + my $yesterday_daily_as_formatted = output_pref( $yesterday_daily ); + + my ( $date_due, $issue_date, $slip, $expected_slip ); + $date_due = $today_daily; + $issue_date = $today_daily->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due, undef, $issue_date ); + $date_due = $today_daily; + $issue_date = $yesterday_daily; + AddIssue( $borrower, $barcode2, $date_due, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip , 'IssueSlip should return a slip with 2 checkouts'); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + + # Test 2: 1 Overdue + $date_due = $today_daily; + $issue_date = $today_daily->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due, undef, $issue_date ); + $date_due = $yesterday_daily; + $issue_date = $yesterday_daily; + AddIssue( $borrower, $barcode2, $date_due, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip, 'IssueSlip should return a slip with 1 checkout and 1 overdue'); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + }; + + subtest 'Hourly loans' => sub { + plan tests => 2; + # Test 1: No overdue + my ( $date_due_in_time, $date_due_in_time_as_formatted, $date_due_in_late, $date_due_in_late_as_formatted, $issue_date, $slip, $expected_slip ); + # Assuming today is not hour = 23 and minute = 59 + $date_due_in_time = $today->clone->set(hour => $today->hour + 1); + $date_due_in_time_as_formatted = output_pref( $date_due_in_time ); + $issue_date = $today->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due_in_time, undef, $issue_date ); + $issue_date = $yesterday->clone; + AddIssue( $borrower, $barcode2, $date_due_in_time, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip, 'IssueSlip should return a slip with 2 checkouts' ); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + + # Test 2: 1 Overdue + $date_due_in_time = $today->clone->set(hour => $today->hour + 1); + $date_due_in_time_as_formatted = output_pref( $date_due_in_time ); + $issue_date = $today->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due_in_time, undef, $issue_date ); + $date_due_in_late = $today->clone->set(hour => $today->hour - 1); + $date_due_in_late_as_formatted = output_pref( $date_due_in_late ); + $issue_date = $yesterday->clone; + AddIssue( $borrower, $barcode2, $date_due_in_late, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip, 'IssueSlip should return a slip with 1 checkout and 1 overdue' ); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + }; + +}; + +subtest 'Quick slip' => sub { + plan tests => 3; + + subtest 'Empty slip' => sub { + plan tests => 1; + my $slip = IssueSlip( $branchcode, $borrowernumber, 'quick slip' ); + my $empty_slip = <{content}, $empty_slip, 'No checked out or overdues return an empty slip, it should return undef (FIXME)' ); + }; + + subtest 'Daily loans' => sub { + plan tests => 2; + # Test 1: No overdue + my $today_daily = $today->clone->set( hour => 23, minute => 59 ); + my $today_daily_as_formatted = output_pref( $today_daily ); + my $yesterday_daily = $yesterday->clone->set( hour => 23, minute => 59 ); + my $yesterday_daily_as_formatted = output_pref( $yesterday_daily ); + + my ( $date_due, $issue_date, $slip, $expected_slip ); + $date_due = $today_daily; + $issue_date = $today_daily->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due, undef, $issue_date ); + $date_due = $today_daily; + $issue_date = $yesterday_daily; + AddIssue( $borrower, $barcode2, $date_due, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip, 'IssueSlip should return 2 checkouts for today'); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + + # Test 2: 1 Overdue + $date_due = $today_daily; + $issue_date = $today_daily->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due, undef, $issue_date ); + $date_due = $yesterday_daily; + $issue_date = $yesterday_daily; + AddIssue( $borrower, $barcode2, $date_due, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip ); + }; + + subtest 'Hourly loans' => sub { + plan tests => 2; + # Test 1: No overdue + my ( $date_due_in_time, $date_due_in_time_as_formatted, $date_due_in_late, $date_due_in_late_as_formatted, $issue_date, $slip, $expected_slip ); + # Assuming today is not hour = 23 and minute = 59 + $date_due_in_time = $today->clone->set(hour => $today->hour + 1); + $date_due_in_time_as_formatted = output_pref( $date_due_in_time ); + $issue_date = $today->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due_in_time, undef, $issue_date ); + $issue_date = $yesterday->clone; + AddIssue( $borrower, $barcode2, $date_due_in_time, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip ); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + + # Test 2: 1 Overdue + $date_due_in_time = $today->clone->set(hour => $today->hour + 1); + $date_due_in_time_as_formatted = output_pref( $date_due_in_time ); + $issue_date = $today->clone->subtract_duration( DateTime::Duration->new( minutes => 1 ) ); + AddIssue( $borrower, $barcode1, $date_due_in_time, undef, $issue_date ); + $date_due_in_late = $today->clone->set(hour => $today->hour - 1); + $date_due_in_late_as_formatted = output_pref( $date_due_in_late ); + $issue_date = $yesterday->clone; + AddIssue( $borrower, $barcode2, $date_due_in_late, undef, $issue_date ); + + $expected_slip = <{content}, $expected_slip ); + + AddReturn( $barcode1, $branchcode ); + AddReturn( $barcode2, $branchcode ); + }; + +}; + +subtest 'bad calls' => sub { + plan tests => 2; + AddIssue( $borrower, $barcode1, $today, undef, $yesterday ); + my $slip = IssueSlip(); + isnt( $slip, undef, 'IssueSlip should return if no param passed FIXME, should return undef' ); + my $empty_slip = <{content}, $empty_slip, 'IssueSlip should not return an empty slip if the borrowernumber passed in param does not exist. But it is what it does for now (FIXME)' ); +}; -- 2.39.5