From 300cba47a51d3ce4f87d339b88d212df67c71b59 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Mon, 26 Jun 2017 12:51:59 +0200 Subject: [PATCH] Bug 17380: Do not allow Default template in merge form This patch makes the following changes: [1] Removes Default from the template list. We should not merge with the Default framework, since it does not have a reporting tag. [2] Rearranges the error section in the template. It is confusing to have two error sections in this template. The error CANNOT_MOVE is not used. The error FRAMEWORK_NOT_SELECTED is replaced by WRONG_FRAMEWORK. [3] Do not allow to merge a record with itself. [4] Check if the merge reference record still contains any MARC tags. [5] Additional polishing: Simplify passing frameworks to template. Remove an unused Koha::Authority::Types->search. Remove obsolete POD header for functions from the script. Test plan: [1] Select two authorities to merge. Verify that you cannot select Default anymore as framework for the reference record. [2] Reproduce error WRONG_COUNT by adding another authid=999 in the URL after you selected two authority records for merging. [3] Remove the third authid from the URL and change the first or second authid into an unexisting record id. You should generate an Internal Server Error. The log should show the exception message. [4] Merge two authorities. Deselect all MARC tags. Should trigger the error EMPTY_MARC in the template. [5] Select the same authority record twice for merging. Should trigger the error DESTRUCTIVE_MERGE in the template. Signed-off-by: Marcel de Rooy Signed-off-by: Josef Moravec Signed-off-by: Nick Clemens --- authorities/merge.pl | 60 ++++++++++------------ .../prog/en/modules/authorities/merge.tt | 58 +++++++++------------ 2 files changed, 50 insertions(+), 68 deletions(-) diff --git a/authorities/merge.pl b/authorities/merge.pl index 060db09..b6a7b27 100755 --- a/authorities/merge.pl +++ b/authorities/merge.pl @@ -23,11 +23,12 @@ use CGI qw ( -utf8 ); use C4::Output; use C4::Auth; use C4::AuthoritiesMarc; -use Koha::MetadataRecord::Authority; use C4::Koha; use C4::Biblio; +use Koha::Authority::Types; use Koha::Exceptions; +use Koha::MetadataRecord::Authority; my $input = new CGI; my @authid = $input->multi_param('authid'); @@ -52,12 +53,28 @@ if ($merge) { # Creating a new record from the html code my $record = TransformHtmlToMarc($input, 0); - my $recordid1 = $input->param('recordid1'); - my $recordid2 = $input->param('recordid2'); + my $recordid1 = $input->param('recordid1') // q{}; + my $recordid2 = $input->param('recordid2') // q{}; my $typecode = $input->param('frameworkcode'); + # Some error checking + if( $recordid1 eq $recordid2 ) { + push @errors, { code => 'DESTRUCTIVE_MERGE' }; + } elsif( !$typecode || !Koha::Authority::Types->find($typecode) ) { + push @errors, { code => 'WRONG_FRAMEWORK' }; + } elsif( scalar $record->fields == 0 ) { + push @errors, { code => 'EMPTY_MARC' }; + } + if( @errors ) { + $template->param( errors => \@errors ); + output_html_with_http_headers $input, $cookie, $template->output; + exit; + } + # Rewriting the leader - $record->leader( GetAuthority($recordid1)->leader() ); + if( my $authrec = GetAuthority($recordid1) ) { + $record->leader( $authrec->leader() ); + } # Modifying the reference record # This triggers a merge for the biblios attached to $recordid1 @@ -87,8 +104,9 @@ else { if ( scalar(@authid) != 2 ) { push @errors, { code => "WRONG_COUNT", value => scalar(@authid) }; - } - else { + } elsif( $authid[0] eq $authid[1] ) { + push @errors, { code => 'DESTRUCTIVE_MERGE' }; + } else { my $recordObj1 = Koha::MetadataRecord::Authority->get_from_authid($authid[0]); Koha::Exceptions::ObjectNotFound->throw( "No authority record found for authid $authid[0]\n" ) if !$recordObj1; @@ -104,8 +122,7 @@ else { my $framework; if ( $recordObj1->authtypecode ne $recordObj2->authtypecode && $mergereference ne 'breeding' ) { - $framework = $input->param('frameworkcode') - or push @errors, { code => 'FRAMEWORK_NOT_SELECTED' }; + $framework = $input->param('frameworkcode'); } else { $framework = $recordObj1->authtypecode; @@ -167,17 +184,9 @@ else { title2 => $recordObj2->authorized_heading, ); if ( $recordObj1->authtypecode ne $recordObj2->authtypecode ) { - my $authority_types = Koha::Authority::Types->search( {}, { order_by => ['authtypecode'] } ); - my @frameworkselect; - while ( my $authority_type = $authority_types->next ) { - my %row = ( - value => $authority_type->authtypecode, - frameworktext => $authority_type->authtypetext, - ); - push @frameworkselect, \%row; - } + my $authority_types = Koha::Authority::Types->search( { authtypecode => { '!=' => '' } }, { order_by => ['authtypecode'] } ); $template->param( - frameworkselect => \@frameworkselect, + frameworkselect => $authority_types->unblessed, frameworkcode1 => $recordObj1->authtypecode, frameworkcode2 => $recordObj2->authtypecode, ); @@ -186,22 +195,7 @@ else { } } -my $authority_types = Koha::Authority::Types->search({}, { order_by => ['authtypetext']}); -$template->param( authority_types => $authority_types ); - if (@errors) { - - # Errors $template->param( errors => \@errors ); } - output_html_with_http_headers $input, $cookie, $template->output; -exit; - -=head1 FUNCTIONS - -=cut - -# ------------------------ -# Functions -# ------------------------ diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/merge.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/merge.tt index 57b1c16..493bcbd 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/merge.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/authorities/merge.tt @@ -54,31 +54,32 @@ function changeFramework(fw) {

Merging records

-[% IF ( result ) %] - [% IF ( errors ) %] + +[% IF ( errors ) %] [% FOREACH error IN errors %]
- - [% IF error.code == 'CANNOT_MOVE' %] - The following items could not be moved from the old record to the new one: [% error.value %] - [% ELSIF error.code == 'FRAMEWORK_NOT_SELECTED' %] - No framework has been selected. Please select a framework for merging. - [% ELSE %] - [% error %] - [% END %] - -
Therefore, the record to be merged has not been deleted.
+ [% IF error.code == 'WRONG_COUNT' %] + Number of records provided for merging: [% error.value %]. Currently only 2 records can be merged at a time. + [% ELSIF error.code == 'DESTRUCTIVE_MERGE' %] + You cannot merge a record with itself. Please select two different authorities. + [% ELSIF error.code == 'WRONG_FRAMEWORK' %] + The Default framework cannot be used, or the framework does not exist. Please select another framework for merging. + [% ELSIF error.code == 'EMPTY_MARC' %] + Sorry, but we did not find any MARC tags in the reference record. + [% ELSE %] + [% error %] + [% END %] + [% END %] - [% ELSE %] +[% ELSIF ( result ) %] +

The merging was successful. Click here to see the merged record.

- [% END %] -[% ELSE %] +[% ELSIF ( choosereference ) %] -[% IF ( choosereference ) %]

Please choose which record will be the reference for the merge. The record chosen as reference will be kept, and the other will be deleted.

@@ -92,11 +93,11 @@ function changeFramework(fw) { @@ -108,21 +109,9 @@ function changeFramework(fw) {
+ [% ELSE %] -[% IF ( errors ) %] -
- [% FOREACH error IN errors %] -

- [% IF error.code == 'WRONG_COUNT' %] - Number of records provided for merging: [% error.value %]. Currently only 2 records can be merged at a time. - [% ELSE %] - [% error %] - [% END %] - -

- [% END %] -
-[% ELSE %] +
@@ -141,8 +130,7 @@ function changeFramework(fw) {
-[% END %] -[% END %] + [% END %] -- 2.7.4