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
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
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.
(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
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>
I think usually the 'actual price' is entered on receive - can you explain the use case for entering it on order?
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};
(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
(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)
(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.
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>
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>
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
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.
Nice work! Pushed to master for 19.11.00
Created attachment 95082 [details] [review] Bug 23523: (RM follow-up) Failing tests
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.
Created attachment 95102 [details] [review] Bug 23523: (follow-up) Adjust tests for new expectations
Created attachment 95103 [details] [review] Bug 23523: (follow-up) Clarify meaning configurations in tests
Thanks Nick, this looks good now.. followups pushed.