From 4654d341fa73608d07d84b9d13d8142837ae79f9 Mon Sep 17 00:00:00 2001
From: Julian Maurice <julian.maurice@biblibre.com>
Date: Fri, 14 Sep 2018 17:26:14 +0200
Subject: [PATCH] Bug 11708: Fix QA issues

- Change how closedate is set in database update
- Reword bookseller to vendor in Koha::Acquisition::Basketgroup
- Do not "cache" baskets in Koha::Acquisition::Basketgroup
- Remove useless IF in basketgroup.tt
- Move JS to bottom in basketgroups.tt
- Fix some links to basketgroup.pl
- Remove useless use in acqui/basketgroup.pl
- Remove useless sub displaybasketgroups in acqui/basketgroup.pl
- Remove useless code in C4::Acquisition
---
 C4/Acquisition.pm                             |   8 +-
 Koha/Acquisition/Basketgroup.pm               |  12 +-
 acqui/basketgroup.pl                          |  41 -------
 .../data/mysql/atomicupdate/bug_11708.perl    |   9 +-
 .../prog/en/modules/acqui/basket.tt           |   2 +-
 .../prog/en/modules/acqui/basketgroup.tt      |  10 +-
 .../prog/en/modules/acqui/basketgroups.tt     | 104 +++++++++---------
 .../prog/en/modules/acqui/parcel.tt           |   4 +-
 8 files changed, 76 insertions(+), 114 deletions(-)

diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm
index c2e065584b..881d25ac9b 100644
--- a/C4/Acquisition.pm
+++ b/C4/Acquisition.pm
@@ -1510,9 +1510,7 @@ sub ModReceiveOrder {
         $order->{datereceived} = $datereceived;
         $order->{invoiceid} = $invoice->{invoiceid};
         $order->{orderstatus} = 'complete';
-        my @columns = Koha::Acquisition::Orders->columns;
-        my %filtered_order = map { exists $order->{$_} ? ($_ => $order->{$_}) : () } @columns;
-        $new_ordernumber = Koha::Acquisition::Order->new(\%filtered_order)->store->ordernumber;
+        $new_ordernumber = Koha::Acquisition::Order->new($order)->store->ordernumber;
 
         if ($received_items) {
             foreach my $itemnumber (@$received_items) {
@@ -2010,9 +2008,7 @@ sub TransferOrder {
     delete $order->{parent_ordernumber};
     $order->{'basketno'} = $basketno;
 
-    my @columns = Koha::Acquisition::Orders->columns;
-    my %filtered_order = map { exists $order->{$_} ? ($_ => $order->{$_}) : () } @columns;
-    my $newordernumber = Koha::Acquisition::Order->new(\%filtered_order)->store->ordernumber;
+    my $newordernumber = Koha::Acquisition::Order->new($order)->store->ordernumber;
 
     $query = q{
         UPDATE aqorders_items
diff --git a/Koha/Acquisition/Basketgroup.pm b/Koha/Acquisition/Basketgroup.pm
index 26df6b94d8..b65d5ed5ba 100644
--- a/Koha/Acquisition/Basketgroup.pm
+++ b/Koha/Acquisition/Basketgroup.pm
@@ -31,15 +31,15 @@ Koha::Acquisition::Basketgroup
 
 =head2 Methods
 
-=head3 bookseller
+=head3 vendor
 
-Returns the basketgroup's bookseller (Koha::Acquisition::Bookseller)
+Returns the basketgroup's vendor (Koha::Acquisition::Bookseller)
 
-    my $bookseller = $basketgroup->bookseller;
+    my $vendor = $basketgroup->vendor;
 
 =cut
 
-sub bookseller {
+sub vendor {
     my ($self) = @_;
 
     return scalar Koha::Acquisition::Booksellers->find($self->booksellerid);
@@ -57,9 +57,9 @@ Returns the basketgroup's baskets
 sub baskets {
     my ($self) = @_;
 
-    $self->{_baskets} ||= Koha::Acquisition::Baskets->search({ basketgroupid => $self->id });
+    my $baskets = Koha::Acquisition::Baskets->search({ basketgroupid => $self->id });
 
-    return wantarray ? $self->{_baskets}->as_list : $self->{_baskets};
+    return wantarray ? $baskets->as_list : $baskets;
 }
 
 =head3 baskets_count
diff --git a/acqui/basketgroup.pl b/acqui/basketgroup.pl
index c4edfe3cd2..162e747cf6 100755
--- a/acqui/basketgroup.pl
+++ b/acqui/basketgroup.pl
@@ -58,8 +58,6 @@ use Koha::Acquisition::Booksellers;
 use Koha::ItemTypes;
 use Koha::Patrons;
 
-use List::MoreUtils qw/uniq/;
-
 our $input=new CGI;
 
 our ($template, $loggedinuser, $cookie)
@@ -87,40 +85,6 @@ sub BasketTotal {
     return $total;
 }
 
-#displays all basketgroups and all closed baskets (in their respective groups)
-sub displaybasketgroups {
-    my ($basketgroups, $bookseller, $baskets, $template) = @_;
-    if (scalar @$basketgroups != 0) {
-        foreach my $basketgroup (@$basketgroups){
-            my $i = 0;
-            my $basketsqty = 0;
-            while($i < scalar(@$baskets)){
-                my $basket = @$baskets[$i];
-                if($basket->{'basketgroupid'} && $basket->{'basketgroupid'} == $basketgroup->{'id'}){
-                    $basket->{total} = BasketTotal($basket->{basketno}, $bookseller);
-                    push(@{$basketgroup->{'baskets'}}, $basket);
-                    splice(@$baskets, $i, 1);
-                    ++$basketsqty;
-                    --$i;
-                }
-                ++$i;
-            }
-            $basketgroup -> {'basketsqty'} = $basketsqty;
-        }
-        $template->param(basketgroups => $basketgroups);
-    }
-    for(my $i=0; $i < scalar @$baskets; ++$i) {
-        if( ! @$baskets[$i]->{'closedate'} ) {
-            splice(@$baskets, $i, 1);
-            --$i;
-        }else{
-            @$baskets[$i]->{total} = BasketTotal(@$baskets[$i]->{basketno}, $bookseller);
-        }
-    }
-    $template->param(baskets => $baskets);
-    $template->param( booksellername => $bookseller->name);
-}
-
 sub printbasketgrouppdf{
     my ($basketgroupid) = @_;
 
@@ -403,9 +367,4 @@ $deliveryplace = $deliveryplace || $borrower->branchcode;
 
 $template->param( booksellerid => $booksellerid );
 
-# the template will display a unique basketgroup
-my $basketgroups = &GetBasketgroups($booksellerid);
-my $baskets = &GetBasketsByBookseller($booksellerid);
-displaybasketgroups($basketgroups, $bookseller, $baskets, $template);
-
 output_html_with_http_headers $input, $cookie, $template->output;
diff --git a/installer/data/mysql/atomicupdate/bug_11708.perl b/installer/data/mysql/atomicupdate/bug_11708.perl
index 35543cdac5..54d7e72f8b 100644
--- a/installer/data/mysql/atomicupdate/bug_11708.perl
+++ b/installer/data/mysql/atomicupdate/bug_11708.perl
@@ -8,9 +8,16 @@ if( CheckVersion( $DBversion ) ) {
     }
 
     if (column_exists('aqbasketgroups', 'closed')) {
+        # Set basketgroup's close date to the latest close date of its baskets,
+        # or NOW() if it has no baskets
         $dbh->do(q{
             UPDATE aqbasketgroups
-            SET closeddate = CAST(NOW() AS DATE)
+                LEFT JOIN (
+                    SELECT basketgroupid, MAX(closedate) AS closedate
+                    FROM aqbasket
+                    GROUP BY basketgroupid
+                ) AS aqbasket ON (aqbasketgroups.id = aqbasket.basketgroupid)
+            SET aqbasketgroups.closeddate = COALESCE(aqbasket.closedate, CAST(NOW() AS DATE))
             WHERE closed = 1
               AND closeddate IS NULL
         });
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
index b870d835e8..ca431e283a 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt
@@ -807,7 +807,7 @@
             $(document).ready(function(){
                 $("#basketgroupid").change(function(){
                     if($(this).val() == "new"){
-                        location.href="/cgi-bin/koha/acqui/basketgroup.pl?op=add&booksellerid=[% booksellerid | html %]";
+                        location.href="/cgi-bin/koha/acqui/basketgroups.pl?op=add&booksellerid=[% booksellerid | html %]";
                     }
                 });
             });
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
index 5ecb65dea9..aa1a0c46b3 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
@@ -4,11 +4,7 @@
 [% USE Price %]
 [% USE KohaDates %]
 [% INCLUDE 'doc-head-open.inc' %]
-[% IF booksellerid %]
-  <title>Koha &rsaquo; Basket groups for [% booksellername |html %]</title>
-[% ELSE %]
-  <title>Koha &rsaquo; Basket groups</title>
-[% END %]
+<title>Koha &rsaquo; Basket groups for [% booksellername |html %]</title>
 [% Asset.css("css/datatables.css") | $raw %]
 [% INCLUDE 'doc-head-close.inc' %]
 [% INCLUDE 'datatables.inc' %]
@@ -120,7 +116,7 @@ function submitForm(form) {
 <div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> &rsaquo; <a href="/cgi-bin/koha/acqui/acqui-home.pl">Acquisitions</a> &rsaquo;
     <a href="/cgi-bin/koha/acqui/supplier.pl?booksellerid=[% booksellerid | html %]">[% booksellername | html %]</a>
     &rsaquo;
-    <a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% booksellerid | html %]">Basket groups</a>
+    <a href="/cgi-bin/koha/acqui/basketgroups.pl?booksellerid=[% booksellerid | html %]">Basket groups</a>
     &rsaquo;
     Add basket group for [% booksellername | html %]
 </div>
@@ -274,7 +270,7 @@ function submitForm(form) {
                                             <input type="hidden" name="basketgroupid" value="[% basketgroupid | html %]" />
                                         [% END %]
                                         <input type="hidden" name="op" value="attachbasket" />
-                                        <input type="submit" value="Save" /> <a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% booksellerid | html %]" class="cancel">Cancel</a>
+                                        <input type="submit" value="Save" /> <a href="/cgi-bin/koha/acqui/basketgroups.pl?booksellerid=[% booksellerid | html %]" class="cancel">Cancel</a>
                                     </fieldset>
                                 [% END %]
                             </form>
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt
index e9258a9f23..67e7cb0b67 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt
@@ -2,61 +2,15 @@
 [% USE Branches %]
 [% USE KohaDates %]
 
+[% SET footerjs = 1 %]
 [% INCLUDE 'doc-head-open.inc' %]
     [% IF bookseller %]
         <title>Koha &rsaquo; Basket groups for [% bookseller.name |html %]</title>
     [% ELSE %]
         <title>Koha &rsaquo; Basket groups</title>
     [% END %]
-
-    [% Asset.css('css/datatables.css') %]
-    [% INCLUDE 'doc-head-close.inc' %]
-    [% INCLUDE 'datatables.inc' %]
-    [% Asset.js('lib/jquery/plugins/jquery.dataTables.columnFilter.js') %]
-    <script type="text/javascript">
-        $(document).ready(function() {
-            var options = {
-                "paging": false,
-                "autoWidth": false,
-                "columnDefs": [
-                    { "visible": false, "targets": 1 },
-                    { "orderable": false, "targets": -1 }
-                ],
-                "orderFixed": [[ 1, 'asc' ]]
-            };
-            [% UNLESS bookseller %]
-                options.drawCallback = function(settings) {
-                    var api = this.api();
-                    var rows = api.rows({page: 'current'}).nodes();
-                    var last = null;
-
-                    api.column(1, {page: 'current'}).data().each(function(group, i) {
-                        if (last !== group) {
-                            $(rows).eq(i).before(
-                                '<tr><td class="group" colspan="8">' + group + '</td></tr>'
-                            );
-                            last = group;
-                        }
-                    });
-                };
-            [% END %]
-            $("#basketgroups-table").kohaDataTable(options);
-
-            $('#basketgroups-table').on('click', '.closeandprint', function(e) {
-                e.preventDefault();
-                var w = window.open($(this).attr('href'));
-                var timer = setInterval(function() {
-                    if (w.closed === true) {
-                        clearInterval(timer);
-                        window.location.reload(true);
-                    }
-                }, 1000);
-            });
-            $('#basketgroups-table').on('click', '.delete', function() {
-                return confirm(_("Are you sure you want to delete this basketgroup ?"));
-            });
-        });
-    </script>
+[% INCLUDE 'doc-head-close.inc' %]
+[% Asset.css('css/datatables.css') %]
 </head>
 <body id="acq_basketgroup" class="acq">
     [% INCLUDE 'header.inc' %]
@@ -113,7 +67,7 @@
                                                 Basket group no. [% basketgroup.id %]
                                             [% END %]
                                         </td>
-                                        <td>[% basketgroup.bookseller.name %]</td>
+                                        <td>[% basketgroup.vendor.name %]</td>
                                         <td>[% Branches.GetName(basketgroup.billingplace) %]</td>
                                         <td>
                                             [% IF (basketgroup.freedeliveryplace) %]
@@ -160,4 +114,54 @@
                 [% INCLUDE 'acquisitions-menu.inc' %]
             </div>
         </div>
+
+        [% MACRO jsinclude BLOCK %]
+            [% INCLUDE 'datatables.inc' %]
+            [% Asset.js('lib/jquery/plugins/jquery.dataTables.columnFilter.js') %]
+            <script type="text/javascript">
+                $(document).ready(function() {
+                    var options = {
+                        "paging": false,
+                        "autoWidth": false,
+                        "columnDefs": [
+                            { "visible": false, "targets": 1 },
+                            { "orderable": false, "targets": -1 }
+                        ],
+                        "orderFixed": [[ 1, 'asc' ]]
+                    };
+                    [% UNLESS bookseller %]
+                        options.drawCallback = function(settings) {
+                            var api = this.api();
+                            var rows = api.rows({page: 'current'}).nodes();
+                            var last = null;
+
+                            api.column(1, {page: 'current'}).data().each(function(group, i) {
+                                if (last !== group) {
+                                    $(rows).eq(i).before(
+                                        '<tr><td class="group" colspan="8">' + group + '</td></tr>'
+                                    );
+                                    last = group;
+                                }
+                            });
+                        };
+                    [% END %]
+                    $("#basketgroups-table").kohaDataTable(options);
+
+                    $('#basketgroups-table').on('click', '.closeandprint', function(e) {
+                        e.preventDefault();
+                        var w = window.open($(this).attr('href'));
+                        var timer = setInterval(function() {
+                            if (w.closed === true) {
+                                clearInterval(timer);
+                                window.location.reload(true);
+                            }
+                        }, 1000);
+                    });
+                    $('#basketgroups-table').on('click', '.delete', function() {
+                        return confirm(_("Are you sure you want to delete this basketgroup ?"));
+                    });
+                });
+            </script>
+        [% END %]
+
     [% INCLUDE 'intranet-bottom.inc' %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
index 3eff906ddf..244813d389 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt
@@ -133,7 +133,7 @@
                 <td class="basketfilterclass">[% loop_order.basketname | html %] (<a href="/cgi-bin/koha/acqui/basket.pl?basketno=[% loop_order.basketno | html %]">[% loop_order.basketno | html %]</a>)</td>
                 <td>
                   [% IF loop_order.basketgroupid %]
-                    [% loop_order.basketgroupname | html %] (<a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% loop_order.booksellerid | html %]">[% loop_order.basketgroupid | html %]</a>)
+                    [% loop_order.basketgroupname | html %] (<a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% loop_order.booksellerid | html %]&basketgroupid=[% loop_order.basketgroupid | html %]">[% loop_order.basketgroupid | html %]</a>)
                   [% ELSE %]
                     No basket group
                   [% END %]
@@ -280,7 +280,7 @@
                 <td>[% order.basketname | html %] (<a href="/cgi-bin/koha/acqui/basket.pl?basketno=[% order.basketno | html %]">[% order.basketno | html %]</a>)</td>
                 <td>
                   [% IF order.basketgroupid %]
-                    [% order.basketgroupname | html %] (<a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% order.booksellerid | html %]">[% order.basketgroupid | html %]</a>)
+                    [% order.basketgroupname | html %] (<a href="/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=[% order.booksellerid | html %]&basketgroupid=[% order.basketgroupid | html %]">[% order.basketgroupid | html %]</a>)
                   [% ELSE %]
                     No basket group
                   [% END %]
-- 
2.17.1