Bug 15503 - Grab Item Information from Order Files
Summary: Grab Item Information from Order Files
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL: http://devs.bywatersolutions.com/proj...
Whiteboard:
Keywords:
Depends on:
Blocks: 18115 18468 18627 19296
  Show dependency treegraph
 
Reported: 2016-01-06 16:47 UTC by Nicole C. Engard
Modified: 2017-11-03 10:42 UTC (History)
13 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
The goal of this development is to automatically generate items in Koha with populated information based on a 9XX field and subfield, with the new syspref MarcItemFieldsToOrder.


Attachments
Bug 15503: Grab Item Information from Order Files (35.63 KB, patch)
2016-07-19 15:39 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Test import record (984 bytes, text/plain)
2016-08-15 14:45 UTC, Nick Clemens
Details
Test Case of 2 bibs, 1 item on one bib, 3 on the other (2.25 KB, text/plain)
2016-08-19 21:28 UTC, Benjamin Daeuber
Details
Bug 15503: Grab Item Information from Order Files (11.38 KB, patch)
2016-08-31 09:31 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (3.91 KB, patch)
2016-09-16 13:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (3.92 KB, patch)
2016-09-16 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503: Grab Item Information from Order Files (35.65 KB, patch)
2016-09-29 13:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503: Grab Item Information from Order Files (11.43 KB, patch)
2016-09-29 13:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (3.97 KB, patch)
2016-09-29 13:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [Followup] - Add replacementprice and itemcallnumber (5.60 KB, patch)
2016-10-26 09:45 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios (1.76 KB, patch)
2016-10-26 09:45 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly (1.38 KB, patch)
2016-11-03 10:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly (1.38 KB, patch)
2016-11-03 10:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Set itype and ccode properly (2.67 KB, patch)
2016-11-03 11:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Test order record with 2 items and all fields mapped (800 bytes, text/plain)
2016-11-22 18:25 UTC, Benjamin Daeuber
Details
Bug 15503: Grab Item Information from Order Files (32.64 KB, patch)
2016-12-12 09:28 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. (4.05 KB, patch)
2016-12-12 16:13 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Import screen (35.91 KB, image/png)
2016-12-13 19:55 UTC, Benjamin Daeuber
Details
Item information after import 1 (43.64 KB, image/png)
2016-12-13 19:56 UTC, Benjamin Daeuber
Details
Item information after import 2 (42.82 KB, image/png)
2016-12-13 19:57 UTC, Benjamin Daeuber
Details
Import screen (70.59 KB, image/png)
2016-12-22 10:09 UTC, Matthias Meusburger
Details
Items (36.66 KB, image/png)
2016-12-22 10:10 UTC, Matthias Meusburger
Details
Bug 15503: Grab Item Information from Order Files (32.63 KB, patch)
2017-01-17 14:17 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503: Grab Item Information from Order Files (11.43 KB, patch)
2017-01-17 16:17 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 15503: Grab Item Information from Order Files (32.63 KB, patch)
2017-01-23 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 - Fix adding multiple items in multiple biblios. (11.39 KB, patch)
2017-01-23 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (3.97 KB, patch)
2017-01-23 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [Followup] - Add replacementprice and itemcallnumber (5.60 KB, patch)
2017-01-23 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios (1.75 KB, patch)
2017-01-23 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Set itype and ccode properly (2.67 KB, patch)
2017-01-23 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. (4.05 KB, patch)
2017-01-23 14:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503 - Populate the order prices (1.22 KB, patch)
2017-01-24 16:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503: Grab Item Information from Order Files (32.75 KB, patch)
2017-02-08 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 - Fix adding multiple items in multiple biblios. (11.52 KB, patch)
2017-02-08 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (4.10 KB, patch)
2017-02-08 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 [Followup] - Add replacementprice and itemcallnumber (5.72 KB, patch)
2017-02-08 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios (1.87 KB, patch)
2017-02-08 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 [QA Followup] - Set itype and ccode properly (2.80 KB, patch)
2017-02-08 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. (4.18 KB, patch)
2017-02-08 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15503 - Populate the order prices (1.34 KB, patch)
2017-02-08 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15503: Grab Item Information from Order Files (32.79 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 - Fix adding multiple items in multiple biblios. (11.56 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (4.14 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [Followup] - Add replacementprice and itemcallnumber (5.76 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios (1.92 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Set itype and ccode properly (2.84 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. (4.22 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 - Populate the order prices (1.38 KB, patch)
2017-02-14 14:55 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15503 (QA Followup) (3.94 KB, patch)
2017-02-14 14:56 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 Nicole C. Engard 2016-01-06 16:47:14 UTC
This development will make it so that we should be able to set item data at the record *or* the item level.

Right now, MarcFieldsToOrder looks like this:
price: 947$a|947$c
quantity: 969$h
budget_code: 922$a

This development would allow for more fields such as:
item_level:
homebranch: 952$a
holdingbranch: 952$b
itype: 952$9

That way we can say “look at the items for this data.” The items are indeed broken off from the main record, but are stored and easily retrievable and can be re-integrated with the main MARC record if we need to do so. Those are details we don’t really need to discuss here though.

So, here’s just a list of things it should do:

* If quantity is not defined, use the count of items on the record as quantity

* We display item boxes within each record box for the detected fields, setting them for each item

* If the item level box has a value that isn’t set, then it falls back to the record level value, which falls back to the order level value



Seeking Co-sponsors: http://devs.bywatersolutions.com/projects/grab-item-information-from-order-files/
Comment 1 Matthias Meusburger 2016-07-19 15:39:49 UTC
Created attachment 53493 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.
Comment 2 Nicole C. Engard 2016-07-19 17:07:26 UTC
This patch needs a test plan (coming soon) and a sign off ;)
Comment 3 Matthias Meusburger 2016-07-26 08:51:17 UTC
Test plan:
Test plan:

1) Edit the MarcItemFieldsToOrder syspref. For example:
homebranch: 975$a
holdingbranch: 975$b
itype: 975$9|975$z
nonpublic_note: 975$x
public_note: 975$y
loc: 975$8
ccode: 975$c
notforloan: 975$7
uri: 975$u
copyno: 975$n
quantity: 969$h
budget_code: 922$a
price: 975$p

2) Create some sample records in koha with values in these fields, then save them as Unicode-UTF-8
Example:
922    _aBudget_code_1
922    _aBudget_code_2
969    _h3
969    _h1
975    _71
       _8loc
       _9CD
       _aHomebranch_1
       _bHoldingbranch_1
       _cccode_1
       _ncopyno
       _uuri
       _xnonpublic_note
       _ypublic_note
       _p12.5
975    _70
       _8loc2
       _aHomebranch_2
       _bHoldingbranch_2
       _cccode_2
       _ncopyno2
       _uuri2
       _xnonpublic_note2
       _ypublic_note2
       _zDVD
       _p22

3) Import your test records with the import tool (Stage MARC records for import)

4) Choose a basket, then click on "Add to basket" => "From a staged file"

5) Choose the file you have imported. For each record that had item informations, you will have a preview of the item that will be created on the left side of the screen. Values can still be modified at this point. Save.

6) Check that the items and corresponding order lines have been created.

Notes:
You will have to select a budget for each item before being able to save.
If items informations are spread across different fields (like in my example) and the number of said fields is not even, an error message will be displayed.
If the budget code or price is missing in the item informations, it will be gathered from record level information (MarcFieldsToOrder)
Comment 4 Nick Clemens 2016-08-15 14:45:27 UTC
Created attachment 54480 [details]
Test import record

Tried usingn this to test, no items added (fund codes are valid in my instance)
Comment 5 Nick Clemens 2016-08-15 14:47:13 UTC
Tested using attached record, no items were created upon staging

Fund codes are valid in my instance
Comment 6 Benjamin Daeuber 2016-08-19 21:26:27 UTC
> You will have to select a budget for each item before being able to save.

What do you mean by this? If the fund is in the file, it should auto-populate, correct?

Tested:

1. Staged a MARC file with two bibs, one with 1 item and the other with 3.
2. Added the staged file to the basket.
3. At this point everything looks good, other than the fact that I needed to manually select the fund. Price populates, I see two records, one with one item and the other with 3.
4. After saving, two bib records are created, but they both have 4 items. It was like it added all the items to both records.
Comment 7 Benjamin Daeuber 2016-08-19 21:28:36 UTC
Created attachment 54660 [details]
Test Case of 2 bibs, 1 item on one bib, 3 on the other
Comment 8 Matthias Meusburger 2016-08-25 14:31:15 UTC
> What do you mean by this? If the fund is in the file, it should auto-populate, correct?

Yes, but if the fund code in the file does not match any koha fund code, the fund will be left blank, and the user will have to select a fund before being able to submit.
Comment 9 Matthias Meusburger 2016-08-25 14:58:29 UTC
Nick, can you post the MarcItemFieldsToOrder syspref you used?
Comment 10 Nick Clemens 2016-08-25 15:28:01 UTC
(In reply to Matthias Meusburger from comment #9)
> Nick, can you post the MarcItemFieldsToOrder syspref you used?

homebranch: 975$a
holdingbranch: 975$b
itype: 975$9|975$z
nonpublic_note: 975$x
public_note: 975$y
loc: 975$8
ccode: 975$c
notforloan: 975$7
uri: 975$u
copyno: 975$n
quantity: 969$h
budget_code: 922$a
price: 975$p
Comment 11 Matthias Meusburger 2016-08-29 13:58:10 UTC
Nick, no items were created using your sample file because it does not contain any homebranch informations.

Currently, homebranch fields are used to know how many items should be added. Is there a better option?
Comment 12 Matthias Meusburger 2016-08-29 14:05:48 UTC
Benjamin, can you post your MarcItemFieldsToOrder as well, please?
Comment 13 Benjamin Daeuber 2016-08-29 16:06:32 UTC
(In reply to Matthias Meusburger from comment #12)
> Benjamin, can you post your MarcItemFieldsToOrder as well, please?

price: 975$p
quantity: 975$q
budget_code: 975$h

We just copied ours from the MarcFieldsToOrder. Note that though the fund code did exist, we needed to manually select it.
Comment 14 Matthias Meusburger 2016-08-31 09:23:45 UTC
Benjamin, you were right about the items that were added to all biblios. Here's a patch that fixes that.

However, when testing, my budgets are always selected when they exist. In your case, they are in the dropdown list, but not automatically selected, is that correct? 
You have to use the budget code in your file, and not the budget name. Are you using the budget code?
Comment 15 Matthias Meusburger 2016-08-31 09:31:29 UTC
Created attachment 55057 [details] [review]
Bug 15503: Grab Item Information from Order Files

 - Fix adding multiple items in multiple biblios.
Comment 16 Benjamin Daeuber 2016-08-31 19:26:06 UTC
(In reply to Matthias Meusburger from comment #14)
> Benjamin, you were right about the items that were added to all biblios.
> Here's a patch that fixes that.
> 
> However, when testing, my budgets are always selected when they exist. In
> your case, they are in the dropdown list, but not automatically selected, is
> that correct? 
> You have to use the budget code in your file, and not the budget name. Are
> you using the budget code?

Mathias, I believe we were using the budget code, yes. We have tested again and now it appears to be mapping properly and creating items properly. Funds are encumbered as expected. 

However, and this might be tangential, the authorized value does not appear to be updating when either creating the items or receiving them. Currently acquisitions would place a record in an Ordered NOT_LOAN status. This may be a local script, but it does not appear to be syspref. When we create an order using acquisitions in the sandbox, the items appear as available. Additionally, if we do set  AcqItemSetSubfieldsWhenReceived in the system preferences, receiving the items does not seem to update the subfields. I know this works in production, but does appear broken here.

Additionally, the how is the itype field being populated? If it's not in the record, does it simply list the top item (I notice that it doesn't actually apply the item type when the item is created, but the field is populated in the item import view).
Comment 17 Kyle M Hall 2016-09-01 14:56:41 UTC
> However, and this might be tangential, the authorized value does not appear
> to be updating when either creating the items or receiving them. Currently
> acquisitions would place a record in an Ordered NOT_LOAN status. This may be
> a local script, but it does not appear to be syspref. When we create an
> order using acquisitions in the sandbox, the items appear as available.

Just thought I'd chime in on this. Benjamin, this works because in your ACQ marc framework, you have 952$7 ( not for loan ) set to have a default value of -1, which in your NOT_LOAN authorised value list is "Ordered". So I don't think this is an issue with this patch.

> Additionally, if we do set  AcqItemSetSubfieldsWhenReceived in the system
> preferences, receiving the items does not seem to update the subfields. I
> know this works in production, but does appear broken here.

This patch doesn't really affect receiving items either. It's most likely an independent regression. It may be worth your time to create a sandbox on master with no patches applied to test this. If it still doesn't work you can file a new bug report for that!
Comment 18 Nick Clemens 2016-09-14 13:33:54 UTC
This patchset uses GetBranchesLoop which has been removed. 

Please provide followup to use Koha::Libraries
Comment 19 Kyle M Hall 2016-09-16 13:22:54 UTC
Created attachment 55616 [details] [review]
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop
Comment 20 Kyle M Hall 2016-09-16 13:25:21 UTC
Created attachment 55617 [details] [review]
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop
Comment 21 Kyle M Hall 2016-09-19 12:53:32 UTC
(In reply to Kyle M Hall from comment #20)
> Created attachment 55617 [details] [review] [review]
> Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop

Nick can you give this patch set another test? I believe my followup should take care of the qa issue!
Comment 22 Nick Clemens 2016-09-29 13:39:50 UTC
Created attachment 55926 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Nick Clemens 2016-09-29 13:40:00 UTC
Created attachment 55927 [details] [review]
Bug 15503: Grab Item Information from Order Files

- Fix adding multiple items in multiple biblios.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 24 Nick Clemens 2016-09-29 13:40:06 UTC
Created attachment 55928 [details] [review]
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 25 Nick Clemens 2016-09-29 14:20:26 UTC
Things worked well in my testing

QA concerns:
QA tool complains on valid tt 
"Item fields are uneven" error is odd in that it shows on all bibs even if only one is problematic

Ran into bug 16493 while testing, worth being aware of
Comment 26 Jonathan Druart 2016-09-30 08:19:09 UTC
What about moving code to modules and provide tests?

We can continue and add code and features to this ugly code, but really it's in nobody's interest...
Comment 27 Kyle M Hall 2016-09-30 10:58:59 UTC
Matthias, I notice this patch acts on a field 'loc' that doesn't exist. I think it was supposed to be "location", should be a trivial fix.

Also, it would be nice to have itemcallnumber supported as well if that's not asking too much.
Comment 28 Kyle M Hall 2016-09-30 11:04:13 UTC
I see the same issue with 'itemprice'. There is no itemprice column in the db, it would support the price and replacementprice fields instead!
Comment 29 Benjamin Daeuber 2016-09-30 15:53:51 UTC
(In reply to Kyle M Hall from comment #28)
> I see the same issue with 'itemprice'. There is no itemprice column in the
> db, it would support the price and replacementprice fields instead!

Just to echo what Kyle said, price and replacement price would be very helpful, as otherwise replacement price would need to be manually updated (price being what we paid, replacement price being what's charged to the customer when the item is lost or damaged).

Really mapping anything in the item record is welcome, but call number, price and replacement price are definitely data we might get from a vendor in an order file.
Comment 30 Kyle M Hall 2016-09-30 16:28:35 UTC
Maybe this is asking too much, but why use a hard coded list of item fields instead of abstracting it so that any set of arbitrary item fields could be given in MarcItemFieldsToOrder?

As I wrote earlier, it seems like typos from the spec were hard-coded into the patch ( e.g. loc vs location ). If the code were abstracted so the fields aren't hard coded, this wouldn't be an issue.
Comment 31 Kyle M Hall 2016-09-30 17:26:32 UTC
I hereby withdraw my 'loc' and 'itemprice' comments! I should have read the code more closely. It would be better if all the terms matched up, but they *do* work.

I can also understand the difficulty of making this work in an abstract way, especially for the view layer.

I would still like to see itemcallnumber implemented.
Comment 32 Matthias Meusburger 2016-10-26 09:45:31 UTC
Created attachment 56851 [details] [review]
Bug 15503 [Followup] - Add replacementprice and itemcallnumber
Comment 33 Matthias Meusburger 2016-10-26 09:45:53 UTC
Created attachment 56852 [details] [review]
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios
Comment 34 Kyle M Hall 2016-11-02 12:57:12 UTC
I think I may have discovered an issue that will need resolution. Consider this:
* Most sites have a default setting for notforloan of -1 ( ordered ) in the ACQ framework
* This patch allows the notforloan setting to be defined in the MARC order record per-item
* However, if the notforloan value is *not* passed in the text field is not populated
* Saving the records/items to the basket causes an empty string to be passed as the value
* Since the value for notforloan is not undefined, the default value for the field ( as set in the ACQ framework ) is not used
* Due to this, the items added are listed as available for loan!
Comment 35 Kyle M Hall 2016-11-02 12:59:45 UTC
In addition, shouldn't the notforloan field be a pulldown based on the authorised value, rather than a free text field?
Comment 36 Kyle M Hall 2016-11-03 10:45:32 UTC
Created attachment 57123 [details] [review]
Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly
Comment 37 Kyle M Hall 2016-11-03 10:46:29 UTC
Created attachment 57124 [details] [review]
Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly
Comment 38 Kyle M Hall 2016-11-03 11:01:28 UTC
Created attachment 57125 [details] [review]
Bug 15503 [QA Followup] - Set itype and ccode properly
Comment 39 Kyle M Hall 2016-11-16 14:00:39 UTC
Just a note, we need to make sure the aqorders_items rows are being correctly created depending on the AcqCreateItem settings ( see Bug 15531 ).
Comment 40 Benjamin Daeuber 2016-11-22 18:25:03 UTC
Created attachment 57715 [details]
Test order record with 2 items and all fields mapped

I want to provide an example of what Kyle is talking about. I will attach the marc record we used to the ticket for reference.

To summarize, the two issues:

Item type did not appear to map at all. Even on initial import (i.e the order edit screen when adding to a basket), it simply shows the our top collection alphabetically.

Second is that on initial import, non for loan appears correct, but does not seem to survive the import (all items show as available). I believe this is what Kyle is referring to.

My mapping:

homebranch: 975$a
holdingbranch: 975$b
itype: 975$c
nonpublic_note: 975$d
public_note: 975$e
loc: 975$f
ccode: 975$g
price: 975$h
replacementprice: 975$i
itemcallnumber: 975$j
quantity: 975$k
budget_code: 975$l
notforloan: 975$m
uri: 975$n
copyno: 975$o
Comment 41 Benjamin Daeuber 2016-11-22 20:10:35 UTC
Looks like collection code is also not mapping, it's appearing to on the initial import screen, but when the item is created, it it has no collection code.

This is also inconsistent with the  behavior of item type, which is mapping to the top item type alphabetically. Both collection code and item type are required by us, so it's odd to have nothing there when item type has the wrong information, but at least has information present. If a library does not map item type at all, will one be assigned?
Comment 42 Matthias Meusburger 2016-12-12 09:28:37 UTC
Created attachment 58103 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Matthias Meusburger 2016-12-12 09:29:14 UTC
(Rebased patch)
Comment 44 Matthias Meusburger 2016-12-12 16:13:49 UTC
Created attachment 58132 [details] [review]
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values.
Comment 45 Matthias Meusburger 2016-12-13 15:25:05 UTC
Benjamin,

I can successfully map ccodes and item types. 

Can you explain this sentence : "Even on initial import (i.e the order edit screen when adding to a basket), it simply shows the our top collection alphabetically."?

Notforloans are working for me (items are shown not available when needed) but I tested it with my last patch. Could you have another look at it?
Comment 46 Benjamin Daeuber 2016-12-13 19:55:37 UTC
Created attachment 58164 [details]
Import screen

I've attached a screenshot of the import screen, where I've highlighted in red the collection. That collection is the top alphabetical, even though the based on the collection in the order file, the first should be a book and the next a book club in a bag. 

It appears at this point that the collection code is correct. I've highlighted in green.

The next two are ccode_1 and ccode_2, which are the item edit screens for both of these items. They both have the top collection code alphabetically, rather than the collection code we saw at import. Again, called in in green.
Comment 47 Benjamin Daeuber 2016-12-13 19:56:17 UTC
Created attachment 58165 [details]
Item information after import 1
Comment 48 Benjamin Daeuber 2016-12-13 19:57:09 UTC
Created attachment 58166 [details]
Item information after import 2
Comment 49 Benjamin Daeuber 2016-12-13 20:45:55 UTC
(In reply to Benjamin Daeuber from comment #46)
> Created attachment 58164 [details]
> Import screen
> 
> I've attached a screenshot of the import screen, where I've highlighted in
> red the collection. That collection is the top alphabetical, even though the
> based on the collection in the order file, the first should be a book and
> the next a book club in a bag. 

By this I mean Item Type, sorry. The item type for both is Audiobook, even though the item types in the order file are different (and both valid).
Comment 50 Matthias Meusburger 2016-12-22 10:06:40 UTC
Hi,

I couldn't reproduce any of the problems you mentioned on my Unimarc instance, so I ended up setting up a MARC21 instance using your values for the syspref and your marc files, and... I couldn't reproduce any of the problems (see screen captures), except one:

BOOKBAG itype is indeed not automatically selected, but this is because there is a trailing space in the BOOKBAG 975$c in your MARC file.

I don't know if Koha should assume that a trailing space in a field is irrelevant and remove it, but the fact is that they are not technically equal. So, no pre-selection.
Comment 51 Matthias Meusburger 2016-12-22 10:09:47 UTC
Created attachment 58376 [details]
Import screen
Comment 52 Matthias Meusburger 2016-12-22 10:10:09 UTC
Created attachment 58377 [details]
Items
Comment 53 Kyle M Hall 2016-12-22 11:51:05 UTC
(In reply to Matthias Meusburger from comment #50)
> Hi,
> 
> I couldn't reproduce any of the problems you mentioned on my Unimarc
> instance, so I ended up setting up a MARC21 instance using your values for
> the syspref and your marc files, and... I couldn't reproduce any of the
> problems (see screen captures), except one:
> 
> BOOKBAG itype is indeed not automatically selected, but this is because
> there is a trailing space in the BOOKBAG 975$c in your MARC file.
> 
> I don't know if Koha should assume that a trailing space in a field is
> irrelevant and remove it, but the fact is that they are not technically
> equal. So, no pre-selection.

I would agree with that. Koha shouldn't munge the data. If there is a trailing space it should be dealt with preferably at the source. A couple other possibilities would be to use a Marc Modification Template or a plugin to clean up the records.
We shouldn't have magic side effects in Koha. Those types of things lead to confusion. The next library might rely on a trailing space and have no idea why it's not working ;)
Comment 54 Matthias Meusburger 2017-01-05 13:22:06 UTC
Benjamin, about the problems you had, as I used a fresh koha install database and faced no problem at all, do you think maybe it could have something to do with your database specifically?
Comment 55 Kyle M Hall 2017-01-06 15:46:51 UTC
Matthias, could you please rebase your patches? Thanks!
Comment 56 Matthias Meusburger 2017-01-17 14:17:16 UTC
Created attachment 59093 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Matthias Meusburger 2017-01-17 16:17:28 UTC
Created attachment 59097 [details] [review]
Bug 15503: Grab Item Information from Order Files

- Fix adding multiple items in multiple biblios.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Matthias Meusburger 2017-01-17 16:21:39 UTC
Patches rebased, but git bz apply wants to apply the patches in the wrong order (59093 and 59097 are supposed to be first and second, but they are last).

Is there a way to change that?
Comment 59 Jonathan Druart 2017-01-17 16:35:24 UTC
(In reply to Matthias Meusburger from comment #58)
> Patches rebased, but git bz apply wants to apply the patches in the wrong
> order (59093 and 59097 are supposed to be first and second, but they are
> last).
> 
> Is there a way to change that?

Apply them using the interactive mode.
Comment 60 Kyle M Hall 2017-01-23 14:04:39 UTC
Created attachment 59415 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Kyle M Hall 2017-01-23 14:04:48 UTC
Created attachment 59416 [details] [review]
Bug 15503 - Fix adding multiple items in multiple biblios.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Kyle M Hall 2017-01-23 14:04:52 UTC
Created attachment 59417 [details] [review]
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Kyle M Hall 2017-01-23 14:04:56 UTC
Created attachment 59418 [details] [review]
Bug 15503 [Followup] - Add replacementprice and itemcallnumber
Comment 64 Kyle M Hall 2017-01-23 14:05:01 UTC
Created attachment 59419 [details] [review]
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios
Comment 65 Kyle M Hall 2017-01-23 14:05:06 UTC
Created attachment 59420 [details] [review]
Bug 15503 [QA Followup] - Set itype and ccode properly
Comment 66 Kyle M Hall 2017-01-23 14:05:11 UTC
Created attachment 59421 [details] [review]
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values.
Comment 67 Kyle M Hall 2017-01-24 16:18:04 UTC
Created attachment 59515 [details] [review]
Bug 15503 - Populate the order prices
Comment 68 Benjamin Daeuber 2017-01-31 15:51:50 UTC
After doing this again with a few records, most fields appear to be mapping correctly now. The two issues are:

1. For items that have been imported, the non-public notes for each appears correctly on the bibliographic record details screen, but appear blank on the item edit screen. Additionally, updating the note doesn't appear to update the bib details screen. From what we can tell, this is only for imported records.

2. The source of acquisitions is blank. I'm fairly sure that was populating before the latest patch.
Comment 69 Kyle M Hall 2017-02-07 15:08:33 UTC
(In reply to Benjamin Daeuber from comment #68)
> 2. The source of acquisitions is blank. I'm fairly sure that was populating
> before the latest patch.

I've been looking at the code and master and cannot find any indication that Koha does this. I also tried it out ( in case I missed the relevant code ) and the source of acquisitions was not auto-populated. In any case, if this is a regression, it wasn't caused by this patch and we should file a separate bug report for it.
Comment 70 Kyle M Hall 2017-02-08 16:50:52 UTC
(In reply to Kyle M Hall from comment #69)
> (In reply to Benjamin Daeuber from comment #68)
> > 2. The source of acquisitions is blank. I'm fairly sure that was populating
> > before the latest patch.
> 
> I've been looking at the code and master and cannot find any indication that
> Koha does this. I also tried it out ( in case I missed the relevant code )
> and the source of acquisitions was not auto-populated. In any case, if this
> is a regression, it wasn't caused by this patch and we should file a
> separate bug report for it.

Just so everyone knows, we figured out what's going on and the source of acquisition field is being filled with the bookseller id when incoming EDIFACT invoices are processed.
Comment 71 Kyle M Hall 2017-02-08 17:02:38 UTC
Created attachment 60025 [details] [review]
[SIGNED-OFF] Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 72 Kyle M Hall 2017-02-08 17:02:48 UTC
Created attachment 60026 [details] [review]
[SIGNED-OFF] Bug 15503 - Fix adding multiple items in multiple biblios.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 73 Kyle M Hall 2017-02-08 17:02:53 UTC
Created attachment 60027 [details] [review]
[SIGNED-OFF] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 74 Kyle M Hall 2017-02-08 17:02:58 UTC
Created attachment 60028 [details] [review]
[SIGNED-OFF] Bug 15503 [Followup] - Add replacementprice and itemcallnumber

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 75 Kyle M Hall 2017-02-08 17:03:03 UTC
Created attachment 60029 [details] [review]
[SIGNED-OFF] Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Kyle M Hall 2017-02-08 17:03:08 UTC
Created attachment 60030 [details] [review]
[SIGNED-OFF] Bug 15503 [QA Followup] - Set itype and ccode properly

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 77 Kyle M Hall 2017-02-08 17:03:13 UTC
Created attachment 60031 [details] [review]
[SIGNED-OFF] Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values.

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 78 Kyle M Hall 2017-02-08 17:03:18 UTC
Created attachment 60032 [details] [review]
[SIGNED-OFF] Bug 15503 - Populate the order prices

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 79 Nick Clemens 2017-02-14 14:55:12 UTC
Created attachment 60203 [details] [review]
Bug 15503: Grab Item Information from Order Files

The goal of this development is to automatically generate items in Koha with
populated information based on a 9XX field and subfield, with the new syspref
MarcItemFieldsToOrder.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Nick Clemens 2017-02-14 14:55:19 UTC
Created attachment 60204 [details] [review]
Bug 15503 - Fix adding multiple items in multiple biblios.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Nick Clemens 2017-02-14 14:55:25 UTC
Created attachment 60205 [details] [review]
Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 82 Nick Clemens 2017-02-14 14:55:30 UTC
Created attachment 60206 [details] [review]
Bug 15503 [Followup] - Add replacementprice and itemcallnumber

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 83 Nick Clemens 2017-02-14 14:55:35 UTC
Created attachment 60207 [details] [review]
Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 84 Nick Clemens 2017-02-14 14:55:47 UTC
Created attachment 60208 [details] [review]
Bug 15503 [QA Followup] - Set itype and ccode properly

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 85 Nick Clemens 2017-02-14 14:55:53 UTC
Created attachment 60209 [details] [review]
Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values.

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 86 Nick Clemens 2017-02-14 14:55:59 UTC
Created attachment 60210 [details] [review]
Bug 15503 - Populate the order prices

Signed-off-by: Benjamin Daeuber <bdaeuber@cityoffargo.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 87 Nick Clemens 2017-02-14 14:56:05 UTC
Created attachment 60211 [details] [review]
Bug 15503 (QA Followup)

Fix tabs
Dereference hash for each
Comment 88 Kyle M Hall 2017-02-14 15:11:48 UTC
Pushed to master for 17.05, thanks Matts, Nick!
Comment 89 Jonathan Druart 2017-02-14 15:37:32 UTC
Seems like an independent pair of eyes was missing here...
Comment 90 Jonathan Druart 2017-02-14 15:53:27 UTC
There are obvious errors in this patch set:

$bookseller is a Koha object and is used as a hashref, several times
=> Wrong rebase

For sure this will introduce regressions.
Comment 91 Jonathan Druart 2017-02-14 15:54:23 UTC
Additionally there are too much code duplicated (rrp and ecost calculation), and new subs are added to pl files.
Comment 92 Katrin Fischer 2017-02-14 21:03:26 UTC
Revert?
Comment 93 Kyle M Hall 2017-02-15 11:22:39 UTC
(In reply to Katrin Fischer from comment #92)
> Revert?

I believe Nick is working on followups to remedy those issuses. I might take a crack at it too. In any case, we don't need to think about reversion until there is another bug to push that conflicts with this one. So if we can fix this up before then we have no need to revert. I'd much rather push forward!