Bug 12827: NewOrder should not return basketno

Since the basketno parameter is needed to insert an order, it is useless
to return it.

This patch changes the prototype for the C4::Acquisition::NewOrder
subroutine. The return value is now a scalar containing the ordernumber
created.

Test plan:
Verify there is no regression on an acquisition workflow:
1/ Create an order with several items
2/ Modify the order
3/ Receive some items
4/ Cancel the receipt
4/ Receive some items
5/ Receive all remaining items
6/ Cancel the receipt

Signed-off-by: Zeno Tajoli <z.tajoli@cineca.it>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

C4/Acquisition.pm

@@ -30,6 +30,7 @@ use C4::Contract;
 use C4::Debug;
 use C4::Bookseller qw(GetBookSellerFromId);
 use C4::Templates qw(gettemplate);
+use Koha::DateUtils qw( dt_from_string output_pref );
 
 use Time::localtime;
 use HTML::Entities;
@@ -1270,14 +1271,12 @@ sub NewOrder {
       if $orderinfo->{ordernumber};
 
     # if these parameters are missing, we can't continue
-    for my $key (qw/basketno quantity biblionumber budget_id/) {
+    for my $key ( qw( basketno quantity biblionumber budget_id ) ) {
-        croak "Mandatory parameter $key missing" unless $orderinfo->{$key};
+        croak "Cannot insert order: Mandatory parameter $key is missing" unless $orderinfo->{$key};
     }
 
-    $orderinfo->{'entrydate'} ||= C4::Dates->new()->output("iso");
+    $orderinfo->{entrydate} ||= output_pref({ dt => dt_from_string, dateformat => 'iso'});
-    if (!$orderinfo->{quantityreceived}) {
+    $orderinfo->{quantityreceived} ||= 0;
-        $orderinfo->{quantityreceived} = 0;
-    }
 
     # get only the columns of Aqorder
     my $schema = Koha::Database->new()->schema;
@@ -1285,21 +1284,19 @@ sub NewOrder {
     my $new_order = { map { $columns =~ / $_ / ? ($_ => $orderinfo->{$_}) : () } keys(%$orderinfo) };
     $new_order->{ordernumber} ||= undef;
 
-    my $rs = $schema->resultset('Aqorder');
+    my $order = $schema->resultset('Aqorder')->create($new_order);
-    my $ordernumber = $rs->create($new_order)->id;
+    my $ordernumber = $order->id;
-    if (not $new_order->{parent_ordernumber}) {
+
-        my $sth = $dbh->prepare("
+    unless ( $new_order->{parent_ordernumber} ) {
-            UPDATE aqorders
+        $order->update({ parent_ordernumber => $ordernumber });
-            SET parent_ordernumber = ordernumber
-            WHERE ordernumber = ?
-        ");
-        $sth->execute($ordernumber);
     }
-    return ( $new_order->{'basketno'}, $ordernumber );
+
+    return $ordernumber;
 }
 
 
 
+
 #------------------------------------------------------------#
 
 =head3 NewOrderItem
@@ -1538,8 +1535,7 @@ q{SELECT * FROM aqorders WHERE biblionumber=? AND aqorders.ordernumber=?},
         $order->{'rrp'} = $rrp;
         $order->{ecost} = $ecost;
         $order->{'orderstatus'} = 'complete';
-        my $basketno;
+        $new_ordernumber = NewOrder($order);
-        ( $basketno, $new_ordernumber ) = NewOrder($order);
 
         if ($received_items) {
             foreach my $itemnumber (@$received_items) {
@@ -1937,7 +1933,7 @@ sub TransferOrder {
     delete $order->{parent_ordernumber};
     $order->{'basketno'} = $basketno;
     my $newordernumber;
-    (undef, $newordernumber) = NewOrder($order);
+    $newordernumber = NewOrder($order);
 
     $query = q{
         UPDATE aqorders_items

acqui/addorder.pl

@@ -275,7 +275,7 @@ if ( $orderinfo->{quantity} ne '0' ) {
         ModOrder( $orderinfo);
     }
     else { # else, it's a new line
-        @$orderinfo{qw(basketno ordernumber )} = NewOrder($orderinfo);
+        $orderinfo->{ordernumber} = NewOrder($orderinfo);
     }
 
     # now, add items if applicable

acqui/addorderiso2709.pl

@@ -256,7 +256,7 @@ if ($op eq ""){
 
         # remove uncertainprice flag if we have found a price in the MARC record
         $orderinfo{uncertainprice} = 0 if $orderinfo{listprice};
-        my ( $basketno, $ordernumber ) = NewOrder( \%orderinfo );
+        my $ordernumber = NewOrder( \%orderinfo );
 
         # 4th, add items if applicable
         # parse the item sent by the form, and create an item just for the import_record_id we are dealing with

t/db_dependent/Acquisition.t

@@ -151,10 +151,10 @@ my ( $biblionumber4, $biblioitemnumber4 ) = AddBiblio( MARC::Record->new, '' );
 my ( $mandatoryparams, $return_error, $basketnum );
 
 # returns undef and croaks if basketno, quantity, biblionumber or budget_id is missing
-eval { ( $basketnum, $ordernumbers[0] ) = C4::Acquisition::NewOrder() };
+eval { $ordernumbers[0] = C4::Acquisition::NewOrder() };
 $return_error = $@;
 ok(
-    ( !( defined $basketnum || defined $ordernumbers[0] ) )
+    ( ! defined $ordernumbers[0] )
       && ( defined $return_error ),
     "NewOrder with no params returns undef and croaks"
 );
@@ -170,11 +170,11 @@ foreach my $mandatoryparams_key (@mandatoryparams_keys) {
     my %test_missing_mandatoryparams = %$mandatoryparams;
     delete $test_missing_mandatoryparams{$mandatoryparams_key};
     eval {
-        ( $basketnum, $ordernumbers[0] ) =
+        $ordernumbers[0] =
           C4::Acquisition::NewOrder( \%test_missing_mandatoryparams );
     };
     $return_error = $@;
-    my $expected_error = "Mandatory parameter $mandatoryparams_key missing";
+    my $expected_error = "Cannot insert order: Mandatory parameter $mandatoryparams_key is missing";
     ok(
         ( !( defined $basketnum || defined $ordernumbers[0] ) )
           && ( index( $return_error, $expected_error ) >= 0 ),
@@ -284,7 +284,7 @@ for ( 0 .. 4 ) {
       values %{ $order_content[$_]->{num} };
     @ocontent{ keys %{ $order_content[$_]->{str} } } =
       values %{ $order_content[$_]->{str} };
-    ( undef, $ordernumbers[$_] ) = C4::Acquisition::NewOrder( \%ocontent );
+    $ordernumbers[$_] = C4::Acquisition::NewOrder( \%ocontent );
     $order_content[$_]->{str}->{ordernumber} = $ordernumbers[$_];
 }
 

t/db_dependent/Acquisition/CancelReceipt.t

@@ -44,7 +44,7 @@ my ($biblionumber, $biblioitemnumber) = AddBiblio(MARC::Record->new, '');
 my $itemnumber = AddItem({}, $biblionumber);
 
 t::lib::Mocks::mock_preference('AcqCreateItem', 'receiving');
-my ( undef, $ordernumber ) = C4::Acquisition::NewOrder(
+my $ordernumber = C4::Acquisition::NewOrder(
     {
         basketno => $basketno1,
         quantity => 2,
@@ -73,7 +73,7 @@ my $itemnumber1 = AddItem({}, $biblionumber);
 my $itemnumber2 = AddItem({}, $biblionumber);
 t::lib::Mocks::mock_preference('AcqCreateItem', 'ordering');
 t::lib::Mocks::mock_preference('AcqItemSetSubfieldsWhenReceiptIsCancelled', '7=9'); # notforloan is mapped with 952$7
-( undef, $ordernumber ) = C4::Acquisition::NewOrder(
+$ordernumber = C4::Acquisition::NewOrder(
     {
         basketno => $basketno1,
         quantity => 2,

t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t

@@ -39,7 +39,7 @@ my ($biblionumber1, $biblioitemnumber1) = AddBiblio(MARC::Record->new, '');
 my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
 my ($biblionumber3, $biblioitemnumber3) = AddBiblio(MARC::Record->new, '');
 
-( undef, $ordernumber1 ) = C4::Acquisition::NewOrder(
+$ordernumber1 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 2,
@@ -48,7 +48,7 @@ my ($biblionumber3, $biblioitemnumber3) = AddBiblio(MARC::Record->new, '');
     }
 );
 
-( undef, $ordernumber2 ) = C4::Acquisition::NewOrder(
+$ordernumber2 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 4,

t/db_dependent/Acquisition/GetOrdersByBiblionumber.t

@@ -39,7 +39,7 @@ my $budget = C4::Budgets::GetBudget( $budgetid );
 my ($ordernumber1, $ordernumber2, $ordernumber3);
 my ($biblionumber1, $biblioitemnumber1) = AddBiblio(MARC::Record->new, '');
 my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
-( undef, $ordernumber1 ) = C4::Acquisition::NewOrder(
+$ordernumber1 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 24,
@@ -48,7 +48,7 @@ my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
     }
 );
 
-( undef, $ordernumber2 ) = C4::Acquisition::NewOrder(
+$ordernumber2 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 42,
@@ -57,7 +57,7 @@ my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
     }
 );
 
-( undef, $ordernumber3 ) = C4::Acquisition::NewOrder(
+$ordernumber3 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 4,

t/db_dependent/Acquisition/Invoices.t

@@ -53,7 +53,7 @@ $bibrec1->append_fields(
 my ($biblionumber1, $biblioitemnumber1) = AddBiblio($bibrec1, '');
 my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
 my ($biblionumber3, $biblioitemnumber3) = AddBiblio(MARC::Record->new, '');
-( undef, $ordernumber1 ) = C4::Acquisition::NewOrder(
+$ordernumber1 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 2,
@@ -62,7 +62,7 @@ my ($biblionumber3, $biblioitemnumber3) = AddBiblio(MARC::Record->new, '');
     }
 );
 
-( undef, $ordernumber2 ) = C4::Acquisition::NewOrder(
+$ordernumber2 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 1,
@@ -71,7 +71,7 @@ my ($biblionumber3, $biblioitemnumber3) = AddBiblio(MARC::Record->new, '');
     }
 );
 
-( undef, $ordernumber3 ) = C4::Acquisition::NewOrder(
+$ordernumber3 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 1,

t/db_dependent/Acquisition/OrderFromSubscription.t

@@ -57,7 +57,7 @@ ok($basketno = NewBasket($booksellerid, 1), "NewBasket(  $booksellerid , 1  ) re
 my $cost = 42.00;
 my $subscription = GetSubscription( $subscriptionid );
 my $ordernumber;
-( $basketno, $ordernumber ) = NewOrder({
+$ordernumber = NewOrder({
     biblionumber => $subscription->{biblionumber},
     entrydate => '01-01-2013',
     quantity => 1,

t/db_dependent/Acquisition/TransferOrder.t

@@ -54,7 +54,7 @@ my $budget = C4::Budgets::GetBudget( $budgetid );
 my ($biblionumber, $biblioitemnumber) = AddBiblio(MARC::Record->new, '');
 my $itemnumber = AddItem({}, $biblionumber);
 
-my ( undef, $ordernumber ) = C4::Acquisition::NewOrder(
+my $ordernumber = C4::Acquisition::NewOrder(
     {
         basketno => $basketno1,
         quantity => 2,

t/db_dependent/Acquisition/close_reopen_basket.t

@@ -44,7 +44,7 @@ my ($biblionumber1, $biblioitemnumber1) = AddBiblio(MARC::Record->new, '');
 my ($biblionumber2, $biblioitemnumber2) = AddBiblio(MARC::Record->new, '');
 
 my ($ordernumber1, $ordernumber2);
-( undef, $ordernumber1 ) = C4::Acquisition::NewOrder(
+$ordernumber1 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 24,
@@ -53,7 +53,7 @@ my ($ordernumber1, $ordernumber2);
     }
 );
 
-( undef, $ordernumber2 ) = C4::Acquisition::NewOrder(
+$ordernumber2 = C4::Acquisition::NewOrder(
     {
         basketno => $basketno,
         quantity => 42,

t/db_dependent/Bookseller.t

@@ -127,9 +127,9 @@ is_deeply( \@booksellers, \@tab,
 my @bookseller1 = C4::Bookseller::GetBookSeller( $sample_supplier1->{name} );
 #FIXME : if there is 0 basket, GetBookSeller returns 1 as basketcount
 #is( $bookseller1[0]->{basketcount}, 0, 'Supplier1 has 0 basket' );
-my $sample_basket1 =
+my $basketno1 =
   C4::Acquisition::NewBasket( $id_supplier1, 'authorisedby1', 'basketname1' );
-my $sample_basket2 =
+my $basketno2 =
   C4::Acquisition::NewBasket( $id_supplier1, 'authorisedby2', 'basketname2' );
 @bookseller1 = C4::Bookseller::GetBookSeller( $sample_supplier1->{name} );
 is( $bookseller1[0]->{basketcount}, 2, 'Supplier1 has 2 baskets' );
@@ -290,22 +290,22 @@ my $id_supplier3 = C4::Bookseller::AddBookseller($sample_supplier3);
 my $id_supplier4 = C4::Bookseller::AddBookseller($sample_supplier4);
 
 #Add 2 baskets
-my $sample_basket3 =
+my $basketno3 =
   C4::Acquisition::NewBasket( $id_supplier3, 'authorisedby3', 'basketname3',
     'basketnote3' );
-my $sample_basket4 =
+my $basketno4 =
   C4::Acquisition::NewBasket( $id_supplier4, 'authorisedby4', 'basketname4',
     'basketnote4' );
 
 #Modify the basket to add a close date
 my $basket1info = {
-    basketno     => $sample_basket1,
+    basketno     => $basketno1,
     closedate    => $today,
     booksellerid => $id_supplier1
 };
 
 my $basket2info = {
-    basketno     => $sample_basket2,
+    basketno     => $basketno2,
     closedate    => $daysago5,
     booksellerid => $id_supplier2
 };
@@ -315,12 +315,12 @@ my $dur10 = DateTime::Duration->new( days => -10 );
 $dt_today2->add_duration($dur10);
 my $daysago10 = output_pref({ dt => $dt_today2, dateformat => 'iso', timeformat => '24hr', dateonly => 1 });
 my $basket3info = {
-    basketno  => $sample_basket3,
+    basketno  => $basketno3,
     closedate => $daysago10,
 };
 
 my $basket4info = {
-    basketno  => $sample_basket4,
+    basketno  => $basketno4,
     closedate => $today,
 };
 ModBasket($basket1info);
@@ -351,10 +351,9 @@ is(scalar(@subscriptions), 1, 'search for subscriptions by location');
 
 #Add 4 orders
 my ( $ordernumber1, $ordernumber2, $ordernumber3, $ordernumber4 );
-my ( $basketno1,    $basketno2,    $basketno3,    $basketno4 );
+$ordernumber1 = C4::Acquisition::NewOrder(
-( $basketno1, $ordernumber1 ) = C4::Acquisition::NewOrder(
     {
-        basketno         => $sample_basket1,
+        basketno         => $basketno1,
         quantity         => 24,
         biblionumber     => $biblionumber,
         budget_id        => $id_budget,
@@ -370,9 +369,9 @@ my ( $basketno1,    $basketno2,    $basketno3,    $basketno4 );
         datereceived     => '01-06-2013'
     }
 );
-( $basketno2, $ordernumber2 ) = C4::Acquisition::NewOrder(
+$ordernumber2 = C4::Acquisition::NewOrder(
     {
-        basketno       => $sample_basket2,
+        basketno       => $basketno2,
         quantity       => 20,
         biblionumber   => $biblionumber,
         budget_id      => $id_budget,
@@ -386,9 +385,9 @@ my ( $basketno1,    $basketno2,    $basketno3,    $basketno4 );
         ecost          => 10,
     }
 );
-( $basketno3, $ordernumber3 ) = C4::Acquisition::NewOrder(
+$ordernumber3 = C4::Acquisition::NewOrder(
     {
-        basketno       => $sample_basket3,
+        basketno       => $basketno3,
         quantity       => 20,
         biblionumber   => $biblionumber,
         budget_id      => $id_budget,
@@ -402,9 +401,9 @@ my ( $basketno1,    $basketno2,    $basketno3,    $basketno4 );
         ecost          => 11,
     }
 );
-( $basketno4, $ordernumber4 ) = C4::Acquisition::NewOrder(
+$ordernumber4 = C4::Acquisition::NewOrder(
     {
-        basketno         => $sample_basket4,
+        basketno         => $basketno4,
         quantity         => 20,
         biblionumber     => $biblionumber,
         budget_id        => $id_budget,
@@ -604,7 +603,7 @@ isnt( exists( $suppliers{$id_supplier4} ), 1, "Supplier4 hasnt late orders" );
 
 #Basket1 closedate -> $daysago10
 $basket1info = {
-    basketno  => $sample_basket1,
+    basketno  => $basketno1,
     closedate => $daysago10,
 };
 ModBasket($basket1info);

t/db_dependent/Budgets.t

@@ -304,7 +304,7 @@ my $item_quantity = 2;
 my $number_of_orders_to_move = 0;
 for my $infos (@order_infos) {
     for ( 1 .. $infos->{pending_quantity} ) {
-        my ( undef, $ordernumber ) = C4::Acquisition::NewOrder(
+        my $ordernumber = C4::Acquisition::NewOrder(
             {
                 basketno           => $basketno,
                 biblionumber       => $biblionumber,
@@ -326,7 +326,7 @@ for my $infos (@order_infos) {
         $number_of_orders_to_move++;
     }
     for ( 1 .. $infos->{spent_quantity} ) {
-        my ( undef, $ordernumber ) = C4::Acquisition::NewOrder(
+        my $ordernumber = C4::Acquisition::NewOrder(
             {
                 basketno           => $basketno,
                 biblionumber       => $biblionumber,