Attachment 66915 Details for Bug 17380 – Bug 17380: Do not allow Default template in merge form

[patch] Bug 17380: Do not allow Default template in merge form

Bug-17380-Do-not-allow-Default-template-in-merge-f.patch (text/plain), 10.56 KB, created by Nick Clemens (kidclamp) on 2017-09-07 09:52:46 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Nick Clemens (kidclamp)

Created: 2017-09-07 09:52:46 UTC

Size: 10.56 KB

patch

obsolete

>From 300cba47a51d3ce4f87d339b88d212df67c71b59 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>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 <m.de.rooy@rijksmuseum.nl>
>
>Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
>
>Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
>---
> 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) {
> 
> 
> <h1>Merging records</h1>
>-[% IF ( result ) %]
>-    [% IF ( errors ) %]
>+
>+[% IF ( errors ) %]
> 
>     [% FOREACH error IN errors %]
>         <div class="dialog alert">
>-
>-                [% 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 %]
>-
>-            <br />Therefore, the record to be merged has not been deleted.</div>
>+            [% 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 %]
>+        </div>
>     [% END %]
> 
>-    [% ELSE %]
>+[% ELSIF ( result ) %]
>+
>         <script type="text/javascript">window.location.href="/cgi-bin/koha/authorities/detail.pl?authid=[% recordid1 %]"</script>
>         <p>The merging was successful. <a href="/cgi-bin/koha/authorities/detail.pl?authid=[% recordid1 %]">Click here to see the merged record.</a></p>
>-    [% END %]
> 
>-[% ELSE %]
>+[% ELSIF ( choosereference ) %]
> 
>-[% IF ( choosereference ) %]
> <p>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.</p>
> <form id="mergeform" action="/cgi-bin/koha/authorities/merge.pl" method="post">
>     <fieldset class="rows">
>@@ -92,11 +93,11 @@ function changeFramework(fw) {
>                       <select name="frameworkcode" id="frameworkcode">
>                                       [% FOREACH frameworkcodeloo IN frameworkselect %]
>                                           [% IF frameworkcodeloo.authtypecode == frameworkcode1 %]
>-                                              <option value="[% frameworkcodeloo.value %]" selected="selected">
>+                                              <option value="[% frameworkcodeloo.authtypecode %]" selected="selected">
>                                           [% ELSE %]
>-                                              <option value="[% frameworkcodeloo.value %]">
>+                                              <option value="[% frameworkcodeloo.authtypecode %]">
>                                           [% END %]
>-                                           [% frameworkcodeloo.frameworktext %]
>+                                           [% frameworkcodeloo.authtypetext %]
>                                            </option>
>                                       [% END %]
>                       </select></li>
>@@ -108,21 +109,9 @@ function changeFramework(fw) {
>     <fieldset class="action"><input type="submit" value="Next" /></fieldset>
>     </fieldset>
> </form>
>+
> [% ELSE %]
>-[% IF ( errors ) %]
>-    <div class="dialog alert">
>-    [% FOREACH error IN errors %]
>-        <p>
>-                [% 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 %]
>-
>-            </p>
>-    [% END %]
>-    </div>
>-[% ELSE %]
>+
> <form id="mergeform" action="/cgi-bin/koha/authorities/merge.pl" method="post" onsubmit="return mergeformsubmit()">
> 
> <div class="yui-g">
>@@ -141,8 +130,7 @@ function changeFramework(fw) {
> <fieldset class="action"><input type="submit" name="merge" value="Merge" /></fieldset>
> </div>
> </form>
>-[% END %]
>-[% END %]
>+
> [% END %]
> 
> </div>
>-- 
>2.7.4

Actions: View | Diff | Splinter Review

Attachments on bug 17380: 55947 | 60592 | 64952 | 64953 | 64954 | 64955 | 66277 | 66278 | 66279 | 66280 | 66313 | 66314 | 66315 | 66316 | 66750 | 66751 | 66796 | 66797 | 66798 | 66799 | 66800 | 66801 | 66912 | 66913 | 66914 | 66915 | 66916 | 66917