Bug 19425 - Adding orders from order file with multiple budgets per record triggers error
Summary: Adding orders from order file with multiple budgets per record triggers error
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-10-06 13:47 UTC by Kyle M Hall
Modified: 2017-12-07 14:42 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error (2.22 KB, patch)
2017-10-06 13:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
MARC file that will trigger the error (6.81 KB, text/plain)
2017-10-06 13:55 UTC, Kyle M Hall
Details
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error (2.35 KB, patch)
2017-10-10 11:32 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19418: (bug 12833 follow-up) Add missing use statement (1.24 KB, patch)
2017-10-13 07:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Koha REST V1 (818 bytes, patch)
2017-10-13 07:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error (2.51 KB, patch)
2017-10-13 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19425: (QA Follow-up) Resolve 'push on reference experimental' warn (1.19 KB, patch)
2017-10-13 07:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2017-10-06 13:47:15 UTC
If you import an order file ( using MarcItemFieldsToOrder ) that has a different budget for each item to be ordered, you will get an error and a partially created basket. This is because Koha attempts to add the item to each order *for each budget*. This is clearly incorrect. Instead, we should be grouping items by budget and for each budget only adding those items that have a matching budget.
Comment 1 Kyle M Hall 2017-10-06 13:54:39 UTC
Created attachment 67715 [details] [review]
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error

If you import an order file ( using MarcItemFieldsToOrder ) that has a different budget for each item to be ordered, you will get an error and a partially created basket. This is because Koha attempts to add the item to each order *for each budget*. This is clearly incorrect. Instead, we should be grouping items by budget and for each budget only adding those items that have a matching budget.

Test Plan:
1) Apply this patch
2) Download the provided MARC record
3) Add the branchcode 'ALD' to your server
4) Add the ccode 'ACOL' to your server
5) Add the budget codes 'adultay' and 'branchay' to your server
6) Stage the order file
7) Create a basket, import the order file
8) No we have 3 records, 2 of them have 2 items each with different budget codes
9) Attempt to import, note the error
10) Apply this patch
11) Repeat steps 6-8, note the order completes and results in 5 order lines being added to the basket!
Comment 2 Kyle M Hall 2017-10-06 13:55:17 UTC
Created attachment 67716 [details]
MARC file that will trigger the error
Comment 3 Kyle M Hall 2017-10-10 11:32:19 UTC
Created attachment 67866 [details] [review]
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error

If you import an order file ( using MarcItemFieldsToOrder ) that has a different budget for each item to be ordered, you will get an error and a partially created basket. This is because Koha attempts to add the item to each order *for each budget*. This is clearly incorrect. Instead, we should be grouping items by budget and for each budget only adding those items that have a matching budget.

Test Plan:
1) Apply this patch
2) Download the provided MARC record
3) Add the branchcode 'ALD' to your server
4) Add the ccode 'ACOL' to your server
5) Add the budget codes 'adultay' and 'branchay' to your server
6) Stage the order file
7) Create a basket, import the order file
8) No we have 3 records, 2 of them have 2 items each with different budget codes
9) Attempt to import, note the error
10) Apply this patch
11) Repeat steps 6-8, note the order completes and results in 5 order lines being added to the basket!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Christopher Kellermeyer <ckellermeyer@altadenalibrary.org>
Comment 4 Marcel de Rooy 2017-10-13 07:23:43 UTC
Test plan contains a typo
Comment 5 Marcel de Rooy 2017-10-13 07:31:07 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2017-10-13 07:31:11 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-10-13 07:31:40 UTC
Oops
Comment 8 Marcel de Rooy 2017-10-13 07:34:20 UTC
Created attachment 68049 [details] [review]
Bug 19425 - Adding orders from order file with multiple budgets per record triggers error

If you import an order file ( using MarcItemFieldsToOrder ) that has a different budget for each item to be ordered, you will get an error and a partially created basket. This is because Koha attempts to add the item to each order *for each budget*. This is clearly incorrect. Instead, we should be grouping items by budget and for each budget only adding those items that have a matching budget.

Test plan:
1) Do not apply this patch
2) Download the provided MARC record
3) Add the branchcode 'ALD' to your server
4) Add the ccode 'ACOL' to your server
5) Add the budget codes 'adultay' and 'branchay' to your server
6) Stage the order file
7) Create a basket, import the order file
8) No we have 3 records, 2 of them have 2 items each with different budget codes
9) Attempt to import, note the error
10) Apply this patch
11) Repeat steps 6-8, note the order completes and results in 5 order lines being added to the basket!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Christopher Kellermeyer <ckellermeyer@altadenalibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
First step of test plan amended (not apply instead of apply).
Comment 9 Marcel de Rooy 2017-10-13 07:34:23 UTC
Created attachment 68050 [details] [review]
Bug 19425: (QA Follow-up) Resolve 'push on reference experimental' warn

QA tools reported for acqui/addorderiso2709.pl
   FAIL   valid: push on reference is experimental

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-10-13 07:36:09 UTC
QA Comment:
Code looks good to me. Note that the test plan is not complete. You need to know what fields goes where and how to fill the two associated preferences. Not tested, but should be fine.
Comment 11 Jonathan Druart 2017-10-13 17:01:38 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 12 Fridolin SOMERS 2017-10-25 08:35:49 UTC
Pushed to 17.05.x, will be in 17.05.06.
Comment 13 Katrin Fischer 2017-10-29 18:16:09 UTC
This doesn't apply cleanly on 16.11.x and I am not sure if it's needed. Please verify and rebase if you want this to be included.
Comment 14 Kyle M Hall 2017-12-07 14:42:09 UTC
16.11.x doesn't have the feature this bug is part of. Marking as resolved!