Bug 12969 introduces a subroutine to centralize the VAT and price calculation. The parcel page should use it.
Created attachment 31901 [details] [review] Bug 13001: Refactor VAT and price calculation - parcel page Bug 12969 introduces a subroutine to centralize VAT and prices calculation. It should be use in the acqui/parcel.pl script. Test plan: 1/ Create 4 suppliers with the different configurations 2/ Create a basket and create several orders 3/ Go on the parcel page 4/ You should see, on the "pending orders" table, the same prices as before this patch. Note that the prices are now correctly formated. You could see one change for the supplier configuration 3 (1 0): If the cost of the item is 82, discount 10% and vat 5%: The "Order cost" = 140.58 instead of 140.57. Indeed, before this patch, the order cost was wrong, now you should have 70.29*2 = 140.58 ( before: 140.58 + 7.03 = 147.61 now: 140.58 + 7.02 = 147.60 ) 5/ Receive the items and return on the parcel page Now the "Already received" table with the same prices as before this patch. Note some differences too: - There was a td tag missing, the table was badly formated, it's now fixed (column below the "Cancel receipt" link). - The prices are now correctly formated. - For the configuration 2 (1 1), if the cost of the item is 82, discount 10% and vat 5%: ( before: 140.57 + 7.03 = 147.60 now: 140.58 + 7.02 = 147.60 ) Note that 7.03 is the "correct" value, but on all other pages, 7.02 is displayed. To be consistent, we should display the same prices everywhere.
Created attachment 31982 [details] [review] Bug 13001: Refactor VAT and price calculation - parcel page I've applied te patch against master 3.17.00.025 Firstly I've applied: 5342 (2), 12852 (3), 12844 (4), 12830 (2), 12896 (1), 12969 (2) I've tested setting pref currencyformat either on FR or US. The tested steps in file-doc at bug 12964 are about parcel.pl: STEP 5) STEP 10) Either on 1-0 or 1-1 I've seen, as required: before: 140.57 + 7.03 = 147.60 now: 140.58 + 7.02 = 147.60 So I pass the patch to "Signed Off" status.
Blocked by bug 12896 currently not applying.
Created attachment 33677 [details] [review] Bug 13001: The total for received order should be based on the unitprice For already received orders, the total should be calculated with the unitprice, not the estimated cost.
(In reply to Jonathan Druart from comment #4) > Created attachment 33677 [details] [review] [review] > Bug 13001: The total for received order should be based on the unitprice > > For already received orders, the total should be calculated with the > unitprice, not the estimated cost. On master 3.17.00.057 HEAD "DBRev 3.17.00.057 (Koha 3.18 beta)", relating to datas in file: <http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31895> I changed the actual value to 70.00 on receiving (steps [9:12]). I hope it can help (and no errors). --------------------------------------------------------------------- "-->" means "has been changed to" qty => quantity rrp_et => recommended retail price excluding tax rrp_it => recommended retail price including tax discount => discount (%) ecost_et => ecost excluding tax ecost_it => ecost including tax gstrate => gst rate gstvalue => gst value I - Configuration 1 (0 0) List item price does not include tax Invoice item price does not include tax 9) On the receiving page (orderreceive.pl) qty = 2 rrp = 82 ecost = 73.80 actual cost = 73.80 --> 70.00 10) On the parcel page (parcel.pl) Est cost = 73.80 --> Actual cost 70.00 --> TOTAL 140.00 --> (Tax exc.) Subtotal Fund 147.60 --> (Tax exc.) Subtotal Est cost 140.00 total_et = 147.60 --> 140.00 gst = 7.38 --> 7 total_it = 154.98 --> 147.00 11) On the Acquisition home page (acqui-home.pl) ordered = 5 spent = 147.6 --> 140.00 12) On the basket page (basket.pl rrp_et = 82 ecost_et = 73.80 rrp_it = 86.10 ecost_it = 77.49 qty = 2 total_et = 147.60 total_it = 154.98 gstrate = 0.05 gstvalue = 7.38 II - Configuration 2 (1 1) List item price includes tax Invoice item price includes tax 9) On the receiving page (orderreceive.pl) qty = 2 rrp = 82 ecost = 73.80 actual cost = 73.80 --> 70.00 10) On the parcel page (parcel.pl) Est cost = 73.80 --> Actual cost 70.00 --> TOTAL 140.00 --> (Tax exc.) Subtotal Fund 147.60 --> (Tax exc.) Subtotal Est cost 140.00 total_et = 140.57 --> 133.33 gst = 7.03 --> 6.67 total_it = 147.60 --> 140.00 11) On the Acquisition home page (acqui-home.pl) ordered = 5 spent = 147.6 --> 140.00 12) On the basket page (basket.pl rrp_et = 78.10 ecost_et = 70.29 rrp_it = 82 ecost_it = 73.80 qty = 2 total_et = 140.58 total_it = 147.60 gstrate = 0.05 gstvalue = 7.02 III - Configuration 3 (1 0) List item price includes tax Invoice item price does not include tax 9) On the receiving page (orderreceive.pl) qty = 2 rrp = 78.10 ecost = 70.29 actual cost = 70.29 --> 70.00 10) On the parcel page (parcel.pl) Est cost = 70.29 --> Actual cost 70.00 --> TOTAL 140.00 --> (Tax exc.) Subtotal Fund 140.58 --> (Tax exc.) Subtotal Est cost 140.00 total_et = 140.58 --> 140.00 gst = 7.03 --> 7 total_it = 147.61 --> 147.00 11) On the Acquisition home page (acqui-home.pl) ordered = 5 spent = 147.61 --> 147.00 12) On the basket page (basket.pl rrp_et = 78.10 ecost_et = 70.29 rrp_it = 82.01 (yes 82.01...) ecost_it = 73.80 qty = 2 total_et = 140.58 total_it = 147.60 gstrate = 0.05 gstvalue = 7.02 IV - Configuration 4 (0 1) List item price does not include tax Invoice item price includes tax 9) On the receiving page (orderreceive.pl) qty = 2 rrp = 86.10 ecost = 77.49 actual cost = 77.49 --> 70.00 10) On the parcel page (parcel.pl) Est cost = 77.49 --> Actual cost 70.00 --> TOTAL 140.00 --> (Tax exc.) Subtotal Fund 140.58 --> 154.98 --> (Tax exc.) Subtotal Est cost --> 140.00 total_et = 147.60 --> 133.33 gst = 7.38 --> 6.67 total_it = 154.98 --> 140.00 11) On the Acquisition home page (acqui-home.pl) ordered = 5 spent = 147.6 --> 133.33 12) On the basket page (basket.pl rrp_et = 82 ecost_et = 73.80 rrp_it = 86.10 ecost_it = 77.49 qty = 2 total_et = 147.60 total_it = 154.98 gstrate = 0.05 gstvalue = 7.38 -------------------------------------------------------------------
(In reply to Paola Rossi from comment #5) > (In reply to Jonathan Druart from comment #4) > > Created attachment 33677 [details] [review] [review] [review] > > Bug 13001: The total for received order should be based on the unitprice > > > > For already received orders, the total should be calculated with the > > unitprice, not the estimated cost. > > On master 3.17.00.057 HEAD "DBRev 3.17.00.057 (Koha 3.18 beta)", > relating to datas in file: > <http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31895> > I changed the actual value to 70.00 on receiving (steps [9:12]). > I hope it can help (and no errors). Thanks a lot Paola. It could you QAer if you add your signoff on the last patch :)
(In reply to Jonathan Druart from comment #6) > (In reply to Paola Rossi from comment #5) [...] > It could you QAer if you add your signoff on the last patch :) To add the signoff to the patch 13001, I tried to apply: 12852 PQA (3: 32275, 32276, 32277) 12896 PQA (2: 33332 33333) 12969 SO (2) 13001 (2) Is it right? But, against master 3.17.00.057 HEAD 13229, : Applying: Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: Koha/Acquisition/Bookseller.pm Please move or remove them before you can merge. Aborting Failed to merge in the changes. Patch failed at 0001 Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller [For now I keep this patch 13001 in the "Signed Off" status.]
(In reply to Paola Rossi from comment #7) > (In reply to Jonathan Druart from comment #6) > > (In reply to Paola Rossi from comment #5) > [...] > > It could you QAer if you add your signoff on the last patch :) > > To add the signoff to the patch 13001, I tried to apply: > 12852 PQA (3: 32275, 32276, 32277) > 12896 PQA (2: 33332 33333) > 12969 SO (2) > 13001 (2) > > Is it right? Yes, it is. > error: The following untracked working tree files would be overwritten by > merge: > Koha/Acquisition/Bookseller.pm > Please move or remove them before you can merge. You need to remove this file.
I've applied the patches against master 3.17.00.057 head 12750. I've precicely applied: 12852 PQA (32275, 32276, 32277) 12896 PQA (33332 33333) 12969 SO (31920 31921) 13001 (31982 33677) On the 0-0 conf, at step 10) the following desired changes didn't occur: total_et = 147.60 --> 140.00 gst = 7.38 --> 7 total_it = 154.98 --> 147.00 So I pass the patch to "Failed QA" status. (In reply to Jonathan Druart from comment #8) > You need to remove this file. Thanks, Jonathan.
Sorry, you need bug 12976 too (changes in C4::Acquitision.pm).
Today I've tried to apply the patches against master 3.17.00.058 head 13297 I should have applied this sequence: 12852 PQA (32275, 32276, 32277) 12896 PQA (33332 33333) 12969 SO (31920 31921) 12976 SO (31983 33573 33613) 13001 (31982 33677) But : Applying: Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging acqui/newordersuggestion.pl CONFLICT (content): Merge conflict in acqui/newordersuggestion.pl Auto-merging acqui/addorderiso2709.pl Failed to merge in the changes. Patch failed at 0001 Bug 12896: Move the bookseller-related code into Koha::Acquisition::Bookseller [I keep this patch 13001 in the "Signed Off" status.]
I rebased patches on bug 12896.
Thanks, Jonathan. Today I've applied the patches against master 3.17.00.058 head 13297 [I have applied this sequence: 12852 PQA (32275, 32276, 32277) 12896 PQA (33855 33856) 12969 SO (31920 31921) 12976 SO (31983 33573 33613) 13001 (31982 33677)] On II - Configuration 2 (1 1) step 10), the expecting last 3 values were (see comment 5): total_et = 140.57 --> 133.33 gst = 7.03 --> 6.67 total_it = 147.60 --> 140.00 but on the contrary I saw: total_et = 140.57 --> 133.34 gst = 7.03 --> 6.66 total_it = 147.60 --> 140.00 I pass the patch to "Failed QA" status.
(In reply to Paola Rossi from comment #13) Yes, it's what I explained in the commit message. There are some differences on this view, because it's the only one where prices are different. To be consistent, I decided to display the same values everywhere.
(In reply to Jonathan Druart from comment #14) > (In reply to Paola Rossi from comment #13) > > Yes, it's what I explained in the commit message. > There are some differences on this view, because it's the only one where > prices are different. > OK, sorry. > To be consistent, I decided to display the same values everywhere. It happens on Configuration 2 and 4. I turn back the status of this patch to "Needs Signoff".
Created attachment 33900 [details] [review] Bug 13001: The total for received order should be based on the unitprice I've applied the 12 patches against master 3.17.00.058 head 13297 Everything matched datas shown in comment 5. As required in comment 6 I add my sign off to the second patch : Bug 13001: The total for received order should be based on the unitprice
(In reply to Jonathan Druart from comment #6) > It could you QAer if you add your signoff on the last patch :) Kind Jonathan, I don't usually do QA's actions, I just verify a patch in "Needs SO" status. [Anyway, I see 2 possible "QAer"'s statuses: "Pushed for QA" and "Passed QA". And anyway the "Pushed for QA" I'd "preferred" is not referenced by the development workflow.] So I'd keep the "Signed Off" status for now.
(In reply to Paola Rossi from comment #17) > (In reply to Jonathan Druart from comment #6) > > > It could you QAer if you add your signoff on the last patch :) > > Kind Jonathan, > I don't usually do QA's actions, I just verify a patch in "Needs SO" status. Sorry, I meant "It could *help* QAers". So it's perfect now :) Thanks!
I saw improvements with these patches applied - A missing cent showed up, fixing a total that was wrong before. :)
Created attachment 34973 [details] [review] [PASSED QA] Bug 13001: Refactor VAT and price calculation - parcel page Bug 12969 introduces a subroutine to centralize VAT and prices calculation. It should be use in the acqui/parcel.pl script. Test plan: 1/ Create 4 suppliers with the different configurations 2/ Create a basket and create several orders 3/ Go on the parcel page 4/ You should see, on the "pending orders" table, the same prices as before this patch. Note that the prices are now correctly formated. You could see one change for the supplier configuration 3 (1 0): If the cost of the item is 82, discount 10% and vat 5%: The "Order cost" = 140.58 instead of 140.57. Indeed, before this patch, the order cost was wrong, now you should have 70.29*2 = 140.58 ( before: 140.58 + 7.03 = 147.61 now: 140.58 + 7.02 = 147.60 ) 5/ Receive the items and return on the parcel page Now the "Already received" table with the same prices as before this patch. Note some differences too: - There was a td tag missing, the table was badly formated, it's now fixed (column below the "Cancel receipt" link). - The prices are now correctly formated. - For the configuration 2 (1 1), if the cost of the item is 82, discount 10% and vat 5%: ( before: 140.57 + 7.03 = 147.60 now: 140.58 + 7.02 = 147.60 ) Note that 7.03 is the "correct" value, but on all other pages, 7.02 is displayed. To be consistent, we should display the same prices everywhere. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 34974 [details] [review] [PASSED QA] Bug 13001: The total for received order should be based on the unitprice For already received orders, the total should be calculated with the unitprice, not the estimated cost. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patches pushed to master. Thanks Jonathan!
Tomas, This is *not* in master.
Created attachment 35427 [details] [review] Bug 13001: Refactor VAT and price calculation - parcel page Bug 12969 introduces a subroutine to centralize VAT and prices calculation. It should be use in the acqui/parcel.pl script. Test plan: 1/ Create 4 suppliers with the different configurations 2/ Create a basket and create several orders 3/ Go on the parcel page 4/ You should see, on the "pending orders" table, the same prices as before this patch. Note that the prices are now correctly formated. You could see one change for the supplier configuration 3 (1 0): If the cost of the item is 82, discount 10% and vat 5%: The "Order cost" = 140.58 instead of 140.57. Indeed, before this patch, the order cost was wrong, now you should have 70.29*2 = 140.58 ( before: 140.58 + 7.03 = 147.61 now: 140.58 + 7.02 = 147.60 ) 5/ Receive the items and return on the parcel page Now the "Already received" table with the same prices as before this patch. Note some differences too: - There was a td tag missing, the table was badly formated, it's now fixed (column below the "Cancel receipt" link). - The prices are now correctly formated. - For the configuration 2 (1 1), if the cost of the item is 82, discount 10% and vat 5%: ( before: 140.57 + 7.03 = 147.60 now: 140.58 + 7.02 = 147.60 ) Note that 7.03 is the "correct" value, but on all other pages, 7.02 is displayed. To be consistent, we should display the same prices everywhere. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 35428 [details] [review] Bug 13001: The total for received order should be based on the unitprice For already received orders, the total should be calculated with the unitprice, not the estimated cost. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Conflict with bug 11944 fixed.
NOw, patches REALLY pushed to master. Sorry for that Jonathan!