Bugzilla – Attachment 111247 Details for
Bug 23376
Cleanup order receive page code
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 23376: Clean up order recipt page
Bug-23376-Clean-up-order-recipt-page.patch (text/plain), 23.12 KB, created by
Martin Renvoize (ashimema)
on 2020-10-05 15:48:39 UTC
(
hide
)
Description:
Bug 23376: Clean up order recipt page
Filename:
MIME Type:
Creator:
Martin Renvoize (ashimema)
Created:
2020-10-05 15:48:39 UTC
Size:
23.12 KB
patch
obsolete
>From a5c561c1e5ba00f3862374421e034030ea09720f Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >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 <agustinmoyano@theke.io> >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> >--- > 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' %] > >-<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/acqui/acqui-home.pl">Acquisitions</a> › <a href="/cgi-bin/koha/acqui/supplier.pl?booksellerid=[% booksellerid | html %]">[% name | html %]</a> › Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]][% END %] (order #[% ordernumber | html %])</div> >+<div id="breadcrumbs"><a href="/cgi-bin/koha/mainpage.pl">Home</a> › <a href="/cgi-bin/koha/acqui/acqui-home.pl">Acquisitions</a> › <a href="/cgi-bin/koha/acqui/supplier.pl?booksellerid=[% order.basket.booksellerid | html %]">[% name | html %]</a> › Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]][% END %] (order #[% order.ordernumber | html %])</div> > > <div class="main container-fluid"> > <div class="row"> > <div class="col-sm-10 col-sm-push-2"> > <main> > >-<h1>Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]] [% END %] (order #[% ordernumber | html %])</h1> >+<h1>Receive items from : [% name | html %] [% IF ( invoice ) %][[% invoice | html %]] [% END %] (order #[% order.ordernumber | html %])</h1> > > [% IF ( count ) %] > <form action="/cgi-bin/koha/acqui/finishreceive.pl" class="noEnterSubmit" method="post" onsubmit="return Check(this);"> >@@ -31,15 +32,15 @@ > > <fieldset class="rows"> > <legend>Catalog details</legend> >- <ol><li><span class="label">Title: </span><span class="title">[% title | html %]</span></li> >+ <ol><li><span class="label">Title: </span><span class="title">[% order.biblio.title | html %]</span></li> > <li> <span class="label">Author: </span> >- [% author | html %]</li> >+ [% order.biblio.author | html %]</li> > <li><span class="label">Copyright: </span> >- [% copyrightdate | html %]</li> >+ [% order.biblio.copyrightdate | html %]</li> > <li> <span class="label">ISBN: </span> >- [% isbn | html %]</li> >+ [% order.biblio.biblioitem.isbn | html %]</li> > <li> <span class="label">Series: </span> >- [% seriestitle | html %]</li> >+ [% order.biblio.seriestitle | html %]</li> > </ol> > </fieldset> > >@@ -85,7 +86,7 @@ > </fieldset> > [% END %] > >- [% IF subscriptionid and orders.count %] >+ [% IF order.subscriptionid and orders.count %] > <fieldset class="rows"> > <legend>Receipt history for this subscription</legend> > <table id="orders"> >@@ -169,7 +170,7 @@ > </div> > </div> > >- [% UNLESS subscriptionid %] >+ [% UNLESS order.subscriptionid %] > <fieldset class="rows" id="itemfieldset"> > <legend>Item</legend> > [% IF ( NoACQframework ) %] >@@ -183,7 +184,7 @@ > </fieldset> > [% END %] > [% ELSIF (AcqCreateItem == 'ordering') %] >- [% IF (items.size) %] >+ [% IF (items.count) %] > <h5>Items</h5> > <div style="width:100%;overflow:auto"> > <table> >@@ -214,15 +215,15 @@ > <td>[% item.barcode | html %]</td> > <td>[% Branches.GetName( item.homebranch ) | html %]</td> > <td>[% Branches.GetName( item.holdingbranch ) | html %]</td> >- <td>[% item.notforloan | html %]</td> >- <td>[% item.restricted | html %]</td> >- <td><span class="shelvingloc">[% item.location | html %]</span></td> >+ <td>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.notforloan', authorised_value => item.notforloan ) | html %]</td> >+ <td>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.restricted', authorised_value => item.restricted ) | html %]</td> >+ <td><span class="shelvingloc">[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.location', authorised_value => item.location ) | html %]</span></td> > <td>[% item.itemcallnumber | html %]</td> > <td>[% item.copynumber | html %]</td> > <td>[% item.stocknumber | html %]</td> >- <td>[% item.collection | html %]</td> >- <td>[% item.itemtype | html %]</td> >- <td>[% item.materials | html %]</td> >+ <td>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.ccode', authorised_value => item.ccode ) | html %]</td> >+ <td>[% ItemTypes.GetDescription( item.itype ) | html %]</td> >+ <td>[% AuthorisedValues.GetDescriptionByKohaField( kohafield => 'items.materials', authorised_value => item.materials ) | html %]</td> > <td>[% item.itemnotes | html %]</td> > </tr> > [% END %] >@@ -231,10 +232,10 @@ > </div> > [% END %] > [% END %] >- <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" /> >+ <input type="hidden" name="biblionumber" value="[% order.biblionumber | html %]" /> > <input type="hidden" name="invoiceid" value="[% invoiceid | html %]" /> >- <input type="hidden" name="ordernumber" value="[% ordernumber | html %]" /> >- <input type="hidden" name="booksellerid" value="[% booksellerid | html %]" /> >+ <input type="hidden" name="ordernumber" value="[% order.ordernumber | html %]" /> >+ <input type="hidden" name="booksellerid" value="[% order.basket.booksellerid | html %]" /> > </div> > <div class="col-sm-6"> > <fieldset class="rows"> >@@ -245,7 +246,7 @@ > <input type="text" size="10" id="datereceived" name="datereceived" value="[% datereceived | $KohaDates %]" class="datepicker" /> > </li> > <li><label for="bookfund">Fund: </label><select id="bookfund" name="bookfund"> >- <option value="">Keep current ([% budget_period_description | html %] - [% bookfund | html %])</option> >+ <option value="">Keep current ([% budget_period_description | html %] - [% order.fund.budget_name | html %])</option> > [% FOREACH period IN budget_loop %] > <optgroup label="[% period.description | html %]"> > [% FOREACH fund IN period.funds %] >@@ -266,30 +267,30 @@ > </span> > </li> > <li><label for="quantity_to_receive">Quantity ordered: </label><span class="label"> >- [% IF edit or subscriptionid %] >- <input type="text" id="quantity_to_receive" name="quantity" value="[% quantity | html %]" /> >+ [% IF edit or order.subscriptionid %] >+ <input type="text" id="quantity_to_receive" name="quantity" value="[% order.quantity | html %]" /> > [% ELSE%] >- <input type="text" readonly="readonly" id="quantity_to_receive" name="quantity" value="[% quantity | html %]" /> >+ <input type="text" readonly="readonly" id="quantity_to_receive" name="quantity" value="[% order.quantity | html %]" /> > [% END %] > </span></li> > <li><label for="quantity">Quantity received: </label> >- [% IF subscriptionid %] >- <input type="text" size="20" name="quantityrec" id="quantity" value="[% quantity | html %]" /> >- <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% quantityreceived | html %]" /> >+ [% IF order.subscriptionid %] >+ <input type="text" size="20" name="quantityrec" id="quantity" value="[% order.quantity | html %]" /> >+ <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% order.quantityreceived | html %]" /> > [% ELSIF AcqCreateItemReceiving %] > <input readonly="readonly" type="text" size="20" name="quantityrec" id="quantity" value="0" /> > [% ELSE %] >- [% IF ( quantityreceived ) %] >+ [% IF ( order.quantityreceived ) %] > [% IF ( edit ) %] >- <input type="text" size="20" name="quantityrec" id="quantity" value="[% quantityreceived | html %]" /> >- <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% quantityreceived | html %]" /> >+ <input type="text" size="20" name="quantityrec" id="quantity" value="[% order.quantityreceived | html %]" /> >+ <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% order.quantityreceived | html %]" /> > [% ELSE %] > [% IF ( items ) %] >- <input readonly="readonly" type="text" size="20" name="quantityrec" id="quantity" value="[% quantityreceivedplus1 | html %]" /> >+ <input readonly="readonly" type="text" size="20" name="quantityrec" id="quantity" value="[% order.quantityreceived + 1 | html %]" /> > [% ELSE %] >- <input type="text" size="20" name="quantityrec" id="quantity" value="[% quantityreceivedplus1 | html %]" /> >+ <input type="text" size="20" name="quantityrec" id="quantity" value="[% quantityreceived + 1 | html %]" /> > [% END %] >- <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% quantityreceived | html %]" /> >+ <input id="origquantityrec" readonly="readonly" type="hidden" name="origquantityrec" value="[% order.quantityreceived | html %]" /> > [% END %] > [% ELSE %] > <input type="text" id="quantity" size="20" name="quantityrec" value="1" /> >@@ -299,13 +300,14 @@ > <p class="error">Warning, you have entered more items than expected. > Items will not be created.</p> > </div> >- [% END %][%# IF (subscriptionid) ELSIF (AcqCreateItemReceiving) %] >+ [% END %][%# IF (order.subscriptionid) ELSIF (AcqCreateItemReceiving) %] > </li> > > [% IF ( gst_values ) %] > <li> > <label for="tax_rate">Tax rate: </label> > <select name="tax_rate" id="tax_rate"> >+ [% tax_rate = order.tax_rate_on_receiving || order.tax_rate_on_ordering %] > [% FOREACH gst IN gst_values %] > [% IF gst.option == tax_rate %] > <option value="[% gst.option | html %]" selected="selected">[% gst.option * 100 | html %]%</option> >@@ -319,16 +321,30 @@ > <input type="hidden" name="tax_rate" value="0" /> > [% END %] > >- <li><label for="rrp">Retail price: </label>[% rrp | $Price %] <span class="hint">(adjusted for [% active_currency.currency | html %], [% IF (invoiceincgst == 1) %]tax inclusive[% ELSE %]tax exclusive[% END %])</span></li> >+ <li><label for="rrp">Retail price: </label> >+ [% IF (invoiceincgst == 1) %] >+ [% order.rrp_tax_included | $Price %]<span class="hint">(adjusted for [% active_currency.currency | html %],tax inclusive)</span></li> >+ [% ELSE %] >+ [% order.rrp_tax_excluded | $Price %]<span class="hint">(adjusted for [% active_currency.currency | html %],tax exclusive)</span></li> >+ [% END %] > <li> > <label for="replacementprice">Replacement price:</label> >- <input type="text" size="20" name="replacementprice" id="replacementprice" value="[% replacementprice | $Price on_editing => 1 %]" /> >+ <input type="text" size="20" name="replacementprice" id="replacementprice" value="[% order.replacementprice | $Price on_editing => 1 %]" /> > </li> >- <li><label for="ecost">Budgeted cost: </label>[% ecost | $Price %] <span class="hint">[% IF (invoiceincgst == 1) %](tax inclusive)[% ELSE %](tax exclusive)[% END %]</span></li> >+ <li> >+ [% IF (invoiceincgst) %] >+ <label for="ecost">Budgeted cost: </label>[% order.ecost_tax_included | $Price %] <span class="hint">(tax inclusive)</span> >+ [% ELSE %] >+ <label for="ecost">Budgeted cost: </label>[% order.ecost_tax_excluded | $Price %] <span class="hint">(tax exclusive)</span> >+ [% END %] >+ </li> > <li> > <label for="unitprice">Actual cost:</label> >- <input type="text" size="20" name="unitprice" id="unitprice" value="[% unitprice | $Price on_editing => 1 %]" /> >- <span class="hint">[% IF (invoiceincgst == 1) %](tax inclusive)[% ELSE %](tax exclusive)[% END %]</span> >+ [% IF (invoiceincgst) %] >+ <input type="text" size="20" name="unitprice" id="unitprice" value="[% order.unitprice_tax_included | $Price on_editing => 1 %]" /> <span class="hint">(tax inclusive)</span> >+ [% ELSE %] >+ <input type="text" size="20" name="unitprice" id="unitprice" value="[% order.unitprice_tax_excluded | $Price on_editing => 1 %]" /> <span class="hint">(tax exclusive)</span> >+ [% END %] > <label style="font-weight: inherit; float:none;"><input type="checkbox" name="change_currency">Change currency</label> > </li> > <li id="select_currency"> >@@ -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.20.1
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=111247">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 23376
:
91774
|
91857
|
91858
|
110692
|
110693
|
110694
| 111247 |
111248
|
111249
|
111250
|
111378
|
111750
|
131857