Bug 34632: (QA follow-up) Use Koha::Uploader for patron import files
Large files were failing to enqueue/process because the whole CSV was embedded in the job's JSON args, hitting DB/broker size limits. This now uploads the file via Koha::Uploader first (same pattern as MARC staging) and only passes a small uploaded_file_id through the job, and fixes the enqueue-failure message to not claim nothing happened when a job was in fact created. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Lisette Scheer <lisette@bywatersolutions.com> Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
This commit is contained in:
Pedro Amorim
parent
5ca53cf2bc
commit
c6bf858253
|
|
@ -20,6 +20,7 @@ use Try::Tiny qw( catch try );
|
|||
|
||||
use Koha::List::Patron qw( AddPatronList AddPatronsToList );
|
||||
use Koha::Patrons::Import;
|
||||
use Koha::UploadedFiles;
|
||||
|
||||
use base 'Koha::BackgroundJob';
|
||||
|
||||
|
|
@ -54,8 +55,12 @@ sub process {
|
|||
|
||||
$self->start;
|
||||
|
||||
my $file_content = $args->{file_content};
|
||||
open my $fh, '<', \$file_content or die "Cannot create in-memory file handle: $!";
|
||||
my $upload = Koha::UploadedFiles->find( $args->{uploaded_file_id} );
|
||||
unless ($upload) {
|
||||
$self->status('failed')->store;
|
||||
return;
|
||||
}
|
||||
my $fh = $upload->file_handle;
|
||||
|
||||
my $result = try {
|
||||
Koha::Patrons::Import->new->import_patrons(
|
||||
|
|
@ -80,7 +85,6 @@ sub process {
|
|||
};
|
||||
|
||||
my $data = $self->decoded_data;
|
||||
delete $data->{file_content}; # No longer needed; free DB space
|
||||
|
||||
if ($result) {
|
||||
if ( $args->{createpatronlist} && @{ $result->{imported_borrowers} // [] } ) {
|
||||
|
|
@ -110,9 +114,17 @@ Enqueue the new job.
|
|||
sub enqueue {
|
||||
my ( $self, $args ) = @_;
|
||||
|
||||
return unless $args->{file_content};
|
||||
return unless $args->{uploaded_file_id};
|
||||
|
||||
my $line_count = scalar grep { /\S/ } split /\n/, $args->{file_content};
|
||||
my $upload = Koha::UploadedFiles->find( $args->{uploaded_file_id} );
|
||||
return unless $upload;
|
||||
|
||||
my $fh = $upload->file_handle;
|
||||
my $line_count = 0;
|
||||
while ( my $line = <$fh> ) {
|
||||
$line_count++ if $line =~ /\S/;
|
||||
}
|
||||
close $fh;
|
||||
|
||||
$self->SUPER::enqueue(
|
||||
{
|
||||
|
|
|
|||
|
|
@ -176,6 +176,7 @@ sub allows_add_by {
|
|||
{ circulate => 'circulate_remaining_permissions' },
|
||||
{ tools => 'stage_marc_import' },
|
||||
{ tools => 'upload_local_cover_images' },
|
||||
{ tools => 'import_patrons' },
|
||||
];
|
||||
require C4::Auth;
|
||||
foreach (@$flags) {
|
||||
|
|
|
|||
|
|
@ -26,6 +26,11 @@
|
|||
code {
|
||||
background-color: yellow;
|
||||
}
|
||||
#fileuploadstatus,
|
||||
#fileuploadfailed,
|
||||
#fileuploadcancel {
|
||||
display: none;
|
||||
}
|
||||
</style>
|
||||
</head>
|
||||
|
||||
|
|
@ -61,6 +66,11 @@
|
|||
<div class="col-sm-6">
|
||||
[% IF error == 'cannot_enqueue_job' %]
|
||||
<div class="alert alert-danger">The import job could not be enqueued. Please try again.</div>
|
||||
[% ELSIF error == 'job_failed_to_queue' %]
|
||||
<div class="alert alert-danger">
|
||||
The import job was created but could not be queued for processing.
|
||||
<a href="/cgi-bin/koha/admin/background_jobs.pl?op=view&id=[% job_id | uri %]">View the job</a> for more details, or try again.
|
||||
</div>
|
||||
[% ELSE %]
|
||||
<div class="alert alert-info">
|
||||
<h1>The patron import job has been enqueued!</h1>
|
||||
|
|
@ -81,18 +91,41 @@
|
|||
<li>If a card number exists in the table, you can choose whether to ignore the new one or overwrite the old one.</li>
|
||||
</ul>
|
||||
|
||||
<form method="post" action="/cgi-bin/koha/tools/import_borrowers.pl" enctype="multipart/form-data">
|
||||
<form method="get" id="uploadfile" enctype="multipart/form-data">
|
||||
<fieldset class="rows" id="uploadform">
|
||||
<legend>Upload a file to import</legend>
|
||||
<ol>
|
||||
<li>
|
||||
<div id="fileuploadform">
|
||||
<label for="fileToUpload">Select the file to import: </label>
|
||||
<input type="file" id="fileToUpload" name="fileToUpload" />
|
||||
</div>
|
||||
</li>
|
||||
</ol>
|
||||
</fieldset>
|
||||
<fieldset class="action">
|
||||
<button id="fileuploadbutton" class="btn btn-primary">Upload file</button>
|
||||
<button id="fileuploadcancel">Cancel</button>
|
||||
</fieldset>
|
||||
|
||||
<div id="fileuploadpanel">
|
||||
<div id="fileuploadstatus" class="progress_panel"
|
||||
>Upload progress:
|
||||
<progress id="fileuploadprogress" max="100" value="0"></progress>
|
||||
<span class="fileuploadpercent">0</span>%
|
||||
</div>
|
||||
<div id="fileuploadfailed"></div>
|
||||
</div>
|
||||
</form>
|
||||
|
||||
<form method="post" id="processfile" action="/cgi-bin/koha/tools/import_borrowers.pl" enctype="multipart/form-data">
|
||||
[% INCLUDE 'csrf-token.inc' %]
|
||||
<input type="hidden" name="op" value="cud-import" />
|
||||
<input type="hidden" name="uploadedfileid" id="uploadedfileid" value="" />
|
||||
<fieldset class="rows">
|
||||
<legend>Import into the borrowers table</legend>
|
||||
|
||||
<ol>
|
||||
<li>
|
||||
<label for="uploadborrowers">Select the file to import: </label>
|
||||
<input type="file" id="uploadborrowers" name="uploadborrowers" />
|
||||
</li>
|
||||
|
||||
<li>
|
||||
<label for="createpatronlist">Create patron list: </label>
|
||||
<input name="createpatronlist" id="createpatronlist" value="1" type="checkbox" />
|
||||
|
|
@ -237,7 +270,7 @@
|
|||
</fieldset>
|
||||
|
||||
<fieldset class="action">
|
||||
<input type="submit" class="btn btn-primary" value="Import" />
|
||||
<input type="submit" id="mainformsubmit" class="btn btn-primary" value="Import" />
|
||||
</fieldset>
|
||||
</form>
|
||||
</div>
|
||||
|
|
@ -295,7 +328,9 @@
|
|||
[% Asset.js("js/tools-menu.js") | $raw %]
|
||||
[% INCLUDE 'str/members-menu.inc' %]
|
||||
[% Asset.js("js/members-menu.js") | $raw %]
|
||||
[% Asset.js("js/file-upload.js") | $raw %]
|
||||
<script nonce="[% Koha.CSPNonce | $raw %]">
|
||||
var xhr;
|
||||
$(document).ready(function () {
|
||||
// Make date fields have the datepicker
|
||||
$("#dateenrolled, #dateexpiry, #dateofbirth").flatpickr({
|
||||
|
|
@ -316,6 +351,27 @@
|
|||
$(".preserve_values").toggle();
|
||||
$(".expand_preserves").toggle();
|
||||
});
|
||||
|
||||
$("#processfile").hide();
|
||||
|
||||
$("#fileuploadbutton").on("click", function (e) {
|
||||
e.preventDefault();
|
||||
StartUpload();
|
||||
});
|
||||
$("#fileuploadcancel").on("click", function (e) {
|
||||
e.preventDefault();
|
||||
CancelUpload();
|
||||
});
|
||||
$("#mainformsubmit").on("click", function (e) {
|
||||
e.preventDefault();
|
||||
if ($("#uploadedfileid").val() == "") {
|
||||
alert(_("Please upload a file first."));
|
||||
return false;
|
||||
} else {
|
||||
$("#processfile").submit();
|
||||
return true;
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
$("#overwrite_cardnumberno").click(function () {
|
||||
|
|
@ -326,6 +382,43 @@
|
|||
$("#overwrite_passwords").prop("disabled", false);
|
||||
$("#update_dateexpiry").prop("disabled", false);
|
||||
});
|
||||
|
||||
function StartUpload() {
|
||||
if ($("#fileToUpload").prop("files").length == 0) return;
|
||||
$("#fileuploadbutton").hide();
|
||||
$("#fileuploadfailed").hide();
|
||||
$("#processfile").hide();
|
||||
$("#fileuploadstatus").show();
|
||||
$("#uploadedfileid").val("");
|
||||
xhr = AjaxUpload($("#fileToUpload"), $("#fileuploadprogress"), "temp=1", cbUpload);
|
||||
$("#fileuploadcancel").show();
|
||||
}
|
||||
function CancelUpload() {
|
||||
if (xhr) xhr.abort();
|
||||
$("#fileuploadstatus").hide();
|
||||
$("#fileuploadbutton").show();
|
||||
$("#fileuploadcancel").hide();
|
||||
$("#fileuploadfailed").show();
|
||||
$("#fileuploadfailed").text(_("Upload status: Cancelled "));
|
||||
}
|
||||
function cbUpload(status, fileid, errors) {
|
||||
if (status == "done") {
|
||||
$("#uploadedfileid").val(fileid);
|
||||
$("#fileToUpload").prop("disabled", true);
|
||||
$("#fileuploadbutton").prop("disabled", true);
|
||||
$("#fileuploadbutton").show();
|
||||
$("#fileuploadcancel").hide();
|
||||
$("#processfile").show();
|
||||
} else {
|
||||
var errMsgs = [_("Error code 0 not used"), _("File already exists"), _("Directory is not writeable"), _("Root directory for uploads not defined"), _("Temporary directory for uploads not defined")];
|
||||
var errCode = errors[$("#fileToUpload").prop("files")[0].name].code;
|
||||
$("#fileuploadbutton").show();
|
||||
$("#fileuploadcancel").hide();
|
||||
$("#fileuploadstatus").hide();
|
||||
$("#fileuploadfailed").show();
|
||||
$("#fileuploadfailed").text(_("Upload status: ") + (status == "failed" ? _("Failed") + " - (" + errCode + ") " + errMsgs[errCode] : status == "denied" ? _("Denied") : status));
|
||||
}
|
||||
}
|
||||
</script>
|
||||
[% END %]
|
||||
[% INCLUDE 'intranet-bottom.inc' %]
|
||||
|
|
|
|||
|
|
@ -17,6 +17,8 @@
|
|||
|
||||
use Modern::Perl;
|
||||
|
||||
use File::Temp qw( tempdir );
|
||||
use Test::MockModule;
|
||||
use Test::NoWarnings;
|
||||
use Test::More tests => 7;
|
||||
|
||||
|
|
@ -24,6 +26,7 @@ use Koha::BackgroundJob::PatronImport;
|
|||
use Koha::BackgroundJobs;
|
||||
use Koha::Database;
|
||||
use Koha::Patrons;
|
||||
use Koha::UploadedFiles;
|
||||
|
||||
use t::lib::Mocks;
|
||||
use t::lib::TestBuilder;
|
||||
|
|
@ -31,6 +34,34 @@ use t::lib::TestBuilder;
|
|||
my $schema = Koha::Database->new->schema;
|
||||
my $builder = t::lib::TestBuilder->new;
|
||||
|
||||
my $tempdir = tempdir( CLEANUP => 1 );
|
||||
my $ctx_mock = Test::MockModule->new('C4::Context');
|
||||
$ctx_mock->mock( 'temporary_directory' => sub { return $tempdir; } );
|
||||
|
||||
my $upload_counter = 0;
|
||||
|
||||
sub build_uploaded_file {
|
||||
my ($csv) = @_;
|
||||
|
||||
$upload_counter++;
|
||||
my $upload = Koha::UploadedFile->new(
|
||||
{
|
||||
hashvalue => sprintf( 'testhash%03d', $upload_counter ),
|
||||
filename => "patrons_$upload_counter.csv",
|
||||
dir => '',
|
||||
uploadcategorycode => 'temp',
|
||||
filesize => length($csv),
|
||||
permanent => 0,
|
||||
}
|
||||
)->store;
|
||||
|
||||
open my $fh, '>', $upload->full_path or die "Cannot write test upload file: $!";
|
||||
print $fh $csv;
|
||||
close $fh;
|
||||
|
||||
return $upload;
|
||||
}
|
||||
|
||||
subtest 'enqueue() tests' => sub {
|
||||
|
||||
plan tests => 5;
|
||||
|
|
@ -39,10 +70,11 @@ subtest 'enqueue() tests' => sub {
|
|||
|
||||
my $csv = "surname,branchcode,categorycode\nSmith,CPL,PT\nJones,CPL,PT\n";
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv,
|
||||
matchpoint => 'cardnumber',
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
}
|
||||
);
|
||||
my $job = Koha::BackgroundJobs->find($job_id)->_derived_class;
|
||||
|
|
@ -53,10 +85,11 @@ subtest 'enqueue() tests' => sub {
|
|||
is( $job->type, 'patron_import', 'job_type is patron_import' );
|
||||
|
||||
my $csv_no_newline = "surname,branchcode,categorycode\nSmith,CPL,PT\nJones,CPL,PT";
|
||||
my $upload2 = build_uploaded_file($csv_no_newline);
|
||||
my $job_id2 = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv_no_newline,
|
||||
matchpoint => 'cardnumber',
|
||||
uploaded_file_id => $upload2->id,
|
||||
matchpoint => 'cardnumber',
|
||||
}
|
||||
);
|
||||
my $job2 = Koha::BackgroundJobs->find($job_id2)->_derived_class;
|
||||
|
|
@ -67,7 +100,7 @@ subtest 'enqueue() tests' => sub {
|
|||
|
||||
subtest 'process() tests' => sub {
|
||||
|
||||
plan tests => 8;
|
||||
plan tests => 7;
|
||||
|
||||
$schema->storage->txn_begin;
|
||||
|
||||
|
|
@ -85,9 +118,10 @@ subtest 'process() tests' => sub {
|
|||
""
|
||||
);
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv,
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 0,
|
||||
defaults => {},
|
||||
|
|
@ -107,7 +141,6 @@ subtest 'process() tests' => sub {
|
|||
is( $data->{already_in_db}, 0, 'No patrons already in db' );
|
||||
is( $data->{invalid}, 0, 'No invalid rows' );
|
||||
is( $data->{total}, 2, 'Total count is correct' );
|
||||
ok( !exists $data->{file_content}, 'file_content cleared from stored data' );
|
||||
is(
|
||||
Koha::Patrons->search( { cardnumber => { '-in' => [ 'TEST001', 'TEST002' ] } } )->count,
|
||||
2, 'Patrons exist in the database'
|
||||
|
|
@ -136,9 +169,10 @@ subtest 'process() with errors' => sub {
|
|||
""
|
||||
);
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv,
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 0,
|
||||
defaults => {},
|
||||
|
|
@ -180,11 +214,12 @@ subtest 'process() already_in_db and overwritten' => sub {
|
|||
""
|
||||
);
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my %base_args = (
|
||||
file_content => $csv,
|
||||
matchpoint => 'cardnumber',
|
||||
defaults => {},
|
||||
preserve_fields => [],
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
defaults => {},
|
||||
preserve_fields => [],
|
||||
);
|
||||
|
||||
# First import creates the patrons
|
||||
|
|
@ -238,9 +273,10 @@ subtest 'process() with patron list creation' => sub {
|
|||
""
|
||||
);
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv,
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 0,
|
||||
defaults => {},
|
||||
|
|
@ -284,9 +320,10 @@ subtest 'process() with cancelled job' => sub {
|
|||
""
|
||||
);
|
||||
|
||||
my $upload = build_uploaded_file($csv);
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
{
|
||||
file_content => $csv,
|
||||
uploaded_file_id => $upload->id,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 0,
|
||||
defaults => {},
|
||||
|
|
|
|||
|
|
@ -46,6 +46,7 @@ use Koha::DateUtils qw( dt_from_string );
|
|||
use Koha::Patron::Attribute::Types;
|
||||
use Koha::Patron::Categories;
|
||||
use Koha::Patrons;
|
||||
use Koha::UploadedFiles;
|
||||
|
||||
use Text::CSV;
|
||||
|
||||
|
|
@ -88,9 +89,9 @@ if ( $input->param('sample') ) {
|
|||
|
||||
my @preserve_fields = $input->multi_param('preserve_existing');
|
||||
|
||||
my $uploadborrowers = $input->param('uploadborrowers');
|
||||
my $matchpoint = $input->param('matchpoint');
|
||||
my $welcome_new = $input->param('welcome_new');
|
||||
my $uploadedfileid = $input->param('uploadedfileid');
|
||||
my $matchpoint = $input->param('matchpoint');
|
||||
my $welcome_new = $input->param('welcome_new');
|
||||
if ($matchpoint) {
|
||||
$matchpoint =~ s/^patron_attribute_//;
|
||||
}
|
||||
|
|
@ -100,11 +101,10 @@ my $createpatronlist = $input->param('createpatronlist') || 0;
|
|||
my $dt = dt_from_string();
|
||||
my $timestamp = $dt->ymd('-') . ' ' . $dt->hms(':');
|
||||
|
||||
if ( $op eq 'cud-import' && $uploadborrowers && length($uploadborrowers) > 0 ) {
|
||||
if ( $op eq 'cud-import' && $uploadedfileid ) {
|
||||
|
||||
my $handle = $input->upload('uploadborrowers');
|
||||
my $file_content = do { local $/; <$handle> };
|
||||
my $patronlistname = $uploadborrowers . ' (' . $timestamp . ')';
|
||||
my $upload = Koha::UploadedFiles->find($uploadedfileid);
|
||||
my $patronlistname = ( $upload ? $upload->filename : $uploadedfileid ) . ' (' . $timestamp . ')';
|
||||
|
||||
my %defaults;
|
||||
for my $k (@columnkeys) {
|
||||
|
|
@ -115,10 +115,11 @@ if ( $op eq 'cud-import' && $uploadborrowers && length($uploadborrowers) > 0 ) {
|
|||
my $overwrite_passwords = defined $input->param('overwrite_passwords') ? 1 : 0;
|
||||
my $update_dateexpiry = $input->param('update_dateexpiry') // "";
|
||||
|
||||
my $job = Koha::BackgroundJob::PatronImport->new;
|
||||
try {
|
||||
my $job_id = Koha::BackgroundJob::PatronImport->new->enqueue(
|
||||
my $job_id = $job->enqueue(
|
||||
{
|
||||
file_content => $file_content,
|
||||
uploaded_file_id => $uploadedfileid,
|
||||
defaults => \%defaults,
|
||||
matchpoint => $matchpoint,
|
||||
overwrite_cardnumber => scalar $input->param('overwrite_cardnumber'),
|
||||
|
|
@ -134,15 +135,30 @@ if ( $op eq 'cud-import' && $uploadborrowers && length($uploadborrowers) > 0 ) {
|
|||
}
|
||||
);
|
||||
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
job_id => $job_id,
|
||||
);
|
||||
if ($job_id) {
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
job_id => $job_id,
|
||||
);
|
||||
} else {
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
error => 'cannot_enqueue_job',
|
||||
);
|
||||
}
|
||||
} catch {
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
error => 'cannot_enqueue_job',
|
||||
);
|
||||
if ( $job->id ) {
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
job_id => $job->id,
|
||||
error => 'job_failed_to_queue',
|
||||
);
|
||||
} else {
|
||||
$template->param(
|
||||
op => 'enqueued',
|
||||
error => 'cannot_enqueue_job',
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
} else {
|
||||
|
|
|
|||
Loading…
Reference in New Issue