Created attachment 31746 [details] [review] 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
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
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.
Paola, to test this patch, you must apply, the patches from: bug 5342, bug 12844, bug 12830, bug 12896 and finally bug 12969.
And don't forget bug 12852 after 5342. Otherwise the serial/claims.pl will raise an error.
12852
Created attachment 31841 [details] [review] Bug 12969: Add a subroutine to calculate VAT and prices I've applied the patch against master 3.17.00.025. Firstly I applied: 5342, 12852 (3), 12844 (4), 12830 (2), 12986. prove is OK. So I pass the patch to "Signed Off" status.
Created attachment 31877 [details] [review] 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
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.
Created attachment 31878 [details] [review] 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
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.
Created attachment 31897 [details] [review] Bug 12969: Fix typo rrpgsti should be rrpgste
(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.
Created attachment 31920 [details] [review] Bug 12969: Add a subroutine to calculate VAT and prices I've applied the patches against 3.17.00.025. Firstly I applied: 5342 (2), 12852 (3), 12844 (4), 12830 (2), 12986. prove is OK. So I pass the patch to "Signed Off" status.
Created attachment 31921 [details] [review] Bug 12969: Fix typo
(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.
Blocked by bug 12896 currently not applying. :(
Maybe some more comments later could improve readability, but no reason to fail :)
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
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>
Patch pushed to master. Thanks Jonathan!
(In reply to Tomás Cohen Arazi from comment #21) > Patch pushed to master. > > Thanks Jonathan! This causes tests to fail.
(In reply to Robin Sheat from comment #22) > This causes tests to fail. I think the problem got fixed now, with bug 13554.