|
Description
Nicole C. Engard
2016-01-06 16:47:14 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. This patch needs a test plan (coming soon) and a sign off ;) 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)
Created attachment 54480 [details]
Test import record
Tried usingn this to test, no items added (fund codes are valid in my instance)
Tested using attached record, no items were created upon staging Fund codes are valid in my instance > 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.
Created attachment 54660 [details]
Test Case of 2 bibs, 1 item on one bib, 3 on the other
> 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.
Nick, can you post the MarcItemFieldsToOrder syspref you used? (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 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? Benjamin, can you post your MarcItemFieldsToOrder as well, please? (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. 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? Created attachment 55057 [details] [review] Bug 15503: Grab Item Information from Order Files - Fix adding multiple items in multiple biblios. (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). > 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! This patchset uses GetBranchesLoop which has been removed. Please provide followup to use Koha::Libraries Created attachment 55616 [details] [review] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop Created attachment 55617 [details] [review] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop (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! 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> 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> Created attachment 55928 [details] [review] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop Signed-off-by: Nick Clemens <nick@bywatersolutions.com> 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 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... 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. I see the same issue with 'itemprice'. There is no itemprice column in the db, it would support the price and replacementprice fields instead! (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. 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. 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. Created attachment 56851 [details] [review] Bug 15503 [Followup] - Add replacementprice and itemcallnumber Created attachment 56852 [details] [review] Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios 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! In addition, shouldn't the notforloan field be a pulldown based on the authorised value, rather than a free text field? Created attachment 57123 [details] [review] Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly Created attachment 57124 [details] [review] Bug 15503 [QA Followup] - Incoming itype doesn't set pulldown value properly Created attachment 57125 [details] [review] Bug 15503 [QA Followup] - Set itype and ccode properly Just a note, we need to make sure the aqorders_items rows are being correctly created depending on the AcqCreateItem settings ( see Bug 15531 ). 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
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? 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> (Rebased patch) Created attachment 58132 [details] [review] Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. 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? 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.
Created attachment 58165 [details]
Item information after import 1
Created attachment 58166 [details]
Item information after import 2
(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). 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. Created attachment 58376 [details]
Import screen
Created attachment 58377 [details]
Items
(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 ;) 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? Matthias, could you please rebase your patches? Thanks! 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> 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> 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? (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. 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> Created attachment 59416 [details] [review] Bug 15503 - Fix adding multiple items in multiple biblios. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 59417 [details] [review] Bug 15503 [QA Followup] - Remove the use of GetBranchesLoop Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 59418 [details] [review] Bug 15503 [Followup] - Add replacementprice and itemcallnumber Created attachment 59419 [details] [review] Bug 15503 [QA Followup] - Display "uneven number of fields" error only in relevant biblios Created attachment 59420 [details] [review] Bug 15503 [QA Followup] - Set itype and ccode properly Created attachment 59421 [details] [review] Bug 15503 [QA Followup] - Use Koha::AuthorisedValues and fetch notforloan values. Created attachment 59515 [details] [review] Bug 15503 - Populate the order prices 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. (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. (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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 60211 [details] [review] Bug 15503 (QA Followup) Fix tabs Dereference hash for each Pushed to master for 17.05, thanks Matts, Nick! Seems like an independent pair of eyes was missing here... 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. Additionally there are too much code duplicated (rrp and ecost calculation), and new subs are added to pl files. Revert? (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! |