Bug 23523

Summary: Unitprice tax column values are not populated if entered upon ordering
Product: Koha Reporter: Nick Clemens <nick>
Component: AcquisitionsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: normal    
Priority: P5 - low CC: david, katrin.fischer, martin.renvoize
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 23522    
Attachments: Bug 23523: unitprice tax column values are not populated if entered upon ordering
Bug 23523: unitprice tax column values are not populated if entered upon ordering
Bug 23523: unitprice tax column values are not populated if entered upon ordering
Bug 23523: (QA follow-up) Minor POD corrections
Bug 23523: (RM follow-up) Failing tests
Bug 23523: (follow-up) Adjust tests for new expectations
Bug 23523: (follow-up) Clarify meaning configurations in tests

Description Nick Clemens 2019-08-29 18:42:49 UTC
To recreate:
 1 - Add an item to an acquisitions basket
 2 - Make sure to enter 'Actual cost'
 3 - Check the db:
    SELECT * FROM aqorders WHERE ordernumber={your ordernumber}
 4 - Note that unitprice_tax_included and unitprice_tax_excluded are not populated
Comment 1 Nick Clemens 2019-08-29 18:43:45 UTC
Created attachment 92500 [details] [review]
Bug 23523: unitprice tax column values are not populated if entered upon ordering

To test:
 1 - Add an item to an acquisitions basket
 2 - Make sure to enter 'Actual cost'
 3 - Check the db:
    SELECT * FROM aqorders WHERE ordernumber={your ordernumber}
 4 - Note that unitprice_tax_included and unitprice_tax_excluded are not populated
 5 - Apply patch
 6 - Edit that order
 7 - Check DB
 8 - Values should be populated
 9 - Place another order, ensude values populated on creation
10 - QA people: prove -v t/db_dependent/Acquisition/populate_order_with_prices.t
Comment 2 David Nind 2019-10-07 09:51:54 UTC
Works for me for new items added after the patch was applied.

However, I couldn't seem to modify existing items that are part of the basket - they reverted to original price.
Comment 3 Nick Clemens 2019-10-09 17:46:37 UTC
(In reply to David Nind from comment #2)
> Works for me for new items added after the patch was applied.
> 
> However, I couldn't seem to modify existing items that are part of the
> basket - they reverted to original price.

Hi David,

I tested again, it seems to work for both new and old orders for. Note that you won't see any change in the basket with bug 23522 - this only affects the DB.

I broke them into two as I thought it made things easier to read and test
Comment 4 Claire Gravely 2019-10-10 13:15:18 UTC
Created attachment 93950 [details] [review]
Bug 23523: unitprice tax column values are not populated if entered upon ordering

To test:
 1 - Add an item to an acquisitions basket
 2 - Make sure to enter 'Actual cost'
 3 - Check the db:
    SELECT * FROM aqorders WHERE ordernumber={your ordernumber}
 4 - Note that unitprice_tax_included and unitprice_tax_excluded are not populated
 5 - Apply patch
 6 - Edit that order
 7 - Check DB
 8 - Values should be populated
 9 - Place another order, ensude values populated on creation
10 - QA people: prove -v t/db_dependent/Acquisition/populate_order_with_prices.t

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 5 Katrin Fischer 2019-10-14 20:46:20 UTC
I think usually the 'actual price' is entered on receive - can you explain the use case for entering it on order?
Comment 6 Katrin Fischer 2019-10-14 20:55:15 UTC
I am not sure about these fallbacks in the code - can you please explain?

+            # we should use the unitprice if included
+            my $cost_tax_included = $order->{unitprice_tax_included} || $order->{ecost_tax_included};
+            my $cost_tax_excluded = $order->{unitprice_tax_excluded} || $order->{ecost_tax_excluded};
+            $order->{tax_value_on_ordering} = ( get_rounded_price($cost_tax_included) - get_rounded_price($cost_tax_excluded) ) * $order->{quantity};
Comment 7 Nick Clemens 2019-10-15 13:44:48 UTC
(In reply to Katrin Fischer from comment #5)
> I think usually the 'actual price' is entered on receive - can you explain
> the use case for entering it on order?

For standing orders, or orders that are completed in one step (i.e. purchasing a database) sometimes the library knows the actual price before 'receiving'

Outside of these workflows, nothing changes, this only allows libraries who do enter actual price on ordering to have values calculated correctly
Comment 8 Nick Clemens 2019-10-15 13:46:25 UTC
(In reply to Katrin Fischer from comment #6)
> I am not sure about these fallbacks in the code - can you please explain?
> 
> +            # we should use the unitprice if included
> +            my $cost_tax_included = $order->{unitprice_tax_included} ||
> $order->{ecost_tax_included};
> +            my $cost_tax_excluded = $order->{unitprice_tax_excluded} ||
> $order->{ecost_tax_excluded};
> +            $order->{tax_value_on_ordering} = (
> get_rounded_price($cost_tax_included) -
> get_rounded_price($cost_tax_excluded) ) * $order->{quantity};

This only affects the 'tax_value_on_ordering' field
  -if you have entered an actual cost, then those values are used to determine the tax
  -if you have not, we use the ecost as we did previously (fallback to original behvaiour)
Comment 9 Martin Renvoize 2019-11-04 19:45:35 UTC
(In reply to Katrin Fischer from comment #5)
> I think usually the 'actual price' is entered on receive - can you explain
> the use case for entering it on order?

The ability to enter this field on order was removed in bug 9775 and then put back again in bug 23294.. I'm still not sure I agree with it having been put back as editable, but Nicks comment regarding 'Databases where they know the price before ordering' sort of makes sense.. 

So... one 'can' enter it... as such we should record it and use it in calculations in my opinion. Having said that, I'm sure this will break someone's workflow to fix someone elses much like bug 9775 did in the first place.. somewhat torn.
Comment 10 Martin Renvoize 2019-11-04 20:14:05 UTC
Created attachment 95034 [details] [review]
Bug 23523: unitprice tax column values are not populated if entered upon ordering

To test:
 1 - Add an item to an acquisitions basket
 2 - Make sure to enter 'Actual cost'
 3 - Check the db:
    SELECT * FROM aqorders WHERE ordernumber={your ordernumber}
 4 - Note that unitprice_tax_included and unitprice_tax_excluded are not populated
 5 - Apply patch
 6 - Edit that order
 7 - Check DB
 8 - Values should be populated
 9 - Place another order, ensude values populated on creation
10 - QA people: prove -v t/db_dependent/Acquisition/populate_order_with_prices.t

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-11-04 20:14:08 UTC
Created attachment 95035 [details] [review]
Bug 23523: (QA follow-up) Minor POD corrections

This just fixes a couple of typos in the POD that predate this patch but
I noticed whilst QAing this ;)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-11-04 20:15:35 UTC
OK, Having spoken to Nick and worked through the various use cases, workflows and scenarios I agree with this patchset.

I'd really like to see more workflows documented so we lower the likelihood of going around in circles again, but that should hold up this bug.

Passing QA
Comment 13 Martin Renvoize 2019-11-04 20:21:09 UTC
Workflows to document.

1) Standing orders (unitprice is passed through from previous 'receipts' - restored functionality in bug 23294)
2) RRP 0, One can 'trick' the encumberment system by setting unitprice and rrp at the same time, setting rrp to 0 ensures ecost is also set to 0 and as such the cost of the item is not included in the encumberment totals (which is a function some libraries aparently want/require)

In short:
* There are workflows where you only want rrp (ecost) entered at ordering and unitprice entered at receipt.
* There are workflows where if you know unitprice at ordering you should be able to enter it, but ecost should be set to match it.
* There are workflows where you want to deliberately enter different values for the two to get the result out of the system you want.

As such, I'm not sure we can easily clarify any one of those workflows at this point as all need to be available.
Comment 14 Martin Renvoize 2019-11-05 08:14:30 UTC
Nice work!

Pushed to master for 19.11.00
Comment 15 Martin Renvoize 2019-11-06 08:55:23 UTC
Created attachment 95082 [details] [review]
Bug 23523: (RM follow-up) Failing tests
Comment 16 Martin Renvoize 2019-11-06 09:01:05 UTC
My followup gets the tests passing but I'm pretty confident it's not right :(.

The deeper down this rabbit hole I dive the less confidence in this original patchset I have I'm afraid.

So, digging deeply it seems that unitprice is pretty much always passed into populate_order_with_prices (regardless of whether you have entered such a price into the UI at order time).  If you have not entered a such a price then it will fallback to using 'ecost' which is calculated clientside by JS and added to a readonly form field.

As such, the t/Prices.t (which before my patch always submitted ecost = unitprice), should continue to pass but it does not.. this to my highlights an issue somewhere, but I'm stuck regarding finding it.
Comment 17 Nick Clemens 2019-11-06 12:12:58 UTC
Created attachment 95102 [details] [review]
Bug 23523: (follow-up) Adjust tests for new expectations
Comment 18 Nick Clemens 2019-11-06 12:13:01 UTC
Created attachment 95103 [details] [review]
Bug 23523: (follow-up) Clarify meaning configurations in tests
Comment 19 Martin Renvoize 2019-11-06 12:30:51 UTC
Thanks Nick, this looks good now.. followups pushed.