From 9fb422bb9f19f83807abc9a0ee64d6719adb52c3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 13 Nov 2014 11:53:28 +0100 Subject: [PATCH] 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 Two small things are adjusted in separate follow-ups. Signed-off-by: Kyle M Hall Signed-off-by: Tomas Cohen Arazi --- 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 0bb34c66ef..2c9ebe8f7c 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}; } @@ -1306,51 +1338,6 @@ sub ModItemOrder { return $sth->execute($ordernumber, $itemnumber); } -#------------------------------------------------------------# - -=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 diff --git a/acqui/basket.pl b/acqui/basket.pl index a44cdd13ef..e184c1c79e 100755 --- a/acqui/basket.pl +++ b/acqui/basket.pl @@ -366,7 +366,7 @@ if ( $op eq 'delete_confirm' ) { push @book_foot_loop, map {$_} values %foot; # Get cancelled orders - my @cancelledorders = GetCancelledOrders($basketno); + my @cancelledorders = GetOrders($basketno, { cancelled => 1 }); my @cancelledorders_loop; for my $order (@cancelledorders) { $order = C4::Acquisition::populate_order_with_prices({ order => $order, booksellerid => $booksellerid, ordering => 1 }); diff --git a/t/db_dependent/Acquisition.t b/t/db_dependent/Acquisition.t index aa09a3afc4..f41a34533d 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'); @@ -525,14 +525,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 @@ -541,21 +539,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" ); # -- 2.39.5