From b91f0063cc93b39c899bedb8f79a5a403b3bd960 Mon Sep 17 00:00:00 2001 From: Stefan Berndtsson Date: Fri, 17 Feb 2017 15:50:45 +0100 Subject: [PATCH] Add borrowernumber to staged imports, and set it to the currently logged in user. SysPref to turn on filtering of staged import list so that only batches staged by logged in user is visible. SysPref to remove Cleaned/Imported batches from staged import list. Link near list to ignore filter, in case one needs to see all batches. https://bugs.koha-community.org/show_bug.cgi?id=18129 --- .gitignore | 3 ++ C4/ImportBatch.pm | 32 ++++++++++-- .../bug_18129-stage-with-username-syspref.sql | 2 + .../atomicupdate/bug_18129-stage-with-username.sql | 1 + .../en/modules/admin/preferences/cataloguing.pref | 12 +++++ .../prog/en/modules/tools/manage-marc-import.tt | 21 ++++++++ t/ImportBatch.t | 61 +++++++++++++++++++++- tools/manage-marc-import.pl | 27 ++++++++-- 8 files changed, 149 insertions(+), 10 deletions(-) create mode 100644 .gitignore create mode 100644 installer/data/mysql/atomicupdate/bug_18129-stage-with-username-syspref.sql create mode 100644 installer/data/mysql/atomicupdate/bug_18129-stage-with-username.sql diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..e48f4f0 --- /dev/null +++ b/.gitignore @@ -0,0 +1,3 @@ +*~ +\#*# +.#* diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index 88d670d..c80fa89 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -237,9 +237,17 @@ sub GetImportRecordMarcXML { sub AddImportBatch { my ($params) = @_; + # Get ID of logged in user. if called from a batch job, + # no user session exists and C4::Context->userenv() returns + # the scalar '0'. + my $userenv = C4::Context->userenv(); + my $usernumber = (ref($userenv) eq 'HASH') ? $userenv->{'number'} : 0; + $usernumber ||= 0; + $params->{borrowernumber} = $usernumber; + my (@fields, @vals); foreach (qw( matcher_id template_id branchcode - overlay_action nomatch_action item_action + overlay_action nomatch_action item_action borrowernumber import_status batch_type file_name comments record_type )) { if (exists $params->{$_}) { push @fields, $_; @@ -1028,13 +1036,29 @@ start at the given offset. =cut sub GetImportBatchRangeDesc { - my ($offset, $results_per_group) = @_; + my ($offset, $results_per_group, $no_filter) = @_; my $dbh = C4::Context->dbh; my $query = "SELECT * FROM import_batches - WHERE batch_type IN ('batch', 'webservice') - ORDER BY import_batch_id DESC"; + WHERE batch_type IN ('batch', 'webservice')"; + my @params; + + if(C4::Context->preference("StageFilterByUser") && !$no_filter) { + my $userenv = C4::Context->userenv(); + my $usernumber = (ref($userenv) eq 'HASH') ? $userenv->{'number'} : 0; + $usernumber ||= 0; + + $query .= " AND borrowernumber = ?"; + push(@params, $usernumber); + } + + if(C4::Context->preference("StageHideCleanedImported") && !$no_filter) { + $query .= " AND import_status NOT IN ('cleaned', 'imported')"; + } + + $query .= " ORDER BY import_batch_id DESC"; + if ($results_per_group){ $query .= " LIMIT ?"; push(@params, $results_per_group); diff --git a/installer/data/mysql/atomicupdate/bug_18129-stage-with-username-syspref.sql b/installer/data/mysql/atomicupdate/bug_18129-stage-with-username-syspref.sql new file mode 100644 index 0000000..ce516aa --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18129-stage-with-username-syspref.sql @@ -0,0 +1,2 @@ +INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type`) VALUES ('StageFilterByUser', '0', '', 'Filter staged batches by user', 'YesNo'); +INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type`) VALUES ('StageHideCleanedImported', '0', '', 'Hide staged batches when cleaned or imported', 'YesNo'); diff --git a/installer/data/mysql/atomicupdate/bug_18129-stage-with-username.sql b/installer/data/mysql/atomicupdate/bug_18129-stage-with-username.sql new file mode 100644 index 0000000..02b125f --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_18129-stage-with-username.sql @@ -0,0 +1 @@ +ALTER TABLE `import_batches` ADD `borrowernumber` int(11); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref index 1aeadf9..8fcfe88 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref @@ -231,6 +231,18 @@ Cataloging: yes: "do" no: "don't" - attempt to match aggressively by trying all variations of the ISSNs in the imported record as a phrase in the ISSN fields of already cataloged records. Note that this preference has no effect if UseQueryParser is on. + - + - In list of staged batches of MARC records, + - pref: StageFilterByUser + choices: + yes: "show only batches uploaded by logged in user." + no: "show all batches regardless of user." + - + - pref: StageHideCleanedImported + choices: + yes: "Hide" + no: "Do not hide" + - cleaned and imported batches of MARC records by default. Exporting: - diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt index 2f0d083..bc08814 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/tools/manage-marc-import.tt @@ -1,3 +1,5 @@ +[% USE Koha %] +rn on filtering of staged import list so that only batches staged by logged in user is visible. SysPref to remove Cleaned/Imported batches from staged import list. Link near list to ignore filter, in case one needs to see all batches. [% INCLUDE 'doc-head-open.inc' %] Koha › Tools › Manage staged MARC records [% IF ( import_batch_id ) %] @@ -185,6 +187,13 @@ $(document).ready(function(){ <div class="dialog message"> <p>No records have been staged.</p> <p><a href="/cgi-bin/koha/tools/stage-marc-import.pl">Stage MARC records for import</a>.</p> + [% IF Koha.Preference( 'StageFilterByUser' ) %] + <p> + [% UNLESS ( no_filter ) %] + <a href="[% batch_lis.script_name %]?no_filter=1">Show unfiltered list</a>. + [% END %] + </p> + [% END %] </div> [% END %] [% END %] @@ -220,6 +229,7 @@ $(document).ready(function(){ <fieldset class="rows" id="staged-record-matching-rules"> <ol> <li><span class="label">File name:</span> [% file_name %]</li> + <li><span class="label">User:</span> [% user_name %]</li> <li><span class="label">Comments:</span> [% IF ( comments ) %][% comments %][% ELSE %](none)[% END %]</li> <li><span class="label">Type:</span> [% IF ( record_type == 'auth' ) %]Authority records[% ELSE %]Bibliographic records[% END %]</li> <li><span class="label">Staged:</span> [% upload_timestamp %]</li> @@ -384,6 +394,15 @@ $(document).ready(function(){ <br style="clear:both;" /> [% IF ( batch_list ) %] + [% IF Koha.Preference( 'StageFilterByUser' ) %] + <p> + [% IF ( no_filter ) %] + <a href="[% batch_lis.script_name %]?no_filter=0">Filter by logged in user</a> + [% ELSE %] + <a href="[% batch_lis.script_name %]?no_filter=1">Do not filter list</a> + [% END %] + </p> + [% END %] [% IF ( pages ) %] <div class="pages"> Page @@ -400,6 +419,7 @@ Page <tr> <th>#</th> <th>File name</th> + <th>User</th> <th>Comments</th> <th>Type</th> <th>Status</th> @@ -412,6 +432,7 @@ Page <tr> <td>[% batch_lis.import_batch_id %]</td> <td><a href="[% batch_lis.script_name %]?import_batch_id=[% batch_lis.import_batch_id %]">[% batch_lis.file_name %]</a></td> + <td>[% batch_lis.user_name %]</td> <td>[% batch_lis.comments %]</td> <td>[% IF ( batch_lis.record_type == 'auth' ) %]Authority[% ELSE %]Bibliographic[% END %]</td> <td> diff --git a/t/ImportBatch.t b/t/ImportBatch.t index 9db2c27..cfea175 100644 --- a/t/ImportBatch.t +++ b/t/ImportBatch.t @@ -21,7 +21,7 @@ use File::Temp qw|tempfile|; use MARC::Field; use MARC::File::XML; use MARC::Record; -use Test::More tests => 3; +use Test::More tests => 4; use t::lib::Mocks; BEGIN { @@ -30,6 +30,54 @@ BEGIN { t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); +subtest 'StageFilterByUser' => sub { + plan tests => 5; + + # Set current user to "user1" + C4::Context->_new_userenv('USER1-ENV'); + C4::Context->set_userenv('12345', 'user1', '12345', '', '', '', '', 1, '', ''); + C4::Context->set_preference( 'StageFilterByUser', '1'); + C4::Context->set_preference( 'StageHideCleanedImported', '1'); + + my $file = create_file({ two => 1, format => 'marc'}); + my ($errors, $recs) = C4::ImportBatch::RecordsFromISO2709File($file, 'biblio', 'UTF-8'); + + my ($batch_id) = C4::ImportBatch::BatchStageMarcRecords('biblio', 'UTF-8', $recs, + 'file1.mrc', undef, undef, '', + '', 1, 0, 0, undef); + + my $batch_list_user1 = C4::ImportBatch::GetImportBatchRangeDesc(0, 100000); + is( find_id_in_batch_list($batch_id, $batch_list_user1), 1, "Found one batch from user1"); + + # Set current user to "user2" + C4::Context->_new_userenv('USER2-ENV'); + C4::Context->set_userenv('23456', 'user2', '23456', '', '', '', '', 1, '', ''); + + my $batch_list_user2 = C4::ImportBatch::GetImportBatchRangeDesc(0, 100000); + is( find_id_in_batch_list($batch_id, $batch_list_user2), 0, "Did not see batches from user1 when logged in as user2"); + + $batch_list_user2 = C4::ImportBatch::GetImportBatchRangeDesc(0, 100000, 1); + is( find_id_in_batch_list($batch_id, $batch_list_user2), 1, "Can see batch from user1 as user2 when filter is disabled"); + + # Set current user back to "user1" + C4::Context->_new_userenv('USER1-ENV'); + C4::Context->set_userenv('12345', 'user1', '12345', '', '', '', '', 1, '', ''); + + C4::Context->set_preference( 'StageHideCleanedImported', '1'); + C4::ImportBatch::SetImportBatchStatus($batch_id, 'imported'); + + $batch_list_user1 = C4::ImportBatch::GetImportBatchRangeDesc(0, 100000); + is( find_id_in_batch_list($batch_id, $batch_list_user1), 0, "Did not see imported batch from user1 hen hidden"); + + C4::Context->set_preference( 'StageHideCleanedImported', '0'); + C4::ImportBatch::SetImportBatchStatus($batch_id, 'imported'); + + $batch_list_user1 = C4::ImportBatch::GetImportBatchRangeDesc(0, 100000); + is( find_id_in_batch_list($batch_id, $batch_list_user1), 1, "Did see imported batch from user1 when not hidden"); + + C4::ImportBatch::DeleteBatch($batch_id); +}; + subtest 'RecordsFromISO2709File' => sub { plan tests => 4; @@ -99,3 +147,14 @@ sub create_file { close $fh; return $name; } + +sub find_id_in_batch_list { + my ($batch_id, $batch_list) = @_; + my $found = 0; + foreach my $batch (@$batch_list) { + if($batch->{import_batch_id} == $batch_id) { + $found = 1; + } + } + return $found; +} diff --git a/tools/manage-marc-import.pl b/tools/manage-marc-import.pl index 249cabe..d7ab98e 100755 --- a/tools/manage-marc-import.pl +++ b/tools/manage-marc-import.pl @@ -32,6 +32,7 @@ use C4::Auth; use C4::AuthoritiesMarc; use C4::Output; use C4::Biblio; +use C4::Members; use C4::ImportBatch; use C4::Matcher; use C4::BackgroundJob; @@ -42,6 +43,7 @@ my $script_name = "/cgi-bin/koha/tools/manage-marc-import.pl"; my $input = new CGI; my $op = $input->param('op') || ''; +my $no_filter = $input->param('no_filter') || 0; my $completedJobID = $input->param('completedJobID'); our $runinbackground = $input->param('runinbackground'); my $import_batch_id = $input->param('import_batch_id') || ''; @@ -91,7 +93,7 @@ if ($op) { if ($op eq "") { # displaying a list if ($import_batch_id eq '') { - import_batches_list($template, $offset, $results_per_page); + import_batches_list($template, $offset, $results_per_page, $no_filter); } else { import_records_list($template, $import_batch_id, $offset, $results_per_page); } @@ -112,14 +114,14 @@ if ($op eq "") { import_records_list($template, $import_batch_id, $offset, $results_per_page); } elsif ($op eq "clean-batch") { CleanBatch($import_batch_id); - import_batches_list($template, $offset, $results_per_page); + import_batches_list($template, $offset, $results_per_page, $no_filter); $template->param( did_clean => 1, import_batch_id => $import_batch_id, ); } elsif ($op eq "delete-batch") { DeleteBatch($import_batch_id); - import_batches_list($template, $offset, $results_per_page); + import_batches_list($template, $offset, $results_per_page, $no_filter); $template->param( did_delete => 1, ); @@ -203,11 +205,17 @@ sub create_labelbatch_from_importbatch { } sub import_batches_list { - my ($template, $offset, $results_per_page) = @_; - my $batches = GetImportBatchRangeDesc($offset, $results_per_page); + my ($template, $offset, $results_per_page, $no_filter) = @_; + my $batches = GetImportBatchRangeDesc($offset, $results_per_page, $no_filter); my @list = (); foreach my $batch (@$batches) { + my $borrower = GetMember(borrowernumber => $batch->{'borrowernumber'}); + my $user_name = ""; + if($borrower) { + $user_name = $borrower->{'userid'}; + } + push @list, { import_batch_id => $batch->{'import_batch_id'}, num_records => $batch->{'num_records'}, @@ -218,6 +226,7 @@ sub import_batches_list { comments => $batch->{'comments'}, can_clean => ($batch->{'import_status'} ne 'cleaned') ? 1 : 0, record_type => $batch->{'record_type'}, + user_name => $user_name, }; } $template->param(batch_list => \@list); @@ -227,6 +236,7 @@ sub import_batches_list { $template->param(range_top => $offset + $results_per_page - 1); $template->param(num_results => $num_batches); $template->param(results_per_page => $results_per_page); + $template->param(no_filter => $no_filter); } @@ -354,6 +364,13 @@ sub import_records_list { my $batch = GetImportBatch($import_batch_id); $template->param(import_batch_id => $import_batch_id); + my $borrower = GetMember(borrowernumber => $batch->{'borrowernumber'}); + my $user_name = ""; + if($borrower) { + $user_name = $borrower->{'userid'}; + } + $template->param("user_name" => $user_name); + my $overlay_action = GetImportBatchOverlayAction($import_batch_id); $template->param("overlay_action_${overlay_action}" => 1); $template->param(overlay_action => $overlay_action); -- 2.7.4