Bug 25750 - Fallback to ecost_tax_included, ecost_tax_excluded not happening when no 'Actual cost' entered
Summary: Fallback to ecost_tax_included, ecost_tax_excluded not happening when no 'Act...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
Depends on: 23522
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-14 22:32 UTC by Alex Buckley
Modified: 2020-09-20 17:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.04, 19.11.10


Attachments
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded (2.58 KB, patch)
2020-06-14 23:14 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded (2.64 KB, patch)
2020-06-15 12:08 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded (5.00 KB, patch)
2020-06-24 00:35 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded (5.07 KB, patch)
2020-06-24 12:20 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded (5.12 KB, patch)
2020-06-29 16:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2020-06-14 22:32:12 UTC
The patchsets on bugs 23523 and 23522 set and display respectively the unitprice_tax_included/unitprice_tax_excluded if 'Actual cost' has been entered.

If 'Actual cost' is not entered entered then Koha should fallback to ecost_tax_included/ecost_tax_excluded. 

However the fallback is not working correctly. When 'Actual cost' is not set it has a value of 0.00 and Koha evaluates 0.00 to true so  ecost_tax_included/ecost_tax_excluded is never used.

e.g. In this fallback:

my $cost_tax_included = $line{unitprice_tax_included} || $line{ecost_tax_included};

my $cost_tax_included = 0.00 || 7.23

$cost_tax_included is set to 0.00 as Perl treats 0.00 as true.
Comment 1 Alex Buckley 2020-06-14 23:14:11 UTC
Created attachment 105866 [details] [review]
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded

If 'Actual cost' has not been set then it has the value of 0.00 which
Perl evaluates to true so this patchset resets it to 0, so the fallback
to ecost_tax_included/ecost_tax_excluded happens.

Test plan:
1. Add item to acquisition basket (make sure the vendor has: tax rate: 15%, 'List prices: Include tax', 'Invoice prices: Include tax')
2. Set 'Vendor price' = 10 and do not set 'Actual cost'
3. Save order
4. Observe basket.pl shows 'Total tax exc.' has a value of 0.00 and GST
column has value of -8.70

5. Jump into the database:
select tax_value_on_ordering from aqorders where
ordernumber=<ordernumber>;
[You can get the ordernumber from clicking on the 'Modify' line the item
is listed in]
6. Observe a negative value: -8.70

7. Apply patch and restart plack
8. Add a second item to the basket
9. Set 'Vendor price' = 10 and don't set 'Actual cost'
10. Save order
11. Observe basket.pl shows 'Total tax exc' has value of 8.70 and GST
has value of 1.30
12. Repeat step 5 and observe tax_value_on_ordering = 1.30

Sponsored-by: Horowhenua District Council, NZ
Comment 2 Andrew Fuerste-Henry 2020-06-15 12:08:19 UTC
Created attachment 105872 [details] [review]
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded

If 'Actual cost' has not been set then it has the value of 0.00 which
Perl evaluates to true so this patchset resets it to 0, so the fallback
to ecost_tax_included/ecost_tax_excluded happens.

Test plan:
1. Add item to acquisition basket (make sure the vendor has: tax rate: 15%, 'List prices: Include tax', 'Invoice prices: Include tax')
2. Set 'Vendor price' = 10 and do not set 'Actual cost'
3. Save order
4. Observe basket.pl shows 'Total tax exc.' has a value of 0.00 and GST
column has value of -8.70

5. Jump into the database:
select tax_value_on_ordering from aqorders where
ordernumber=<ordernumber>;
[You can get the ordernumber from clicking on the 'Modify' line the item
is listed in]
6. Observe a negative value: -8.70

7. Apply patch and restart plack
8. Add a second item to the basket
9. Set 'Vendor price' = 10 and don't set 'Actual cost'
10. Save order
11. Observe basket.pl shows 'Total tax exc' has value of 8.70 and GST
has value of 1.30
12. Repeat step 5 and observe tax_value_on_ordering = 1.30

Sponsored-by: Horowhenua District Council, NZ

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 3 Jonathan Druart 2020-06-18 13:21:18 UTC
Alex, try:
  $order->{unitprice} += 0;
instead.

And add a quick test to cover the change.
Comment 4 Alex Buckley 2020-06-24 00:35:54 UTC
Created attachment 106217 [details] [review]
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded

If 'Actual cost' has not been set then it has the value of 0.00 which
Perl evaluates to true so this patchset resets it to 0, so the fallback
to ecost_tax_included/ecost_tax_excluded happens.

Test plan:
1. Add item to acquisition basket (make sure the vendor has: tax rate: 15%, 'List prices: Include tax', 'Invoice prices: Include tax')
2. Set 'Vendor price' = 10 and do not set 'Actual cost'
3. Save order
4. Observe basket.pl shows 'Total tax exc.' has a value of 0.00 and GST
column has value of -8.70

5. Jump into the database:
select tax_value_on_ordering from aqorders where
ordernumber=<ordernumber>;
[You can get the ordernumber from clicking on the 'Modify' line the item
is listed in]
6. Observe a negative value: -8.70

7. Apply patch and restart plack
8. Add a second item to the basket
9. Set 'Vendor price' = 10 and don't set 'Actual cost'
10. Save order
11. Observe basket.pl shows 'Total tax exc' has value of 8.70 and GST
has value of 1.30
12. Repeat step 5 and observe tax_value_on_ordering = 1.30
13. Run t/Prices.t unit test:
sudo koha-shell <instancename>
prove t/Prices.t

Sponsored-by: Horowhenua District Council, NZ
Comment 5 Alex Buckley 2020-06-24 00:38:16 UTC
(In reply to Jonathan Druart in comment #3):

Thanks very much Jonathan. I have made those changes :) 

Andrew if you have time could you please re-test the patch now?
Comment 6 Andrew Fuerste-Henry 2020-06-24 12:20:00 UTC
Created attachment 106244 [details] [review]
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded

If 'Actual cost' has not been set then it has the value of 0.00 which
Perl evaluates to true so this patchset resets it to 0, so the fallback
to ecost_tax_included/ecost_tax_excluded happens.

Test plan:
1. Add item to acquisition basket (make sure the vendor has: tax rate: 15%, 'List prices: Include tax', 'Invoice prices: Include tax')
2. Set 'Vendor price' = 10 and do not set 'Actual cost'
3. Save order
4. Observe basket.pl shows 'Total tax exc.' has a value of 0.00 and GST
column has value of -8.70

5. Jump into the database:
select tax_value_on_ordering from aqorders where
ordernumber=<ordernumber>;
[You can get the ordernumber from clicking on the 'Modify' line the item
is listed in]
6. Observe a negative value: -8.70

7. Apply patch and restart plack
8. Add a second item to the basket
9. Set 'Vendor price' = 10 and don't set 'Actual cost'
10. Save order
11. Observe basket.pl shows 'Total tax exc' has value of 8.70 and GST
has value of 1.30
12. Repeat step 5 and observe tax_value_on_ordering = 1.30
13. Run t/Prices.t unit test:
sudo koha-shell <instancename>
prove t/Prices.t

Sponsored-by: Horowhenua District Council, NZ

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 7 Alex Buckley 2020-06-29 13:51:56 UTC
Hey Nick, 

I see you set bug 25322 as a dependent patchset of this bug report. I was wondering did you mean to set something else instead? As Bug 25322 does not touch the Acquisitions module. 

Thanks,
Alex
Comment 8 Alex Buckley 2020-06-29 14:16:47 UTC
Thanks Nick!
Comment 9 Nick Clemens 2020-06-29 16:17:35 UTC
Created attachment 106410 [details] [review]
Bug 25750: fix fallback to ecost_tax_included/ecost_tax_excluded

If 'Actual cost' has not been set then it has the value of 0.00 which
Perl evaluates to true so this patchset resets it to 0, so the fallback
to ecost_tax_included/ecost_tax_excluded happens.

Test plan:
1. Add item to acquisition basket (make sure the vendor has: tax rate: 15%, 'List prices: Include tax', 'Invoice prices: Include tax')
2. Set 'Vendor price' = 10 and do not set 'Actual cost'
3. Save order
4. Observe basket.pl shows 'Total tax exc.' has a value of 0.00 and GST
column has value of -8.70

5. Jump into the database:
select tax_value_on_ordering from aqorders where
ordernumber=<ordernumber>;
[You can get the ordernumber from clicking on the 'Modify' line the item
is listed in]
6. Observe a negative value: -8.70

7. Apply patch and restart plack
8. Add a second item to the basket
9. Set 'Vendor price' = 10 and don't set 'Actual cost'
10. Save order
11. Observe basket.pl shows 'Total tax exc' has value of 8.70 and GST
has value of 1.30
12. Repeat step 5 and observe tax_value_on_ordering = 1.30
13. Run t/Prices.t unit test:
sudo koha-shell <instancename>
prove t/Prices.t

Sponsored-by: Horowhenua District Council, NZ

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Jonathan Druart 2020-07-09 10:05:56 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-09-03 15:44:46 UTC
backported to 20.05.x for 20.05.04
Comment 12 Aleisha Amohia 2020-09-09 04:43:18 UTC
backported to 19.11.x for 19.11.10
Comment 13 Victor Grousset/tuxayo 2020-09-20 17:37:07 UTC
Missing dependencies for 19.05.x, can't backport.