From cf2153419e76c8e3d67084eef006473a31b10907 Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Thu, 25 Jul 2019 19:52:25 +0000 Subject: [PATCH] Bug 23376: Clean up order recipt page This patchset switches from using DB lookups to using an order object for most things on orderreceive.pl It simplifies the script and makes minimal changes to the template To test: 1 - Place some orders and receive them 2 - Have orders with or without subscriptions attached 3 - Try with different AcqCreateItems settings 4 - Apply patch 5 - No behaviour should change 6 - Read code to ensure things make sense Signed-off-by: Agustin Moyano --- acqui/orderreceive.pl | 106 +++++------------- .../prog/en/modules/acqui/orderreceive.tt | 94 +++++++++------- 2 files changed, 86 insertions(+), 114 deletions(-) diff --git a/acqui/orderreceive.pl b/acqui/orderreceive.pl index 022c5e9ec9..7702b34fcd 100755 --- a/acqui/orderreceive.pl +++ b/acqui/orderreceive.pl @@ -89,30 +89,26 @@ my $freight = $invoice->{shipmentcost}; my $ordernumber = $input->param('ordernumber'); my $bookseller = Koha::Acquisition::Booksellers->find( $booksellerid ); -my $results; -$results = SearchOrders({ - ordernumber => $ordernumber -}) if $ordernumber; +my $order = Koha::Acquisition::Orders->find( $ordernumber ); my ( $template, $loggedinuser, $cookie, $userflags ) = get_template_and_user( { template_name => "acqui/orderreceive.tt", query => $input, type => "intranet", + authnotrequired => 0, flagsrequired => {acquisition => 'order_receive'}, debug => 1, } ); -unless ( $results and @$results) { +unless ( $order ) { output_html_with_http_headers $input, $cookie, $template->output; exit; } # prepare the form for receiving -my $order = $results->[0]; -my $order_object = Koha::Acquisition::Orders->find( $ordernumber ); -my $basket = $order_object->basket; +my $basket = $order->basket; my $currencies = Koha::Acquisition::Currencies->search; my $active_currency = $currencies->get_active; @@ -130,87 +126,61 @@ if ($AcqCreateItem eq 'receiving') { ); } elsif ($AcqCreateItem eq 'ordering') { my $fw = ($acq_fw) ? 'ACQ' : ''; - my $items = $order_object->items; - my @items; - while ( my $i = $items->next ) { - my $item = $i->unblessed; - my $descriptions; - $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $fw, kohafield => 'items.notforloan', authorised_value => $item->{notforloan} }); - $item->{notforloan} = $descriptions->{lib} // ''; - - $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $fw, kohafield => 'items.restricted', authorised_value => $item->{restricted} }); - $item->{restricted} = $descriptions->{lib} // ''; - - $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $fw, kohafield => 'items.location', authorised_value => $item->{location} }); - $item->{location} = $descriptions->{lib} // ''; - - $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $fw, kohafield => 'items.ccode', authorised_value => $item->{ccode} }); - $item->{collection} = $descriptions->{lib} // ''; - - $descriptions = Koha::AuthorisedValues->get_description_by_koha_field({frameworkcode => $fw, kohafield => 'items.materials', authorised_value => $item->{materials} }); - $item->{materials} = $descriptions->{lib} // ''; - - my $itemtype = Koha::ItemTypes->find($i->effective_itemtype); - if (defined $itemtype) { - # We should not do that here, but call ->itemtype->description when needed instead - $item->{itemtype} = $itemtype->description; # FIXME Should not it be translated_description? - } - push @items, $item; - } - $template->param(items => \@items); + my $items = $order->items; + $template->param(items => $items); } -$order->{quantityreceived} = '' if $order->{quantityreceived} == 0; +$order->quantityreceived = '' if $order->quantityreceived == 0; -my $unitprice = $order->{unitprice}; +my $unitprice = $order->unitprice; my ( $rrp, $ecost ); if ( $bookseller->invoiceincgst ) { - $rrp = $order->{rrp_tax_included}; - $ecost = $order->{ecost_tax_included}; + $rrp = $order->rrp_tax_included; + $ecost = $order->ecost_tax_included; unless ( $unitprice != 0 and defined $unitprice) { - $unitprice = $order->{ecost_tax_included}; + $unitprice = $order->ecost_tax_included; } } else { - $rrp = $order->{rrp_tax_excluded}; - $ecost = $order->{ecost_tax_excluded}; + $rrp = $order->rrp_tax_excluded; + $ecost = $order->ecost_tax_excluded; unless ( $unitprice != 0 and defined $unitprice) { - $unitprice = $order->{ecost_tax_excluded}; + $unitprice = $order->ecost_tax_excluded; } } my $tax_rate; -if( defined $order->{tax_rate_on_receiving} ) { - $tax_rate = $order->{tax_rate_on_receiving} + 0.0; +if( defined $order->tax_rate_on_receiving ) { + $tax_rate = $order->tax_rate_on_receiving + 0.0; } else { - $tax_rate = $order->{tax_rate_on_ordering} + 0.0; + $tax_rate = $order->tax_rate_on_ordering + 0.0; } -my $creator = Koha::Patrons->find( $order->{created_by} ); +my $creator = Koha::Patrons->find( $order->created_by ); -my $budget = GetBudget( $order->{budget_id} ); +my $budget = GetBudget( $order->budget_id ); -my $datereceived = $order->{datereceived} ? dt_from_string( $order->{datereceived} ) : dt_from_string; +my $datereceived = $order->datereceived ? dt_from_string( $order->datereceived ) : dt_from_string; # get option values for gist syspref my @gst_values = map { option => $_ + 0.0 }, split( '\|', C4::Context->preference("gist") ); -my $order_internalnote = $order->{order_internalnote}; -my $order_vendornote = $order->{order_vendornote}; -if ( $order->{subscriptionid} ) { +my $order_internalnote = $order->order_internalnote; +my $order_vendornote = $order->order_vendornote; +if ( $order->subscriptionid ) { # Order from a subscription, we will display an history of what has been received my $orders = Koha::Acquisition::Orders->search( { - subscriptionid => $order->{subscriptionid}, - parent_ordernumber => $order->{ordernumber}, - ordernumber => { '!=' => $order->{ordernumber} } + subscriptionid => $order->subscriptionid, + parent_ordernumber => $order->ordernumber, + ordernumber => { '!=' => $order->ordernumber } } ); - if ( $order->{parent_ordernumber} != $order->{ordernumber} ) { - my $parent_order = Koha::Acquisition::Orders->find($order->{parent_ordernumber}); - $order_internalnote = $parent_order->{order_internalnote}; - $order_vendornote = $parent_order->{order_vendornote}; + if ( $order->parent_ordernumber != $order->ordernumber ) { + my $parent_order = Koha::Acquisition::Orders->find($order->parent_ordernumber); + $order_internalnote = $parent_order->order_internalnote; + $order_vendornote = $parent_order->order_vendornote; } $template->param( orders => $orders, @@ -220,29 +190,15 @@ if ( $order->{subscriptionid} ) { $template->param( AcqCreateItem => $AcqCreateItem, count => 1, - biblionumber => $order->{'biblionumber'}, - ordernumber => $order->{'ordernumber'}, - subscriptionid => $order->{subscriptionid}, - booksellerid => $order->{'booksellerid'}, + order => $order, freight => $freight, name => $bookseller->name, active_currency => $active_currency, currencies => scalar $currencies->search({ rate => { '!=' => 1 } }), invoiceincgst => $bookseller->invoiceincgst, - title => $order->{'title'}, - author => $order->{'author'}, - copyrightdate => $order->{'copyrightdate'}, - isbn => $order->{'isbn'}, - seriestitle => $order->{'seriestitle'}, bookfund => $budget->{budget_name}, - quantity => $order->{'quantity'}, - quantityreceivedplus1 => $order->{'quantityreceived'} + 1, - quantityreceived => $order->{'quantityreceived'}, rrp => $rrp, - replacementprice => $order->{'replacementprice'}, ecost => $ecost, - unitprice => $unitprice, - tax_rate => $tax_rate, creator => $creator, invoiceid => $invoice->{invoiceid}, invoice => $invoice->{invoicenumber}, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt index c02a783c19..864a90b5af 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt @@ -3,6 +3,7 @@ [% USE KohaDates %] [% USE Branches %] [% USE AuthorisedValues %] +[% USE ItemTypes %] [% USE Price %] [% SET footerjs = 1 %] [% INCLUDE 'doc-head-open.inc' %] @@ -14,14 +15,14 @@ [% INCLUDE 'header.inc' %] [% INCLUDE 'acquisitions-search.inc' %] - +
-

Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]] [% END %] (order #[% ordernumber | html %])

+

Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]] [% END %] (order #[% order.ordernumber | html %])

[% IF ( count ) %]
@@ -31,15 +32,15 @@
Catalog details -
  1. Title: [% title | html %]
    2. +
    1. Title: [% order.biblio.title | html %]
    2. Author: - [% author | html %]
      3. + [% order.biblio.author | html %]
    3. Copyright: - [% copyrightdate | html %]
      4. + [% order.biblio.copyrightdate | html %]
    4. ISBN: - [% isbn | html %]
      5. + [% order.biblio.biblioitem.isbn | html %]
    5. Series: - [% seriestitle | html %]
      6. + [% order.biblio.seriestitle | html %]
@@ -85,7 +86,7 @@ [% END %] - [% IF subscriptionid and orders.count %] + [% IF order.subscriptionid and orders.count %]
Receipt history for this subscription @@ -169,7 +170,7 @@ - [% UNLESS subscriptionid %] + [% UNLESS order.subscriptionid %]
Item [% IF ( NoACQframework ) %] @@ -183,7 +184,7 @@
[% END %] [% ELSIF (AcqCreateItem == 'ordering') %] - [% IF (items.size) %] + [% IF (items.count) %]
Items
@@ -214,15 +215,15 @@ - - - + + + - - - + + + [% END %] @@ -231,10 +232,10 @@ [% END %] [% END %] - + - - + +
@@ -245,7 +246,7 @@
  • + [% IF edit or order.subscriptionid %] + [% ELSE%] - + [% END %]
  • - [% IF subscriptionid %] - - + [% IF order.subscriptionid %] + + [% ELSIF AcqCreateItemReceiving %] [% ELSE %] - [% IF ( quantityreceived ) %] + [% IF ( order.quantityreceived ) %] [% IF ( edit ) %] - - + + [% ELSE %] [% IF ( items ) %] - + [% ELSE %] - + [% END %] - + [% END %] [% ELSE %] @@ -299,13 +300,14 @@

    Warning, you have entered more items than expected. Items will not be created.

    • - [% END %][%# IF (subscriptionid) ELSIF (AcqCreateItemReceiving) %] + [% END %][%# IF (order.subscriptionid) ELSIF (AcqCreateItemReceiving) %] [% IF ( gst_values ) %]
  • [% END %] -
  • [% rrp | $Price %] (adjusted for [% active_currency.currency | html %], [% IF (invoiceincgst == 1) %]tax inclusive[% ELSE %]tax exclusive[% END %])
    • +
  • + [% IF (invoiceincgst == 1) %] + [% order.rrp_tax_included | $Price %](adjusted for [% active_currency.currency | html %],tax inclusive)
    • + [% ELSE %] + [% order.rrp_tax_excluded | $Price %](adjusted for [% active_currency.currency | html %],tax exclusive) + [% END %]
  • - +
    • -
  • [% ecost | $Price %] [% IF (invoiceincgst == 1) %](tax inclusive)[% ELSE %](tax exclusive)[% END %]
    • +
  • + [% IF (invoiceincgst) %] + [% order.ecost_tax_included | $Price %] (tax inclusive) + [% ELSE %] + [% order.ecost_tax_excluded | $Price %] (tax exclusive) + [% END %] +
  • - - [% IF (invoiceincgst == 1) %](tax inclusive)[% ELSE %](tax exclusive)[% END %] + [% IF (invoiceincgst) %] + (tax inclusive) + [% ELSE %] + (tax exclusive) + [% END %]
  • @@ -465,7 +481,7 @@ $(document).ready(function() { [% IF (AcqCreateItemReceiving) %] cloneItemBlock(0, '[% UniqueItemFields | html %]'); - [% ELSIF (AcqCreateItem == 'ordering') && not subscriptionid %] + [% ELSIF (AcqCreateItem == 'ordering') && not order.subscriptionid %] $("input[name='items_to_receive']").change(function() { CalcQtyToReceive(); }); -- 2.25.0
    • [% item.barcode | html %] [% Branches.GetName( item.homebranch ) | html %] [% Branches.GetName( item.holdingbranch ) | html %][% item.notforloan | html %][% item.restricted | html %][% item.location | html %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.notforloan', authorised_value => item.notforloan ) | html %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.restricted', authorised_value => item.restricted ) | html %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => item.location ) | html %] [% item.itemcallnumber | html %] [% item.copynumber | html %] [% item.stocknumber | html %][% item.collection | html %][% item.itemtype | html %][% item.materials | html %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => item.ccode ) | html %][% ItemTypes.GetDescription( item.itype ) | html %][% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.materials', authorised_value => item.materials ) | html %] [% item.itemnotes | html %]