Bug 13321 - Fix tax and prices calculation
Summary: Fix tax and prices calculation
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 12826
Blocks: 17439 12825 13323 13324 17562 17563 17564 18639 18736 20922 24277
  Show dependency treegraph
 
Reported: 2014-11-21 11:57 UTC by Jonathan Druart
Modified: 2019-12-23 15:09 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Now 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 idea is to create additional DB fields to store value tax included and tax excluded. 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 Check your SQL reports on tax and price.
Version(s) released in:


Attachments
Bug 13321: Rename variables (117.78 KB, patch)
2014-11-21 16:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.63 KB, patch)
2014-11-21 16:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.66 KB, patch)
2014-11-21 16:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.89 KB, patch)
2014-11-21 16:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: [DO NOT PUSH] Tax and prices calculation - DBIC files (6.14 KB, patch)
2014-11-21 16:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.23 KB, patch)
2014-12-15 15:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
rounded issue for conf 1 0 (4.87 KB, image/png)
2015-01-13 11:57 UTC, Jonathan Druart
Details
Bug 13321: Fix unitprice rounding (1.98 KB, patch)
2015-01-13 16:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.10 KB, patch)
2016-02-01 09:19 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.64 KB, patch)
2016-02-01 09:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.59 KB, patch)
2016-02-01 09:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.31 KB, patch)
2016-02-01 09:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.23 KB, patch)
2016-02-01 09:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (1.98 KB, patch)
2016-02-01 09:20 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.31 KB, patch)
2016-03-09 14:49 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.82 KB, patch)
2016-03-09 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.80 KB, patch)
2016-03-09 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.49 KB, patch)
2016-03-09 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.42 KB, patch)
2016-03-09 14:50 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.17 KB, patch)
2016-03-09 14:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.45 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.82 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.79 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.49 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.42 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.17 KB, patch)
2016-05-02 10:11 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.77 KB, patch)
2016-05-20 12:26 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.85 KB, patch)
2016-05-20 12:26 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.79 KB, patch)
2016-05-20 12:26 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.49 KB, patch)
2016-05-20 12:27 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.42 KB, patch)
2016-05-20 12:27 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.17 KB, patch)
2016-05-20 12:27 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Adds renaming of gstrate column in kohastructure.sql (1.29 KB, patch)
2016-05-20 12:27 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Add missing Price TT plugin in orderreceive.tt (911 bytes, patch)
2016-06-04 11:47 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 13321: Recalculate tax_value for partially received orders (2.02 KB, patch)
2016-09-05 10:01 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.77 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.85 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.79 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.49 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.42 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.17 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Adds renaming of gstrate column in kohastructure.sql (1.29 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Add missing Price TT plugin in orderreceive.tt (931 bytes, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Recalculate tax_value for partially received orders (2.02 KB, patch)
2016-09-09 10:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Update DBIx::Class schema files (4.51 KB, patch)
2016-09-09 11:45 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Fix table display in invoice page (1.10 KB, patch)
2016-10-13 14:05 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.81 KB, patch)
2016-10-13 15:40 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.89 KB, patch)
2016-10-13 15:40 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.84 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.54 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.46 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.21 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Adds renaming of gstrate column in kohastructure.sql (1.34 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Add missing Price TT plugin in orderreceive.tt (979 bytes, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Recalculate tax_value for partially received orders (2.06 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Update DBIx::Class schema files (4.56 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Fix table display in invoice page (1.15 KB, patch)
2016-10-13 15:41 UTC, Koha Team University Lyon 3
Details | Diff | Splinter Review
Bug 13321: Rename variables (117.87 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: use the populate_with_prices where needed (2.95 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Fix the prices calculation method (25.90 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Tax and prices calculation - DB Changes (8.59 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Csv export - Display rrp and ecost with and without taxes (4.52 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Fix unitprice rounding (2.27 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Adds renaming of gstrate column in kohastructure.sql (1.40 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Add missing Price TT plugin in orderreceive.tt (1.01 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Recalculate tax_value for partially received orders (2.12 KB, patch)
2016-10-27 01:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Update DBIx::Class schema files (4.61 KB, patch)
2016-10-27 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Fix table display in invoice page (1.20 KB, patch)
2016-10-27 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321 - (QA Followup) Restore standing order functionality (4.08 KB, patch)
2016-10-27 01:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321 - (QA Followup) Restore standing order functionality (4.08 KB, patch)
2016-10-27 14:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13321: Fix tests in Budgets.t (2.01 KB, patch)
2016-10-28 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-11-21 11:57:30 UTC
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.
Comment 1 Jonathan Druart 2014-11-21 16:05:34 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-11-21 16:05:39 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-11-21 16:05:46 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-11-21 16:05:50 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-11-21 16:05:53 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-11-21 16:07:21 UTC
These patches will be tested with BibLibre coworker before.
But you can help us and test them if you like :)
Comment 7 Koha Team University Lyon 3 2014-12-12 15:19:16 UTC
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
Comment 8 Jonathan Druart 2014-12-15 15:49:34 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-12-17 12:56:38 UTC
(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.
Comment 10 Jonathan Druart 2015-01-07 08:20:42 UTC
To test these patches, have a look at http://lists.koha-community.org/pipermail/koha-devel/2014-December/041024.html
Comment 11 Jonathan Druart 2015-01-13 11:57:21 UTC
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?
Comment 12 Francois Charbonnier 2015-01-13 13:52:13 UTC
+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.
Comment 13 Marc Véron 2015-01-13 15:05:36 UTC
+1 for exact value stored in database
Comment 14 Jonathan Druart 2015-01-13 15:11:51 UTC
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
Comment 15 Jonathan Druart 2015-01-13 16:20:34 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-01-13 16:23:20 UTC
New commit on kohac/ft/bug_13323 and applied on the sandbox.
It should fix the rounding problem for unit price on receiving.
Comment 17 Colin Campbell 2015-01-13 17:07:36 UTC
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
Comment 18 Jonathan Druart 2015-01-14 09:25:55 UTC
(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?
Comment 19 Paola Rossi 2015-02-24 13:59:57 UTC
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?
Comment 20 Jonathan Druart 2015-02-24 15:34:08 UTC
(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.
Comment 21 Francois Charbonnier 2015-02-25 01:22:49 UTC
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?
Comment 22 Jonathan Druart 2015-02-25 10:24:49 UTC
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.
Comment 23 Francois Charbonnier 2015-02-25 19:08:25 UTC
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.
Comment 24 Jonathan Druart 2015-02-26 08:37:10 UTC
(In reply to Francois Charbonnier from comment #23)
> Jonathan, could you sign this off for me? Thanks.

Added on the remote branch :)
Comment 25 Katrin Fischer 2015-04-20 06:18:42 UTC
Working on this...
Comment 26 Katrin Fischer 2015-04-20 06:21:54 UTC
Hi Jonathan, can you please point me the right way for the branch?
ft/bug_13321 looks not to be the right one.
Comment 27 Katrin Fischer 2015-04-20 06:38:37 UTC
... was my fault. I forgot to git fetch the biblibre repository.
Comment 28 Katrin Fischer 2015-04-20 22:19:01 UTC
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'
Comment 29 Katrin Fischer 2015-04-20 22:40:57 UTC
I will continue with code review tomorrow!
Comment 30 Jonathan Druart 2015-04-21 08:27:57 UTC
(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.
Comment 31 Kyle M Hall 2015-07-24 17:06:23 UTC
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
Comment 32 Jonathan Druart 2015-08-04 08:06:57 UTC
(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?).
Comment 33 Katrin Fischer 2015-08-04 08:46:36 UTC
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.
Comment 34 Jonathan Druart 2015-08-20 07:30:04 UTC
Back to the needs QA queue.
Comment 35 Julian Maurice 2016-02-01 09:19:50 UTC Comment hidden (obsolete)
Comment 36 Julian Maurice 2016-02-01 09:20:03 UTC Comment hidden (obsolete)
Comment 37 Julian Maurice 2016-02-01 09:20:17 UTC Comment hidden (obsolete)
Comment 38 Julian Maurice 2016-02-01 09:20:31 UTC Comment hidden (obsolete)
Comment 39 Julian Maurice 2016-02-01 09:20:41 UTC Comment hidden (obsolete)
Comment 40 Julian Maurice 2016-02-01 09:20:51 UTC Comment hidden (obsolete)
Comment 41 Julian Maurice 2016-03-09 14:49:43 UTC Comment hidden (obsolete)
Comment 42 Julian Maurice 2016-03-09 14:50:01 UTC Comment hidden (obsolete)
Comment 43 Julian Maurice 2016-03-09 14:50:20 UTC Comment hidden (obsolete)
Comment 44 Julian Maurice 2016-03-09 14:50:33 UTC Comment hidden (obsolete)
Comment 45 Julian Maurice 2016-03-09 14:50:52 UTC Comment hidden (obsolete)
Comment 46 Julian Maurice 2016-03-09 14:51:05 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2016-03-18 07:17:59 UTC
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.
Comment 48 Julian Maurice 2016-05-02 10:11:27 UTC Comment hidden (obsolete)
Comment 49 Julian Maurice 2016-05-02 10:11:31 UTC Comment hidden (obsolete)
Comment 50 Julian Maurice 2016-05-02 10:11:35 UTC Comment hidden (obsolete)
Comment 51 Julian Maurice 2016-05-02 10:11:39 UTC Comment hidden (obsolete)
Comment 52 Julian Maurice 2016-05-02 10:11:43 UTC Comment hidden (obsolete)
Comment 53 Julian Maurice 2016-05-02 10:11:47 UTC Comment hidden (obsolete)
Comment 54 Julian Maurice 2016-05-02 10:14:32 UTC
Patches rebased on master + 1st patch amended to use $order->{ordernumber} instead of $ordernumber
Comment 55 Alex Arnaud 2016-05-20 12:26:41 UTC Comment hidden (obsolete)
Comment 56 Alex Arnaud 2016-05-20 12:26:50 UTC Comment hidden (obsolete)
Comment 57 Alex Arnaud 2016-05-20 12:26:59 UTC Comment hidden (obsolete)
Comment 58 Alex Arnaud 2016-05-20 12:27:07 UTC Comment hidden (obsolete)
Comment 59 Alex Arnaud 2016-05-20 12:27:16 UTC Comment hidden (obsolete)
Comment 60 Alex Arnaud 2016-05-20 12:27:25 UTC Comment hidden (obsolete)
Comment 61 Alex Arnaud 2016-05-20 12:27:33 UTC Comment hidden (obsolete)
Comment 62 Alex Arnaud 2016-05-20 12:28:36 UTC
Patches rebased on master + Adds renaming of the column gstrate in kohastructure.sql
Comment 63 Koha Team University Lyon 3 2016-06-04 11:36:14 UTC
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.
Comment 64 Alex Arnaud 2016-06-04 11:47:12 UTC Comment hidden (obsolete)
Comment 65 Alex Arnaud 2016-06-04 11:47:52 UTC
(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
Comment 66 Koha Team University Lyon 3 2016-06-04 13:13:13 UTC
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
Comment 67 Julian Maurice 2016-09-05 09:20:59 UTC
The problem is tax value is not recalculated when order lines are split.
I'll write a patch to fix that.
Comment 68 Julian Maurice 2016-09-05 10:01:52 UTC Comment hidden (obsolete)
Comment 69 Julian Maurice 2016-09-09 10:02:23 UTC
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>
Comment 70 Julian Maurice 2016-09-09 10:02:27 UTC
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>
Comment 71 Julian Maurice 2016-09-09 10:02:31 UTC
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>
Comment 72 Julian Maurice 2016-09-09 10:02:36 UTC
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>
Comment 73 Julian Maurice 2016-09-09 10:02:40 UTC
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>
Comment 74 Julian Maurice 2016-09-09 10:02:44 UTC
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>
Comment 75 Julian Maurice 2016-09-09 10:02:49 UTC
Created attachment 55421 [details] [review]
Bug 13321: Adds renaming of gstrate column in kohastructure.sql
Comment 76 Julian Maurice 2016-09-09 10:02:53 UTC
Created attachment 55422 [details] [review]
Bug 13321: Add missing Price TT plugin in orderreceive.tt
Comment 77 Julian Maurice 2016-09-09 10:02:58 UTC
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
Comment 78 Julian Maurice 2016-09-09 10:03:20 UTC
All patches rebased on master
Comment 79 Julian Maurice 2016-09-09 11:45:59 UTC
Created attachment 55427 [details] [review]
Bug 13321: Update DBIx::Class schema files
Comment 80 Koha Team University Lyon 3 2016-10-13 10:18:11 UTC
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
Comment 81 Julian Maurice 2016-10-13 14:05:40 UTC
Created attachment 56408 [details] [review]
Bug 13321: Fix table display in invoice page
Comment 82 Koha Team University Lyon 3 2016-10-13 15:40:48 UTC
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>
Comment 83 Koha Team University Lyon 3 2016-10-13 15:40:58 UTC
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>
Comment 84 Koha Team University Lyon 3 2016-10-13 15:41:04 UTC
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>
Comment 85 Koha Team University Lyon 3 2016-10-13 15:41:10 UTC
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>
Comment 86 Koha Team University Lyon 3 2016-10-13 15:41:20 UTC
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>
Comment 87 Koha Team University Lyon 3 2016-10-13 15:41:24 UTC
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>
Comment 88 Koha Team University Lyon 3 2016-10-13 15:41:30 UTC
Created attachment 56428 [details] [review]
Bug 13321: Adds renaming of gstrate column in kohastructure.sql

Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Comment 89 Koha Team University Lyon 3 2016-10-13 15:41:35 UTC
Created attachment 56429 [details] [review]
Bug 13321: Add missing Price TT plugin in orderreceive.tt

Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Comment 90 Koha Team University Lyon 3 2016-10-13 15:41:40 UTC
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>
Comment 91 Koha Team University Lyon 3 2016-10-13 15:41:44 UTC
Created attachment 56431 [details] [review]
Bug 13321: Update DBIx::Class schema files

Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Comment 92 Koha Team University Lyon 3 2016-10-13 15:41:49 UTC
Created attachment 56432 [details] [review]
Bug 13321: Fix table display in invoice page

Signed-off-by: Sonia Bouis <koha@univ-lyon3.fr>
Comment 93 Koha Team University Lyon 3 2016-10-13 15:51:34 UTC
tested once again and works as described
Comment 94 Nick Clemens 2016-10-27 01:01:00 UTC
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>
Comment 95 Nick Clemens 2016-10-27 01:01:08 UTC
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>
Comment 96 Nick Clemens 2016-10-27 01:01:14 UTC
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>
Comment 97 Nick Clemens 2016-10-27 01:01:21 UTC
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>
Comment 98 Nick Clemens 2016-10-27 01:01:28 UTC
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>
Comment 99 Nick Clemens 2016-10-27 01:01:35 UTC
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>
Comment 100 Nick Clemens 2016-10-27 01:01:42 UTC
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>
Comment 101 Nick Clemens 2016-10-27 01:01:49 UTC
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>
Comment 102 Nick Clemens 2016-10-27 01:01:56 UTC
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>
Comment 103 Nick Clemens 2016-10-27 01:02:03 UTC
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>
Comment 104 Nick Clemens 2016-10-27 01:02:10 UTC
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>
Comment 105 Nick Clemens 2016-10-27 01:02:42 UTC
Created attachment 56909 [details] [review]
Bug 13321 - (QA Followup) Restore standing order functionality

Some lines deleted during rebase, restored and updated, tests repaired
Comment 106 Nick Clemens 2016-10-27 14:57:10 UTC
Created attachment 56930 [details] [review]
Bug 13321 - (QA Followup) Restore standing order functionality

Some lines deleted during rebase, restored and updated, tests repaired
Comment 107 Kyle M Hall 2016-10-28 14:39:41 UTC
Pushed to master for 16.11, thanks everyone!
Comment 108 Jonathan Druart 2016-10-28 15:24:02 UTC
Created attachment 56959 [details] [review]
Bug 13321: Fix tests in Budgets.t
Comment 109 Jonathan Druart 2016-11-07 11:13:36 UTC
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.
Comment 110 Mason James 2016-11-30 07:53:37 UTC
(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