When all patches blocking bug 12826 will be pushed, all the plumbing part will be done. We will be ready to properly handle the prices calculation. The idea is to create additional DB fields to store value tax included and tax excluded. For instance, for the ecost we will have ecost_tax_included and ecost_tax_excluded. On this way the calculation is done only one, on inserting/updating the order, and not every time we need to display these values. Note that the existing field will be kept, for 2 reasons: 1/ in some place in the code, we just need to retrieve what the user has inserted and 2/ it will be possible to go back if needed. In the long term, they will be removed. To be consistent, all occurrences of 'gste' will be replaced with 'tax_excluded", same for gsti. On the way, gstrate will be replaced with tax_rate, gstvalue with tax_value. Important note: It seems that the prices are generally correctly calculated (on master), the only problem I found is inconsistency in rounding. So we should not have a "migration problem" as I expected.
Created attachment 33789 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed.
Created attachment 33790 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving
Created attachment 33791 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us.
Created attachment 33792 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method.
Created attachment 33793 [details] [review] Bug 13321: [DO NOT PUSH] Tax and prices calculation - DBIC files This patch is for testing purpose only. Correct files will be generated by the RM on push.
These patches will be tested with BibLibre coworker before. But you can help us and test them if you like :)
I try to test theses patchs on biblibre sandbox n° 17 (wich has been frozen with these patchs, among others). I've done several tests with 4 vendors (with discount): 1. configured with List prices don't include tax / Invoice prices don't include tax (0 - 0) 2. configured with List prices don't include tax / Invoice prices include tax (0 - 1) 3. configured with List prices include tax / Invoice prices don't include tax (1 - 0) 4. configured with List prices include tax / Invoice prices include tax (1 - 1) For each vendor, I try receipt without any change, partial receipt, receipt with change of price, receipt with change of GST, receipt after a transfer from another vendor, with price needing some round calculation. In all the case (calculation in the basket, print of the basket group, export CSV of the basket group, receipt and invoice), the fair prices are calculated, consistently with the setting of the tax in the vendor page. Except in one case : the CSV export of a basket group after a transfer of an order from a vendor where list prices don't include tax forward a vendor where List prices include tax. Sonia BOUIS
Created attachment 34432 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion.
(In reply to Koha Team Lyon 3 from comment #7) Thanks for testing Sonia! > For each vendor, I try receipt without any change, partial receipt, receipt > with change of price, receipt with change of GST, receipt after a transfer > from another vendor, with price needing some round calculation. Good to know (almost) everything is fine :) > In all the case (calculation in the basket, print of the basket group, > export CSV of the basket group, receipt and invoice), the fair prices are > calculated, consistently with the setting of the tax in the vendor page. > Except in one case : the CSV export of a basket group after a transfer of an > order from a vendor where list prices don't include tax forward a vendor > where List prices include tax. Good catch, I submitted a new patch (applied on the sandbox). With this patch, both prices (including and excluding tax) is displayed in the csv file.
To test these patches, have a look at http://lists.koha-community.org/pipermail/koha-devel/2014-December/041024.html
Created attachment 35152 [details] rounded issue for conf 1 0 This screenshot highlights the problem raised by François and Marc on bug 13324 comment 3 to 6. On receiving, the rrp and ecost is rounded to '9.52' (for the display), but the exact values are 9.523810 (which is stored in DB). So the unitprice is displayed 9.52 too and will be stored in DB with this value (the rounded value). Should we store the exact unitprice value even if "9.52" is displayed?
+1 for your option : exact value stored in database and rounded value displayed. This way, we are more likely to get accurate values down to the last cents.
+1 for exact value stored in database
Yes, it should be quite easy to do. But I am wondering how to be clear on the order receive screen. The input will contain 9.52, but the inserted value will be 9.52XXX. Side effect: the user won't be able to insert 9.52
Created attachment 35165 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value.
New commit on kohac/ft/bug_13323 and applied on the sandbox. It should fix the rounding problem for unit price on receiving.
The real question is what we are doing with the values less than a cent? Remember also floats are not accurate. Financial apps that value accuracy convert the database value to an integer in cents and do all calculations on that then store it in a monetary form when saving it. That way any rounding is done predictably and it avoids errors arising from the approximation in how floats are stored and the inaccuracies in floating point arithmetic. If calculating a percentage for tax or discount purposes it needs to be calculated on the summed values, adding the individual values where they contain subdivisions of a cent are prone to error
(In reply to Colin Campbell from comment #17) > The real question is what we are doing with the values less than a cent? Hum, what are the different choices? :) > Remember also floats are not accurate. Financial apps that value accuracy > convert the database value to an integer in cents and do all calculations on > that then store it in a monetary form when saving it. That way any rounding > is done predictably and it avoids errors arising from the approximation in > how floats are stored and the inaccuracies in floating point arithmetic. Yes, you are totally right, we should not use float. But I did not want to change to many things. It could be done later. > If calculating a percentage for tax or discount purposes it needs to be > calculated on the summed values, adding the individual values where they > contain subdivisions of a cent are prone to error The estimated cost (discounted) is calculated for 1 item, and the total is * by the quantity. On the contrary, the tax value should be correctly calculated. I did not see any problem with the estimated cost so far. Did you understand the purpose of the last patch (Fix unitprice rounding)? Do you think it makes sense to fix the problem on this way?
On bug 12826 all the 11 "Depends-on" bugs [except the "Overview" Bug 12964 ...] have already been pushed to master/stable. So I've tried to applied the 7 patches of this bug 13321 against master 3.19.00.011 head 13645. The first patch: Applying: Bug 13321: Rename variables fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 13321: Rename variables 1) So I pass the patch to "Patch doesn't apply" status. 2) Otherwise, should the 7 patches of this bug 13321 still be tested on the sandbox 17? If this is the case, I beg your pardon and ask you, please, to pass this bug back to "Needs Signoff" status. 3) Are the problems in comment 18 been resolved by the 7 current patches? Or would this Bug 13321 be passed to an "In discussion" status?
(In reply to Paola Rossi from comment #19) Hi Paola, See comment 10, the patches attached to this bug are only there for reference. I don't update/maintain them. > 3) Are the problems in comment 18 been resolved by the 7 current patches? > Or would this Bug 13321 be passed to an "In discussion" status? I did not find any issues when testing, so yes, it is the way it's done.
I tested on BibLibre's sandbox 17 all the configurations from the RFC (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC) and compared every displayed numbers. All calculations are good except for one. It's a very small glitch on the invoice page. The tax valule is not correct. It doesn't take the quantity into consideration. To reproduce the problem : 1. Create a vendor (ordre price includes tax, invoice price includes tax) 2. Add an order : - 2 items - vendor price : 10 euros - tax rate : 5% tax 3. Close the basket and receive the 2 ordered items 4. Check the "Already received" table : the tax is 0,95 euros 5. Click finish receiving (invoice.pl) 6. Check the "Invoice details" table : the tax is 0.48 euros (the value for 1 item). It should be multiply by the quantity. Jonathan, do you want to fix this here OR do you want to open another issue specific to the invoice page?
Very nice catch François :) That comes from a commit on bug 13323: commit b5e9c8a8821f950933c19b8524fe2af3a98d05d7 Author: Jonathan Druart <jonathan.druart@biblibre.com> Date: Wed Nov 19 17:01:17 2014 +0100 Bug 13323: Tax rate can change on receiving This commit permits to update the tax rate on receiving. diff --git a/acqui/invoice.pl b/acqui/invoice.pl index 3a163c5..9fcef0e 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -126,7 +126,8 @@ foreach my $order (@$orders) { - $line->{tax_value} *= $line->{quantity}; + $line->{tax_value} = $line->{tax_value_on_receiving}; + $line->{tax_rate} = $line->{tax_rate_on_receiving}; It should have been: - $line->{tax_value} *= $line->{quantity}; + $line->{tax_value} = $line->{tax_value_on_receiving} * $line->{quantity}; + $line->{tax_rate} = $line->{tax_rate_on_receiving}; The quantity has been forgotten. I updated the commit, the branch and the sandbox with this change.
I tested jonathan's last patch today. It works nicely. I placed orders with different vendors (different settings) and coulnd't find any side effects. Considering all the tests I did yesterday, I can safely sign off these patches. I tested on BibLibre's sandbox 17. Jonathan, could you sign this off for me? Thanks. PS : about comment 15, I tested the patch for BZ13324 (see comment 8). It works perfectly.
(In reply to Francois Charbonnier from comment #23) > Jonathan, could you sign this off for me? Thanks. Added on the remote branch :)
Working on this...
Hi Jonathan, can you please point me the right way for the branch? ft/bug_13321 looks not to be the right one.
... was my fault. I forgot to git fetch the biblibre repository.
Starting for real now - testing this bug together with 13323 and 13324. Some first feedback, I will number every point and keep a list outside of bugzilla too: DATABASE UPDATE 1) Some warns: Use of uninitialized value in string eq at ./installer/data/mysql/updatedatabase.pl line 10228. Use of uninitialized value in string eq at ./installer/data/mysql/updatedatabase.pl line 10228. Use of uninitialized value in string eq at ./installer/data/mysql/updatedatabase.pl line 10228. Use of uninitialized value in string eq at ./installer/data/mysql/updatedatabase.pl line 10228. Upgrade to 3.19.00.XXX done (Bug 13321 - Tax and prices calculation: add new columns to aqorders) Upgrade to 3.19.00.XXX done (Bug 13321 - Tax and prices calculation: tax_value contains the value for 1 item) Upgrade to 3.19.00.XXX done (Bug 13323 - Tax rate can change on receiving) 2) Shouldn't the second database update entry refer to the quantity somehow? Why the hardcoded 2? And shouldn't we fix the first database entry here? Bit worried about the round: + for my $order ( @$orders ) { + $sth->execute( $order->{tax_value} * 2, $order->{ordernumber} ); + } QA SCRIPT AND AUTOMATED TESTS 3) Failing tests: - t/db_dependent/Budgets.t not ok 61 - total spent for budget1 is 160 # Failed test 'total spent for budget1 is 160' # at t/db_dependent/Budgets.t line 371. # got: '0' # expected: '160' not ok 62 - total spent for budget11 is 100 # Failed test 'total spent for budget11 is 100' # at t/db_dependent/Budgets.t line 372. # got: '0' # expected: '100' not ok 63 - total spent for budget111 is 20 # Failed test 'total spent for budget111 is 20' # at t/db_dependent/Budgets.t line 373. # got: '0' # expected: '20'
I will continue with code review tomorrow!
(In reply to Katrin Fischer from comment #28) > Starting for real now - testing this bug together with 13323 and 13324. Thanks! :) > DATABASE UPDATE > > 1) Some warns: > Use of uninitialized value in string eq at > ./installer/data/mysql/updatedatabase.pl line 10228. Yes, wrong check, the column is "orderstatus", not "status". (Commit "Bug 13321: Tax and prices calculation - DB Changes" amended) > 2) Shouldn't the second database update entry refer to the quantity somehow? > Why the hardcoded 2? And shouldn't we fix the first database entry here? Bit > worried about the round: > > + for my $order ( @$orders ) { > + $sth->execute( $order->{tax_value} * 2, $order->{ordernumber} ); > + } Hum... That's weird :) Should be tax_value = tax_value / quantity. Commit "Bug 13321: tax_value should contain the value for 1 item" amended. > QA SCRIPT AND AUTOMATED TESTS > > 3) Failing tests: > > - t/db_dependent/Budgets.t > > not ok 61 - total spent for budget1 is 160 > # Failed test 'total spent for budget1 is 160' > # at t/db_dependent/Budgets.t line 371. > # got: '0' > # expected: '160' > not ok 62 - total spent for budget11 is 100 > # Failed test 'total spent for budget11 is 100' > # at t/db_dependent/Budgets.t line 372. > # got: '0' > # expected: '100' > not ok 63 - total spent for budget111 is 20 > # Failed test 'total spent for budget111 is 20' > # at t/db_dependent/Budgets.t line 373. > # got: '0' > # expected: '20' I forgot to change the call to ModReceiveOrder. Same for t/db_dependent/Acquisition/OrderUsers.t (new file). Commit "Bug 13321: Rename variables" amended. Branch updated.
I've been testing https://git.biblibre.com/biblibre/kohac/commits/ft/bug_13323 Branch definitely needs rebased, but I tested it as is. My own attempts to rebase it ended with the unit tests failing, which I'm hoping is just my fault Not sure which of the three bugs to put this on, so it's going here ; ) There seems to be an issue where if you receive multiple, the receiving screen lists the total, but then the total is multiplied by the number of items received! http://screencast.com/t/0gpbhP0P2tdC http://screencast.com/t/NDwekDjesa http://screencast.com/t/Fk6PBDv15o2K Other than that, everything looks good! So, to keep going: 1) Fix the bug above 2) Rebase your branch to master 3) Update the patches here on bugzilla
(In reply to Kyle M Hall from comment #31) > I've been testing > https://git.biblibre.com/biblibre/kohac/commits/ft/bug_13323 > > Branch definitely needs rebased, but I tested it as is. My own attempts to > rebase it ended with the unit tests failing, which I'm hoping is just my > fault I didn't get any conflict (except updatedatabase.pl) on rebasing the branch on master. > Not sure which of the three bugs to put this on, so it's going here ; ) > > There seems to be an issue where if you receive multiple, the receiving > screen lists the total, but then the total is multiplied by the number of > items received! > > http://screencast.com/t/0gpbhP0P2tdC > http://screencast.com/t/NDwekDjesa > http://screencast.com/t/Fk6PBDv15o2K 1 screen is missing, the neworderempty when you have created the order. I have tested on my local branch (before the jajm rebase) and with the remote branch, it seems to work as excepted. - Create an order without filling "actual cost" - Receive 1 or 2 item(s) (the actual cost is automatically filled with the budgeted cost, for 1 item. For you, it's 18, it should be 9 on your second screenshot. Did you update it manually or it was automatically filled with 18?).
Looking at this - could it be auser error? I think Kyle wanted to receive 2 items (9 was the calculated cost), so he entered the price for 2 = 18 into actual cost on receive - but it expects the price for a single item, so he should have entered 9 and let Koha mulitply it. (I checked, that's how it's currently working too in 3.18.8). It could be a nice enh for the future to add a little text hint there.
Back to the needs QA queue.
Created attachment 47489 [details] [review] Bug 13321: Rename variables Rebased on master
Created attachment 47490 [details] [review] Bug 13321: use the populate_with_prices where needed Rebased on master
Created attachment 47491 [details] [review] Bug 13321: Fix the prices calculation method Rebased on master
Created attachment 47492 [details] [review] Bug 13321: Tax and prices calculation - DB Changes Rebased on master
Created attachment 47493 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes Rebased on master
Created attachment 47494 [details] [review] Bug 13321: Fix unitprice rounding Rebased on master
Created attachment 48867 [details] [review] Bug 13321: Rename variables Rebased on master and added Signed-off-by lines that were lost in the process
Created attachment 48868 [details] [review] Bug 13321: use the populate_with_prices where needed Rebased on master and added Signed-off-by lines that were lost in the process
Created attachment 48869 [details] [review] Bug 13321: Fix the prices calculation method Rebased on master and added Signed-off-by lines that were lost in the process
Created attachment 48870 [details] [review] Bug 13321: Tax and prices calculation - DB Changes Rebased on master and added Signed-off-by lines that were lost in the process
Created attachment 48871 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes Rebased on master and added Signed-off-by lines that were lost in the process
Created attachment 48872 [details] [review] Bug 13321: Fix unitprice rounding Rebased on master and added Signed-off-by lines that were lost in the process
Sorry to see that this patchset fails: If you run qa tools (you should..), you have a lot of fails like: FAIL t/db_dependent/Serials/Claims.t OK critic OK forbidden patterns OK pod SKIP spelling FAIL valid Global symbol "$ordernumber" requires explicit package name Compilation failed in require This comes from C4/Acquistion.pm L 1458 NotifyOrderUsers( $ordernumber ); Note that your patch removes its declaration: sub ModReceiveOrder { - my ( $params ) = @_; - my $biblionumber = $params->{biblionumber}; - my $ordernumber = $params->{ordernumber}; When running qa tools, I also saw: 5xUse of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485. Argument "" isn't numeric in multiplication (*) at /usr/share/koha/masterclone/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/supplier.tt line 350.
Created attachment 51080 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51081 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51082 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51083 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51084 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51085 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Patches rebased on master + 1st patch amended to use $order->{ordernumber} instead of $ordernumber
Created attachment 51643 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51644 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51645 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51646 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51647 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51648 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 51649 [details] [review] Bug 13321: Adds renaming of gstrate column in kohastructure.sql
Patches rebased on master + Adds renaming of the column gstrate in kohastructure.sql
On http://pro.salers.biblibre.com/cgi-bin/koha/acqui/orderreceive.pl?ordernumber=6&invoiceid=2 Software error: Template process failed: undef error - : filter not found at /home/koha/src/C4/Templates.pm line 119. For help, please send mail to the webmaster (webmaster@test4-sandbox), giving this error message and the time and date of the error.
Created attachment 52046 [details] [review] Bug 13321: Add missing Price TT plugin in orderreceive.tt
(In reply to Koha Team Lyon 3 from comment #63) > On > http://pro.salers.biblibre.com/cgi-bin/koha/acqui/orderreceive. > pl?ordernumber=6&invoiceid=2 > > > Software error: > > Template process failed: undef error - : filter not found at > /home/koha/src/C4/Templates.pm line 119. > > For help, please send mail to the webmaster (webmaster@test4-sandbox), > giving this error message and the time and date of the error. Fixed
There's a problem with the GST calculation on the parcel and invoice pages : if you order 2 items but receive only one, the GST is calculated for 2 (number of items ordered) Here what is display in the table : Total tax exc. 18.20 Total (GST 5.00%) 1.82 Total tax inc. 19.11
The problem is tax value is not recalculated when order lines are split. I'll write a patch to fix that.
Created attachment 55168 [details] [review] Bug 13321: Recalculate tax_value for partially received orders Test plan: 1. Create an order with at least 2 items 2. Receive only 1 item 3. Check that the tax value of the received order line is correct on the parcel and invoice pages 4. Check that the tax value of the original order line is correct (on the basket page for example) 5. Cancel the receipt 6. Check that the tax value of the original order line is correct
Created attachment 55415 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55416 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55417 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55418 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55419 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55420 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 55421 [details] [review] Bug 13321: Adds renaming of gstrate column in kohastructure.sql
Created attachment 55422 [details] [review] Bug 13321: Add missing Price TT plugin in orderreceive.tt
Created attachment 55423 [details] [review] Bug 13321: Recalculate tax_value for partially received orders Test plan: 1. Create an order with at least 2 items 2. Receive only 1 item 3. Check that the tax value of the received order line is correct on the parcel and invoice pages 4. Check that the tax value of the original order line is correct (on the basket page for example) 5. Cancel the receipt 6. Check that the tax value of the original order line is correct
All patches rebased on master
Created attachment 55427 [details] [review] Bug 13321: Update DBIx::Class schema files
There's the same issue : There's a problem with the GST calculation on the parcel and invoice pages : if you order 2 items but receive only one, the GST is calculated for 2 (number of items ordered) Here what is display : Total tax exc. 10.00 Total (GST 10.00%) 2.00 Total tax inc. 11.00
Created attachment 56408 [details] [review] Bug 13321: Fix table display in invoice page
Created attachment 56422 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56423 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56424 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56425 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56426 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56427 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56428 [details] [review] Bug 13321: Adds renaming of gstrate column in kohastructure.sql Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56429 [details] [review] Bug 13321: Add missing Price TT plugin in orderreceive.tt Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56430 [details] [review] Bug 13321: Recalculate tax_value for partially received orders Test plan: 1. Create an order with at least 2 items 2. Receive only 1 item 3. Check that the tax value of the received order line is correct on the parcel and invoice pages 4. Check that the tax value of the original order line is correct (on the basket page for example) 5. Cancel the receipt 6. Check that the tax value of the original order line is correct Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56431 [details] [review] Bug 13321: Update DBIx::Class schema files Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Created attachment 56432 [details] [review] Bug 13321: Fix table display in invoice page Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
tested once again and works as described
Created attachment 56898 [details] [review] Bug 13321: Rename variables This patch renames the variable according to the new DB column names * gste => tax_excluded * gsti => tax_included * gstrate => tax_rate * gstvalue => tax_value This patch also modify the ModReceiveOrder subroutine: * Edit vendor note on receiving is not possible, so the code should not permit that. * Update ModReceiveOrder to pass a hashref And that's all! git grep on gste, gsti, gstrate and gstvalue should not return any code that can be executed. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56899 [details] [review] Bug 13321: use the populate_with_prices where needed Now we need to calculate the prices (ecost, rrp, unitprice) and tax (tax_rate, tax_value) when the price is set for an order. This only appends in the 3 files impacted by this patch. addorder*.pl on ordering finishreceive.pl on receiving Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56900 [details] [review] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56901 [details] [review] Bug 13321: Tax and prices calculation - DB Changes This patch adds 7 columns to the aqorders table: * unitprice_tax_excluded * unitprice_tax_included * rrp_tax_excluded * rrp_tax_included * ecost_tax_excluded * ecost_tax_included * tax_value It also renames: * aqorders.gstrate with aqorders.tax_rate * aqbooksellers.gstrate with aqbooksellers.tax_rate The new columns are filled with the previous calculation method. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56902 [details] [review] Bug 13321: Csv export - Display rrp and ecost with and without taxes If a basketgroup is exported as a CSV file, both prices should be displayed to avoid confusion. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56903 [details] [review] Bug 13321: Fix unitprice rounding We need to display the rounded value, but insert the exact value. The unit price can be change on receiving, so the input value can be modified. But if it's not, we must insert the exact (not rounded) unit price, which is the ecost value. Signed-off-by: Laurence Rault <laurence.rault@biblibre.com> Signed-off-by: Francois Charbonnier <francois.charbonnier@inlibro.com> Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56904 [details] [review] Bug 13321: Adds renaming of gstrate column in kohastructure.sql Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56905 [details] [review] Bug 13321: Add missing Price TT plugin in orderreceive.tt Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56906 [details] [review] Bug 13321: Recalculate tax_value for partially received orders Test plan: 1. Create an order with at least 2 items 2. Receive only 1 item 3. Check that the tax value of the received order line is correct on the parcel and invoice pages 4. Check that the tax value of the original order line is correct (on the basket page for example) 5. Cancel the receipt 6. Check that the tax value of the original order line is correct Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56907 [details] [review] Bug 13321: Update DBIx::Class schema files Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56908 [details] [review] Bug 13321: Fix table display in invoice page Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 56909 [details] [review] Bug 13321 - (QA Followup) Restore standing order functionality Some lines deleted during rebase, restored and updated, tests repaired
Created attachment 56930 [details] [review] Bug 13321 - (QA Followup) Restore standing order functionality Some lines deleted during rebase, restored and updated, tests repaired
Pushed to master for 16.11, thanks everyone!
Created attachment 56959 [details] [review] Bug 13321: Fix tests in Budgets.t
Bug 17562 - Acquisition.t is broken Bug 17563 - Acquisition/CancelReceipt.t is broken Bug 17564 - Acquisition/OrderUsers.t is broken Would have been judicious to launch the acquisition tests to confirm they won't break something.
(In reply to Kyle M Hall from comment #107) > Pushed to master for 16.11, thanks everyone! Contains DB changes, passing for 16.05.x series