From 87aed7999beb72194c98a5dbee3af04123f67f34 Mon Sep 17 00:00:00 2001
From: Jonathan Druart <jonathan.druart@biblibre.com>
Date: Thu, 13 Nov 2014 11:53:28 +0100
Subject: [PATCH] [PASSED QA] Bug 13244: Merge GetOrders and GetCancelledOrders

These two subroutines did the same job (same select, same join, etc.)

Test plan:
Go on the basket list page and verify you see the pending and the
cancelled baskets.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Two small things are adjusted in separate follow-ups.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
---
 C4/Acquisition.pm            |  105 ++++++++++++++++++-----------------------
 acqui/basket.pl              |    2 +-
 t/db_dependent/Acquisition.t |   18 +++----
 3 files changed, 55 insertions(+), 70 deletions(-)

diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm
index 1c9b3fe..dfa4cee 100644
--- a/C4/Acquisition.pm
+++ b/C4/Acquisition.pm
@@ -62,7 +62,7 @@ BEGIN {
         &GetLateOrders &GetOrderFromItemnumber
         &SearchOrders &GetHistory &GetRecentAcqui
         &ModReceiveOrder &CancelReceipt
-        &GetCancelledOrders &TransferOrder
+        &TransferOrder
         &GetLastOrderNotReceivedFromSubscriptionid &GetLastOrderReceivedFromSubscriptionid
         &ModItemOrder
 
@@ -1036,43 +1036,75 @@ sub GetBasketgroups {
 
 =head3 GetOrders
 
-  @orders = &GetOrders($basketnumber, $orderby);
+  @orders = &GetOrders( $basketno, { orderby => 'biblio.title', cancelled => 0|1 } );
 
 Looks up the pending (non-cancelled) orders with the given basket
-number. If C<$booksellerID> is non-empty, only orders from that seller
-are returned.
+number.
 
-return :
-C<&basket> returns a two-element array. C<@orders> is an array of
-references-to-hash, whose keys are the fields from the aqorders,
-biblio, and biblioitems tables in the Koha database.
+If cancelled is set, only cancelled orders will be returned.
 
 =cut
 
 sub GetOrders {
-    my ( $basketno, $orderby ) = @_;
+    my ( $basketno, $params ) = @_;
+
     return () unless $basketno;
+
+    my $orderby = $params->{orderby};
+    my $cancelled = $params->{cancelled} || 0;
+
     my $dbh   = C4::Context->dbh;
-    my $query  ="
+    my $query = q|
         SELECT biblio.*,biblioitems.*,
                 aqorders.*,
                 aqbudgets.*,
+        |;
+    $query .= $cancelled
+      ? q|
+                aqorders_transfers.ordernumber_from AS transferred_to,
+                aqorders_transfers.timestamp AS transferred_to_timestamp
+    |
+      : q|
                 aqorders_transfers.ordernumber_from AS transferred_from,
                 aqorders_transfers.timestamp AS transferred_from_timestamp
+    |;
+    $query .= q|
         FROM    aqorders
             LEFT JOIN aqbudgets        ON aqbudgets.budget_id = aqorders.budget_id
             LEFT JOIN biblio           ON biblio.biblionumber = aqorders.biblionumber
             LEFT JOIN biblioitems      ON biblioitems.biblionumber =biblio.biblionumber
+    |;
+    $query .= $cancelled
+      ? q|
+            LEFT JOIN aqorders_transfers ON aqorders_transfers.ordernumber_from = aqorders.ordernumber
+    |
+      : q|
             LEFT JOIN aqorders_transfers ON aqorders_transfers.ordernumber_to = aqorders.ordernumber
+
+    |;
+    $query .= q|
         WHERE   basketno=?
+    |;
+
+    if ($cancelled) {
+        $orderby ||= q|biblioitems.publishercode, biblio.title|;
+        $query .= q|
+            AND (datecancellationprinted IS NOT NULL
+               AND datecancellationprinted <> '0000-00-00')
+        |;
+    }
+    else {
+        $orderby ||=
+          q|aqorders.datecancellationprinted desc, aqorders.timestamp desc|;
+        $query .= q|
             AND (datecancellationprinted IS NULL OR datecancellationprinted='0000-00-00')
-    ";
+        |;
+    }
 
-    $orderby = "biblioitems.publishercode,biblio.title" unless $orderby;
     $query .= " ORDER BY $orderby";
-    my $result_set =
+    my $orders =
       $dbh->selectall_arrayref( $query, { Slice => {} }, $basketno );
-    return @{$result_set};
+    return @{$orders};
 
 }
 
@@ -1307,51 +1339,6 @@ sub ModItemOrder {
 
 #------------------------------------------------------------#
 
-=head3 GetCancelledOrders
-
-  my @orders = GetCancelledOrders($basketno, $orderby);
-
-Returns cancelled orders for a basket
-
-=cut
-
-sub GetCancelledOrders {
-    my ( $basketno, $orderby ) = @_;
-
-    return () unless $basketno;
-
-    my $dbh   = C4::Context->dbh;
-    my $query = "
-        SELECT
-            biblio.*,
-            biblioitems.*,
-            aqorders.*,
-            aqbudgets.*,
-            aqorders_transfers.ordernumber_to AS transferred_to,
-            aqorders_transfers.timestamp AS transferred_to_timestamp
-        FROM aqorders
-          LEFT JOIN aqbudgets   ON aqbudgets.budget_id = aqorders.budget_id
-          LEFT JOIN biblio      ON biblio.biblionumber = aqorders.biblionumber
-          LEFT JOIN biblioitems ON biblioitems.biblionumber = biblio.biblionumber
-          LEFT JOIN aqorders_transfers ON aqorders_transfers.ordernumber_from = aqorders.ordernumber
-        WHERE basketno = ?
-          AND (datecancellationprinted IS NOT NULL
-               AND datecancellationprinted <> '0000-00-00')
-    ";
-
-    $orderby = "aqorders.datecancellationprinted desc, aqorders.timestamp desc"
-        unless $orderby;
-    $query .= " ORDER BY $orderby";
-    my $sth = $dbh->prepare($query);
-    $sth->execute($basketno);
-    my $results = $sth->fetchall_arrayref( {} );
-
-    return @$results;
-}
-
-
-#------------------------------------------------------------#
-
 =head3 ModReceiveOrder
 
   &ModReceiveOrder({
diff --git a/acqui/basket.pl b/acqui/basket.pl
index 921c67e..55770ea 100755
--- a/acqui/basket.pl
+++ b/acqui/basket.pl
@@ -365,7 +365,7 @@ if ( $op eq 'delete_confirm' ) {
     push @book_foot_loop, map {$_} values %foot;
 
     # Get cancelled orders
-    @results = GetCancelledOrders($basketno);
+    my @results = GetOrders($basketno, { cancelled => 1 });
     my @cancelledorders_loop;
     for my $order (@results) {
         my $line = get_order_infos( $order, $bookseller);
diff --git a/t/db_dependent/Acquisition.t b/t/db_dependent/Acquisition.t
index 427cdfe..d0e1cf6 100755
--- a/t/db_dependent/Acquisition.t
+++ b/t/db_dependent/Acquisition.t
@@ -19,7 +19,7 @@ use Modern::Perl;
 
 use POSIX qw(strftime);
 
-use Test::More tests => 88;
+use Test::More tests => 87;
 
 BEGIN {
     use_ok('C4::Acquisition');
@@ -527,14 +527,12 @@ ok(
 );
 
 #
-# Test GetCancelledOrders
+# Test GetOrders { cancelled => 1 }
 #
 
 @expectedfields =
   ( @base_expectedfields, ( 'transferred_to_timestamp', 'transferred_to' ) );
-is( GetCancelledOrders(), undef,
-    "GetCancelledOrders with no params returns undef" );
-@get_orders = GetCancelledOrders($basketno);
+@get_orders = GetOrders($basketno, { cancelled => 1 });
 (
     $test_missing_fields,   $test_extra_fields,
     $test_different_fields, $test_nbr_fields
@@ -543,21 +541,21 @@ is( GetCancelledOrders(), undef,
 is(
     $$test_nbr_fields[0],
     scalar @expectedfields,
-    "GetCancelledOrders gets orders with the right number of fields"
+    "GetOrders { cancelled => 1 } gets orders with the right number of fields"
 );
 is( join( " ", @$test_missing_fields ),
-    '', "GetCancelledOrders gets orders with no missing fields" );
+    '', "GetOrders { cancelled => 1 } gets orders with no missing fields" );
 is( join( " ", @$test_extra_fields ),
-    '', "GetCancelledOrders gets orders with no unexpected fields" );
+    '', "GetOrders { cancelled => 1 } gets orders with no unexpected fields" );
 is( join( " ", @$test_different_fields ),
     '',
-    "GetCancelledOrders gets orders with the right content in every fields" );
+    "GetOrders { cancelled => 1 } gets orders with the right content in every fields" );
 ok(
     (
         ( scalar @get_orders == 1 )
           and grep ( $_->{ordernumber} eq $ordernumbers[3], @get_orders )
     ),
-    "GetCancelledOrders only gets cancelled orders"
+    "GetOrders { cancelled => 1 } only gets cancelled orders"
 );
 
 #
-- 
1.7.2.5