Attachment 112796 Details for Bug 26584 – Bug 26584: Remove unused C4::Acquisition::CloseBasket function

[patch] Bug 26584: Remove unused C4::Acquisition::CloseBasket function

Bug-26584-Remove-unused-C4AcquisitionCloseBasket-f.patch (text/plain), 9.06 KB, created by David Nind on 2020-11-01 01:11:02 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2020-11-01 01:11:02 UTC

Size: 9.06 KB

patch

obsolete

>From 66abd35fa9dd16b31fc5cd3848485b7af70a5cbf Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>Date: Wed, 30 Sep 2020 15:39:05 -0300
>Subject: [PATCH] Bug 26584: Remove unused C4::Acquisition::CloseBasket
> function
>
>This patch makes code use the new Koha::Acquisition::Basket->close
>method and makes CloseBasket obsolete.
>
>It then removes it, and adapts the few places in which it was used.
>
>1. Apply this patch
>2. Run:
>   $ kshell
>  k$ git diff origin/master --name-only | grep -e '\.t$' | xargs prove
>=> SUCCESS: Tests pass!
>3. Try playing with baskets, closing them
>=> SUCCESS: All works as expected!
>4. Sign off :-D
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> C4/Acquisition.pm                                  | 24 +----------------
> Koha/EDI.pm                                        |  4 +--
> acqui/basket.pl                                    | 31 ++++++++++++----------
> t/db_dependent/Acquisition.t                       |  2 +-
> .../Acquisition/GetBasketsInfosByBookseller.t      |  4 +--
> t/db_dependent/Acquisition/close_reopen_basket.t   |  6 ++---
> t/db_dependent/Letters.t                           |  2 +-
> 7 files changed, 27 insertions(+), 46 deletions(-)
>
>diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm
>index e29cbe8733..2ac15473b4 100644
>--- a/C4/Acquisition.pm
>+++ b/C4/Acquisition.pm
>@@ -53,7 +53,7 @@ BEGIN {
>     require Exporter;
>     @ISA    = qw(Exporter);
>     @EXPORT = qw(
>-        &GetBasket &NewBasket &CloseBasket &ReopenBasket &ModBasket
>+        &GetBasket &NewBasket &ReopenBasket &ModBasket
>         &GetBasketAsCSV &GetBasketGroupAsCSV
>         &GetBasketsByBookseller &GetBasketsByBasketgroup
>         &GetBasketsInfosByBookseller
>@@ -206,28 +206,6 @@ sub NewBasket {
>     return $basket;
> }
> 
>-#------------------------------------------------------------#
>-
>-=head3 CloseBasket
>-
>-  &CloseBasket($basketno);
>-
>-close a basket (becomes unmodifiable, except for receives)
>-
>-=cut
>-
>-sub CloseBasket {
>-    my ($basketno) = @_;
>-    my $dbh        = C4::Context->dbh;
>-    $dbh->do('UPDATE aqbasket SET closedate=now() WHERE basketno=?', {}, $basketno );
>-
>-    $dbh->do(
>-q{UPDATE aqorders SET orderstatus = 'ordered' WHERE basketno = ? AND orderstatus NOT IN ( 'complete', 'cancelled')},
>-        {}, $basketno
>-    );
>-    return;
>-}
>-
> =head3 ReopenBasket
> 
>   &ReopenBasket($basketno);
>diff --git a/Koha/EDI.pm b/Koha/EDI.pm
>index 8fc8d0d828..445998d51e 100644
>--- a/Koha/EDI.pm
>+++ b/Koha/EDI.pm
>@@ -28,7 +28,7 @@ use DateTime;
> use C4::Context;
> use Koha::Database;
> use Koha::DateUtils;
>-use C4::Acquisition qw( NewBasket CloseBasket ModOrder);
>+use C4::Acquisition qw( NewBasket ModOrder);
> use C4::Suggestions qw( ModSuggestion );
> use C4::Biblio qw( AddBiblio TransformKohaToMarc GetMarcBiblio GetFrameworkCode GetMarcFromKohaField );
> use Koha::Edifact::Order;
>@@ -592,7 +592,7 @@ sub process_quote {
>                     basketno => $b,
>                 }
>             );
>-            CloseBasket($b);
>+            Koha::Acquisition::Baskets->find($b)->close;
>         }
>     }
> 
>diff --git a/acqui/basket.pl b/acqui/basket.pl
>index d0fa4f2eb7..7d1f432c41 100755
>--- a/acqui/basket.pl
>+++ b/acqui/basket.pl
>@@ -191,9 +191,12 @@ if ( $op eq 'delete_confirm' ) {
> } elsif ($op eq 'close') {
>     my $confirm = $query->param('confirm') || $confirm_pref eq '2';
>     if ($confirm) {
>-        my $basketno = $query->param('basketno');
>-        my $booksellerid = $query->param('booksellerid');
>-        $basketno =~ /^\d+$/ and CloseBasket($basketno);
>+
>+        # close the basket
>+        # FIXME: we should fetch the object at the beginning of this script
>+        #        and get rid of the hash that is passed around
>+        Koha::Acquisition::Baskets->find($basketno)->close;
>+
>         # if requested, create basket group, close it and attach the basket
>         if ($query->param('createbasketgroup')) {
>             my $branchcode;
>@@ -524,17 +527,17 @@ sub get_order_infos {
> sub edi_close_and_order {
>     my $confirm = $query->param('confirm') || $confirm_pref eq '2';
>     if ($confirm) {
>-            my $edi_params = {
>-                basketno => $basketno,
>-                ean    => $ean,
>-            };
>-            if ( $basket->{branch} ) {
>-                $edi_params->{branchcode} = $basket->{branch};
>-            }
>-            if ( create_edi_order($edi_params) ) {
>-                #$template->param( edifile => 1 );
>-            }
>-        CloseBasket($basketno);
>+        my $edi_params = {
>+            basketno => $basketno,
>+            ean    => $ean,
>+        };
>+        if ( $basket->{branch} ) {
>+            $edi_params->{branchcode} = $basket->{branch};
>+        }
>+        if ( create_edi_order($edi_params) ) {
>+            #$template->param( edifile => 1 );
>+        }
>+        Koha::Acquisition::Baskets->find($basketno)->close;
> 
>         # if requested, create basket group, close it and attach the basket
>         if ( $query->param('createbasketgroup') ) {
>diff --git a/t/db_dependent/Acquisition.t b/t/db_dependent/Acquisition.t
>index d6d830bd50..5669923b65 100755
>--- a/t/db_dependent/Acquisition.t
>+++ b/t/db_dependent/Acquisition.t
>@@ -394,7 +394,7 @@ ok( GetBudgetByOrderNumber( $ordernumbers[0] )->{'budget_id'} eq $budgetid,
> my $lateorders = Koha::Acquisition::Orders->filter_by_lates({ delay => 0 });
> is( $lateorders->search({ 'me.basketno' => $basketno })->count,
>     0, "GetLateOrders does not get orders from opened baskets" );
>-C4::Acquisition::CloseBasket($basketno);
>+Koha::Acquisition::Baskets->find($basketno)->close;
> $lateorders = Koha::Acquisition::Orders->filter_by_lates({ delay => 0 });
> isnt( $lateorders->search({ 'me.basketno' => $basketno })->count,
>     0, "GetLateOrders gets orders from closed baskets" );
>diff --git a/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t b/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t
>index 0cd4f599f4..fce3cf6033 100755
>--- a/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t
>+++ b/t/db_dependent/Acquisition/GetBasketsInfosByBookseller.t
>@@ -103,7 +103,7 @@ is( $basket->{total_items_cancelled}, 6, 'Both orders deleted, 6 items cancelled
> is( $basket->{expected_items}, 0, 'Both orders delete, now 0 items are expected' );
> is( $basket->{total_biblios_cancelled}, 2, 'Both orders deleted, 2 biblios cancelled' );
> 
>-C4::Acquisition::CloseBasket( $basketno );
>+Koha::Acquisition::Baskets->find( $basketno )->close;
> $baskets = C4::Acquisition::GetBasketsInfosByBookseller( $supplierid );
> is( scalar(@$baskets), 0, 'Basket is closed, 0 basket opened' );
> $baskets = C4::Acquisition::GetBasketsInfosByBookseller( $supplierid, 1 );
>@@ -120,7 +120,7 @@ my $order3 = Koha::Acquisition::Order->new(
> )->store;
> my $ordernumber3 = $order3->ordernumber;
> 
>-C4::Acquisition::CloseBasket( $basketno );
>+Koha::Acquisition::Baskets->find( $basketno )->close;
> $baskets = C4::Acquisition::GetBasketsInfosByBookseller( $supplierid );
> is( scalar(@$baskets), 1, 'Basket is closed and has items to receive' );
> $basket = $baskets->[0];
>diff --git a/t/db_dependent/Acquisition/close_reopen_basket.t b/t/db_dependent/Acquisition/close_reopen_basket.t
>index 09225e8588..7faaac1611 100755
>--- a/t/db_dependent/Acquisition/close_reopen_basket.t
>+++ b/t/db_dependent/Acquisition/close_reopen_basket.t
>@@ -72,7 +72,7 @@ my @orders = C4::Acquisition::GetOrders( $basketno );
> is( scalar(@orders), 2, "2 orders are created" );
> is ( scalar( map { $_->{orderstatus} eq 'new' ? 1 : () } @orders ), 2, "2 orders are new before closing the basket" );
> 
>-C4::Acquisition::CloseBasket( $basketno );
>+Koha::Acquisition::Baskets->find( $basketno )->close;
> @orders = C4::Acquisition::GetOrders( $basketno );
> is ( scalar( map { $_->{orderstatus} eq 'ordered' ? 1 : () } @orders ), 2, "2 orders are ordered, the basket is closed" );
> 
>@@ -86,10 +86,10 @@ my ( $order ) = C4::Acquisition::GetOrders( $basketno, {cancelled => 1} );
> is( $order->{ordernumber}, $ordernumber1, 'The order returned by GetOrders should have been the right one' );
> is( $order->{orderstatus}, 'cancelled', 'cancelling the order should have set status to cancelled' );
> 
>-C4::Acquisition::CloseBasket( $basketno );
>+Koha::Acquisition::Baskets->find( $basketno )->close;
> ( $order ) = C4::Acquisition::GetOrders( $basketno, {cancelled => 1} );
> is( $order->{ordernumber}, $ordernumber1, 'The order returned by GetOrders should have been the right one' );
>-is( $order->{orderstatus}, 'cancelled', 'CloseBasket should not reset the status to ordered for cancelled orders' );
>+is( $order->{orderstatus}, 'cancelled', '$basket->close should not reset the status to ordered for cancelled orders' );
> 
> C4::Acquisition::ReopenBasket( $basketno );
> ( $order ) = C4::Acquisition::GetOrders( $basketno, {cancelled => 1} );
>diff --git a/t/db_dependent/Letters.t b/t/db_dependent/Letters.t
>index 9a629bdfbf..766e21d7f4 100755
>--- a/t/db_dependent/Letters.t
>+++ b/t/db_dependent/Letters.t
>@@ -436,7 +436,7 @@ my $order = Koha::Acquisition::Order->new(
> )->store;
> my $ordernumber = $order->ordernumber;
> 
>-C4::Acquisition::CloseBasket( $basketno );
>+Koha::Acquisition::Baskets->find( $basketno )->close;
> my $err;
> warning_like {
>     $err = SendAlerts( 'claimacquisition', [ $ordernumber ], 'TESTACQCLAIM' ) }
>-- 
>2.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 26584: 111010 | 112796 | 112801