Attachment 19984 Details for Bug 10641 – [SIGNED-OFF] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences

[patch] [SIGNED-OFF] Bug 10641 - GetBooksellerWithLateOrders in C4::Bookseller.pm has some incoherences

Bug-10641---GetBooksellerWithLateOrders-in-C4Books.patch (text/plain), 9.43 KB, created by Srdjan Jankovic on 2013-07-30 04:18:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Srdjan Jankovic

Created: 2013-07-30 04:18:57 UTC

Size: 9.43 KB

patch

obsolete

>From 6cb789ae6d640d0f5f18edb37f8b1b52dc526cdd Mon Sep 17 00:00:00 2001
>From: root <root@kenza-VirtualBox>
>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.
>
>Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
>---
> C4/Bookseller.pm            | 32 ++++++++++++++++-----------
> t/db_dependent/Bookseller.t | 53 +++++++++++++++++++--------------------------
> 2 files changed, 41 insertions(+), 44 deletions(-)
>
>diff --git a/C4/Bookseller.pm b/C4/Bookseller.pm
>index 3e1454f..3bd0b28 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 ( $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;
>+        $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;
>+            } else {
>+                    $query .= ' AND ADDDATE(aqbasket.closedate, INTERVAL COALESCE(aqbooksellers.deliverytime, 0) DAY) <= CAST(now() AS date)';
>+            }
>     }
>-    if ( defined $estimateddeliverydatefrom and defined $estimateddeliverydateto ) {
>-        $strsth .= ' AND ADDDATE(aqbasket.closedate, INTERVAL aqbooksellers.deliverytime DAY) <= ?';
>+    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.8.1.2

Actions: View | Diff | Splinter Review

Attachments on bug 10641: 19942 | 19975 | 19984 | 20628 | 20629 | 20662