Bug 34788

Summary: Add the ability to import KBART files to ERM
Product: Koha Reporter: Matt Blenkinsop <matt.blenkinsop>
Component: ERMAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: Pushed to main --- QA Contact:
Severity: new feature    
Priority: P5 - low CC: edith.speller, jeremy.evans, jonathan.druart, jonathan.field, koha, lucas, martin.renvoize, mathsabypro, matt.blenkinsop, maude.boudreau, michaela.sieber, nick, pedro.amorim
Version: MainKeywords: rel_24_05_candidate
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Bug Depends on: 34789, 36618, 34468    
Bug Blocks: 36831    
Attachments: Bug 34788: Add an API endpoint to import a KBART file
Bug 34788: Add Vue components, api client and navigation route
Bug 34788: Add a background job to import a KBART file
Bug 34788: Add unit test
Bug 34788: Add a relationship to a package for the imported file
Bug 34788: Allow for import of CSV files
Bug 34788: Allow for import of CSV files
Example TSV file
Example CSV file (contains the 9 titles from the TSV file)
Bug 34788: Add an API endpoint to import a KBART file
Bug 34788: Add Vue components, api client and navigation route
Bug 34788: Add a background job to import a KBART file
Bug 34788: Add unit test
Bug 34788: Add a relationship to a package for the imported file
Bug 34788: Allow for import of CSV files
Bug 34788: Make biblio creation optional
Bug 34788: Allow import of file with additional columns
Bug 34788: Add an API endpoint to import a KBART file
Bug 34788: Add Vue components, api client and navigation route
Bug 34788: Add a background job to import a KBART file
Bug 34788: Add unit test
Bug 34788: Add a relationship to a package for the imported file
Bug 34788: Allow for import of CSV files
Bug 34788: Make biblio creation optional
Bug 34788: Allow import of file with additional columns
Bug 34788: (QA follow-up)
Bug 34788: Add an API endpoint to import a KBART file
Bug 34788: Add Vue components, api client and navigation route
Bug 34788: Add a background job to import a KBART file
Bug 34788: Add unit test
Bug 34788: Add a relationship to a package for the imported file
Bug 34788: Allow for import of CSV files
Bug 34788: Make biblio creation optional
Bug 34788: Allow import of file with additional columns
Bug 34788: (QA follow-up)
Bug 34788: (follow-up): Fix translations

Description Matt Blenkinsop 2023-09-14 12:10:31 UTC
We are developing the ability to import local titles directly into ERM from a KBART file

Only KBART Phase II files will be supported
See: https://groups.niso.org/higherlogic/ws/public/download/16900/RP-9-2014_KBART.pdf
Comment 1 Koha Team University Lyon 3 2024-01-22 12:38:47 UTC
Hi,
do you have a schedule to work on this one?
We (several french users from KohaLa) are very interested.

Thanks :)
Sonia
Comment 2 Matt Blenkinsop 2024-01-22 14:02:29 UTC
Hi Sonia,

There is a solution ready which is in testing with one of our customers. When I get a chance I'll upload it to the bug for you to test as well. The more feedback the better!
Comment 3 Matt Blenkinsop 2024-01-25 09:41:45 UTC
Created attachment 161379 [details] [review]
Bug 34788: Add an API endpoint to import a KBART file

This patch adds the endpoint needed to queue an import_from_kbart_file background job
Comment 4 Matt Blenkinsop 2024-01-25 09:41:47 UTC
Created attachment 161380 [details] [review]
Bug 34788: Add Vue components, api client and navigation route

This patch adds a new component to handle the file import, a route to that component and the API client route needed to access the API
Comment 5 Matt Blenkinsop 2024-01-25 09:41:49 UTC
Created attachment 161381 [details] [review]
Bug 34788: Add a background job to import a KBART file

This patch adds a background job that will import a KBART file

Sponsored-by: UKHSA
Comment 6 Matt Blenkinsop 2024-01-25 09:41:52 UTC
Created attachment 161382 [details] [review]
Bug 34788: Add unit test
Comment 7 Matt Blenkinsop 2024-01-25 09:41:55 UTC
Created attachment 161383 [details] [review]
Bug 34788: Add a relationship to a package for the imported file
Comment 8 Matt Blenkinsop 2024-01-25 09:41:58 UTC
Created attachment 161384 [details] [review]
Bug 34788: Allow for import of CSV files

This commit allows CSV files to be imported alongside TSV files.
It also adds some performance improvements relating to the max_allowed_packet and the matching of titles, as well as some small bugfixes and unit test changes

Test plan:
1) Enable the ERM module
2) Navigate to E-resource management > eHoldings > Local > Packages
3) Create at least one package
4) Navigate to E-resource management > eHoldings > Local > Titles
5) There should be a button for "Import from KBART file"
6) Click this button
7) Select the package that you created from the dropdown and then choose your KBART file using the "Choose file" button
8) Click Submit
9) If your file is a valid file, a background job will be queued, if not then a warning will display showing what is incorrect in your file
10) To test the file format warning, edit your file and add a random column heading into the file e.g. test_column. When you upload it, the warning should show that an invalid column "test_column" has been detected
11) Click on the background job. (If you have uploaded a very large file, the system will chunk the file into smaller pieces and create multiple background jobs)
12) It should display a progress bar followed by a report and any error messages
13) Navigate to E-resource management > eHoldings > Local > Titles and you should see your new titles.
14) Run the unit test: prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Comment 9 Matt Blenkinsop 2024-01-25 10:58:16 UTC
Created attachment 161391 [details] [review]
Bug 34788: Allow for import of CSV files

This commit allows CSV files to be imported alongside TSV files.
It also adds some performance improvements relating to the max_allowed_packet and the matching of titles, as well as some small bugfixes and unit test changes

Test plan:
1) Enable the ERM module
2) Navigate to E-resource management > eHoldings > Local > Packages
3) Create at least one package
4) Navigate to E-resource management > eHoldings > Local > Titles
5) There should be a button for "Import from KBART file"
6) Click this button
7) Select the package that you created from the dropdown and then choose your KBART file using the "Choose file" button
8) Click Submit
9) If your file is a valid file, a background job will be queued, if not then a warning will display showing what is incorrect in your file
10) To test the file format warning, edit your file and add a random column heading into the file e.g. test_column. When you upload it, the warning should show that an invalid column "test_column" has been detected
11) Click on the background job. (If you have uploaded a very large file, the system will chunk the file into smaller pieces and create multiple background jobs)
12) It should display a progress bar followed by a report and any error messages
13) Navigate to E-resource management > eHoldings > Local > Titles and you should see your new titles.
14) Run the unit test: prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Comment 10 Lucas Gass 2024-01-25 23:28:33 UTC
Hi Matt,

I am not terribly familiar with KBART files. I am attempting to test this and getting a "Something went wrong: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data"

I'm sure this is my file. Do you have a file we can use to test with? 

Other minor things of note: 

 FAIL	koha-tmpl/intranet-tmpl/prog/en/includes/background_jobs/import_from_kbart_file.inc
   FAIL	  valid_template
		: filter not found


 FAIL	t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 84)
		forbidden pattern: tab char (line 85)
		forbidden pattern: tab char (line 86)
		forbidden pattern: tab char (line 124)
		forbidden pattern: tab char (line 125)
		forbidden pattern: tab char (line 126)
		forbidden pattern: tab char (line 212)
		forbidden pattern: tab char (line 213)
		forbidden pattern: tab char (line 214)
Comment 11 Jonathan Field 2024-01-29 09:37:47 UTC
Hi Lucas, 
Springer provide their KBART files in the public domain. You could take a few of these to use as sample files. I'll leave Matt to answer the other point!

https://adminportal.springernature.com/metadata/kbart
Thanks
Jonathan
Comment 12 Koha Team University Lyon 3 2024-03-19 10:41:19 UTC
Hello,
it's also possible to export Kbart file from BACON : 
https://bacon.abes.fr/exporter.html

Only in french I'm afraid.

Sonia
Comment 13 Mathieu Saby 2024-03-19 16:15:17 UTC
For your information, the KBART files used in France and available from https://bacon.abes.fr/exporter.html have an extra field "ppn" after the mandatory KBART fields (it is permitted, as per the specification :
"In order to supply additional information for other purposes not required by the KBART Recommended Practice, content providers are permitted to include any extra data fields after the last KBART utilized position (access_type in position 25)." 

An other import source is https://gokb.org/ , whitch adds 3 fields : zdb_id, 
gokb_tipp_uid, gokb_title_uid

It could be interesting to test how Koha behaves with those kind of "enhanced" Kbart Files
Comment 14 Mathieu Saby 2024-03-26 14:15:53 UTC
I cannot activate the ERMModule preference ("Error") in sandbox. It does not seem related with this bug, but I cannot test it.
Comment 15 Matt Blenkinsop 2024-04-19 13:34:25 UTC
Created attachment 165189 [details]
Example TSV file
Comment 16 Matt Blenkinsop 2024-04-19 13:34:57 UTC
Created attachment 165190 [details]
Example CSV file (contains the 9 titles from the TSV file)
Comment 17 Matt Blenkinsop 2024-04-19 13:36:04 UTC
Created attachment 165191 [details] [review]
Bug 34788: Add an API endpoint to import a KBART file

This patch adds the endpoint needed to queue an import_from_kbart_file background job
Comment 18 Matt Blenkinsop 2024-04-19 13:36:07 UTC
Created attachment 165192 [details] [review]
Bug 34788: Add Vue components, api client and navigation route

This patch adds a new component to handle the file import, a route to that component and the API client route needed to access the API
Comment 19 Matt Blenkinsop 2024-04-19 13:36:09 UTC
Created attachment 165193 [details] [review]
Bug 34788: Add a background job to import a KBART file

This patch adds a background job that will import a KBART file

Sponsored-by: UKHSA
Comment 20 Matt Blenkinsop 2024-04-19 13:36:12 UTC
Created attachment 165194 [details] [review]
Bug 34788: Add unit test
Comment 21 Matt Blenkinsop 2024-04-19 13:36:15 UTC
Created attachment 165195 [details] [review]
Bug 34788: Add a relationship to a package for the imported file
Comment 22 Matt Blenkinsop 2024-04-19 13:36:18 UTC
Created attachment 165196 [details] [review]
Bug 34788: Allow for import of CSV files

This commit allows CSV files to be imported alongside TSV files.
It also adds some performance improvements relating to the max_allowed_packet and the matching of titles, as well as some small bugfixes and unit test changes

Test plan:
1) Enable the ERM module
2) Navigate to E-resource management > eHoldings > Local > Packages
3) Create at least one package
4) Navigate to E-resource management > eHoldings > Local > Titles
5) There should be a button for "Import from KBART file"
6) Click this button
7) Select the package that you created from the dropdown and then choose your KBART file using the "Choose file" button. I have attached some example files to the bug but feel free to use your own if you have them.
8) Click Submit
9) If your file is a valid file, a background job will be queued, if not then a warning will display showing what is incorrect in your file
10) To test the file format warning, edit your file and add a random column heading into the file e.g. test_column. When you upload it, the warning should show that an invalid column "test_column" has been detected
11) Click on the background job. (If you have uploaded a very large file, the system will chunk the file into smaller pieces and create multiple background jobs)
12) It should display a progress bar followed by a report and any error messages
13) Navigate to E-resource management > eHoldings > Local > Titles and you should see your new titles.
14) Run the unit test: prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Comment 23 Matt Blenkinsop 2024-04-19 13:36:20 UTC
Created attachment 165197 [details] [review]
Bug 34788: Make biblio creation optional

This patch rebases in the changes from bug 36618 to make biblio creation optional
Comment 24 Matt Blenkinsop 2024-04-19 13:38:20 UTC
(In reply to Mathieu Saby from comment #13)
> For your information, the KBART files used in France and available from
> https://bacon.abes.fr/exporter.html have an extra field "ppn" after the
> mandatory KBART fields (it is permitted, as per the specification :
> "In order to supply additional information for other purposes not required
> by the KBART Recommended Practice, content providers are permitted to
> include any extra data fields after the last KBART utilized position
> (access_type in position 25)." 
> 
> An other import source is https://gokb.org/ , whitch adds 3 fields : zdb_id, 
> gokb_tipp_uid, gokb_title_uid
> 
> It could be interesting to test how Koha behaves with those kind of
> "enhanced" Kbart Files

Hi, for these file the system will display a warning to say that those columns are not compatible with the database structure and ask the user to remove them from the document. Local titles are structured in line with the standard KBART fields and so any additional fields are not currently stored as there is no valid database column to store them in
Comment 25 Mathieu Saby 2024-04-26 13:02:31 UTC
Thanks you for the explanation.
By "warning" do you mean that only the standard kbart columns are imported (if there are other columns), or that no columns at all are imported ?
Comment 26 Matt Blenkinsop 2024-04-26 13:04:48 UTC
It will return the warning before the job is queued so no columns will be imported at that stage. Once the file is edited to remove the columns it can then be re-imported and the job will be queued
Comment 27 Mathieu Saby 2024-04-26 13:13:48 UTC
Well, I am not your sponsor but as an average librarian I can say that it not the most effective workflow : when we librarians download valid kbart files from gokb or other sources, we usually wont want to edit the file to remove columns that are not defined in kbart specs. If all mandatory fields are in the file, won't it be possible to import the file with a "simple" warning telling the user that the other (non mandatory) fields were discarded?
Comment 28 Martin Renvoize 2024-04-30 08:30:08 UTC
I think I like that suggestion, warn that those fields will be dropped, but continue with the import.

We could later add a general 'other fields' catch somewhere to allow importing those too, but I'm not sure how they'd actually ever get surfaced/used.. they could be anything so it'll be difficult to use them in standard reports.
Comment 29 Katrin Fischer 2024-04-30 08:38:59 UTC
(In reply to Martin Renvoize from comment #28)
> I think I like that suggestion, warn that those fields will be dropped, but
> continue with the import.
> 
> We could later add a general 'other fields' catch somewhere to allow
> importing those too, but I'm not sure how they'd actually ever get
> surfaced/used.. they could be anything so it'll be difficult to use them in
> standard reports.

I think it would require some mapping, but could be potentially be very interesting for matching catalog records to ERM titles later on. ZDB-ID = national serial database identifier (that's one of ours) and ppn = PICA production number, I assume ABES union catalog record ID (?).

Totally a separate bug of course :)
Comment 30 Katrin Fischer 2024-04-30 08:39:32 UTC
(In reply to Mathieu Saby from comment #27)
> Well, I am not your sponsor but as an average librarian I can say that it
> not the most effective workflow : when we librarians download valid kbart
> files from gokb or other sources, we usually wont want to edit the file to
> remove columns that are not defined in kbart specs. If all mandatory fields
> are in the file, won't it be possible to import the file with a "simple"
> warning telling the user that the other (non mandatory) fields were
> discarded?

+10
Comment 31 Jonathan Field 2024-04-30 08:49:14 UTC
(In reply to Mathieu Saby from comment #27)
> Well, I am not your sponsor but as an average librarian I can say that it
> not the most effective workflow : when we librarians download valid kbart
> files from gokb or other sources, we usually wont want to edit the file to
> remove columns that are not defined in kbart specs. If all mandatory fields
> are in the file, won't it be possible to import the file with a "simple"
> warning telling the user that the other (non mandatory) fields were
> discarded?

So, just so we understand this, is this because gokb actually don't follow the NISO standard, they add "proprietary" fields, is that correct? https://gokb.org/documentation/kbart-introduction.html#proprietary-gokb-fields. The majority of sample files we've been given or used are in NISO format and therefore load without problem. Are there other vendors who are also adding additional fields to the KBART NISO format?
Comment 32 Katrin Fischer 2024-04-30 09:02:17 UTC
I believe adding custom fields at the end is actually according to the standard. I found this in a quick search:

https://www.niso.org/standards-committees/kbart/kbart-content-providers
Links to Style Guide:
https://docs.google.com/document/d/1jF154euh8s_RWKX0Tb0ci1W_gzziYKd5ayy5SrjyVEg/edit#heading=h.2esyo4reee80

On last page:

Extra Fields
This data field is applicable to serials and monographs.
If it is desired to transmit additional identifiers, they may be included as extra data in fields after the last KBART utilized position, (access_type in position 25).
Multiple values should be delimited using a semi-colon.

As we can't do anything with those right now, but it might be a lot of work for libraries (and error prone) to manually edit the files, I think Mathieu's suggestion would be perfect for now. Make them aware we are not importing these, while still allowing to continue.
Comment 33 Jonathan Field 2024-04-30 09:15:56 UTC
(In reply to Katrin Fischer from comment #32)
> I believe adding custom fields at the end is actually according to the
> standard. I found this in a quick search:
> 
> https://www.niso.org/standards-committees/kbart/kbart-content-providers
> Links to Style Guide:
> https://docs.google.com/document/d/
> 1jF154euh8s_RWKX0Tb0ci1W_gzziYKd5ayy5SrjyVEg/edit#heading=h.2esyo4reee80
> 
> On last page:
> 
> Extra Fields
> This data field is applicable to serials and monographs.
> If it is desired to transmit additional identifiers, they may be included as
> extra data in fields after the last KBART utilized position, (access_type in
> position 25).
> Multiple values should be delimited using a semi-colon.
> 
> As we can't do anything with those right now, but it might be a lot of work
> for libraries (and error prone) to manually edit the files, I think
> Mathieu's suggestion would be perfect for now. Make them aware we are not
> importing these, while still allowing to continue.

Thanks, we'll look at that.
Comment 34 Matt Blenkinsop 2024-04-30 10:29:37 UTC
Created attachment 165837 [details] [review]
Bug 34788: Allow import of file with additional columns

This patch allows a file with additional columns to be imported. When the file is submitted, the system will enqueue the background job and send back information to the UI that certain columns have been ignored. The data will stil l import as normal but only the standard KBART columns will be parsed and imported
Comment 35 Katrin Fischer 2024-04-30 10:52:17 UTC
I've not given up on this one yet, if you can make me believe it's unlikely to have bad side effects :D
Comment 36 Michaela Sieber 2024-04-30 12:59:22 UTC
There is a Checkbox 'Create linked biblio record' 

Althought I didn't activate this checkbox biblio records were created.

But I think it should be optional if you want to create biblio records, this should not be the default because KBART metadata is not enough for displaying in the OPAC !
Comment 37 Matt Blenkinsop 2024-04-30 13:21:14 UTC
Hi Michaela, sorry, I forgot to link a dependency to another bug. If you reapply the bug it should now skip biblio creation.

The logic behind the default behaviour being to not create linked biblios was that it will be a lot easier to create biblios retrospectively than to revert them and delete all the linked records if you didn't want them
Comment 38 Clemens Tubach 2024-04-30 13:50:05 UTC
Created attachment 165887 [details] [review]
Bug 34788: Add an API endpoint to import a KBART file

This patch adds the endpoint needed to queue an import_from_kbart_file background job

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 39 Clemens Tubach 2024-04-30 13:50:08 UTC
Created attachment 165888 [details] [review]
Bug 34788: Add Vue components, api client and navigation route

This patch adds a new component to handle the file import, a route to that component and the API client route needed to access the API

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 40 Clemens Tubach 2024-04-30 13:50:11 UTC
Created attachment 165889 [details] [review]
Bug 34788: Add a background job to import a KBART file

This patch adds a background job that will import a KBART file

Sponsored-by: UKHSA
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 41 Clemens Tubach 2024-04-30 13:50:14 UTC
Created attachment 165890 [details] [review]
Bug 34788: Add unit test

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 42 Clemens Tubach 2024-04-30 13:50:17 UTC
Created attachment 165891 [details] [review]
Bug 34788: Add a relationship to a package for the imported file

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 43 Clemens Tubach 2024-04-30 13:50:20 UTC
Created attachment 165892 [details] [review]
Bug 34788: Allow for import of CSV files

This commit allows CSV files to be imported alongside TSV files.
It also adds some performance improvements relating to the max_allowed_packet and the matching of titles, as well as some small bugfixes and unit test changes

Test plan:
1) Enable the ERM module
2) Navigate to E-resource management > eHoldings > Local > Packages
3) Create at least one package
4) Navigate to E-resource management > eHoldings > Local > Titles
5) There should be a button for "Import from KBART file"
6) Click this button
7) Select the package that you created from the dropdown and then choose your KBART file using the "Choose file" button. I have attached some example files to the bug but feel free to use your own if you have them.
8) Click Submit
9) If your file is a valid file, a background job will be queued, if not then a warning will display showing what is incorrect in your file
10) To test the file format warning, edit your file and add a random column heading into the file e.g. test_column. When you upload it, the warning should show that an invalid column "test_column" has been detected
11) Click on the background job. (If you have uploaded a very large file, the system will chunk the file into smaller pieces and create multiple background jobs)
12) It should display a progress bar followed by a report and any error messages
13) Navigate to E-resource management > eHoldings > Local > Titles and you should see your new titles.
14) Run the unit test: prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 44 Clemens Tubach 2024-04-30 13:50:23 UTC
Created attachment 165893 [details] [review]
Bug 34788: Make biblio creation optional

This patch rebases in the changes from bug 36618 to make biblio creation optional

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 45 Clemens Tubach 2024-04-30 13:50:26 UTC
Created attachment 165894 [details] [review]
Bug 34788: Allow import of file with additional columns

This patch allows a file with additional columns to be imported. When the file is submitted, the system will enqueue the background job and send back information to the UI that certain columns have been ignored. The data will stil l import as normal but only the standard KBART columns will be parsed and imported

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Comment 46 Katrin Fischer 2024-05-03 08:38:40 UTC
Can we have some QA please? :)
Comment 47 Nick Clemens (kidclamp) 2024-05-07 19:38:54 UTC
Hi Matt,

This seems to work well and is self contained, I think we could still move this one forward, I have a couple things:

Blockers:
When uploading the file you get the blue box about the import - you cannot dismiss this, it might be better as a modal? Even if I click back to 'Titles' the blue box remains - I have to go to packages and back to titles

I forced a smaller packet size for testing:
-        my $max_allowed_packet = C4::Context->dbh->selectrow_array(q{SELECT @@max_allowed_packet});
+        my $max_allowed_packet = 10384;
This did create a bunch of smaller jobs, however, I found that biblios were not created even if I checked the box once the job was chunked.

QA tool complaints

Other notes:
When editing a title, I can add it to two packages, however, if I try to import a duplicate into another package it is simply ignored. I don't know if this is a blocker or not.

When adding biblios for the titles, there is an ES update for each title added. It would be nice to batch these

It would be nice see the package that was selected, and to have a way to get from the job results back to the package that the titles were imported into.

If you import a list and forget to check 'Create biblios' there is no way to update the files - it would be nice to be able to reimport the duplicates and update - I assume this is future enhancement. These patches do what they set out to - but they imply the need for more batch tools in this area to edit and update the titles

The routines for fixing Ebsco files, or fixing 'coverage_notes' smell a bit, but make sense for practical purposes - is there any sense Ebsco would fix their issue?
Comment 48 Matt Blenkinsop 2024-05-08 14:34:59 UTC
Created attachment 166420 [details] [review]
Bug 34788: (QA follow-up)

This patch fixes a few things:
1) The blue dialog box now clears when navigating away from the page
2) The background job now uses skip_record_index to avoid queuing indexing jobs for every new biblio and instead queues one job at the end
3) Large files that get chunked now successfully create linked biblios if requested
4) Title matching rules have been expanded to check the package ID so that we can have duplicate titles in different packages
5) A link to the package is now included on the job report page
Comment 49 Matt Blenkinsop 2024-05-08 14:38:23 UTC
Thanks for looking at this!

> 
> Blockers:
> When uploading the file you get the blue box about the import - you cannot
> dismiss this, it might be better as a modal? Even if I click back to
> 'Titles' the blue box remains - I have to go to packages and back to titles

Fixed

> 
> I forced a smaller packet size for testing:
> -        my $max_allowed_packet = C4::Context->dbh->selectrow_array(q{SELECT
> @@max_allowed_packet});
> +        my $max_allowed_packet = 10384;
> This did create a bunch of smaller jobs, however, I found that biblios were
> not created even if I checked the box once the job was chunked.

Fixed

> 
> QA tool complaints
 
Fixed

> 
> Other notes:
> When editing a title, I can add it to two packages, however, if I try to
> import a duplicate into another package it is simply ignored. I don't know
> if this is a blocker or not.

This is a good catch, I've fixed this so you can now have the same title in multiple packages

> 
> When adding biblios for the titles, there is an ES update for each title
> added. It would be nice to batch these

I've added skip_record_index in here now and am indexing at the end of the job

> 
> It would be nice see the package that was selected, and to have a way to get
> from the job results back to the package that the titles were imported into.

There's now a link on the report page

> 
> If you import a list and forget to check 'Create biblios' there is no way to
> update the files - it would be nice to be able to reimport the duplicates
> and update - I assume this is future enhancement. These patches do what they
> set out to - but they imply the need for more batch tools in this area to
> edit and update the titles

I agree, I think there should be some follow-ups to this for batch editing

> 
> The routines for fixing Ebsco files, or fixing 'coverage_notes' smell a bit,
> but make sense for practical purposes - is there any sense Ebsco would fix
> their issue?

We've not approached them about it, probably worth asking the question so we can get rid of that method at some point
Comment 50 Nick Clemens (kidclamp) 2024-05-08 19:36:29 UTC
Created attachment 166433 [details] [review]
Bug 34788: Add an API endpoint to import a KBART file

This patch adds the endpoint needed to queue an import_from_kbart_file background job

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 51 Nick Clemens (kidclamp) 2024-05-08 19:36:32 UTC
Created attachment 166434 [details] [review]
Bug 34788: Add Vue components, api client and navigation route

This patch adds a new component to handle the file import, a route to that component and the API client route needed to access the API

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 52 Nick Clemens (kidclamp) 2024-05-08 19:36:35 UTC
Created attachment 166435 [details] [review]
Bug 34788: Add a background job to import a KBART file

This patch adds a background job that will import a KBART file

Sponsored-by: UKHSA
Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 Nick Clemens (kidclamp) 2024-05-08 19:36:38 UTC
Created attachment 166436 [details] [review]
Bug 34788: Add unit test

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens (kidclamp) 2024-05-08 19:36:41 UTC
Created attachment 166437 [details] [review]
Bug 34788: Add a relationship to a package for the imported file

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens (kidclamp) 2024-05-08 19:36:44 UTC
Created attachment 166438 [details] [review]
Bug 34788: Allow for import of CSV files

This commit allows CSV files to be imported alongside TSV files.
It also adds some performance improvements relating to the max_allowed_packet and the matching of titles, as well as some small bugfixes and unit test changes

Test plan:
1) Enable the ERM module
2) Navigate to E-resource management > eHoldings > Local > Packages
3) Create at least one package
4) Navigate to E-resource management > eHoldings > Local > Titles
5) There should be a button for "Import from KBART file"
6) Click this button
7) Select the package that you created from the dropdown and then choose your KBART file using the "Choose file" button. I have attached some example files to the bug but feel free to use your own if you have them.
8) Click Submit
9) If your file is a valid file, a background job will be queued, if not then a warning will display showing what is incorrect in your file
10) To test the file format warning, edit your file and add a random column heading into the file e.g. test_column. When you upload it, the warning should show that an invalid column "test_column" has been detected
11) Click on the background job. (If you have uploaded a very large file, the system will chunk the file into smaller pieces and create multiple background jobs)
12) It should display a progress bar followed by a report and any error messages
13) Navigate to E-resource management > eHoldings > Local > Titles and you should see your new titles.
14) Run the unit test: prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens (kidclamp) 2024-05-08 19:36:46 UTC
Created attachment 166439 [details] [review]
Bug 34788: Make biblio creation optional

This patch rebases in the changes from bug 36618 to make biblio creation optional

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens (kidclamp) 2024-05-08 19:36:49 UTC
Created attachment 166440 [details] [review]
Bug 34788: Allow import of file with additional columns

This patch allows a file with additional columns to be imported. When the file is submitted, the system will enqueue the background job and send back information to the UI that certain columns have been ignored. The data will stil l import as normal but only the standard KBART columns will be parsed and imported

Signed-off-by: Clemens Tubach <clemens.tubach@kit.edu>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens (kidclamp) 2024-05-08 19:36:52 UTC
Created attachment 166441 [details] [review]
Bug 34788: (QA follow-up)

This patch fixes a few things:
1) The blue dialog box now clears when navigating away from the page
2) The background job now uses skip_record_index to avoid queuing indexing jobs for every new biblio and instead queues one job at the end
3) Large files that get chunked now successfully create linked biblios if requested
4) Title matching rules have been expanded to check the package ID so that we can have duplicate titles in different packages
5) A link to the package is now included on the job report page

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Katrin Fischer 2024-05-10 13:05:10 UTC
As a non-native speaker, I have a giant pet-peeve: Translatability.

1) Translatability (please follow-up)

a) It looks like you are building somewhat nice error messages in ImportKBARTFile.pm, but this is the wrong spot. 

Example:
+                    my $formatted_title = format_title($new_title);
+                    if ( !$formatted_title->{publication_title} ) {
+                        push @messages, {
+                            code          => 'title_failed',
+                            type          => 'error',
+                            error_message => "No publication_title found for title_id: ",
+                            title         => '(Unknown)',
+                            title_id      => $formatted_title->{external_id}
+                        };

I think these should be codes and then resolved in the template to make them translatable in the templates. 

b) I believe these are also not translatable in .vue files?

+                        <li>Column headings row</li>
+                        <li>Title data row</li>


We have this construct just a few lines above:
<li>{{ $__("The file must be in TSV or CSV format") }}</li>

c) Do not include the HTML in the translatable string. Use concatenation. These are likely to get broken in process or by translators getting confused:

+                            message += this.$__(
+                                "<p style='font-weight: normal; font-size: medium; margin-top: 1em;'>Your file was too large to process in one job, the file has been split into %s jobs to meet the maximum size limits.</p>"

+                                '<li>Job %s for uploaded file has been queued, <a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=%s" target="_blank">click here</a> to check its progress.</li>

(some more of these in the file below)


2) File endings (separate bug/future)

Most of the time KBART files are received as .txt. It looks like the code expects the "correct" file endings .csv and .tsv. This can be a problem for libraries especially if they are working in Windows and file endings are set to not show. Some might not have the choice to change that easily. It would be great if we could do some kind of auto-detection for .txt files. (maybe checking for the tab chars?)

3) Accessibility (follow-up)

Accessibility guidelines say we should not use constructs like "click here" - a link should already be clear by it's formatting. And we should always make sure to have a good link text that has "sense".

+                <a href="/cgi-bin/koha/erm/eholdings/local/packages/[% report.package_id | $raw %]">Click here to see the package</a>

Why not just "Package"?

4) Terminology (done)

We are trying hard to get rid of "biblio" which is not a real word :) (see wiki/terminology list). 

+                            >{{ $__("Create linked biblio record") }}:</label

I'll fix that one in a follow-up (bibliographic)
Comment 60 Katrin Fischer 2024-05-10 13:19:56 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 61 Matt Blenkinsop 2024-05-10 14:16:15 UTC
Created attachment 166571 [details] [review]
Bug 34788: (follow-up): Fix translations
Comment 62 Katrin Fischer 2024-05-10 15:08:01 UTC
Please reset to "Passed QA" when supplying a follow-up, makes it easier to spot for me in all the bugzilla noise :)
Comment 63 Katrin Fischer 2024-05-10 15:13:34 UTC
Pushed "Fix translations" follow-up, but keeping additional_work_needed, if only to remind me of filing 2) as a separate bug.