As part of the process to be added in bug 34355, the addorderiso2709.pl script should be refactored to make use of object methods that can then be re-used in a cron script for bug 34355
Created attachment 157050 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code.
Created attachment 157051 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 975$a holdingbranch: 975$b itype: 975$y nonpublic_note: 975$x public_note: 975$z loc: 975$c ccode: 975$8 notforloan: 975$7 uri: 975$u copyno: 975$n quantity: 975$q budget_code: 975$h price: 975$p replacementprice: 975$v Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Ensure that you have a marc file that contains mappings matching some of the fields that you entered into the two sysprefs and select this file to upload 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to an item and confirm that the data matches the mappings from the file that you imported 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 157052 [details] [review] Bug 35026: Tidy file
Created attachment 157056 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code.
Created attachment 157057 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 975$a holdingbranch: 975$b itype: 975$y nonpublic_note: 975$x public_note: 975$z loc: 975$c ccode: 975$8 notforloan: 975$7 uri: 975$u copyno: 975$n quantity: 975$q budget_code: 975$h price: 975$p replacementprice: 975$v Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Ensure that you have a marc file that contains mappings matching some of the fields that you entered into the two sysprefs and select this file to upload 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to an item and confirm that the data matches the mappings from the file that you imported 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 157058 [details] [review] Bug 35026: Tidy file
Let me start by saying thank you for taking on some work that has been needed for a ling time. I like the way this is going very much, but I do have a few concerns: I think we need some test coverage for the new module if we want to move it forward. The templates should not be passed to the object, the matcher/batch info would be better served as template plugins I think. I wonder if add_items_from_import_record could combine a little more of the code for the two agents - perhaps there could be a top level routine that calls a lower level - after getting the parameters needed for the two different agents? add_biblios_from_import_record - I don't think the plural is correct here, orders->biblios is 1:1?
Created attachment 159305 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170
Created attachment 159306 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed
Created attachment 159969 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code.
Created attachment 159970 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 975$a holdingbranch: 975$b itype: 975$y nonpublic_note: 975$x public_note: 975$z loc: 975$c ccode: 975$8 notforloan: 975$7 uri: 975$u copyno: 975$n quantity: 975$q budget_code: 975$h price: 975$p replacementprice: 975$v Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Ensure that you have a marc file that contains mappings matching some of the fields that you entered into the two sysprefs and select this file to upload 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to an item and confirm that the data matches the mappings from the file that you imported 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 159971 [details] [review] Bug 35026: Tidy file
Created attachment 159972 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170
Created attachment 159973 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed
Using the settings and test file from bug 34645 I expect to get 8 items - I only get 2. I suspect there is a problem in _get_MarcItemFieldsToOrder_syspref_data as it builds an array, but only returns a single: return $result[0]; Two routines that are used have not been imported: + GetImportBatchRangeDesc + GetNumberOfNonZ3950ImportBatches Why not use C4::Context->yaml_preference in the _get_MarcItem... subroutines? Can you provide a test plan with sample data (feel free to steal from 34645) we should test with a file with multiple records and multiple orders per record
Created attachment 160833 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 160834 [details] [review] Bug 35026: Tidy file
Created attachment 160835 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170
Created attachment 160836 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed
Created attachment 160837 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above
Created attachment 160838 [details] Sample data
Thanks for the feedback Nick, I've used the data from bug 34654 and am now seeing the items correctly populated and added to the basket. I will need to do some re-work to the cronjob side of the code but I'll do that in the background and push that to bug 34355 with the rest of that code.
Here is what I found in testing this bug: * The location code of 'AV' was not imported into the 949$c tag even though it looks correct in the sample file and the location code exists. * No data was imported into replacement price (949$v) because there was no data in the sample file. It would be good to test this field because I hear many complaints from librarians when the replacement price doesn't come through. * The itemcallnumber from the sample file was not imported into the 949$o. It was not specified to be included in MarcItemFieldsToOrder in the test plan. * The barcode# from the sample file was not imported into the 949$p. It was not specified to be included in MarcItemFieldsToOrder in the test plan. * The sample file contains three fields with the date of 2014-09-04. These are in subfields d - Date acquired and w - Price effective from. Those fields were not specified to be included in MarcItemFieldsToOrder in the test plan which is probably why they populated with today's date. That date was also in subfield r in the sample file. * There are other fields that were not contained in the sample data that I think it would be good to test. They are the 949$7 (Not for loan), 949$u (Uniform resource identifier) and 949$x (Nonpublic note) * When I saved the file and viewed it in steps 9 and 10 I got the message: No records imported No records have been imported because they all match existing records in your catalog. You'll have to treat them individually. Since that's not something I do very often I opted to not match existing records which were successfully imported. The item data looked correct to me in the other fields, the prices appeared correct and the ordered were split up amongst the three funds. Once you are ready I'll be glad to retest this one.
Forgot to set the status to Failed QA after testing the bug yesterday.
Thanks for testing Barbara, I've worked through your feedback. It looks like the new release of master has introduced some bugs into the page and all of the text inputs appear blank even though the values are being passed through. I'll need to resolve that before uploading my patches for testing
Created attachment 162647 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code.
Created attachment 162648 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 162649 [details] [review] Bug 35026: Tidy file
Created attachment 162650 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170
Created attachment 162651 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed
Created attachment 162652 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above
Created attachment 162653 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this
Created attachment 162654 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t
I tested this in a ByWater sandbox but am getting an error. I used the test plan from comment 27. In step 9 I am getting an error when saving on addorderiso2709.pl that says 'Some required item subfields are not set.' The only field that displays required next to it is budget_code which is already filled in from the xml file. I tried changing the budget_code to a different value but still got the error. I tried globally changing the budget_code via the 'Default accounting details' setting but am still getting the error message. I also noticed that the price (949$g) and replacementprice (949$v) were not populated on addorderiso2709.pl. I tried entering prices for those fields but that also did not clear the error. In case it matters, I also saw that the data in the xml file contained barcodes in the 949$p but those did not populate on addorderiso2709.pl.
(In reply to Barbara Johnson from comment #34) > I tested this in a ByWater sandbox but am getting an error. I used the test > plan from comment 27. In step 9 I am getting an error when saving on > addorderiso2709.pl that says 'Some required item subfields are not set.' The > only field that displays required next to it is budget_code which is already > filled in from the xml file. I tried changing the budget_code to a different > value but still got the error. I tried globally changing the budget_code via > the 'Default accounting details' setting but am still getting the error > message. > > I also noticed that the price (949$g) and replacementprice (949$v) were not > populated on addorderiso2709.pl. I tried entering prices for those fields > but that also did not clear the error. > > In case it matters, I also saw that the data in the xml file contained > barcodes in the 949$p but those did not populate on addorderiso2709.pl. Hi Barbara, I think you might be seeing bug 32209 - can you try setting the itemtype field in the item information tab?
Using Nick's suggestion from comment #35 solved the problem I reported in comment #34. * The location code of 'AV' successfully imported into the 949$c tag. * No data was imported into the price (949$g) field - everything is 0.00. * No data was imported into replacement price (949$v) because there was no data in the sample file. * The barcode# from the sample file was not imported into the 949$p. It was not specified to be included in MarcItemFieldsToOrder in the test plan. * The sample file contains three fields with the date of 2014-09-04. These are in subfields d - Date acquired and w - Price effective from and subfield r. Subfields d and w were populated with the current date instead of the date in the xml file. I'm not sure what subfield r stands for. On a side note, I tried to not match on the biblio because of the error I received in previous testing telling me there was a match and I would have to handle the records individually. However, even when not matching I got that error message. I had to delete the existing bib and attached items before I could bring in the order.
Created attachment 163787 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code.
Created attachment 163788 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported
Created attachment 163789 [details] [review] Bug 35026: Tidy file
Created attachment 163790 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170
Created attachment 163791 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed
Created attachment 163792 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above
Created attachment 163793 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this
Created attachment 163794 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t
Thanks again for testing Barbara, just addressing your comments below: (In reply to Barbara Johnson from comment #36) > * No data was imported into the price (949$g) field - everything is 0.00. Are you looking at the price inputs on the left or on the right under "Item record 1" etc? The boxes on the left should display 0.00 as there is no 975 field on the record. The boxes on the right are correctly displaying the prices for me. > * No data was imported into replacement price (949$v) because there was no > data in the sample file. Same as above > * The barcode# from the sample file was not imported into the 949$p. It was > not specified to be included in MarcItemFieldsToOrder in the test plan. If you add "barcode: 949$p" into the MarcItemFieldsToOrder it should import correctly > * The sample file contains three fields with the date of 2014-09-04. These > are in subfields d - Date acquired and w - Price effective from and subfield > r. Subfields d and w were populated with the current date instead of the > date in the xml file. I'm not sure what subfield r stands for. None of these fields are included in the import scope of MarcItemFieldsToOrder so the values should not transfer from the marc file > On a side note, I tried to not match on the biblio because of the error I > received in previous testing telling me there was a match and I would have > to handle the records individually. However, even when not matching I got > that error message. I had to delete the existing bib and attached items > before I could bring in the order. This is expected behaviour due to the matching record. There is a dropdown menu at the top of the page underneath the "Item information" tab. If you change this to "Do not look for matching records" you can bypass this behaviour and you will see the basket populated :)
I retested in a sandbox. The replacement price is still coming over as 0.00 for me on the right hand side on addorderiso2709.pl. It's also 0.00 on basket.pl. I attached screenshots so you can see what I am seeing.
Created attachment 163819 [details] Replacement price missing on addorderiso2709.pl
Created attachment 163820 [details] Replacement price missing on basket.pl
Looking at the file I attached to this bug there are no replacement price fields in the marc file (949$v). If you add a 949$v field to the file then you should see whatever price you added being imported correctly
Success! Attached two more screenshots just to show that the different replacement prices I put in came over as expected.
Created attachment 163835 [details] Correct replacement price imported on addorderiso2709.pl
Created attachment 163836 [details] Correct replacement price imported on basket.pl
Created attachment 163838 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code. Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163839 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163840 [details] [review] Bug 35026: Tidy file Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163841 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170 Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163842 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163843 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163844 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 163845 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
I signed off on all patches. I realize now that one of them was to 'prove t/db_dependent/Koha/MarcOrder.t' which I don't think I can do in a sandbox? And, if I can, I don't know how.
Thanks for signing off Barbara, the unit tests can be checked in QA :)
Created attachment 164695 [details] [review] Bug 35026: Refactor syspref mapping This patch refactors the way we read MarcFieldsToOrder and MarcItemFieldsToOrder to only use one method
Created attachment 164937 [details] [review] Bug 35026: Refactor syspref mapping This patch refactors the way we read MarcFieldsToOrder and MarcItemFieldsToOrder to only use one method
Created attachment 165378 [details] Two record order fikle This is a test file to use all the fields listed on MarcItemFieldsToOrder - two records - multiple orders
I tested with the newly attached file and sysprefs set as below. I had to set the barcodes individually while importing (to make them unique, quantity and barcode are not compatible) Before: I had 3 orderlines, grouped by fund Item barcodes were filled Replacement price was correct for items, but order used the last one from multiple items sort 1 and 2 were correctly populated After: There was 1 orderline per item, 8 total, no grouping Item barcodes did not come through Replacement price for order and item were correct Sort 1 and 2 were not populated I believe all other fields were the same before and after, can you test and confirm and fix the sort fields and barcode? I am not sure what is correct for the grouping of orders. MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g|949$j replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l barcode: 949$p coded_location_qualifier: 949$f enumchron: 949$h MarcFieldsToOrder price: 949$g quantity: 949$k budget_code: 949$l discount: 949$m sort1: 949$n sort2: 949$q Testing with the same file, but empyting MarcItemFieldsToOrder and only using MarcFieldsToOrder I get two order records and 4 items before these patches (the second 949 seems to be ignored) but after applying these patches the records import but I had no orders or items
Created attachment 165681 [details] [review] Bug 35026: (QA follow-up): Restore order grouping This patch restores the grouping of orderlines by fund rather than one line per item. It also re-enables the reading of the barcoe, enumchron and coded_location_qualifier fields as well as passing the sort1 and sort2 values to the orderline. It also reintroduces some missed functionality for when MarcItemFieldsToOrder is not passed through and can now create order lines and records for an imported file when this is the case
Created attachment 168123 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code. Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168124 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168125 [details] [review] Bug 35026: Tidy file Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168126 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170 Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168127 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168128 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168129 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168130 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168131 [details] [review] Bug 35026: Refactor syspref mapping This patch refactors the way we read MarcFieldsToOrder and MarcItemFieldsToOrder to only use one method
Created attachment 168132 [details] [review] Bug 35026: (QA follow-up): Restore order grouping This patch restores the grouping of orderlines by fund rather than one line per item. It also re-enables the reading of the barcoe, enumchron and coded_location_qualifier fields as well as passing the sort1 and sort2 values to the orderline. It also reintroduces some missed functionality for when MarcItemFieldsToOrder is not passed through and can now create order lines and records for an imported file when this is the case
This all looks good, my testing all succeeded, two qa problems: test failing: t/db_dependent/Koha/MarcOrder.t QA tools tidy error: WARN Koha/MarcOrder.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 0, now: 15)
Created attachment 168971 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code. Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168972 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168973 [details] [review] Bug 35026: Tidy file Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168974 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170 Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168975 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168976 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168977 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168978 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Created attachment 168979 [details] [review] Bug 35026: Refactor syspref mapping This patch refactors the way we read MarcFieldsToOrder and MarcItemFieldsToOrder to only use one method
Created attachment 168980 [details] [review] Bug 35026: (QA follow-up): Restore order grouping This patch restores the grouping of orderlines by fund rather than one line per item. It also re-enables the reading of the barcoe, enumchron and coded_location_qualifier fields as well as passing the sort1 and sort2 values to the orderline. It also reintroduces some missed functionality for when MarcItemFieldsToOrder is not passed through and can now create order lines and records for an imported file when this is the case
Created attachment 168981 [details] [review] Bug 35026: (QA follow-up): Fix QA tests
Created attachment 169475 [details] [review] Bug 35026: Add a MarcOrder object class This patch adds a MarcOrder object class. This will primarily be used for the functionality in bug 34355 to automate order creation from a Marc file. The automation functionality is similar in principle to the functionality in the addorderis02709.pl script so it makes sense to refactor this script to use the same code. Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169476 [details] [review] Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class This script uses the newly created MarcOrder class to refactor the script into a cleaner controller function. This MarcOrder class will also be used in bug 34355 to automate the creation of order lines from marc records. Test plan: 1) In system preferences, click Search and then select the Acquisitions option from the left hand menu 2) Paste the following into MarcFieldsToOrder price: 975$p quantity: 975$q budget_code: 975$h 3) Paste the following into MarcItemFieldsToOrder homebranch: 949$a holdingbranch: 949$b itype: 949$y nonpublic_note: 949$x public_note: 949$z loc: 949$c ccode: 949$8 notforloan: 949$7 uri: 949$u copyno: 949$t price: 949$g replacementprice: 949$v itemcallnumber: 949$o quantity: 949$k budget_code: 949$l Now save the sysprefs 4) Navigate to acquisitions and go into a basket 5) Click Add to basket and select “From a new file” 6) Download the file attached to this bug 7) Import the file and when the job is complete click “Add staged files to basket” 8) Click the checkbox next to the record to display the items and confirm that the data matches the mappings from the file that you imported - there should be 8 items, grouped in 4 pairs of identical items 9) Fill in the required fields and click “Save” 10) That basket should be correctly populated with an order matching the file that you imported Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169477 [details] [review] Bug 35026: Tidy file Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169478 [details] [review] Bug 35026: Make add_items_from_import_record DRY This patch improves the code in add_items_from_import_record to make sure that the code is not repeating itself It also fixes a minor error in the template file and rebases in changes from bugs 35254 and 33170 Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169479 [details] [review] Bug 35026: Add unit tests This commit adds a set of unit tests for the new methods in Koha::MarcOrder The two controller functions at the start of the file have not been included as they simply call the functions in these tests The _stage_file method is also not included as this is a copy of the existing file staging code with the background job functionality removed Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169480 [details] [review] Bug 35026: Allow multiple records from one marc file This patch addresses a bug where only one record would be read from a marc file and other records would be lost. All records are now read from the file and displayed in the UI. Test plan as above Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169481 [details] [review] Bug 35026: Adjust price field to match UI Following bug 36036 the price field has been renamed in the UI- this patch removes the logic that was renaming that field in the backend to match this Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169482 [details] [review] Bug 35026: Fix unit tests This patch fixes the unit tests that were failing and also removes some tests that should not be introduced until bug 34355 Test plan: prove t/db_dependent/Koha/MarcOrder.t Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169483 [details] [review] Bug 35026: Refactor syspref mapping This patch refactors the way we read MarcFieldsToOrder and MarcItemFieldsToOrder to only use one method Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169484 [details] [review] Bug 35026: (QA follow-up): Restore order grouping This patch restores the grouping of orderlines by fund rather than one line per item. It also re-enables the reading of the barcoe, enumchron and coded_location_qualifier fields as well as passing the sort1 and sort2 values to the orderline. It also reintroduces some missed functionality for when MarcItemFieldsToOrder is not passed through and can now create order lines and records for an imported file when this is the case Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 169485 [details] [review] Bug 35026: (QA follow-up): Fix QA tests Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Patches still apply cleanly, QA script is green. So far so good! This is quite a big piece of code and as such hard to test. I think it might have been nice to move the code in smaller bits. But overall this is of course quite an improvement for the unit tests alone. Trusting Nick to have done a thorough review here considering him our current expert for this specific piece of code. Some questions remain: 1) Koha/MarcOrder.pm I was wondering if that is the right spot for the new module and if it should not be within Acquisition or Import? It feels a bit like it's missing context or being something in the middle. 2) Beware terminology! marc record based on the mappings in the syspref MARC record based on the mappings in the system preference... biblio records bibliographic records Some more marc vs. MARC: Couldn't translate marc information 3) _format_price_to_CurrencyFormat_syspref This one feels a bit risky. As it's now it assumes that the chosen display format also governs the format of data that a library might want to import. It doesn't check where the comma is in the string, but blindly replaces. I can see it's done multiple times in the original code, so we will keep the behavior consistent. I'd still question if it's in the right spot being in the new MarcOrder module and if it should not be somewhat refined. Maybe a case for a follow-up bug for later?
Pushed for 24.11! Well done everyone, thank you!
We have a failing test, please fix urgently: t_db_dependent_Koha_MarcOrder_t.No tests run for subtest "add_items_from_import_record() - addorderiso2709.pl" (367)
Created attachment 170504 [details] [review] Bug 35026: (follow-up) Create data for tests and do not rely on state of DB
Thanks Nick!
I wonder if we could make the bug's description a bit more "release note friendly" or we move it to "Plumbing"? And thanks for the quick follow-up! Pushed to main now.
I think plumbing maybe makes more sense, this is basically entirely refactoring with no change to the UI or to the functionality
Switched category to "Architecture, internals and plumbing".
Great work done here, however I won't backport to 24.05.x.