From 9d996c95f84c8f8f6a924956dd63184aed8e384d Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Tue, 26 Aug 2014 16:06:10 +0200 Subject: [PATCH] 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 --- C4/Acquisition.pm | 38 +++++++++------------- acqui/addorder.pl | 2 +- acqui/addorderiso2709.pl | 2 +- t/db_dependent/Acquisition.t | 10 +++--- t/db_dependent/Acquisition/CancelReceipt.t | 4 +-- .../Acquisition/GetBasketsInfosByBookseller.t | 4 +-- .../Acquisition/GetOrdersByBiblionumber.t | 6 ++-- t/db_dependent/Acquisition/Invoices.t | 6 ++-- t/db_dependent/Acquisition/OrderFromSubscription.t | 2 +- t/db_dependent/Acquisition/TransferOrder.t | 2 +- t/db_dependent/Acquisition/close_reopen_basket.t | 4 +-- t/db_dependent/Bookseller.t | 35 ++++++++++---------- t/db_dependent/Budgets.t | 4 +-- 13 files changed, 55 insertions(+), 64 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index 5fc45c7..3159a16 100644 --- a/C4/Acquisition.pm +++ b/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; @@ -1263,40 +1264,32 @@ The following keys are used: "biblionumber", "title", "basketno", "quantity", "o sub NewOrder { my $orderinfo = shift; - my $dbh = C4::Context->dbh; - my @params; - - # if these parameters are missing, we can't continue - for my $key (qw/basketno quantity biblionumber budget_id/) { - croak "Mandatory parameter $key missing" unless $orderinfo->{$key}; + for my $key ( qw( basketno quantity biblionumber budget_id ) ) { + croak "Cannot insert order: Mandatory parameter $key is missing" unless $orderinfo->{$key}; } - $orderinfo->{'entrydate'} ||= C4::Dates->new()->output("iso"); - if (!$orderinfo->{quantityreceived}) { - $orderinfo->{quantityreceived} = 0; - } + $orderinfo->{entrydate} ||= output_pref({ dt => dt_from_string, dateformat => 'iso'}); + $orderinfo->{quantityreceived} ||= 0; # get only the columns of Aqorder my $schema = Koha::Database->new()->schema; my $columns = ' '.join(' ', $schema->source('Aqorder')->columns).' '; my $new_order = { map { $columns =~ / $_ / ? ($_ => $orderinfo->{$_}) : () } keys(%$orderinfo) }; - my $rs = $schema->resultset('Aqorder'); - my $ordernumber = $rs->create($new_order)->id; - if (not $new_order->{parent_ordernumber}) { - my $sth = $dbh->prepare(" - UPDATE aqorders - SET parent_ordernumber = ordernumber - WHERE ordernumber = ? - "); - $sth->execute($ordernumber); + my $order = $schema->resultset('Aqorder')->create($new_order); + my $ordernumber = $order->id; + + unless ( $new_order->{parent_ordernumber} ) { + $order->update({ parent_ordernumber => $ordernumber }); } - return ( $new_order->{'basketno'}, $ordernumber ); + + return $ordernumber; } + #------------------------------------------------------------# =head3 NewOrderItem @@ -1535,8 +1528,7 @@ q{SELECT * FROM aqorders WHERE biblionumber=? AND aqorders.ordernumber=?}, $order->{'rrp'} = $rrp; $order->{ecost} = $ecost; $order->{'orderstatus'} = 'complete'; - my $basketno; - ( $basketno, $new_ordernumber ) = NewOrder($order); + $new_ordernumber = NewOrder($order); if ($received_items) { foreach my $itemnumber (@$received_items) { @@ -1913,7 +1905,7 @@ sub TransferOrder { delete $order->{parent_ordernumber}; $order->{'basketno'} = $basketno; my $newordernumber; - (undef, $newordernumber) = NewOrder($order); + $newordernumber = NewOrder($order); $query = q{ UPDATE aqorders_items diff --git a/acqui/addorder.pl b/acqui/addorder.pl index e048d05..da1beb0 100755 --- a/acqui/addorder.pl +++ b/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 diff --git a/acqui/addorderiso2709.pl b/acqui/addorderiso2709.pl index f4d1a25..601482a 100755 --- a/acqui/addorderiso2709.pl +++ b/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 diff --git a/t/db_dependent/Acquisition.t b/t/db_dependent/Acquisition.t index 837024a..2a3e8c7 100755 --- a/t/db_dependent/Acquisition.t +++ b/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 ), @@ -283,7 +283,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[$_]; } diff --git a/t/db_dependent/Acquisition/CancelReceipt.t b/t/db_dependent/Acquisition/CancelReceipt.t index e2d4e4d..f00cafb 100644 --- a/t/db_dependent/Acquisition/CancelReceipt.t +++ b/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, @@ -70,7 +70,7 @@ my $order = GetOrder( $ordernumber ); is(scalar GetItemnumbersFromOrder($order->{ordernumber}), 0, "Create items on receiving: 0 item exist after cancelling a receipt"); t::lib::Mocks::mock_preference('AcqCreateItem', 'ordering'); -( undef, $ordernumber ) = C4::Acquisition::NewOrder( +$ordernumber = C4::Acquisition::NewOrder( { basketno => $basketno1, quantity => 2, diff --git a/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t b/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t index 3b4c96b..1c82ced 100644 --- a/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t +++ b/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, diff --git a/t/db_dependent/Acquisition/GetOrdersByBiblionumber.t b/t/db_dependent/Acquisition/GetOrdersByBiblionumber.t index 8bd0735..be6146d 100644 --- a/t/db_dependent/Acquisition/GetOrdersByBiblionumber.t +++ b/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, diff --git a/t/db_dependent/Acquisition/Invoices.t b/t/db_dependent/Acquisition/Invoices.t index 4123c1e..9c6dcbc 100644 --- a/t/db_dependent/Acquisition/Invoices.t +++ b/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, diff --git a/t/db_dependent/Acquisition/OrderFromSubscription.t b/t/db_dependent/Acquisition/OrderFromSubscription.t index 72d4c68..2753248 100644 --- a/t/db_dependent/Acquisition/OrderFromSubscription.t +++ b/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, diff --git a/t/db_dependent/Acquisition/TransferOrder.t b/t/db_dependent/Acquisition/TransferOrder.t index 91ee37a..dfe57ff 100644 --- a/t/db_dependent/Acquisition/TransferOrder.t +++ b/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, diff --git a/t/db_dependent/Acquisition/close_reopen_basket.t b/t/db_dependent/Acquisition/close_reopen_basket.t index 3e7a280..d5a4789 100644 --- a/t/db_dependent/Acquisition/close_reopen_basket.t +++ b/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, diff --git a/t/db_dependent/Bookseller.t b/t/db_dependent/Bookseller.t index 4cfd3c5..3c713b3 100644 --- a/t/db_dependent/Bookseller.t +++ b/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 ); -( $basketno1, $ordernumber1 ) = C4::Acquisition::NewOrder( +$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); diff --git a/t/db_dependent/Budgets.t b/t/db_dependent/Budgets.t index a565c8d..41bac90 100755 --- a/t/db_dependent/Budgets.t +++ b/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, -- 2.1.0