From 309208ee7b6c94cf4fdfac1edbb3ba25a22ae94e Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Mon, 31 Aug 2020 17:01:24 +0000 Subject: [PATCH] Bug 22785: Allow option to choose which record match is applied during import This patchset adds the display of all matches found during import to the import management screen A staff member with the permission to manage batches will be able to select for any individual record which match, or none, should be used during import To test: 1 - Import a batch of records or export existing records from your catalog 2 - Import the file (again) and select a matching rule that will find matches 3 - Note that you now have radio buttons allowing you to select a record, or none 4 - Test scenarios: I - When 'Action if matching record found' is 'Ignore' a - Imported record ignored if match is selected b - 'Action if no match found' followed if no match is selected (Ignore matches) II - When 'Action if matching record found' is 'Replace' a - The chosen record is the one overlayed (you can edit the chosen record before importing to confirm) b - 'Action if no match found' followed if no match is selected (Ignore matches) III - When 'Action if matching record found' is 'Add incoming record' a - Record is added regardless of matches 5 - Confirm 'Diff' 'View' links work as expected 6 - Confirm that after records are imported the radio buttons to choose are disabled Signed-off-by: Andrew Fuerste-Henry --- C4/ImportBatch.pm | 58 +++++++--------- api/v1/swagger/definitions.json | 3 + api/v1/swagger/parameters.json | 3 + api/v1/swagger/paths.json | 3 + api/v1/swagger/x-primitives.json | 8 +++ .../data/mysql/atomicupdate/bug_22785.pl | 16 +++++ installer/data/mysql/kohastructure.sql | 1 + .../en/modules/tools/manage-marc-import.tt | 69 +++++++++++++++---- reports/acquisitions_stats.pl | 1 + t/db_dependent/ImportBatch.t | 29 ++++++++ t/db_dependent/Koha/Import/Record/Matches.t | 14 +++- tools/batch_records_ajax.pl | 35 +++++----- 12 files changed, 174 insertions(+), 66 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_22785.pl diff --git a/C4/ImportBatch.pm b/C4/ImportBatch.pm index db26883354..c81ad2cd2c 100644 --- a/C4/ImportBatch.pm +++ b/C4/ImportBatch.pm @@ -701,7 +701,6 @@ sub BatchCommitRecords { } elsif ($record_result eq 'ignore') { $recordid = $record_match; $num_ignored++; - $recordid = $record_match; if ($record_type eq 'biblio' and defined $recordid and ( $item_result eq 'create_new' || $item_result eq 'replace' ) ) { my ($bib_items_added, $bib_items_replaced, $bib_items_errored) = BatchCommitItems($rowref->{'import_record_id'}, $recordid, $item_result); $num_items_added += $bib_items_added; @@ -1190,6 +1189,7 @@ sub GetBestRecordMatch { WHERE import_record_matches.import_record_id = ? AND ( (import_records.record_type = 'biblio' AND biblio.biblionumber IS NOT NULL) OR (import_records.record_type = 'auth' AND auth_header.authid IS NOT NULL) ) + AND chosen = 1 ORDER BY score DESC, candidate_match_id DESC"); $sth->execute($import_record_id); my ($record_id) = $sth->fetchrow_array(); @@ -1470,12 +1470,13 @@ sub GetImportRecordMatches { my $dbh = C4::Context->dbh; # FIXME currently biblio only my $sth = $dbh->prepare_cached("SELECT title, author, biblionumber, - candidate_match_id, score, record_type + candidate_match_id, score, record_type, + chosen FROM import_records JOIN import_record_matches USING (import_record_id) LEFT JOIN biblio ON (biblionumber = candidate_match_id) WHERE import_record_id = ? - ORDER BY score DESC, biblionumber DESC"); + ORDER BY score DESC, chosen DESC, biblionumber DESC"); $sth->bind_param(1, $import_record_id); my $results = []; $sth->execute(); @@ -1508,10 +1509,12 @@ sub SetImportRecordMatches { $delsth->execute($import_record_id); $delsth->finish(); - my $sth = $dbh->prepare("INSERT INTO import_record_matches (import_record_id, candidate_match_id, score) - VALUES (?, ?, ?)"); + my $sth = $dbh->prepare("INSERT INTO import_record_matches (import_record_id, candidate_match_id, score, chosen) + VALUES (?, ?, ?, ?)"); + my $chosen = 1; #The first match is defaulted to be chosen foreach my $match (@matches) { - $sth->execute($import_record_id, $match->{'record_id'}, $match->{'score'}); + $sth->execute($import_record_id, $match->{'record_id'}, $match->{'score'}, $chosen); + $chosen = 0; #After the first we do not default to other matches } } @@ -1728,41 +1731,30 @@ sub _get_commit_action { if ($record_type eq 'biblio') { my ($bib_result, $bib_match, $item_result); - if ($overlay_status ne 'no_match') { - $bib_match = GetBestRecordMatch($import_record_id); - if ($overlay_action eq 'replace') { - $bib_result = defined($bib_match) ? 'replace' : 'create_new'; - } elsif ($overlay_action eq 'create_new') { - $bib_result = 'create_new'; - } elsif ($overlay_action eq 'ignore') { - $bib_result = 'ignore'; - } - if($item_action eq 'always_add' or $item_action eq 'add_only_for_matches'){ + $bib_match = GetBestRecordMatch($import_record_id); + if ($overlay_status ne 'no_match' && defined($bib_match)) { + + $bib_result = $overlay_action; + + if($item_action eq 'always_add' or $item_action eq 'add_only_for_matches'){ $item_result = 'create_new'; - } - elsif($item_action eq 'replace'){ - $item_result = 'replace'; - } - else { - $item_result = 'ignore'; - } + } elsif($item_action eq 'replace'){ + $item_result = 'replace'; + } else { + $item_result = 'ignore'; + } + } else { $bib_result = $nomatch_action; - $item_result = ($item_action eq 'always_add' or $item_action eq 'add_only_for_new') ? 'create_new' : 'ignore'; + $item_result = ($item_action eq 'always_add' or $item_action eq 'add_only_for_new') ? 'create_new' : 'ignore'; } return ($bib_result, $item_result, $bib_match); } else { # must be auths my ($auth_result, $auth_match); - if ($overlay_status ne 'no_match') { - $auth_match = GetBestRecordMatch($import_record_id); - if ($overlay_action eq 'replace') { - $auth_result = defined($auth_match) ? 'replace' : 'create_new'; - } elsif ($overlay_action eq 'create_new') { - $auth_result = 'create_new'; - } elsif ($overlay_action eq 'ignore') { - $auth_result = 'ignore'; - } + $auth_match = GetBestRecordMatch($import_record_id); + if ($overlay_status ne 'no_match' && defined($auth_match)) { + $auth_result = $overlay_action; } else { $auth_result = $nomatch_action; } diff --git a/api/v1/swagger/definitions.json b/api/v1/swagger/definitions.json index 402eed6b25..dcd17e7ece 100644 --- a/api/v1/swagger/definitions.json +++ b/api/v1/swagger/definitions.json @@ -47,6 +47,9 @@ "import_batch_profiles": { "$ref": "definitions/import_batch_profiles.json" }, + "import_record_match": { + "$ref": "definitions/import_record_match.json" + }, "library": { "$ref": "definitions/library.json" }, diff --git a/api/v1/swagger/parameters.json b/api/v1/swagger/parameters.json index 95464e1461..71374834ee 100644 --- a/api/v1/swagger/parameters.json +++ b/api/v1/swagger/parameters.json @@ -23,6 +23,9 @@ "hold_id_pp": { "$ref": "parameters/hold.json#/hold_id_pp" }, + "import_record_id_pp": { + "$ref": "parameters/import_record_match.json#/import_record_id_pp" + }, "club_id_pp": { "$ref": "parameters/club.json#/club_id_pp" }, diff --git a/api/v1/swagger/paths.json b/api/v1/swagger/paths.json index 3d3338e0bd..7288899c90 100644 --- a/api/v1/swagger/paths.json +++ b/api/v1/swagger/paths.json @@ -80,6 +80,9 @@ "/holds/{hold_id}/pickup_location": { "$ref": "paths/holds.json#/~1holds~1{hold_id}~1pickup_location" }, + "/import/{import_batch_id}/records/{import_record_id}/matches/chosen": { + "$ref": "paths/import_record_matches.json#/~1import~1{import_batch_id}~1records~1{import_record_id}~1matches~1chosen" + }, "/items": { "$ref": "paths/items.json#/~1items" }, diff --git a/api/v1/swagger/x-primitives.json b/api/v1/swagger/x-primitives.json index 491f8304ff..1388d5a873 100644 --- a/api/v1/swagger/x-primitives.json +++ b/api/v1/swagger/x-primitives.json @@ -3,6 +3,14 @@ "type": "integer", "description": "Internal biblio identifier" }, + "import_record_id": { + "type": "integer", + "description": "Internal import record identifier" + }, + "candidate_match_id": { + "type": "integer", + "description": "Internal import record match identifier" + }, "advancededitormacro_id": { "type": "integer", "description": "Internal advanced editor macro identifier", diff --git a/installer/data/mysql/atomicupdate/bug_22785.pl b/installer/data/mysql/atomicupdate/bug_22785.pl new file mode 100644 index 0000000000..8a4b92ada8 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_22785.pl @@ -0,0 +1,16 @@ +use Modern::Perl; + +return { + bug_number => "22785", + description => "Add chosen column to import_record_matches", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + unless( column_exists('import_record_matches','chosen') ){ + $dbh->do(q{ + ALTER TABLE import_record_matches ADD COLUMN chosen TINYINT null DEFAULT null AFTER score + }); + say $out "chosen column added to import_record_matches"; + } + }, +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index b59bec3885..7fb0ac2c71 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -2901,6 +2901,7 @@ CREATE TABLE `import_record_matches` ( `import_record_id` int(11) NOT NULL COMMENT 'the id given to the imported bib record (import_records.import_record_id)', `candidate_match_id` int(11) NOT NULL COMMENT 'the biblio the imported record matches (biblio.biblionumber)', `score` int(11) NOT NULL DEFAULT 0 COMMENT 'the match score', + `chosen` tinyint(1) NULL DEFAULT NULL COMMENT 'whether this match has been allowed or denied', PRIMARY KEY (`import_record_id`,`candidate_match_id`), KEY `record_score` (`import_record_id`,`score`), CONSTRAINT `import_record_matches_ibfk_1` FOREIGN KEY (`import_record_id`) REFERENCES `import_records` (`import_record_id`) ON DELETE CASCADE ON UPDATE CASCADE 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 75777fc3a3..672358bd17 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 @@ -13,6 +13,7 @@ @@ -485,8 +486,8 @@ { "mDataProp": "citation" }, { "mDataProp": "status" }, { "mDataProp": "overlay_status" }, - { "mDataProp": "match_citation" }, - { "mDataProp": "diff_url" }, + { "mDataProp": "" }, + { "mDataProp": "" }, { "mDataProp": "matched" } ], "fnServerData": function(sSource, aoData, fnCallback) { @@ -547,17 +548,45 @@ aData['overlay_status'] ); - if (aData['match_id']) { - [% IF(record_type == 'auth') -%] - var matching_msg = _("Matches authority %s (score=%s):%s"); - [%- ELSE -%] - var matching_msg = _("Matches bibliographic record %s (score=%s):%s"); - [%- END %] - $('td:eq(4)', nRow).html( - matching_msg.format(aData['match_id'], aData['score'], - '' + aData['match_citation'] + '') - ); + if ( aData['matches'].length > 0 ) { + + var any_checked = 0; + $('td:eq(4)', nRow).html(''); + $('td:eq(5)', nRow).html(''); + var checked = ""; + var disabled = ""; + if( aData['status'] == "imported" || aData['status'] == "ignored" ){ + disabled = ' disabled '; + } + aData['matches'].forEach(function(item,index){ + if( item.chosen == 1 ){ + checked = 'checked="checked"'; + any_checked = 1; + } + [% IF(item.record_type == 'auth') -%] + var matching_msg = _("Matches authority %s (score=%s):%s"); + [%- ELSE -%] + var matching_msg = _("Matches bibliographic record %s (score=%s):%s"); + [%- END %] + var match_option = ""; + match_option = ' '; + + var diff_url = '/cgi-bin/koha/tools/showdiffmarc.pl?batchid=%s&importid=%s&id=%s&type=%s'; + var match_citation = ''; + if( item.title ){ match_citation += item.title + ' ' } + if( item.author ){ match_citation += item.author } + $('td:eq(4) ul', nRow).append('
    • ') + ); + $('td:eq(5) ul', nRow).append('
  • ' + _("View") + '
    • '); + }); + checked = ""; + if( !any_checked ){ checked = 'checked="checked"'; } + $('td:eq(4) ul', nRow).prepend('
    • '); + $('td:eq(5) ul', nRow).prepend('
  •  
    • '); } if (aData['diff_url']) { $('td:eq(5)', nRow).html( @@ -584,6 +613,20 @@ }); [% END %] + $("body").on("change", ".chosen", function(e) { + let apimethod = "DELETE"; + let apidata =""; + if( $(this).val() != 'none' ){ + apimethod = 'PUT'; + apidata = JSON.stringify({ candidate_match_id: $(this).val() }); + } + $.ajax({ + url: '/api/v1/import/[% import_batch_id | html %]/records/'+$(this).data('import_record_id')+'/matches/chosen', + method: apimethod, + data: apidata + }).fail(function(){ alert("Unable to update match choices"); return false; }); + }); + $("body").on("click", ".previewMARC", function(e) { e.preventDefault(); var ltitle = $(this).text(); diff --git a/reports/acquisitions_stats.pl b/reports/acquisitions_stats.pl index 1d27870c52..92749065e4 100755 --- a/reports/acquisitions_stats.pl +++ b/reports/acquisitions_stats.pl @@ -494,6 +494,7 @@ sub calculate { if ( @$filters[10] ); $strcalc .= " GROUP BY $linefield, $colfield ORDER BY $linefield,$colfield"; + warn $strcalc; my $dbcalc = $dbh->prepare($strcalc); $dbcalc->execute; diff --git a/t/db_dependent/ImportBatch.t b/t/db_dependent/ImportBatch.t index c96222de5e..93aed5844e 100755 --- a/t/db_dependent/ImportBatch.t +++ b/t/db_dependent/ImportBatch.t @@ -213,4 +213,33 @@ subtest "RecordsFromMarcPlugin" => sub { 'Checked one field in second record' ); }; +subtest "_get_commit_action" => sub { + plan tests => 24; + my $mock_import = Test::MockModule->new("C4::ImportBatch"); + + $mock_import->mock( GetBestRecordMatch => sub { return 5; } ); + foreach my $record_type ( ('biblio','authority') ){ + foreach my $match_action ( ('replace','create_new','ignore') ){ + foreach my $no_match_action ( ('create_new','ignore') ){ + my ($result, $match, $item_result) = + C4::ImportBatch::_get_commit_action($match_action, $no_match_action, 'always_add', 'auto_match', 42, $record_type); + is( $result, $match_action, "When match found amd chosen we return the match_action for $record_type records with match action $match_action and no match action $no_match_action"); + } + } + } + + $mock_import->mock( GetBestRecordMatch => sub { my $matches = undef; return $matches; } ); + foreach my $record_type ( ('biblio','authority') ){ + foreach my $match_action ( ('replace','create_new','ignore') ){ + foreach my $no_match_action ( ('create_new','ignore') ){ + my ($result, $match, $item_result) = + C4::ImportBatch::_get_commit_action($match_action, $no_match_action, 'always_add', 'auto_match', 42, $record_type); + is( $result, $no_match_action, "When no match found or chosen we return the match_action for $record_type records with match action $match_action and no match action $no_match_action"); + } + } + } + +}; + + $schema->storage->txn_rollback; diff --git a/t/db_dependent/Koha/Import/Record/Matches.t b/t/db_dependent/Koha/Import/Record/Matches.t index f37653b363..87c16d94ac 100755 --- a/t/db_dependent/Koha/Import/Record/Matches.t +++ b/t/db_dependent/Koha/Import/Record/Matches.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 5; +use Test::More tests => 7; use Koha::Import::Record::Matches; use Koha::Database; @@ -31,15 +31,23 @@ $schema->storage->txn_begin; my $builder = t::lib::TestBuilder->new; my $nb_of_matches = Koha::Import::Record::Matches->search->count; +my $nb_of_chosen_matches = Koha::Import::Record::Matches->search({ chosen => 1 })->count; -my $match_1 = $builder->build({ source => 'ImportRecordMatch', }); -my $match_2 = $builder->build({ source => 'ImportRecordMatch', value => { import_record_id => $match_1->{import_record_id} } }); +my $match_1 = $builder->build({ source => 'ImportRecordMatch', value => { chosen => 1 } }); +my $match_2 = $builder->build({ source => 'ImportRecordMatch', value => { chosen => 1, import_record_id => $match_1->{import_record_id} } }); is( Koha::Import::Record::Matches->search->count, $nb_of_matches + 2, 'The 2 matches should have been added' ); +is( Koha::Import::Record::Matches->search({chosen => 1 })->count, $nb_of_chosen_matches + 2, 'The 2 chosen matches should have been added' ); + my $retrieved_match_1 = Koha::Import::Record::Matches->search({ import_record_id => $match_1->{import_record_id}, candidate_match_id => $match_1->{candidate_match_id} })->next; is_deeply( $retrieved_match_1->unblessed, $match_1, 'Find a match by import record id and candidate should return the correct match' ); +my $added_chose = Koha::Import::Record::Matches->search({ import_record_id => $match_1->{import_record_id} }); +$added_chose->unset_chosen(); + +is( Koha::Import::Record::Matches->search({chosen => 1 })->count, $nb_of_chosen_matches, 'The 2 added chosen matches should have been unchosen' ); + $retrieved_match_1->delete; is( Koha::Import::Record::Matches->search->count, $nb_of_matches + 1, 'Delete should have deleted the match' ); diff --git a/tools/batch_records_ajax.pl b/tools/batch_records_ajax.pl index 06e3bf69de..3d706013ba 100755 --- a/tools/batch_records_ajax.pl +++ b/tools/batch_records_ajax.pl @@ -68,21 +68,23 @@ my @list = (); foreach my $record (@$records) { my $citation = $record->{'title'} || $record->{'authorized_heading'}; - my $match = GetImportRecordMatches( $record->{'import_record_id'}, 1 ); - my $match_citation = ''; + my $matches = GetImportRecordMatches( $record->{'import_record_id'} ); my $match_id; - if ( $#$match > -1 ) { - if ( $match->[0]->{'record_type'} eq 'biblio' ) { - $match_citation .= $match->[0]->{'title'} - if defined( $match->[0]->{'title'} ); - $match_citation .= ' ' . $match->[0]->{'author'} - if defined( $match->[0]->{'author'} ); - $match_id = $match->[0]->{'biblionumber'}; - } - elsif ( $match->[0]->{'record_type'} eq 'auth' ) { - if ( defined( $match->[0]->{'authorized_heading'} ) ) { - $match_citation .= $match->[0]->{'authorized_heading'}; - $match_id = $match->[0]->{'candidate_match_id'}; + if ( scalar @$matches > 0 ) { + foreach my $match (@$matches){ + my $match_citation = ''; + if ( $match->{'record_type'} eq 'biblio' ) { + $match_citation .= $match->{'title'} + if defined( $match->{'title'} ); + $match_citation .= ' ' . $match->{'author'} + if defined( $match->{'author'} ); + $match->{'match_citation'} = $match_citation; + } + elsif ( $match->{'record_type'} eq 'auth' ) { + if ( defined( $match->{'authorized_heading'} ) ) { + $match_citation .= $match->{'authorized_heading'}; + $match->{'match_citation'} = $match_citation; + } } } } @@ -97,12 +99,11 @@ foreach my $record (@$records) { isbn => $record->{'isbn'}, status => $record->{'status'}, overlay_status => $record->{'overlay_status'}, - match_citation => $match_citation, matched => $record->{'matched_biblionumber'} || $record->{'matched_authid'} || q{}, - score => $#$match > -1 ? $match->[0]->{'score'} : 0, - match_id => $match_id, + score => scalar @$matches > 0 ? $matches->[0]->{'score'} : 0, + matches => $matches, diff_url => $match_id ? "/cgi-bin/koha/tools/showdiffmarc.pl?batchid=$import_batch_id&importid=$record->{import_record_id}&id=$match_id&type=$record->{record_type}" : undef }; } -- 2.20.1