Bug 22225 - Tax hints and prices on orderreceive.pl may not match
Summary: Tax hints and prices on orderreceive.pl may not match
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords: rel_19_05_candidate
Depends on: 21387 21619
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-29 13:02 UTC by Nick Clemens
Modified: 2019-06-18 15:05 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 22225: Correctly use invoiceincgst over listincgst (2.87 KB, patch)
2019-02-11 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22225: Correctly use invoiceincgst over listincgst (2.93 KB, patch)
2019-04-23 17:58 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22225: Correctly use invoiceincgst over listincgst (2.99 KB, patch)
2019-05-11 16:26 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-01-29 13:02:56 UTC
In the perl script we test whether the vendor invoice prices include tax:
if ( $bookseller->invoiceincgst ) {
    $rrp = $order->{rrp_tax_included};
    $ecost = $order->{ecost_tax_included};
    unless ( $unitprice != 0 and defined $unitprice) {
        $unitprice = $order->{ecost_tax_included};
    }
} else {
    $rrp = $order->{rrp_tax_excluded};
    $ecost = $order->{ecost_tax_excluded};
    unless ( $unitprice != 0 and defined $unitprice) {
        $unitprice = $order->{ecost_tax_excluded};
    }
}


In the template we test whether the vendor list prices include tax:
        <li><label for="rrp">Retail price: </label>[% rrp | $Price %] <span class="hint">(adjusted for [% cur_active | html %], [% IF (listincgst == 1) %]tax inclusive[% ELSE %]tax exclusive[% END %])</span></li>
        <li>
            <label for="replacementprice">Replacement price:</label>
            <input type="text" size="20" name="replacementprice" id="replacementprice" value="[% replacementprice | $Price %]" />
        </li>
        <li><label for="ecost">Budgeted cost: </label>[% ecost | $Price %] <span class="hint">[% IF (listincgst == 1) %](tax inclusive)[% ELSE %](tax exclusive)[% END %]</span></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>
        </li>
Comment 1 Martin Renvoize 2019-02-11 14:05:36 UTC
Created attachment 84971 [details] [review]
Bug 22225: Correctly use invoiceincgst over listincgst

Mistakenly, we were looking at listincgst in the templates but using
invoiceincgst in the scripts. This patch rectifies that by switching to
use invoiceincgst in the templates too.
Comment 2 Pierre-Marc Thibault 2019-02-15 20:55:46 UTC
Can you please provide a test plan so we can easily confirm it works?

Thank you
Comment 3 Martin Renvoize 2019-02-20 09:17:29 UTC
Test Plan,

1) Navigate to a vendor page (acquisitions, search for vendor, click vendor name)
2) Click 'Edit vendor'
3) Check the values of 'List prices - include/don't include tax' and 'Invoice prices - include/don't include tax'
4) Find an invoice from that vendor, check that the tax hints displayed to the right of the prices match the settings above.
5) Change the settings in step 3 and repeat step 4.
Comment 4 Liz Rea 2019-04-08 21:17:31 UTC
Hi,

I tested this and the labels didn't seem to be correct per the vendor settings. This works correctly on current master near as I can tell.

Sorry :(

Liz
Comment 5 Nick Clemens 2019-04-22 13:28:15 UTC
The issue appears during order receipt, not on the invoice view.

To test:
 1 - Set a vendor to:
    List prices - include tax
    Invoive prices - don't include tax
 2 - Create a basket for this vendor and order an item
 3 - Ensure the order has tax applied - the price you enter should include that tax
 4 - Close the basket
 5 - Receive shipment
 6 - Select the order to receive
 7 - On the receipt page note 'Retail price' and 'Budgeted cost' fields claim to be tax inclusive, but display the price adjusted to be tax exclusive
 8 - In another tab edit the vendor to:
    List prices - don't include tax
 9 - Reload receipt page, prices display correctly
10 - Edit vendor in other tab to set
    Invoice prices - include tax
11 - Reload receipt page - now prices include tax but hints claim they exclude tax
Comment 6 Liz Rea 2019-04-23 17:58:12 UTC
Created attachment 88550 [details] [review]
Bug 22225: Correctly use invoiceincgst over listincgst

Mistakenly, we were looking at listincgst in the templates but using
invoiceincgst in the scripts. This patch rectifies that by switching to
use invoiceincgst in the templates too.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 7 Martin Renvoize 2019-04-23 18:58:43 UTC
Ack, thanks both, nick for spotting it and correcting my test plan and Liz for having the patience to test it.. too work, thanks allot.
Comment 8 Katrin Fischer 2019-04-23 19:08:41 UTC
Actually... I am not sure if this is correct. 

If Retail price is entered (in Germany it would even be printed on the book and including tax) that should always be used and not displayed differently on different pages.

How I think it should work:

The setting list price should always be used for list price, budgeted cost etc. that are calculated when entering the order. 

The setting inovice price should be used for the actual price entered on receive.

So the settings are not about the pages, but the kind of prices?
Comment 9 Martin Renvoize 2019-05-07 09:52:27 UTC
I personally feel that this patch should go through as is right now to provoke thought and allow us to clarify and resolve bugs later.

This patch merely corrects the display hints to conform with the actual data that is being displayed.. to me it's a worse case left as is as it's misleading as to what data is being displayed on screen without this patch.

This may provoke further thought and lead to bugfixes.
Comment 10 Katrin Fischer 2019-05-07 11:45:00 UTC
I am ok with that.
Comment 11 Katrin Fischer 2019-05-11 16:26:36 UTC
Created attachment 89614 [details] [review]
Bug 22225: Correctly use invoiceincgst over listincgst

Mistakenly, we were looking at listincgst in the templates but using
invoiceincgst in the scripts. This patch rectifies that by switching to
use invoiceincgst in the templates too.

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Nick Clemens 2019-05-15 12:13:51 UTC
Awesome work all!

Pushed to master for 19.05
Comment 13 Martin Renvoize 2019-05-30 07:16:21 UTC
Pushed to 18.11.x for 18.11.06
Comment 14 Liz Rea 2019-06-18 15:05:12 UTC
Patch does not apply on 18.05, skipping and marking resolved. :)