Bug 13001

Summary: Refactor VAT and price calculation - parcel page
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, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9225
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 12969, 12976    
Bug Blocks: 13320, 17668    
Attachments: Bug 13001: Refactor VAT and price calculation - parcel page
Bug 13001: Refactor VAT and price calculation - parcel page
Bug 13001: The total for received order should be based on the unitprice
Bug 13001: The total for received order should be based on the unitprice
[PASSED QA] Bug 13001: Refactor VAT and price calculation - parcel page
[PASSED QA] Bug 13001: The total for received order should be based on the unitprice
Bug 13001: Refactor VAT and price calculation - parcel page
Bug 13001: The total for received order should be based on the unitprice

Description Jonathan Druart 2014-09-26 14:47:16 UTC
Bug 12969 introduces a subroutine to centralize the VAT and price calculation.
The parcel page should use it.
Comment 1 Jonathan Druart 2014-09-26 15:04:07 UTC Comment hidden (obsolete)
Comment 2 Paola Rossi 2014-10-03 09:59:50 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-10-28 22:28:31 UTC
Blocked by bug 12896 currently not applying.
Comment 4 Jonathan Druart 2014-11-19 12:41:04 UTC Comment hidden (obsolete)
Comment 5 Paola Rossi 2014-11-19 17:09:47 UTC
(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

-------------------------------------------------------------------
Comment 6 Jonathan Druart 2014-11-20 08:39:46 UTC
(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 :)
Comment 7 Paola Rossi 2014-11-20 16:13:25 UTC
(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.]
Comment 8 Jonathan Druart 2014-11-20 16:26:28 UTC
(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.
Comment 9 Paola Rossi 2014-11-21 14:48:21 UTC
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.
Comment 10 Jonathan Druart 2014-11-24 12:03:35 UTC
Sorry, you need bug 12976 too (changes in C4::Acquitision.pm).
Comment 11 Paola Rossi 2014-11-24 13:39:11 UTC
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.]
Comment 12 Jonathan Druart 2014-11-24 13:56:40 UTC
I rebased patches on bug 12896.
Comment 13 Paola Rossi 2014-11-25 10:03:03 UTC
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.
Comment 14 Jonathan Druart 2014-11-25 10:15:39 UTC
(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.
Comment 15 Paola Rossi 2014-11-25 12:54:12 UTC
(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".
Comment 16 Paola Rossi 2014-11-25 13:05:33 UTC Comment hidden (obsolete)
Comment 17 Paola Rossi 2014-11-25 13:38:57 UTC
(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.
Comment 18 Jonathan Druart 2014-11-25 13:51:25 UTC
(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!
Comment 19 Katrin Fischer 2015-01-05 23:58:27 UTC
I saw improvements with these patches applied - A missing cent showed up, fixing a total that was wrong before. :)
Comment 20 Katrin Fischer 2015-01-05 23:59:08 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2015-01-05 23:59:28 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2015-01-08 00:03:27 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 23 Jonathan Druart 2015-01-21 08:38:02 UTC
Tomas, This is *not* in master.
Comment 24 Jonathan Druart 2015-01-21 08:45:38 UTC
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>
Comment 25 Jonathan Druart 2015-01-21 08:45:41 UTC
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>
Comment 26 Jonathan Druart 2015-01-21 08:48:58 UTC
Conflict with bug 11944 fixed.
Comment 27 Tomás Cohen Arazi 2015-01-21 14:41:03 UTC
NOw, patches REALLY pushed to master.

Sorry for that Jonathan!