From 5d4740dfb02bbf4305f66045fc0a01182c717873 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 receipt page code 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 Signed-off-by: Martin Renvoize Signed-off-by: Jonathan Druart --- 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 9dcff30a26..899b595e91 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 %]
  • @@ -464,7 +480,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.39.5
    • [% 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 %]