From a302192d0aa8e77f0c214e067e7a7dbccf6bcb61 Mon Sep 17 00:00:00 2001 From: root Date: Thu, 25 Jul 2013 10:49:21 +0200 Subject: [PATCH] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences This patch fixes some incoherences of the routine GetBooksellerWithOrders. Now it considers the field $estimateddeliverydateto and it replaces it by now() only if it is undef. More it doesn't test if $aqbookseller.deliverytime is not Null anymore but if $deliverytime = null or undef, it replaces it by 0. It also verifies if $delay is >= 0 and return undef if it is a negative value. To Test: prove t/db_dependent/Bookseller.t t/db_dependent/Bookseller.t .. [Some warnings about uninitialized values] WARNING: GetBooksellerWithLateOrders is called with a negative value at C4/Bookseller.pm line 135. t/db_dependent/Bookseller.t .. ok All tests successful. --- C4/Bookseller.pm | 36 +++++++++++++++++------------ t/db_dependent/Bookseller.t | 53 ++++++++++++++++++------------------------- 2 files changed, 43 insertions(+), 46 deletions(-) diff --git a/C4/Bookseller.pm b/C4/Bookseller.pm index 3e1454f..707f971 100644 --- a/C4/Bookseller.pm +++ b/C4/Bookseller.pm @@ -111,10 +111,10 @@ sub GetBooksellersWithLateOrders { # FIXME NOT quite sure that this operation is valid for DBMs different from Mysql, HOPING so # should be tested with other DBMs - my $strsth; + my $query; my @query_params = (); my $dbdriver = C4::Context->config("db_scheme") || "mysql"; - $strsth = " + $query = " SELECT DISTINCT aqbasket.booksellerid, aqbooksellers.name FROM aqorders LEFT JOIN aqbasket ON aqorders.basketno=aqbasket.basketno LEFT JOIN aqbooksellers ON aqbasket.booksellerid = aqbooksellers.id @@ -128,24 +128,30 @@ sub GetBooksellersWithLateOrders { AND aqorders.quantity - COALESCE(aqorders.quantityreceived,0) <> 0 AND aqbasket.closedate IS NOT NULL "; - if ( defined $delay ) { - $strsth .= " AND (closedate <= DATE_SUB(CAST(now() AS date),INTERVAL ? DAY)) "; + if ( defined $delay && $delay >= 0 ) { + $query .= " AND (closedate <= DATE_SUB(CAST(now() AS date),INTERVAL ? + COALESCE(aqbooksellers.deliverytime,0) DAY)) "; push @query_params, $delay; - } + } elsif ( defined $delay && $delay < 0 ){ + warn 'WARNING: GetBooksellerWithLateOrders is called with a negative value'; + return; + } if ( defined $estimateddeliverydatefrom ) { - $strsth .= ' - AND aqbooksellers.deliverytime IS NOT NULL - AND ADDDATE(aqbasket.closedate, INTERVAL aqbooksellers.deliverytime DAY) >= ?'; - push @query_params, $estimateddeliverydatefrom; - } - if ( defined $estimateddeliverydatefrom and defined $estimateddeliverydateto ) { - $strsth .= ' AND ADDDATE(aqbasket.closedate, INTERVAL aqbooksellers.deliverytime DAY) <= ?'; + $query .= ' + AND ADDDATE(aqbasket.closedate, INTERVAL COALESCE(aqbooksellers.deliverytime,0) DAY) >= ?'; + push @query_params, $estimateddeliverydatefrom; + if ( defined $estimateddeliverydateto ) { + $query .= ' AND ADDDATE(aqbasket.closedate, INTERVAL COALESCE(aqbooksellers.deliverytime, 0) DAY) <= ?'; + push @query_params, $estimateddeliverydateto; + } elsif ( !defined $estimateddeliverydateto ) { + $query .= ' AND ADDDATE(aqbasket.closedate, INTERVAL COALESCE(aqbooksellers.deliverytime, 0) DAY) <= CAST(now() AS date)'; + } + } + if ( defined $estimateddeliverydateto ) { + $query .= ' AND ADDDATE(aqbasket.closedate, INTERVAL COALESCE(aqbooksellers.deliverytime,0) DAY) <= ?'; push @query_params, $estimateddeliverydateto; - } elsif ( defined $estimateddeliverydatefrom ) { - $strsth .= ' AND ADDDATE(aqbasket.closedate, INTERVAL aqbooksellers.deliverytime DAY) <= CAST(now() AS date)'; } - my $sth = $dbh->prepare($strsth); + my $sth = $dbh->prepare($query); $sth->execute( @query_params ); my %supplierlist; while ( my ( $id, $name ) = $sth->fetchrow ) { diff --git a/t/db_dependent/Bookseller.t b/t/db_dependent/Bookseller.t index 71307f1..0ad91ce 100644 --- a/t/db_dependent/Bookseller.t +++ b/t/db_dependent/Bookseller.t @@ -2,7 +2,7 @@ use Modern::Perl; -use Test::More tests => 53; +use Test::More tests => 61; use C4::Context; use Koha::DateUtils; use DateTime::Duration; @@ -476,17 +476,15 @@ isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ) %suppliers = C4::Bookseller::GetBooksellersWithLateOrders( 4, undef, undef ); isnt( exists( $suppliers{$id_supplier1} ), 1, "Supplier1 has late orders but today > now() - 4 days" ); -#FIXME: If only the field delay is given, it doen't consider the deliverytime -#isnt( exists( $suppliers{$id_supplier2} ), -# 1, "Supplier2 has late orders and $daysago5 <= now() - (4 days+2)" ); +isnt( exists( $suppliers{$id_supplier2} ), + 1, "Supplier2 has late orders and $daysago5 <= now() - (4 days+2)" ); ok( exists( $suppliers{$id_supplier3} ), "Supplier3 has late orders and $daysago10 <= now() - (4 days+3)" ); isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ); #Case 3: With $delay = -1 -#FIXME: GetBooksellersWithLateOrders doesn't test if the delay is a positive value -#is( C4::Bookseller::GetBooksellersWithLateOrders( -1, undef, undef ), -# undef, "-1 is a wrong value for a delay" ); +is( C4::Bookseller::GetBooksellersWithLateOrders( -1, undef, undef ), + undef, "-1 is a wrong value for a delay" ); #Case 4: With $delay = 0 # today == now-0 -LATE- (if no deliverytime or deliverytime == 0) @@ -516,9 +514,8 @@ my $daysago4 = output_pref( $dt_today3, 'iso', '24hr', 1 ); %suppliers = C4::Bookseller::GetBooksellersWithLateOrders( undef, $daysago4, undef ); -#FIXME: if the deliverytime is undef, it doesn't consider the supplier -#ok( exists( $suppliers{$id_supplier1} ), -# "Supplier1 has late orders and $today >= $daysago4 -deliverytime undef" ); +ok( exists( $suppliers{$id_supplier1} ), + "Supplier1 has late orders and $today >= $daysago4 -deliverytime undef" ); ok( exists( $suppliers{$id_supplier2} ), "Supplier2 has late orders and $daysago5 + 2 days >= $daysago4 " ); isnt( exists( $suppliers{$id_supplier3} ), @@ -552,13 +549,11 @@ isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ); # quantityreceived = quantity -NOT LATE- %suppliers = C4::Bookseller::GetBooksellersWithLateOrders( undef, undef, $daysago5 ); -#FIXME: if only the estimateddeliverydatefrom is given, it doesn't consider the parameters, -#but it replaces it today's date -#isnt( exists( $suppliers{$id_supplier1} ), -# 1, -# "Supplier1 has late orders but $today >= $daysago5 - deliverytime undef" ); -#isnt( exists( $suppliers{$id_supplier2} ), -# 1, "Supplier2 has late orders but $daysago5 + 2 days > $daysago5 " ); +isnt( exists( $suppliers{$id_supplier1} ), + 1, + "Supplier1 has late orders but $today >= $daysago5 - deliverytime undef" ); +isnt( exists( $suppliers{$id_supplier2} ), + 1, "Supplier2 has late orders but $daysago5 + 2 days > $daysago5 " ); ok( exists( $suppliers{$id_supplier3} ), "Supplier3 has late orders and $daysago10 + 3 <= $daysago5" ); isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ); @@ -616,13 +611,11 @@ isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ); C4::Bookseller::GetBooksellersWithLateOrders( 5, undef, $daysago5 ); isnt( exists( $suppliers{$id_supplier1} ), 1, "Supplier2 has late orders but today > $daysago5 today > now() -5" ); -#FIXME: GetBookSellersWithLateOrders replace estimateddeliverydateto by -#today's date when no estimateddeliverydatefrom is give -#isnt( -# exists( $suppliers{$id_supplier2} ), -# 1, -#"Supplier2 has late orders but $daysago5 + 2 days > $daysago5 and $daysago5 > now() - 2+5 days" -#); +isnt( + exists( $suppliers{$id_supplier2} ), + 1, +"Supplier2 has late orders but $daysago5 + 2 days > $daysago5 and $daysago5 > now() - 2+5 days" +); ok( exists( $suppliers{$id_supplier3} ), "Supplier2 has late orders and $daysago10 + 3 days <= $daysago5 and $daysago10 <= now() - 3+5 days " @@ -643,10 +636,9 @@ $basket1info = { ModBasket($basket1info); %suppliers = C4::Bookseller::GetBooksellersWithLateOrders( undef, $daysago10, $daysago10 ); -#FIXME :GetBookSellers doesn't take care if the closedate is ==$estimateddeliverydateto -# ok( exists( $suppliers{$id_supplier1} ), -# "Supplier1 has late orders and $daysago10==$daysago10==$daysago10 " ) -# ; +ok( exists( $suppliers{$id_supplier1} ), + "Supplier1 has late orders and $daysago10==$daysago10==$daysago10 " ) + ; isnt( exists( $suppliers{$id_supplier2} ), 1, "Supplier2 has late orders but $daysago10==$daysago10<$daysago5+2" ); @@ -658,9 +650,8 @@ isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" ); #Case 12: closedate == $estimateddeliverydatefrom =today-10 %suppliers = C4::Bookseller::GetBooksellersWithLateOrders( undef, $daysago10, undef ); -#FIXME :GetBookSellers doesn't take care if the closedate is ==$estimateddeliverydateto -#ok( exists( $suppliers{$id_supplier1} ), -# "Supplier1 has late orders and $daysago10==$daysago10 " ); +ok( exists( $suppliers{$id_supplier1} ), + "Supplier1 has late orders and $daysago10==$daysago10 " ); #Case 13: closedate == $estimateddeliverydateto =today-10 %suppliers = -- 1.7.9.5