Bugzilla – Attachment 81179 Details for
Bug 14957
Write protecting MARC fields based on source of import
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 14957: Fix control field rules regression and add rule validation
Bug-14957-Fix-control-field-rules-regression-and-a.patch (text/plain), 11.26 KB, created by
David Gustafsson
on 2018-10-25 15:23:02 UTC
(
hide
)
Description:
Bug 14957: Fix control field rules regression and add rule validation
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2018-10-25 15:23:02 UTC
Size:
11.26 KB
patch
obsolete
>From 1b26f853207b0dd778f92a89edec77d478c5cf37 Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >Date: Thu, 25 Oct 2018 17:16:13 +0200 >Subject: [PATCH] Bug 14957: Fix control field rules regression and add rule > validation > >--- > Koha/Exceptions/MarcMergeRule.pm | 55 ++++++++++++++++ > Koha/MarcMergeRule.pm | 27 ++++++++ > Koha/MarcMergeRules.pm | 75 ++++++++++++++-------- > admin/marc-merge-rules.pl | 39 +++++++---- > .../prog/en/modules/admin/marc-merge-rules.tt | 13 ++++ > 5 files changed, 169 insertions(+), 40 deletions(-) > create mode 100644 Koha/Exceptions/MarcMergeRule.pm > >diff --git a/Koha/Exceptions/MarcMergeRule.pm b/Koha/Exceptions/MarcMergeRule.pm >new file mode 100644 >index 0000000000..a3cd708c45 >--- /dev/null >+++ b/Koha/Exceptions/MarcMergeRule.pm >@@ -0,0 +1,55 @@ >+package Koha::Exceptions::MarcMergeRule; >+ >+# This file is part of Koha. >+# >+# Koha is free software; you can redistribute it and/or modify it under the >+# terms of the GNU General Public License as published by the Free Software >+# Foundation; either version 3 of the License, or (at your option) any later >+# version. >+# >+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY >+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR >+# A PARTICULAR PURPOSE. See the GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License along >+# with Koha; if not, write to the Free Software Foundation, Inc., >+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. >+ >+use Modern::Perl; >+ >+use Exception::Class ( >+ >+ 'Koha::Exceptions::MarcMergeRule' => { >+ description => 'Something went wrong!', >+ }, >+ 'Koha::Exceptions::MarcMergeRule::InvalidTagRegExp' => { >+ isa => 'Koha::Exceptions::MarcMergeRule', >+ description => 'Invalid regular expression for tag' >+ }, >+ 'Koha::Exceptions::MarcMergeRule::InvalidControlFieldActions' => { >+ isa => 'Koha::Exceptions::MarcMergeRule', >+ description => 'Invalid control field actions' >+ } >+); >+ >+=head1 NAME >+ >+Koha::Exceptions::MarcMergeRule - Base class for MarcMergeRule exceptions >+ >+=head1 Exceptions >+ >+=head2 Koha::Exceptions::MarcMergeRule >+ >+Generic MarcMergeRule exception >+ >+=head2 Koha::Exceptions::MarcMergeRule::InvalidTagRegExp >+ >+Exception for rule validation when rule tag is an invalid regular expression >+ >+=head2 Koha::Exceptions::MarcMergeRule::InvalidControlFieldActions >+ >+Exception for rule validation for control field rules with invalid combination of actions >+ >+=cut >+ >+1; >diff --git a/Koha/MarcMergeRule.pm b/Koha/MarcMergeRule.pm >index 7e42e292b5..5adf18bb31 100644 >--- a/Koha/MarcMergeRule.pm >+++ b/Koha/MarcMergeRule.pm >@@ -16,6 +16,8 @@ package Koha::MarcMergeRule; > # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > > use Modern::Perl; >+use Koha::Exceptions::MarcMergeRule; >+use Try::Tiny; > > use parent qw(Koha::Object); > >@@ -27,6 +29,31 @@ Koha::MarcMergeRule - Koha SearchField Object class > > =cut > >+sub set { >+ my $self = shift @_; >+ my ($rule_data) = @_; >+ >+ if ($rule_data->{tag} ne '*') { >+ eval { qr/$rule_data->{tag}/ }; >+ if ($@) { >+ Koha::Exceptions::MarcMergeRule::InvalidTagRegExp->throw( >+ "Invalid tag regular expression" >+ ); >+ } >+ } >+ if ( >+ $rule_data->{tag} < 10 && >+ $rule_data->{append} && >+ !$rule_data->{remove} >+ ) { >+ Koha::Exceptions::MarcMergeRule::InvalidControlFieldActions->throw( >+ "Combination of allow append and skip remove not permitted for control fields" >+ ); >+ } >+ >+ $self->SUPER::set(@_); >+} >+ > sub store { > my $self = shift @_; > $cache->clear_from_cache('marc_merge_rules'); >diff --git a/Koha/MarcMergeRules.pm b/Koha/MarcMergeRules.pm >index a58c0b9c07..229c242da2 100644 >--- a/Koha/MarcMergeRules.pm >+++ b/Koha/MarcMergeRules.pm >@@ -18,6 +18,7 @@ package Koha::MarcMergeRules; > use Modern::Perl; > use List::Util qw(first); > use Koha::MarcMergeRule; >+use Carp; > > use parent qw(Koha::Objects); > >@@ -50,7 +51,7 @@ sub context_rules { > > my $rules = $cache->get_from_cache('marc_merge_rules', { unsafe => 1 }); > >- if (!$rules || 1) { >+ if (!$rules) { > $rules = {}; > my @rules_rows = $self->_resultset()->search( > undef, >@@ -206,43 +207,63 @@ sub merge_records { > } > } > >- # Then we get the intersection of control fields, present both in >+ # Then we get the intersection of fields, present both in > # current and incoming record (possibly to be overwritten) > my @common_field_tags = grep { exists $incoming_fields->{$_} } keys %{$current_fields}; > foreach my $tag (@common_field_tags) { > my $rule = $get_matching_field_rule->($tag); >- # Compute intersection and diff using field data >- my $sort_weight = 0; >- my %current_fields_by_data = map { $hash_field_data->($_) => [$sort_weight++, $_] } @{$current_fields->{$tag}}; > >- # Always put incoming fields after current fields >- my %incoming_fields_by_data = map { $hash_field_data->($_) => [$sort_weight++, $_] } @{$incoming_fields->{$tag}}; >+ # Special handling for control fields >+ if ($tag < 10) { >+ if ( >+ $rule->{append}->{allow} && >+ !$rule->{remove}->{allow} >+ ) { >+ # This should be highly unlikely since we have input validation to protect against this case >+ carp "Allowing \"append\" and skipping \"remove\" is not permitted for control fields, falling back to skipping both \"append\" and \"remove\""; >+ push @{$merged_record_fields{$tag}}, @{$current_fields->{$tag}}; >+ } >+ elsif ($rule->{append}->{allow}) { >+ push @{$merged_record_fields{$tag}}, @{$incoming_fields->{$tag}}; >+ } >+ else { >+ push @{$merged_record_fields{$tag}}, @{$current_fields->{$tag}}; >+ } >+ } >+ else { >+ # Compute intersection and diff using field data >+ my $sort_weight = 0; >+ my %current_fields_by_data = map { $hash_field_data->($_) => [$sort_weight++, $_] } @{$current_fields->{$tag}}; > >- my ($current_fields_only, $common_fields, $incoming_fields_only) = $diff_by_key->(\%current_fields_by_data, \%incoming_fields_by_data); >+ # Always put incoming fields after current fields >+ my %incoming_fields_by_data = map { $hash_field_data->($_) => [$sort_weight++, $_] } @{$incoming_fields->{$tag}}; > >- my @merged_fields; >+ my ($current_fields_only, $common_fields, $incoming_fields_only) = $diff_by_key->(\%current_fields_by_data, \%incoming_fields_by_data); > >- # First add common fields (intersection) >- # Unchanged >- if (@{$common_fields}) { >- push @merged_fields, @{$common_fields}; >- } >- # Removed >- if (@{$current_fields_only}) { >- if (!$rule->{remove}->{allow}) { >- push @merged_fields, @{$current_fields_only}; >+ my @merged_fields; >+ >+ # First add common fields (intersection) >+ # Unchanged >+ if (@{$common_fields}) { >+ push @merged_fields, @{$common_fields}; > } >- } >- # Appended >- if (@{$incoming_fields_only}) { >- if ($rule->{append}->{allow}) { >- push @merged_fields, @{$incoming_fields_only}; >+ # Removed >+ if (@{$current_fields_only}) { >+ if (!$rule->{remove}->{allow}) { >+ push @merged_fields, @{$current_fields_only}; >+ } > } >- } >- $merged_record_fields{$tag} //= []; >+ # Appended >+ if (@{$incoming_fields_only}) { >+ if ($rule->{append}->{allow}) { >+ push @merged_fields, @{$incoming_fields_only}; >+ } >+ } >+ $merged_record_fields{$tag} //= []; > >- # Sort ascending according to weight (original order) >- push @{$merged_record_fields{$tag}}, map { $_->[1] } sort { $a->[0] <=> $b->[0] } @merged_fields; >+ # Sort ascending according to weight (original order) >+ push @{$merged_record_fields{$tag}}, map { $_->[1] } sort { $a->[0] <=> $b->[0] } @merged_fields; >+ } > } > > my $merged_record = MARC::Record->new(); >diff --git a/admin/marc-merge-rules.pl b/admin/marc-merge-rules.pl >index 5f62fb3d34..3e45ba08f2 100755 >--- a/admin/marc-merge-rules.pl >+++ b/admin/marc-merge-rules.pl >@@ -22,6 +22,7 @@ use warnings; > use CGI qw ( -utf8 ); > use CGI::Cookie; > use MARC::File::USMARC; >+use Try::Tiny; > > # Koha modules used > use C4::Context; >@@ -116,21 +117,33 @@ elsif ($op eq 'edit') { > } > elsif ($op eq 'doedit' || $op eq 'add') { > my $rule_data = $rule_from_cgi->($input); >- if ($rule_data->{tag} ne '*') { >- eval { qr/$rule_data->{tag}/ }; >- if ($@) { >- push @{$errors}, { >- type => 'error', >- code => 'invalid_tag_regexp', >- tag => $rule_data->{tag}, >- message => $@ >- }; >- } >- } > if (!@{$errors}) { > my $rule = Koha::MarcMergeRules->find_or_create($rule_data); >- $rule->set($rule_data); >- $rule->store(); >+ try { >+ $rule->set($rule_data); >+ $rule->store(); >+ } >+ catch { >+ die $_ unless blessed $_ && $_->can('rethrow'); >+ >+ if ($_->isa('Koha::Exceptions::MarcMergeRule::InvalidTagRegExp')) { >+ push @{$errors}, { >+ type => 'error', >+ code => 'invalid_tag_regexp', >+ tag => $rule_data->{tag}, >+ }; >+ } >+ elsif ($_->isa('Koha::Exceptions::MarcMergeRule::InvalidControlFieldActions')) { >+ push @{$errors}, { >+ type => 'error', >+ code => 'invalid_control_field_actions', >+ tag => $rule_data->{tag}, >+ }; >+ } >+ else { >+ $_->rethrow; >+ } >+ }; > $rules = $get_rules->(); > } > } >diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-merge-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-merge-rules.tt >index 3fcedf2086..8983c29f2e 100644 >--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-merge-rules.tt >+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-merge-rules.tt >@@ -208,6 +208,19 @@ $(document).ready(function(){ > > <h1>Manage MARC merge rules</h1> > >+[% FOR m IN messages %] >+ <div class="dialog [% m.type | html %]"> >+ [% SWITCH m.code %] >+ [% CASE 'invalid_tag_regexp' %] >+ Invalid regular expression "[% m.tag | html %]". >+ [% CASE 'invalid_control_field_actions' %] >+ Invalid combination of actions for tag [% m.tag | html %]. Control field rules do not allow "Appended: Append" and "Removed: Skip". >+ [% CASE %] >+ [% m.code | html %] >+ [% END %] >+ </div> >+[% END %] >+ > [% UNLESS Koha.Preference( 'MARCMergeRules' ) %] > <div class="dialog message"> > The <b>MARCMergeRules</b> preference is not set, don't forget to enable it for rules to take effect. >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 14957
:
44056
|
44057
|
44058
|
51723
|
51724
|
56637
|
56638
|
56643
|
56644
|
56645
|
56646
|
56647
|
56648
|
57114
|
57115
|
57116
|
57117
|
57118
|
57119
|
59811
|
59812
|
59885
|
60711
|
60824
|
60825
|
61065
|
61641
|
61736
|
64792
|
72498
|
74855
|
75118
|
76201
|
76202
|
76203
|
76204
|
76205
|
76206
|
76207
|
76208
|
76209
|
76210
|
78651
|
78652
|
78653
|
79367
|
79368
|
79369
|
79370
|
79890
|
80972
|
80973
|
80974
|
80975
|
81176
|
81177
|
81178
|
81179
|
81621
|
81699
|
81700
|
81705
|
81706
|
81707
|
81708
|
81969
|
81970
|
81971
|
82030
|
82031
|
82775
|
87709
|
87711
|
87771
|
87929
|
88205
|
88206
|
89961
|
90668
|
90669
|
90671
|
90672
|
90722
|
90723
|
90740
|
90741
|
92933
|
92934
|
92935
|
92936
|
92937
|
92938
|
92999
|
93499
|
94909
|
94911
|
99998
|
100052
|
100053
|
100054
|
100055
|
100056
|
100057
|
100058
|
100059
|
100060
|
100061
|
101510
|
109955
|
109956
|
109957
|
109958
|
109959
|
109960
|
109961
|
109962
|
109963
|
116622
|
116623
|
116624
|
116625
|
117844
|
117845
|
117849
|
117850
|
117853
|
117859
|
117964
|
117965
|
117966
|
118918
|
118919
|
118920
|
118921
|
118922
|
118923
|
118924
|
118925
|
118926
|
118927
|
118934
|
118935
|
118936
|
118937
|
118938
|
118939
|
118940
|
118941
|
118942
|
118943
|
118944
|
118945
|
118946
|
118957
|
120533
|
120534
|
120535
|
120536
|
120537
|
120538
|
120539
|
120540
|
120541
|
120542
|
120543
|
120544
|
120545
|
120546
|
120547
|
120548
|
120550
|
120553
|
120554
|
120555
|
120556
|
120557
|
120581
|
120582
|
120583
|
120584
|
120585
|
120586
|
120587
|
120588
|
120589
|
120590
|
120591
|
120592
|
120593
|
120594
|
120595
|
120596
|
120597
|
120598
|
120599
|
120600
|
120601
|
120602
|
120603
|
120604
|
120605
|
120670
|
120671
|
120705
|
125514
|
125515
|
125516
|
125517
|
125518
|
125519
|
125520
|
125521
|
125522
|
125523
|
125524
|
125525
|
125526
|
125527
|
125528
|
125529
|
125530
|
125531
|
125532
|
125533
|
125534
|
125535
|
125536
|
125537
|
125538
|
125539
|
125540
|
125632
|
125633
|
125636
|
126424
|
126425
|
126431
|
126592
|
126666
|
126667
|
126752
|
126753
|
126754
|
126755
|
126756
|
126757
|
126758
|
126759
|
126760
|
126761
|
126762
|
126763
|
126764
|
126765
|
126766
|
126767
|
126768
|
126769
|
126770
|
126771
|
126772
|
126773
|
126774
|
126775
|
126776
|
126777
|
126778
|
126779
|
126780
|
126781
|
126782
|
126783
|
126784
|
126785
|
126786
|
126950
|
126951