Patch applied cleanly, go forth and signoff

Hi
I did not test yet, but I noticed some months ago that invoices seem to use ecost, while they should use the real unit cost (MT11242).
Could you say me if this bug is also changing this behavior?

M. Saby

(In reply to mathieu saby from comment #3)
> Hi
> I did not test yet, but I noticed some months ago that invoices seem to use
> ecost, while they should use the real unit cost (MT11242).
> Could you say me if this bug is also changing this behavior?

I don't remember.

Well, I will test and describe what I see.

Mathieu

I made 2 tests. 
I think some points need to be fixed, or maybe I am not well understanding the price management in Koha


first test with "yes" (list price) and "yes" (invoiced price) for the 2 prefs of the vendor.

10 euros
discount 10%
tax rate : 5,5%

on receipt page:
- ecost : 9 euros
- actual price : 9 euros

on invoice, before clicking on "finish receiving" :
- ecost : 9 euros
- actual price : 9 euros
- total tax inc : 9 euros
=> OK

on invoice, after clicking on "finish receiving" :
- est. : 9 euros
- total tax inc : 9.48 euros
=> KO

================
2d test with "yes" and "no" for the 2 prefs of the vendor.

10 euros
discount 10%
tax rate : 5,5%

on receipt page:
- ecost : 9,48 euros
- actual price : 8.53 euros
=> KO. Ecost should be 9 euros. Probably not linked with your patch.

on invoice, before clicking on "finish receiving" :
- ecost : 8.53 euros
- actual price : 8.53 euros
- Total tax inc. : 9.00	euros
=> KO. Why adding the tax on the "Total tax inc."? We don't pay tax for this vendor.

on invoice, after clicking on "finish receiving" :
- est. : 9 euros
- total tax inc : 9.48 euros
=> KO

=> Failed QA : 
* for "yes" and "no" (2d test) : the "total tax inc." amount should be 8.53, because we don't add tax for this vendor (if I understand well price management)

* after the reception is finished, the table for "invoice details"
- does not give me the actual price
- still add tax even when it is not needed



I note an other bug, probably in master : 
with "yes" / "no" preference (list price includes tax and invoiced does not include tax), the Replacement price on receipt page is bad (9.48 euros, while it should be 9 euros)

Mathieu

AHHH
Sorry
In fact your patch did not apply well on my VM!
So I tested the current master ;-)

Mathieu

(my previous test was made on master, and I made a confusion between receipt page and invoiced page, so forget it)

I tested with your patch
for all test, a basket with 1 book
- list price (the one I entered) : 10 eur
- tax : vat = 5,5%
- discount = 10%

I have made a complete acq workflow and found some weird things. Most of all are are not related with your patch. Only the 4th step of each test (invoice page) is directly related with it.
I made 4 full tests for all the possible vendor setting. 

My conclusion is : the more I test, the less I understand price management in Koha, and the documentation about it is too poor, so I cannot be sure, but I think your patch was not working well for test 1,2,3.
I also think there are some other bugs in baskets and receipt pages on current master.

So all that I can do is Fail QA for the moment.

If I misunderstood price management, please correct me. If there are bugs to be fixed in basket and receipt page, I would like to know it (And I would also DREAM of a page on the wiki or on Koha documentation that explain precisely what Koha is supposed to do with prices and taxes..., or maybe a kind of UT for prices, but I don't know how it could be possible)

Mathieu

test 1 : 
vendor no (list price) / no (invoiced price).  The list price entered by the librarian does NOT include tax, and the library does NOT pay tax

basket :
- RRP tax exc. : 10
- RRP tax inc. : 10.55
- ecost price tax exc. : 9
- ecost price tax inc. : 9.49
=> maybe KO, but out of the scope of this bug : if the library does not pay tax, the total ecost price tax inc. should probably be the same as the total price tax exc. Or the column "ecost tax inc." should be hidden if it means nothing in this configuration.

receipt page :
- replacement : 10
- budgeted : 9
- actual cost : 9
=> maybe KO, but out of the scope (I suppose replacement price should be tax included in remplacement cost)

receipt page (after reception, table "already received") :
- est cost : 9
- actual cost : 9
- total : 9
- Total tax inc. : 9.49
=> KO for "Total tax inc. : 9.49", Should be "9", but out of the scope of this bug

invoice page :
- actual cost tax ecl : 9
- actual cost tax incl : 9.49
=> KO : we don't pay tax for this vendor, so actual cost tax incl should be "9"

=========
test 2:
vendor yes  (list price) / yes  (invoiced price).  The list price entered by the librarian does include tax, and the library does pay tax

basket :
- RRP tax exc. : 9.48
- RRP tax inc. : 10
- total price tax exc. : 8.53
- total price tax inc. : 9
=> OK

receipt page :
- replacement : 10
- budgeted : 9
- actual cost : 9
=> OK

receipt page (after reception, table "already received") :
- est cost : 9
- actual cost : 9
- total : 9
- Total tax inc. : 9
=> OK

invoice page :
- actual cost tax ecl : 9
- actual cost tax incl : 9.49
=> KO : we should have "8.53" and "9"

======
Test 3:
vendor yes  (list price) / no  (invoiced price). The list price entered by the librarian does include tax, and the library does NOT pay tax

basket :
- RRP tax exc. : 9.48
- RRP tax inc. : 10
- total price tax exc. : 8.53
- total price tax inc. : 9
=> maybe KO but out of the scope of the bug : If the library does not pay tax, ecost tax inc. should be the same as tax exc. and the 2 equal 8.53 ? Or the column should be hidden

receipt page :
- replacement : 9.48
- budgeted : 8.53
- actual cost : 8.53
=> maybe KO : replacement price must be tax inc. (I suppose so, if the aim is to know the public price of the book to get it paid back by a reader who had lost it). 

receipt page (after reception, table "already received") :
- est cost : 8.53
- actual cost : 8.53
- total : 8.53
- Total tax inc. : 9
=> KO. Total tax inc. should be "8.53" as we pay no tax

invoice page :
- actual cost tax ecl : 9
- actual cost tax incl : 9.49
=> KO : we should have "8.53" in both column

======
test 4:
vendor no  (list price) / yes  (invoiced price).  The list price entered by the librarian does NOT include tax, and the library does pay tax

basket :
- RRP tax exc. : 10
- RRP tax inc. : 10.55
- total price tax exc. : 9
- total price tax inc. : 9.49
=> OK

receipt page :
- replacement : 10.55
- budgeted : 9.49
- actual cost : 9.49
=> OK

receipt page (after reception, table "already received") :
- est cost : 9.49
- actual cost : 9.49
- total : 9.49
- Total tax inc. (bottom of table) : 9.49
=> OK

invoice page :
- actual cost tax ecl : 9
- actual cost tax incl : 9.49
=> OK

Patch applied cleanly

testing 2 commit(s) (applied to 5fa6168 'Bug 11204: make pendingreserves use d')

 [1;32mOK[0m	acqui/invoice.pl
 [1;32mOK[0m	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/invoice.tt

It is not working well, and most of all, improving the display of rrp and ecost price on invoices is maybe a loss of time.

The prices that we MUST have on invoice page are the prices based on unitcost, aka the real cost of the item, entered at receipt stage.
Other prices (rrp/ecost could be displayed but they are not needed)

What we absolutely need in library is: 
- unitprice HT
- tax on unitprice HT
- unitprice TTC
- quantity
- unitprice TTC * quantity
with clear labels on header of table to show at first glance is a price is with or without tax


Here are my tests :

1st test : 

with no/no parameters, and an order for a  book costing 100 euros in vendor's catalog (so 100 euros without tax), invoice gives me this :

RRP : 100
Est : 90
Total : 90
=> OK if RRP and Est are without taxes
=> KO if RRP and Est are WITH taxes

in the bottom of the invoice table, I read that :

Total tax exc.	100.00	 	1	90.00	 
Tax (5.50%)	5.50	 	 	5.50	 
Total tax inc. (EUR)	105.50	 	1	105.50	 
Total + shipment cost (EUR)	 	 	1	105.50	 

As we don't pay taxes, the taxes should not be displayed here.

2d test :
with yes/yes parameters, and an order for a  book costing 100 euros in vendor's catalog (so 100 euros including tax), invoice gives me this :


Invoice details
RRP	Est.	Qty.	Total
100     90      1       90
We don't know if RRP and Est are with or without tax
=> OK if RRP and Est. are WITH tax
=> KO if RRP and Est. are WITHOUT tax

in the bottom of the invoice table, I read that :

Total tax exc.	94.79	 	1	94.79	 
Tax (5.50%)	5.21	 	 	5.21	 
Total tax inc. (EUR)	100.00	 	1	90.00	 
Total + shipment cost (EUR)	 	 	1	90.00	 

The total tax. exc. and the tax are the values calculated on public price (rrp).
That's not good, Most of time, we don't need them at all. It could be displayed, but only as an indication, and with clear mention that it is based on RRP price.
What we need here is the ecost price without tax.

Mathieu

(In reply to mathieu saby from comment #11)
> It is not working well, and most of all, improving the display of rrp and
> ecost price on invoices is maybe a loss of time.
> 

Mathieu, are you sure your last test was with the patches correctly applied?

From what I see, patched version is calculating & displaying unitcost (aka actual cost) on invoice page, just like it should - and there is an clear indication stating if given column is with, or without tax.

We are currently testing those patches; so far with the good results
for listincgst/invoiceincgst both yes/yes && no/no.

Less usual combinations (no/yes, yes/no) might be a different story - I believe invoice.pl may need an additional small change like that:

185c185
>     if ( $bookseller->{'invoiceincgst'} ) {
---
<     if ( $bookseller->{'listincgst'} ) {

to behave properly for "no/yes" and "yes/no" (???).

Anyway, handling prices for yes/no and no/yes combinations are apparently even more problematic in other places (like parcel.pl, basket.pl, .. - price calculations/display/column labeling etc. are not always working correctly in those other scripts - depending on the circumstances). I'm wondering if anyone is actually using such unusual combinations IRL?

Hi
I won't have time to test this patch again. Maybe I did a mistake.
So, if your tests are fine, you can change status to Signed Off.

Mathieu

Back to needs signoff.

(In reply to Jacek Ablewicz from comment #12)

> Less usual combinations (no/yes, yes/no) might be a different story - I
> believe invoice.pl may need an additional small change like that:
> 
> 185c185
> >     if ( $bookseller->{'invoiceincgst'} ) {
> ---
> <     if ( $bookseller->{'listincgst'} ) {
> 
> to behave properly for "no/yes" and "yes/no" (???).

Previously we tried to test it (in 3.14 and master, with above small change included, and without it), still getting some rather weird results for "no/yes" and "yes/no" settings.. Getting bug 11755 into account (which explains at least some of this "weirdness"), question is - how should this bug be retested before sign-off: with 11755 applied first, or maybe not?

(In reply to Jacek Ablewicz from comment #15)
> how should this
> bug be retested before sign-off: with 11755 applied first, or maybe not?

Yes, of course.

(In reply to Jonathan Druart from comment #16)
> (In reply to Jacek Ablewicz from comment #15)
> > how should this
> > bug be retested before sign-off: with 11755 applied first, or maybe not?
> 
> Yes, of course.

I did some Q&D tests with 11755 applied (figuratively speaking - right now, 11755 apparently does not apply for current master? - for testing purposes I just altered C3/Acquisition.pm manually).
So far, so good: 11755 patch seems to be f*ing golden - prices w/ & wo/ tax now being handled properly on the invoice page, for all "yes/yes", "no/no", "yes/no" and "no/yes" listpriceincgst/invoiceincgst setting combinations.

(In reply to Jacek Ablewicz from comment #17)
> I did some Q&D tests with 11755 applied (figuratively speaking - right now,
> 11755 apparently does not apply for current master? - for testing purposes I
> just altered C3/Acquisition.pm manually).

I rebased it.

(In reply to Jacek Ablewicz from comment #17)
> So far, so good: 11755 patch seems to be f*ing golden - prices w/ & wo/ tax
> now being handled properly on the invoice page, for all "yes/yes", "no/no",
> "yes/no" and "no/yes" listpriceincgst/invoiceincgst setting combinations.

Great!
May you sign it off then? :)

My feeling is, that we need to centralize the code doing all the calculations in acq, as the tax calculation has proven to be an endless source of pain and confusion. That said... going to QA this and hoping for an improvement :)

Looking at this a bit more, I think I might have found a small mistake. With both patches applied: 

In acqui/invoice.pl:

103 my $bookseller = GetBookSellerFromId($details->{booksellerid});
151 suppliername     => $details->{'suppliername'},

Jonathan, could you check please? There is no "Need Feedback" status, so setting "Failed QA".

Hm also: 

 FAIL	acqui/invoice.pl
   OK	  pod
   OK	  forbidden patterns
   FAIL	  valid
		Useless use of private variable in void context 
   OK	  critic

(In reply to Katrin Fischer from comment #23)

> 151 suppliername     => $details->{'suppliername'},

IMO this part is fine (GetInvoiceDetails() does return 'suppliername' and it seems to work like intended, at least for me), but this one:

    152 supplierid       => $details->{'supplierid'},

is indeed not; as a result, link via "Vendor: " name is leading to nowhere. Which I should've spotted before signing-off :(.

Hi Jacek, thx for checking :)

(In reply to Katrin Fischer from comment #24)
>  FAIL	acqui/invoice.pl
>    FAIL	  valid
> 		Useless use of private variable in void context 


(In reply to Jacek Ablewicz from comment #25)
>     152 supplierid       => $details->{'supplierid'},
> 
> is indeed not; as a result, link via "Vendor: " name is leading to nowhere.
> Which I should've spotted before signing-off :(.

Good catches!
Fixed.

(In reply to Jonathan Druart from comment #28)

> >    FAIL	  valid
> > 		Useless use of private variable in void context 

IMO

   126: my %row = %{ $order, $line };

was actually a little better ;), being equivalent of:

   my %row = %$line;

and it tested just fine in that form ("Useles use: " warning aside). Looks like merging %$line with %order is 100% redundant (%$line is allways the superset of %$order).

> >     152 supplierid       => $details->{'supplierid'},
> Fixed.

Not fully yet - vendor link still doesn't work; there is no 'supplierid' key in $details->{}, we have 'booksellerid' instead.

(In reply to Jacek Ablewicz from comment #29)
> and it tested just fine in that form ("Useles use: " warning aside). Looks
> like merging %$line with %order is 100% redundant (%$line is allways the
> superset of %$order).

Yes, you are right, sorry for that.

> 
> > >     152 supplierid       => $details->{'supplierid'},
> > Fixed.
> 
> Not fully yet - vendor link still doesn't work; there is no 'supplierid' key
> in $details->{}, we have 'booksellerid' instead.

I amended the first patch, which introduced this mistake.

Thanks Jacek for your vigilance!

The last patch needs a signoff.

Btw, the patches don't have [SIGNED-OFF] added in headers (I signed off the 3rd one without it too, for the sake of consistency). I don't know if this is really important or not - if yes, I'll try to re-sign-off all 3 patches.

Hi Jacek, it's not a problem as long as the status is correct "Signed off" it will show up in our searches :)

Working on this again... it IS confusing, will need some more time for testing.

*** Bug 11681 has been marked as a duplicate of this bug. ***

Ok, this gives me a headache :( Can I get a consolidated test plan?

(In reply to Katrin Fischer from comment #39)
> Ok, this gives me a headache :( Can I get a consolidated test plan?

A test plan could be:
0) Add a value in the gist pref (0.1 or 0.25 or something else easy).
1)
  a) Create a supplier "10613 0 0" with
    List item price includes tax: No
    Invoice item price includes tax: No
  b) Create a supplier "10613 0 1" with
    List item price includes tax: No
    Invoice item price includes tax: Yes
  c) Create a supplier "10613 1 0" with
    List item price includes tax: Yes
    Invoice item price includes tax: No
  d) Create a supplier "10613 1 1" with
    List item price includes tax: Yes
    Invoice item price includes tax: Yes
2) Create a basket for each supplier
3) Create 1+ order(s) with 1+ item(s) for each basket with a discount and a gst value.
4) Close the baskets
5) Receive the items
6) Go on acqui/invoice.pl?invoiceid=XX acqui/basket.pl?basketno=YY for each basket/invoice, click on the "Show all details" checkbox and verify that the values are the same.

Hi Jonathan, thx for the test plan, I am back on testing this now.

Created attachment 26924 [details] [review]
[PASSED QA] Bug 10613: The gst rate is not correctly calculated on the invoice page.

Test plan:
Defined a gst rate on creating an order, receive it and check that all
prices are correctly calculated.
/!\ Behavior change function of supplier parameters (Include/Don't
include tax for list prices and invoice prices)

Notes: patch tested with Bug 11755 applied first; confirmed that:
- price calculations are correct for all combinations of
  listincgst/invoiceincgst settings in the vendor record
- unitprice (aka "Actual cost") is taken into account on the
  invoice page instead of rrp/ecost, like it should.

Amended patch: Don't change the supplierid/booksellerid parameter.

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 26925 [details] [review]
[PASSED QA] Bug 10613: FIX typo supplierid vs booksellerid

GetInvoiceDetails returns a hashref with a key named booksellerid, not
supplierid.
The bookseller was not retrieved from the DB and the listincgst value
was always false.

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

Created attachment 26926 [details] [review]
[PASSED QA] Bug 10613: FIX QA issues

This patch fixes the following QA issue:

 FAIL    acqui/invoice.pl
   FAIL      valid
        Useless use of private variable in void context

Signed-off-by: Jacek Ablewicz <abl@biblos.pk.edu.pl>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Patch passes all tests and QA script. Specifically checked
the t/db_depenedent/Acq* tests.

A test plan could be:
0) Add a value in the gist pref (0.1 or 0.25 or something else easy).
1)
  a) Create a supplier "10613 0 0" with
    List item price includes tax: No
    Invoice item price includes tax: No
    Tax: 10%
  b) Create a supplier "10613 0 1" with
    List item price includes tax: No
    Invoice item price includes tax: Yes
    Tax: 10%
  c) Create a supplier "10613 1 0" with
    List item price includes tax: Yes
    Invoice item price includes tax: No
    Tax: 10%
  d) Create a supplier "10613 1 1" with
    List item price includes tax: Yes
    Invoice item price includes tax: Yes
    Tax: 10%
2) Create a basket for each supplier
  a) 00 List price: 10.00 (11.00 with 10% taxes)
  b) 01 List price: 10.00 (11.00 with 10% taxes)
  c) 10 List price: 10.00 (9.09 without taxes)
  d) 11 List price: 10.00 (9.09 without taxes)
  Note: Information on the basket page is shown correctly.
  If you look at the list of ordered items for the fund,
  the list price is used.
3) Create 1+ order(s) with 1+ item(s) for each basket with
   a discount and a gst value.
4) Close the baskets
5) Receive the items
   Left actual price as suggested:
   a) 00 Actual cost: 10.00
   b) 01 Actual cost: 11.00
   c) 10 Actual cost:  9.09
   d) 11 Actual cost: 10.00
   Calculations on the invoice page now all appear to be correct.
   Note: When you take a look at the 'ordered' list for the fund,
   the actual price is used as entered.
6) Go on acqui/invoice.pl?invoiceid=XX acqui/basket.pl?basketno=YY for
   each basket/invoice, click on the "Show all details" checkbox
   and verify that the values are all correct.

Calculations are exactly the same for tax registered yes and no.

Pushed to master.  Thanks, Jonathan!