Bug 36831 - Add support for .txt files to the KBART import tool
Summary: Add support for .txt files to the KBART import tool
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matt Blenkinsop
QA Contact:
URL:
Keywords:
: 36903 (view as bug list)
Depends on: 36895 36935 34788
Blocks: 36942
  Show dependency treegraph
 
Reported: 2024-05-10 15:16 UTC by Matt Blenkinsop
Modified: 2024-07-03 11:20 UTC (History)
8 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 36831: Add a method to determine the file delimiter (3.35 KB, patch)
2024-05-21 10:02 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36831: Allow for multiple file types (3.93 KB, patch)
2024-05-21 10:02 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36831: Add unit test (2.14 KB, patch)
2024-05-21 10:02 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
test file (42.61 KB, text/plain)
2024-05-21 10:04 UTC, Matt Blenkinsop
Details
Bug 36831: Add a method to determine the file delimiter (3.38 KB, patch)
2024-05-21 22:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 36831: Allow for multiple file types (3.96 KB, patch)
2024-05-21 22:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 36831: Add unit test (2.17 KB, patch)
2024-05-21 22:07 UTC, David Nind
Details | Diff | Splinter Review
Bug 36831: Add a method to determine the file delimiter (2.08 KB, patch)
2024-07-03 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36831: Move logic from the background job to the Title object class (14.43 KB, patch)
2024-07-03 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36831: Allow for multiple file types (4.17 KB, patch)
2024-07-03 10:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36831: Add unit test (8.70 KB, patch)
2024-07-03 10:11 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 2024-05-10 15:16:59 UTC
It would be beneficial to be able to import KBART files that are in .txt format and detect what the separation character is
Comment 1 Matt Blenkinsop 2024-05-21 09:46:16 UTC
*** Bug 36903 has been marked as a duplicate of this bug. ***
Comment 2 Matt Blenkinsop 2024-05-21 10:02:12 UTC
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
Comment 3 Matt Blenkinsop 2024-05-21 10:02:15 UTC
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
Comment 4 Matt Blenkinsop 2024-05-21 10:02:17 UTC
Created attachment 166960 [details] [review]
Bug 36831: Add unit test

prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Comment 5 Matt Blenkinsop 2024-05-21 10:04:05 UTC
Created attachment 166961 [details]
test file
Comment 6 David Nind 2024-05-21 22:06:57 UTC
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>
Comment 7 David Nind 2024-05-21 22:07:00 UTC
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>
Comment 8 David Nind 2024-05-21 22:07:02 UTC
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>
Comment 9 Tomás Cohen Arazi 2024-05-22 11:32:55 UTC
I'm not sure about the use of the static method from the background job class in the controller...
Comment 10 Tomás Cohen Arazi 2024-05-22 11:47:14 UTC
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.
Comment 11 Katrin Fischer 2024-05-22 12:06:05 UTC
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.
Comment 12 Matt Blenkinsop 2024-05-23 11:22:56 UTC
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?
Comment 13 Matt Blenkinsop 2024-05-23 11:28:36 UTC
(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?
Comment 14 Martin Renvoize 2024-05-23 12:34:43 UTC
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);
}
Comment 15 Katrin Fischer 2024-05-23 14:39:32 UTC
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.
Comment 16 Matt Blenkinsop 2024-07-03 10:11:05 UTC
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
Comment 17 Matt Blenkinsop 2024-07-03 10:11:09 UTC
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
Comment 18 Matt Blenkinsop 2024-07-03 10:11:12 UTC
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
Comment 19 Matt Blenkinsop 2024-07-03 10:11:16 UTC
Created attachment 168434 [details] [review]
Bug 36831: Add unit test

prove t/db_dependent/Koha/BackgroundJob/ImportKBARTFile.t
Comment 20 Matt Blenkinsop 2024-07-03 10:12:33 UTC
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