Bug 12969

Summary: VAT and price calculation should be done in a subroutine
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, paola.rossi, robin, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13554
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12852, 12896    
Bug Blocks: 12970, 12975, 12976, 13001, 13554    
Attachments: Bug 12969: Add a subroutine to calculate VAT and prices
Bug 12969: Add a subroutine to calculate VAT and prices
Bug 12969: Add a subroutine to calculate VAT and prices
Bug 12969: Add a subroutine to calculate VAT and prices
Bug 12969: Fix typo
Bug 12969: Add a subroutine to calculate VAT and prices
Bug 12969: Fix typo
[PASSED QA] Bug 12969: Add a subroutine to calculate VAT and prices
[PASSED QA] Bug 12969: Fix typo

Description Jonathan Druart 2014-09-19 15:15:42 UTC

    
Comment 1 Jonathan Druart 2014-09-19 15:30:12 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-09-22 15:21:59 UTC
I've applied the patch against master 3.17.00.025.

[Firstly I've applied: 5342, 12830, 12896]

perl t/Prices.t 
1..4
Can't locate Koha/Number/Price.pm in @INC (@INC contains: /var/root-koha/bug-XXXX /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr/local/lib/site_perl .) at /var/root-koha/bug-XXXX/C4/Acquisition.pm line 35.
BEGIN failed--compilation aborted at /var/root-koha/bug-XXXX/C4/Acquisition.pm line 35.
Compilation failed in require at t/Prices.t line 5.
BEGIN failed--compilation aborted at t/Prices.t line 5.
# Looks like your test exited with 2 before it could output anything.

So I pass the patch to "Failed QA" status
Comment 3 Jonathan Druart 2014-09-22 15:33:17 UTC
Paola, yes sorry, I forgot 1 dependency.
Patches on bug 12844 have to be applied too, otherwise the prices are not rounded and the calculation is wrong.
Comment 4 Jonathan Druart 2014-09-23 09:49:26 UTC
Paola, to test this patch, you must apply, the patches from:
bug 5342, bug 12844, bug 12830, bug 12896
and finally bug 12969.
Comment 5 Jonathan Druart 2014-09-23 10:51:02 UTC
And don't forget bug 12852 after 5342.
Otherwise the serial/claims.pl will raise an error.
Comment 6 Jonathan Druart 2014-09-23 10:51:26 UTC
12852
Comment 7 Paola Rossi 2014-09-23 16:56:05 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-09-25 14:05:37 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-09-25 14:06:34 UTC
I reset the status to NSO, the patch didn't take into account the CurrencyFormat.
The unit tests didn't pass if the value 'FR' was chosen.
Comment 10 Jonathan Druart 2014-09-25 14:16:19 UTC Comment hidden (obsolete)
Comment 11 Paola Rossi 2014-09-26 13:17:08 UTC
1) Reading the file Prices.t of the patch, the "$order_x->rrpgste" always has "rrpgsti" as the corresponding "field", instead of "rrpgste".

For example this is the compare of the first configuration 0-0:
+        compare(
+            {
+                got      => $order_0_0->{rrpgste},
+                expected => 82.00,
+                conf     => '0 0',
+                field    => 'rrpgsti'
+            }
+        );

In all the 4 Conf this compare has the same "field" rrpgsti. 
Is it an error?

2) Comparing the two files, in Conf III 1 0 I have some doubts:

A) unitprice is 73.804500, whilst the 12964's file-doc says 73.80;
B) rrp is 82.01, instead of 82.00 (in the 12964's file-doc)
C) so, in the following:
+        compare(
+            {
+                got      => $order_1_0->{rrpgsti},
+                expected => 82.01,

the "expected" should be 82.00 instead.

Anyway soon after B) you wrote:
+        # Note that this configuration is *not* correct!
+        # rrp gsti should be 82 (what we inserted!)
+        # gstvalue should be 7.03 instead of 7.02

so 2) could be right.

The 2) note is the only difference I've seen between the patch and the 12964's file-doc.
Comment 12 Jonathan Druart 2014-09-26 13:28:30 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-09-26 13:32:14 UTC
(In reply to Paola Rossi from comment #11)
> 1) Reading the file Prices.t of the patch, the "$order_x->rrpgste" always
> has "rrpgsti" as the corresponding "field", instead of "rrpgste".
> 
> For example this is the compare of the first configuration 0-0:
> +        compare(
> +            {
> +                got      => $order_0_0->{rrpgste},
> +                expected => 82.00,
> +                conf     => '0 0',
> +                field    => 'rrpgsti'
> +            }
> +        );
> 
> In all the 4 Conf this compare has the same "field" rrpgsti. 
> Is it an error?

It's a typo. It is not important, this is only used to display the test description.

Fixed in the followup.

> 2) Comparing the two files, in Conf III 1 0 I have some doubts:
> 
> A) unitprice is 73.804500, whilst the 12964's file-doc says 73.80;
> B) rrp is 82.01, instead of 82.00 (in the 12964's file-doc)
> C) so, in the following:
> +        compare(
> +            {
> +                got      => $order_1_0->{rrpgsti},
> +                expected => 82.01,
> 
> the "expected" should be 82.00 instead.
> 
> Anyway soon after B) you wrote:
> +        # Note that this configuration is *not* correct!
> +        # rrp gsti should be 82 (what we inserted!)
> +        # gstvalue should be 7.03 instead of 7.02
> 
> so 2) could be right.
> 
> The 2) note is the only difference I've seen between the patch and the
> 12964's file-doc.

Actually, I thought it was always 82.01 (what you found this morning on bug 12964.
In master, if you didn't receive the order yet, the displayed value is 82.00 BUT it the order is received, the value becomes 82.01 (...)
The tests consider that the orders have already been received (the datereceived and quantityreceived are set).
The 82.01 is correct here.
Comment 14 Paola Rossi 2014-09-29 13:53:58 UTC Comment hidden (obsolete)
Comment 15 Paola Rossi 2014-09-29 13:54:36 UTC Comment hidden (obsolete)
Comment 16 Paola Rossi 2014-09-29 14:02:17 UTC
(In reply to Jonathan Druart from comment #9)
> [...] the patch didn't take into account the
> CurrencyFormat.
> The unit tests didn't pass if the value 'FR' was chosen.

Today this test is OK too.
Comment 17 Katrin Fischer 2014-10-28 22:27:48 UTC
Blocked by bug 12896 currently not applying. :(
Comment 18 Katrin Fischer 2014-12-27 19:11:32 UTC
Maybe some more comments later could improve readability, but no reason to fail :)
Comment 19 Katrin Fischer 2014-12-27 19:14:04 UTC
Created attachment 34731 [details] [review]
[PASSED QA] Bug 12969: Add a subroutine to calculate VAT and prices

This patch adds a new subroutine populate_order_with_prices in the
C4::Acquisition module.

Its goal is to refactore the VAT and prices calculation into Koha.
All scripts will use this subroutine.

Test plan:
Verify that the prices in t/Prices.t are consistent with the values
listed in the file "Prices and VAT calculation - before" submit on bug
12964.
Verify that
  prove t/Prices.t
returns green

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

http://bugs.koha-community.org/show_bug.cgi?id=12896
Comment 20 Katrin Fischer 2014-12-27 19:15:52 UTC
Created attachment 34732 [details] [review]
[PASSED QA] Bug 12969: Fix typo

rrpgsti should be rrpgste

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Tomás Cohen Arazi 2015-01-04 16:07:47 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 22 Robin Sheat 2015-01-11 23:19:40 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> Patch pushed to master.
> 
> Thanks Jonathan!

This causes tests to fail.
Comment 23 Katrin Fischer 2015-01-18 17:35:36 UTC
(In reply to Robin Sheat from comment #22)
> This causes tests to fail.

I think the problem got fixed now, with bug 13554.