It would be beneficial to be able to import KBART files that are in .txt format and detect what the separation character is
*** Bug 36903 has been marked as a duplicate of this bug. ***
Created attachment 166958 [details] [review] Bug 36831: Add a method to determine the file delimiter This patch adds a method to determine whether a file is a CSV or TSV file
Created attachment 166959 [details] [review] Bug 36831: Allow for multiple file types This patch allows the KBART import tool to accept different filetypes and then work out whether they are CSV or TSV files. Test plan: 1) Try to import the file attached to the bug using the KBART import tool (E-resource management > eHoldings > Local > Titles > Import from a KBART file) 2) It should return an error saying that the file must be .csv or .tsv 3) Apply patch 4) yarn build 5) restart_all 6) Repeat step 1 7) The file should now successfully import
Created attachment 166960 [details] [review] Bug 36831: Add unit test prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Created attachment 166961 [details] test file
Created attachment 166989 [details] [review] Bug 36831: Add a method to determine the file delimiter This patch adds a method to determine whether a file is a CSV or TSV file Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166990 [details] [review] Bug 36831: Allow for multiple file types This patch allows the KBART import tool to accept different filetypes and then work out whether they are CSV or TSV files. Test plan: 1) Try to import the file attached to the bug using the KBART import tool (E-resource management > eHoldings > Local > Titles > Import from a KBART file) 2) It should return an error saying that the file must be .csv or .tsv 3) Apply patch 4) yarn build 5) restart_all 6) Repeat step 1 7) The file should now successfully import Signed-off-by: David Nind <david@davidnind.com>
Created attachment 166991 [details] [review] Bug 36831: Add unit test prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t Signed-off-by: David Nind <david@davidnind.com>
I'm not sure about the use of the static method from the background job class in the controller...
Are you sure we can really rest assured there won't be a tab character inside some column? I think this won't be robust enough. I propose that we ask the user which delimiter the file uses. One of the options should be *detect* but the current implementation looks fragile. BTW: what about semicolon? I've seen CSV files that use it.
I think asking would be OK too, maybe confirming a "guess" we made first? The problem was that the file ending is most of the time just .txt for KBART. I haven't encountered a ; separated one so far, not sure if it's defined somewhere in the spec.
I tried to work out if the Text::CSV_XS library has a method for working out what the delimiter is but couldn't find anything - is there anywhere else in Koha that we would check something like that?
(In reply to Tomás Cohen Arazi from comment #9) > I'm not sure about the use of the static method from the background job > class in the controller... Would it make more sense in Koha/ERM/EHoldings/Title or in a new class for KBART files?
I had a bit of a think about this.. I've seen the following cod in use somewhere.. it tries to best guess using counts of occurrences.. maybe this is an approach we could be happy with? sub detect_delimiter_and_quote { my ($filename) = @_; my $sample_lines = 5; # Number of lines to sample for detection open my $fh, '<', $filename or die "Could not open '$filename': $!"; my @lines; while (<$fh>) { push @lines, $_; last if $. >= $sample_lines; } close $fh; my %delimiter_count; my %quote_count; foreach my $line (@lines) { foreach my $char (',', '\t', ';', '|') { my $count = () = $line =~ /\Q$char\E/g; $delimiter_count{$char} += $count if $count; } foreach my $char ('"', "'") { my $count = () = $line =~ /\Q$char\E/g; $quote_count{$char} += $count if $count; } } # Guess the delimiter with the highest count my ($delimiter) = sort { $delimiter_count{$b} <=> $delimiter_count{$a} } keys %delimiter_count; # Guess the quote character with the highest count my ($quote) = sort { $quote_count{$b} <=> $quote_count{$a} } keys %quote_count; # Fallback to common defaults if nothing is detected $delimiter //= ','; $quote //= '"'; return ($delimiter, $quote); }
I am happy with whatever you guys think is best - I am glad to see this one moving! I was wondering earlier: can we trust the header row to be a good source? There might be the least surprised there concerning other characters maybe. But I was not sure if only the sequence mattered or if also the header names were mandatory.
Created attachment 168431 [details] [review] Bug 36831: Add a method to determine the file delimiter This patch adds a method that will detect the delimiter and quote character for a file
Created attachment 168432 [details] [review] Bug 36831: Move logic from the background job to the Title object class This patch moves logic out of the background job file and into the Koha::ERM::EHoldings::Title class where it should belong. Methods are also renamed to differentiate between object methods and helper methods
Created attachment 168433 [details] [review] Bug 36831: Allow for multiple file types This patch allows the KBART import tool to accept different filetypes and then work out whether they are CSV or TSV files. Test plan: 1) Try to import the file attached to the bug using the KBART import tool (E-resource management > eHoldings > Local > Titles > Import from a KBART file) 2) It should return an error saying that the file must be .csv or .tsv 3) Apply patch 4) yarn build 5) restart_all 6) Repeat step 1 7) The file should now successfully import
Created attachment 168434 [details] [review] Bug 36831: Add unit test prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Setting back to NSO as there are some fairly large changes. I've used Martin's suggested method but also moved all of the file parsing logic out of the background job file and into the Title class
Apologies for taking so long to re-test. The patch no longer applies 8-(... git bz apply 36831 Bug 36831 - Add support for .txt files to the KBART import tool 168431 - Bug 36831: Add a method to determine the file delimiter 168432 - Bug 36831: Move logic from the background job to the Title object class 168433 - Bug 36831: Allow for multiple file types 168434 - Bug 36831: Add unit test Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 36831: Add a method to determine the file delimiter Applying: Bug 36831: Move logic from the background job to the Title object class Applying: Bug 36831: Allow for multiple file types Using index info to reconstruct a base tree... M Koha/REST/V1/ERM/EHoldings/Titles/Local.pm Falling back to patching base and 3-way merge... Auto-merging Koha/REST/V1/ERM/EHoldings/Titles/Local.pm CONFLICT (content): Merge conflict in Koha/REST/V1/ERM/EHoldings/Titles/Local.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 36831: Allow for multiple file types
Created attachment 170549 [details] [review] Bug 36831: Add a method to determine the file delimiter This patch adds a method that will detect the delimiter and quote character for a file
Created attachment 170550 [details] [review] Bug 36831: Move logic from the background job to the Title object class This patch moves logic out of the background job file and into the Koha::ERM::EHoldings::Title class where it should belong. Methods are also renamed to differentiate between object methods and helper methods
Created attachment 170551 [details] [review] Bug 36831: Allow for multiple file types This patch allows the KBART import tool to accept different filetypes and then work out whether they are CSV or TSV files. Test plan: 1) Try to import the file attached to the bug using the KBART import tool (E-resource management > eHoldings > Local > Titles > Import from a KBART file) 2) It should return an error saying that the file must be .csv or .tsv 3) Apply patch 4) yarn build 5) restart_all 6) Repeat step 1 7) The file should now successfully import
Created attachment 170552 [details] [review] Bug 36831: Add unit test prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
See 4 and 5 Testing notes (using KTD): 1. Enable ERMModule system preference 2. Need to set up a package: ERM > eHoldings > Local > Packages > + New package 3. For step 2, I don't get any error - something flashes up very quickly, but then just displays the screen again 4. After applying the patches, yarn build, and restart_all, and repeating Import from KBART file, I get this error: Something went wrong: Error: Something went wrong, check Koha logs for details. 5. Error in /var/log/koha/kohadev/plack-api-error.log: [2024/08/21 11:12:19] [ERROR] POST /api/v1/erm/eholdings/local/titles/import_kbart: unhandled exception (Mojo::Exception)<<Use of inherited AUTOLOAD for non-method Koha:: BackgroundJob::ImportKBARTFile::read_file() is no longer allowed at /kohadevbox/koha/Koha/REST/V1/ERM/EHoldings/Titles/Local.pm line 284.>>
Created attachment 170562 [details] [review] Bug 36831: Add a method to determine the file delimiter This patch adds a method that will detect the delimiter and quote character for a file
Created attachment 170563 [details] [review] Bug 36831: Move logic from the background job to the Title object class This patch moves logic out of the background job file and into the Koha::ERM::EHoldings::Title class where it should belong. Methods are also renamed to differentiate between object methods and helper methods
Created attachment 170564 [details] [review] Bug 36831: Allow for multiple file types This patch allows the KBART import tool to accept different filetypes and then work out whether they are CSV or TSV files. Test plan: 1) Try to import the file attached to the bug using the KBART import tool (E-resource management > eHoldings > Local > Titles > Import from a KBART file) 2) It should return an error saying that the file must be .csv or .tsv 3) Apply patch 4) yarn build 5) restart_all 6) Repeat step 1 7) The file should now successfully import
Created attachment 170565 [details] [review] Bug 36831: Add unit test prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
(In reply to David Nind from comment #26) > See 4 and 5 > > Testing notes (using KTD): > > 1. Enable ERMModule system preference > 2. Need to set up a package: ERM > eHoldings > Local > Packages > + New > package > 3. For step 2, I don't get any error - something flashes up very quickly, > but then just displays the screen again > 4. After applying the patches, yarn build, and restart_all, and repeating > Import from KBART file, I get this error: > Something went wrong: Error: Something went wrong, check Koha logs for > details. > 5. Error in /var/log/koha/kohadev/plack-api-error.log: > [2024/08/21 11:12:19] [ERROR] POST > /api/v1/erm/eholdings/local/titles/import_kbart: unhandled exception > (Mojo::Exception)<<Use of inherited AUTOLOAD for non-method Koha:: > BackgroundJob::ImportKBARTFile::read_file() is no longer allowed at > /kohadevbox/koha/Koha/REST/V1/ERM/EHoldings/Titles/Local.pm line 284.>> Mistake in the rebase on my part. The issue you saw in step 3 will also be solved in this patch
I tried two different files and in both cases the job failed. 1.) the test file attached to this bug (OUP journals) , separator in the file is comma " Information: Additional columns were detected in your report, please see the list below: OCN Empty column MARC Control Number Subject Collections Year Started at OUP Title History Current Title The data in these columns will not be imported. " That is OK. But the job failed --> 0 % failed There is no error message --- 2.) Springer KBART file Palgrave Education Collection 2011 (https://adminportal.springernature.com/metadata/kbart) , separator in the file is tab " Information: Additional columns were detected in your report, please see the list below: publication_title print_identifier online_identifier date_first_issue_online num_first_vol_online num_first_issue_online date_last_issue_online num_last_vol_online num_last_issue_online title_url first_author title_id embargo_info coverage_depth notes publisher_name publication_type date_monograph_published_print date_monograph_published_online monograph_volume monograph_edition first_editor parent_publication_title_id preceding_publication_title_id access_type The data in these columns will not be imported. " It seems the tab as separation character is not detected. The job also fails.