Bug 22802 - When ordering from a staged file, if funds are populated per item order level fund should not be required
Summary: When ordering from a staged file, if funds are populated per item order level...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 20830 22734
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-29 12:34 UTC by Nick Clemens
Modified: 2019-07-23 14:55 UTC (History)
8 users (show)

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


Attachments
Bug 22802: Check item funds, order funds, and default funds when ordering from a file (10.12 KB, patch)
2019-05-14 13:30 UTC, Nick Clemens
Details | Diff | Splinter Review
Orders to test with, using default funds (1.35 KB, application/x-wine-extension-mrc)
2019-05-14 13:32 UTC, Nick Clemens
Details
Bug 22802: Check item funds, order funds, and default funds when ordering from a file (10.15 KB, patch)
2019-05-15 17:47 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22802: Check item funds, order funds, and default funds when ordering from a file (10.20 KB, patch)
2019-05-23 14:40 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-04-29 12:34:41 UTC
To recreate:
1 - Set MarcItemFieldsToOder:
  homebranch: 975$a
  holdingbranch: 975$b
  itype: 975$y
  nonpublic_note: 975$x 
  public_note: 975$z
  loc: 975$c
  ccode: 970$c
  notforloan: 975$7
  uri: 975$u
  copyno: 975$n
  quantity: 975$q
  budget_code: 975$h
  price: 975$g
  replacementprice: 975$p
2 - Create a MARC record with a 975 like:
  =975  \\$aCPL$bCPL$g27.99$oONORDER$q1$p22.39$7-1$yBK$hBUDGET_CODE
3 - Ensure budget code, itype, and branches are valid values for your system
4 - In a basket with create items so to 'on ordering' add the file above
5 - Try to add the order
6 - Order level fund is required
Comment 1 Jonathan Druart 2019-05-04 13:45:36 UTC
This is not trivial at all and I have no idea how to fix it easily before the release. I would opt to revert bug 22734 if no fix is provided here.
Comment 2 Nick Clemens 2019-05-14 13:30:51 UTC
Created attachment 89744 [details] [review]
Bug 22802: Check item funds, order funds, and default funds when ordering from a file

This patch tries to centralise the logic of order funds requirements into a single routine.
If propogates defaults down into items/order as necessary, it also prevents unsetting lower values if
a default is set.

To test:
1 - Populate 'MarcItemFieldsToOrder'
2 - Add to a basket from a file
3 - Use a file containing some item records with funds defined or not
4 - Confirm if all items have funds defined, you can order
5 - Confirm that setting the default for an order populates items and you can order
6 - Confirm that setting the default for all orders populates items if present/ order if not
7 - Confirm that if defaults are set the lower levels cannot be unset
8 - Try various settings and confirm all work as expected
Comment 3 Nick Clemens 2019-05-14 13:32:13 UTC
Created attachment 89745 [details]
Orders to test with, using default funds
Comment 4 Liz Rea 2019-05-15 17:47:14 UTC
Created attachment 89791 [details] [review]
Bug 22802: Check item funds, order funds, and default funds when ordering from a file

This patch tries to centralise the logic of order funds requirements into a single routine.
If propogates defaults down into items/order as necessary, it also prevents unsetting lower values if
a default is set.

To test:
1 - Populate 'MarcItemFieldsToOrder'
2 - Add to a basket from a file
3 - Use a file containing some item records with funds defined or not
4 - Confirm if all items have funds defined, you can order
5 - Confirm that setting the default for an order populates items and you can order
6 - Confirm that setting the default for all orders populates items if present/ order if not
7 - Confirm that if defaults are set the lower levels cannot be unset
8 - Try various settings and confirm all work as expected

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 5 Martin Renvoize 2019-05-23 14:40:48 UTC
Created attachment 90035 [details] [review]
Bug 22802: Check item funds, order funds, and default funds when ordering from a file

This patch tries to centralise the logic of order funds requirements into a single routine.
If propogates defaults down into items/order as necessary, it also prevents unsetting lower values if
a default is set.

To test:
1 - Populate 'MarcItemFieldsToOrder'
2 - Add to a basket from a file
3 - Use a file containing some item records with funds defined or not
4 - Confirm if all items have funds defined, you can order
5 - Confirm that setting the default for an order populates items and you can order
6 - Confirm that setting the default for all orders populates items if present/ order if not
7 - Confirm that if defaults are set the lower levels cannot be unset
8 - Try various settings and confirm all work as expected

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-05-23 14:45:49 UTC
Not the cleanest implementation ever, but it does work well and is sufficiently commented to understand..

It would be nice to see some selenium tests in a followup next cycle.

PQA
Comment 7 Nick Clemens 2019-05-23 15:48:23 UTC
Awesome work all!

Pushed to master for 19.05
Comment 8 Martin Renvoize 2019-05-30 20:07:13 UTC
I'm not entirely sure if we need a port of this or not for 18.11.x series, but I don't feel confident I can deliver on in time to 18.11.06 I'm afraid.. deferring to next rmaint (sorry Frido)
Comment 9 Nick Clemens 2019-05-31 13:11:11 UTC
(In reply to Martin Renvoize from comment #8)
> I'm not entirely sure if we need a port of this or not for 18.11.x series,
> but I don't feel confident I can deliver on in time to 18.11.06 I'm afraid..
> deferring to next rmaint (sorry Frido)

The two easiest paths here I think are:
1 - Back 22743 and apply this on top - that gets things in line with master
2 - revert 20830 - this removes the problematic code that causes this