Both the table on the acq start page and the table on the funds page don't include the shipping costs. This means, that the total spent is not correct. You can easily check this by comparing with the full list shown when clicking on the spent amount from the acq start page table.
Created attachment 65446 [details] [review] Bug 18999- Modified SQL query in GetBudgetSpent() in C4/Budgets.pm Removed the SQL select condition 'AND closedate IS NOT NULL' because this was not returning shippingcost values and it does not exist in the SQL query to return the shipping cost in spent.pl Also removed the retrieval of shipping cost and the associated addition of item(s) cost and shipping cost in GetBudgetOrdered() in C4/Budgets.pm to prevent the shipping costs being subtracted off the fund total twice Test plan: 1. Go to Acquisition and create a currency, budget (make this value of 100), fund ( make this the value of 50), vendor (if neccessary) 2. Create a basket and click 'Add to basket' 3. Add 2 items with the vendor price of 10 4. Click 'Receive shipment' and write in the shipment cost of 6 5. Click 'Finish receiving' and go back to Acquisitions 6. Notice the spent column value is 0.00 but if you click on the value then the spent.pl page is displayed and shows that the shipment cost was 6.00 7. On the acquisition page also notice that the ordered column value is 26.00 8. Click on the name of the fund and notice the spent column value is 0.00 in the fund page table 9. Apply patch 10. Refresh acquisition page and notice that 6.00 is the value in the Spent column and 20.00 is the value in the ordered column. Both of which match the subtotal of the full-list tables displayed when you click on these values 11. Also notice the spent value in the fund page table is 6.00 12 Observe the changes to GetBudgetSpent() and GetBudgetOrdered() C4/Budgets.pm and check they make sense Sponsored-by: Catalyst IT
Thx Alex! I hope to be able to test soon!
Thanks Katrin that would be great :)
There is a slight change in behaviour from these patches, but after giving it some thought, I think it makes more sense this way: Before: - Receive the order, but invoice is not closed - Acq home: - shipping cost is added to 'ordered', but not shown in list (not ok) - Fund table: - shipping cost is added to 'total ordered' - Close invoice - Acq home: - shipping cost is NOT added to 'spent' and shows in list (not ok) - Fund table: - shipping cost is added to 'total spent' After: - Receive the order, but invoice is not closed - Acq home: - shipping cost is added to 'spent' and shows in list (ok) - Fund table: - shipping cost is added to 'total spent' - Close invoice - Acq home: - shipping cost is added to 'spent' and shows in list (ok) - Fund table: - shipping cost is added to 'total spent' So with the new patch the funds table and acq table always match, which is good. Also the sum on the acq home page for 'spent' and the sum on the linked list of ordered items match. There is no longer a different behaviour for open/closed invoiced. But as this was only applying to the shipping cost and not the cost for the items itself, it seems more consistent this way.
Created attachment 65626 [details] [review] [SIGNED OFF] Bug 18999- Modified SQL query in GetBudgetSpent() in C4/Budgets.pm Removed the SQL select condition 'AND closedate IS NOT NULL' because this was not returning shippingcost values and it does not exist in the SQL query to return the shipping cost in spent.pl Also removed the retrieval of shipping cost and the associated addition of item(s) cost and shipping cost in GetBudgetOrdered() in C4/Budgets.pm to prevent the shipping costs being subtracted off the fund total twice Test plan: 1. Go to Acquisition and create a currency, budget (make this value of 100), fund ( make this the value of 50), vendor (if neccessary) 2. Create a basket and click 'Add to basket' 3. Add 2 items with the vendor price of 10 4. Click 'Receive shipment' and write in the shipment cost of 6 5. Click 'Finish receiving' and go back to Acquisitions 6. Notice the spent column value is 0.00 but if you click on the value then the spent.pl page is displayed and shows that the shipment cost was 6.00 7. On the acquisition page also notice that the ordered column value is 26.00 8. Click on the name of the fund and notice the spent column value is 0.00 in the fund page table 9. Apply patch 10. Refresh acquisition page and notice that 6.00 is the value in the Spent column and 20.00 is the value in the ordered column. Both of which match the subtotal of the full-list tables displayed when you click on these values 11. Also notice the spent value in the fund page table is 6.00 12 Observe the changes to GetBudgetSpent() and GetBudgetOrdered() C4/Budgets.pm and check they make sense Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
QA might ask for a regression test here. Note: All Acquisition related tests still pass for me (Acquisitions.t, Acquisition/*, Budgets.t)
(In reply to Katrin Fischer from comment #6) > QA might ask for a regression test here. Yes :) A regression test would be really useful.
Created attachment 66378 [details] [review] Bug 18999 - Added regression test Added regression test for GetBudgetSpent() and GetBudgetOrdered() into the t/db_dependent/Budgets.t Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT
Created attachment 66855 [details] [review] Bug 18999 - Added regression test Added regression test for GetBudgetSpent() and GetBudgetOrdered() into the t/db_dependent/Budgets.t Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT Signed-off-by: Lee Jamison <ldjamison@marywood.edu> All tests pass as expected
Hi Alex, the tests you introduce: - Mask an earlier declaration of $budget - Call Koha::Acquisition::Order->new->insert is wrong (should probably be ->store)
Created attachment 67173 [details] [review] Bug 18999 - Followup based on tester feedback Removed unneccessary declaration of $budget and changed Koha::Acquisition::Order->new->insert into Koha::Acquisition::Order->new->store as requested in tester feedback Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT
Hi Tomas I have made the changes you requested in comment 10 on the new follow-up patch applied, can you please test again
Created attachment 67316 [details] [review] Bug 18282: operationId must be unique operationId has the following documentation: "Unique string used to identify the operation. The id MUST be unique among all operations described in the API." This patch modifies operationIds to be unique accross our API operations. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 67317 [details] [review] Bug 18999- Modified SQL query in GetBudgetSpent() in C4/Budgets.pm Removed the SQL select condition 'AND closedate IS NOT NULL' because this was not returning shippingcost values and it does not exist in the SQL query to return the shipping cost in spent.pl Also removed the retrieval of shipping cost and the associated addition of item(s) cost and shipping cost in GetBudgetOrdered() in C4/Budgets.pm to prevent the shipping costs being subtracted off the fund total twice Test plan: 1. Go to Acquisition and create a currency, budget (make this value of 100), fund ( make this the value of 50), vendor (if neccessary) 2. Create a basket and click 'Add to basket' 3. Add 2 items with the vendor price of 10 4. Click 'Receive shipment' and write in the shipment cost of 6 5. Click 'Finish receiving' and go back to Acquisitions 6. Notice the spent column value is 0.00 but if you click on the value then the spent.pl page is displayed and shows that the shipment cost was 6.00 7. On the acquisition page also notice that the ordered column value is 26.00 8. Click on the name of the fund and notice the spent column value is 0.00 in the fund page table 9. Apply patch 10. Refresh acquisition page and notice that 6.00 is the value in the Spent column and 20.00 is the value in the ordered column. Both of which match the subtotal of the full-list tables displayed when you click on these values 11. Also notice the spent value in the fund page table is 6.00 12 Observe the changes to GetBudgetSpent() and GetBudgetOrdered() C4/Budgets.pm and check they make sense Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 67318 [details] [review] Bug 18999 - Added regression test Added regression test for GetBudgetSpent() and GetBudgetOrdered() into the t/db_dependent/Budgets.t Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 67319 [details] [review] Bug 18999 - Followup based on tester feedback Removed unneccessary declaration of $budget and changed Koha::Acquisition::Order->new->insert into Koha::Acquisition::Order->new->store as requested in tester feedback Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 67471 [details] [review] Bug 18999: Add regression tests Added regression test for GetBudgetSpent() and GetBudgetOrdered() into the t/db_dependent/Budgets.t Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT Signed-off-by: Lee Jamison <ldjamison@marywood.edu> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67472 [details] [review] Bug 18999: (QA followup) Minor fixes for tests Removed unneccessary declaration of $budget and changed Koha::Acquisition::Order->new->insert into Koha::Acquisition::Order->new->store as requested in tester feedback Test plan: 1. Go into your koha-shell 2. set the PERL5LIB variable 3. Run t/db_dependent/Budgets.t All tests should pass Sponsored-by: Catalyst IT Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67473 [details] [review] Bug 18999: Modified SQL query in GetBudgetSpent() in C4/Budgets.pm Removed the SQL select condition 'AND closedate IS NOT NULL' because this was not returning shippingcost values and it does not exist in the SQL query to return the shipping cost in spent.pl Also removed the retrieval of shipping cost and the associated addition of item(s) cost and shipping cost in GetBudgetOrdered() in C4/Budgets.pm to prevent the shipping costs being subtracted off the fund total twice Test plan: 1. Go to Acquisition and create a currency, budget (make this value of 100), fund ( make this the value of 50), vendor (if neccessary) 2. Create a basket and click 'Add to basket' 3. Add 2 items with the vendor price of 10 4. Click 'Receive shipment' and write in the shipment cost of 6 5. Click 'Finish receiving' and go back to Acquisitions 6. Notice the spent column value is 0.00 but if you click on the value then the spent.pl page is displayed and shows that the shipment cost was 6.00 7. On the acquisition page also notice that the ordered column value is 26.00 8. Click on the name of the fund and notice the spent column value is 0.00 in the fund page table 9. Apply patch 10. Refresh acquisition page and notice that 6.00 is the value in the Spent column and 20.00 is the value in the ordered column. Both of which match the subtotal of the full-list tables displayed when you click on these values 11. Also notice the spent value in the fund page table is 6.00 12 Observe the changes to GetBudgetSpent() and GetBudgetOrdered() C4/Budgets.pm and check they make sense Sponsored-by: Catalyst IT Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 67474 [details] [review] Bug 18999: (QA followup) ModReceiveOrder expects a hashref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.06. I have added a followup to correct UT : Bug 18999: (17.05.x followup) Koha::Acquisition::Order is not an object yet
These patches have been pushed to 16.11.x and will be in 16.11.14. Thx, Alex!