Bug 35026 - Refactor addorderiso2709.pl to use object methods
Summary: Refactor addorderiso2709.pl to use object methods
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matt Blenkinsop
QA Contact: Testopia
URL:
Keywords:
Depends on: 33171 36035 36036
Blocks: 34355
  Show dependency treegraph
 
Reported: 2023-10-11 12:11 UTC by Matt Blenkinsop
Modified: 2024-04-26 16:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 35026: Add a MarcOrder object class (40.42 KB, patch)
2023-10-13 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (35.09 KB, patch)
2023-10-13 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (11.16 KB, patch)
2023-10-13 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add a MarcOrder object class (40.42 KB, patch)
2023-10-13 10:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (36.53 KB, patch)
2023-10-13 10:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (11.16 KB, patch)
2023-10-13 10:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.07 KB, patch)
2023-11-28 17:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.43 KB, patch)
2023-11-28 17:15 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add a MarcOrder object class (40.42 KB, patch)
2023-12-18 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (37.55 KB, patch)
2023-12-18 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (10.79 KB, patch)
2023-12-18 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.07 KB, patch)
2023-12-18 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.43 KB, patch)
2023-12-18 14:39 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (37.55 KB, patch)
2024-01-11 11:29 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (10.79 KB, patch)
2024-01-11 11:29 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.07 KB, patch)
2024-01-11 11:29 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.43 KB, patch)
2024-01-11 11:29 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Allow multiple records from one marc file (6.08 KB, patch)
2024-01-11 11:30 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Sample data (6.28 KB, application/xml)
2024-01-11 11:30 UTC, Matt Blenkinsop
Details
Bug 35026: Add a MarcOrder object class (40.42 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (37.55 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (10.79 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.07 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.43 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Allow multiple records from one marc file (6.08 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Adjust price field to match UI (1.00 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Fix unit tests (13.57 KB, patch)
2024-03-01 09:59 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add a MarcOrder object class (40.42 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (37.45 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Tidy file (10.39 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.07 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.43 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Allow multiple records from one marc file (6.08 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Adjust price field to match UI (1.00 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Fix unit tests (13.57 KB, patch)
2024-03-25 10:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Replacement price missing on addorderiso2709.pl (45.76 KB, image/png)
2024-03-25 14:56 UTC, Barbara Johnson
Details
Replacement price missing on basket.pl (39.26 KB, image/png)
2024-03-25 14:57 UTC, Barbara Johnson
Details
Correct replacement price imported on addorderiso2709.pl (60.27 KB, image/png)
2024-03-25 16:32 UTC, Barbara Johnson
Details
Correct replacement price imported on basket.pl (73.64 KB, image/png)
2024-03-25 16:32 UTC, Barbara Johnson
Details
Bug 35026: Add a MarcOrder object class (40.46 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Refactor addorderiso2709.pl to use the MarcOrder class (37.49 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Tidy file (10.44 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Make add_items_from_import_record DRY (47.12 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Add unit tests (21.47 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Allow multiple records from one marc file (6.13 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Adjust price field to match UI (1.05 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Fix unit tests (13.61 KB, patch)
2024-03-25 16:36 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35026: Refactor syspref mapping (5.50 KB, patch)
2024-04-11 13:35 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 35026: Refactor syspref mapping (5.50 KB, patch)
2024-04-16 13:43 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Two record order fikle (13.70 KB, text/xml)
2024-04-23 11:28 UTC, Nick Clemens (kidclamp)
Details
Bug 35026: (QA follow-up): Restore order grouping (21.09 KB, patch)
2024-04-26 16:25 UTC, Matt Blenkinsop
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Blenkinsop 2023-10-11 12:11:47 UTC
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
Comment 1 Matt Blenkinsop 2023-10-13 10:11:05 UTC
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.
Comment 2 Matt Blenkinsop 2023-10-13 10:11:07 UTC
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
Comment 3 Matt Blenkinsop 2023-10-13 10:11:09 UTC
Created attachment 157052 [details] [review]
Bug 35026: Tidy file
Comment 4 Matt Blenkinsop 2023-10-13 10:46:53 UTC
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.
Comment 5 Matt Blenkinsop 2023-10-13 10:46:55 UTC
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
Comment 6 Matt Blenkinsop 2023-10-13 10:46:57 UTC
Created attachment 157058 [details] [review]
Bug 35026: Tidy file
Comment 7 Nick Clemens (kidclamp) 2023-11-03 15:43:33 UTC
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?
Comment 8 Matt Blenkinsop 2023-11-28 17:15:24 UTC
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
Comment 9 Matt Blenkinsop 2023-11-28 17:15:26 UTC
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
Comment 10 Matt Blenkinsop 2023-12-18 14:39:22 UTC
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.
Comment 11 Matt Blenkinsop 2023-12-18 14:39:24 UTC
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
Comment 12 Matt Blenkinsop 2023-12-18 14:39:27 UTC
Created attachment 159971 [details] [review]
Bug 35026: Tidy file
Comment 13 Matt Blenkinsop 2023-12-18 14:39:29 UTC
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
Comment 14 Matt Blenkinsop 2023-12-18 14:39:31 UTC
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
Comment 15 Nick Clemens (kidclamp) 2024-01-03 13:14:26 UTC
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
Comment 16 Matt Blenkinsop 2024-01-11 11:29:52 UTC
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
Comment 17 Matt Blenkinsop 2024-01-11 11:29:54 UTC
Created attachment 160834 [details] [review]
Bug 35026: Tidy file
Comment 18 Matt Blenkinsop 2024-01-11 11:29:57 UTC
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
Comment 19 Matt Blenkinsop 2024-01-11 11:29:59 UTC
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
Comment 20 Matt Blenkinsop 2024-01-11 11:30:02 UTC
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
Comment 21 Matt Blenkinsop 2024-01-11 11:30:57 UTC
Created attachment 160838 [details]
Sample data
Comment 22 Matt Blenkinsop 2024-01-11 11:32:41 UTC
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.
Comment 23 Barbara Johnson 2024-02-01 22:36:29 UTC
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.
Comment 24 Barbara Johnson 2024-02-02 14:31:41 UTC
Forgot to set the status to Failed QA after testing the bug yesterday.
Comment 25 Matt Blenkinsop 2024-02-08 10:56:25 UTC
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
Comment 26 Matt Blenkinsop 2024-03-01 09:59:41 UTC
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.
Comment 27 Matt Blenkinsop 2024-03-01 09:59:44 UTC
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
Comment 28 Matt Blenkinsop 2024-03-01 09:59:46 UTC
Created attachment 162649 [details] [review]
Bug 35026: Tidy file
Comment 29 Matt Blenkinsop 2024-03-01 09:59:49 UTC
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
Comment 30 Matt Blenkinsop 2024-03-01 09:59:51 UTC
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
Comment 31 Matt Blenkinsop 2024-03-01 09:59:54 UTC
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
Comment 32 Matt Blenkinsop 2024-03-01 09:59:57 UTC
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
Comment 33 Matt Blenkinsop 2024-03-01 09:59:59 UTC
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
Comment 34 Barbara Johnson 2024-03-01 16:52:57 UTC
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.
Comment 35 Nick Clemens (kidclamp) 2024-03-04 12:06:28 UTC
(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?
Comment 36 Barbara Johnson 2024-03-06 23:29:03 UTC
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.
Comment 37 Matt Blenkinsop 2024-03-25 10:43:30 UTC
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.
Comment 38 Matt Blenkinsop 2024-03-25 10:43:33 UTC
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
Comment 39 Matt Blenkinsop 2024-03-25 10:43:35 UTC
Created attachment 163789 [details] [review]
Bug 35026: Tidy file
Comment 40 Matt Blenkinsop 2024-03-25 10:43:39 UTC
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
Comment 41 Matt Blenkinsop 2024-03-25 10:43:42 UTC
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
Comment 42 Matt Blenkinsop 2024-03-25 10:43:46 UTC
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
Comment 43 Matt Blenkinsop 2024-03-25 10:43:49 UTC
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
Comment 44 Matt Blenkinsop 2024-03-25 10:43:52 UTC
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
Comment 45 Matt Blenkinsop 2024-03-25 10:50:25 UTC
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 :)
Comment 46 Barbara Johnson 2024-03-25 14:55:56 UTC
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.
Comment 47 Barbara Johnson 2024-03-25 14:56:51 UTC
Created attachment 163819 [details]
Replacement price missing on addorderiso2709.pl
Comment 48 Barbara Johnson 2024-03-25 14:57:18 UTC
Created attachment 163820 [details]
Replacement price missing on basket.pl
Comment 49 Matt Blenkinsop 2024-03-25 15:45:49 UTC
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
Comment 50 Barbara Johnson 2024-03-25 16:31:39 UTC
Success! Attached two more screenshots just to show that the different replacement prices I put in came over as expected.
Comment 51 Barbara Johnson 2024-03-25 16:32:21 UTC
Created attachment 163835 [details]
Correct replacement price imported on addorderiso2709.pl
Comment 52 Barbara Johnson 2024-03-25 16:32:55 UTC
Created attachment 163836 [details]
Correct replacement price imported on basket.pl
Comment 53 PTFS Europe Sandboxes 2024-03-25 16:36:36 UTC
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>
Comment 54 PTFS Europe Sandboxes 2024-03-25 16:36:39 UTC
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>
Comment 55 PTFS Europe Sandboxes 2024-03-25 16:36:41 UTC
Created attachment 163840 [details] [review]
Bug 35026: Tidy file

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 56 PTFS Europe Sandboxes 2024-03-25 16:36:44 UTC
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>
Comment 57 PTFS Europe Sandboxes 2024-03-25 16:36:47 UTC
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>
Comment 58 PTFS Europe Sandboxes 2024-03-25 16:36:50 UTC
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>
Comment 59 PTFS Europe Sandboxes 2024-03-25 16:36:52 UTC
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>
Comment 60 PTFS Europe Sandboxes 2024-03-25 16:36:55 UTC
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>
Comment 61 Barbara Johnson 2024-03-25 16:39:50 UTC
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.
Comment 62 Matt Blenkinsop 2024-03-25 16:47:31 UTC
Thanks for signing off Barbara, the unit tests can be checked in QA :)
Comment 63 Matt Blenkinsop 2024-04-11 13:35:27 UTC
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
Comment 64 Matt Blenkinsop 2024-04-16 13:43:14 UTC
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
Comment 65 Nick Clemens (kidclamp) 2024-04-23 11:28:13 UTC
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
Comment 66 Nick Clemens (kidclamp) 2024-04-23 11:50:06 UTC
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
Comment 67 Matt Blenkinsop 2024-04-26 16:25:16 UTC
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