From 424cb691d6a95270097daf48c8e316732ef9ee34 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Tue, 14 Nov 2023 19:56:32 +0100 Subject: [PATCH] Bug 33268: Overlay rules don't work correctly when source is set to * Fallback to overlay rules with wildcard filter if no match found for exact filter match. To test: 1) Run tests t/db_dependent/Biblio/MarcOverlayRules.t Sponsored-by: Gothenburg University Library Signed-off-by: David Nind Signed-off-by: Tomas Cohen Arazi Edit: tidied inline (tcohen) Signed-off-by: Martin Renvoize --- Koha/MarcOverlayRules.pm | 134 ++++++++++++---- t/db_dependent/Biblio/MarcOverlayRules.t | 195 +++++++++++++++++++++-- 2 files changed, 284 insertions(+), 45 deletions(-) diff --git a/Koha/MarcOverlayRules.pm b/Koha/MarcOverlayRules.pm index b874f7cd075..26484723e85 100644 --- a/Koha/MarcOverlayRules.pm +++ b/Koha/MarcOverlayRules.pm @@ -16,13 +16,14 @@ package Koha::MarcOverlayRules; # along with Koha; if not, see . use Modern::Perl; -use List::Util qw(first); +use List::Util qw(first any); use Koha::MarcOverlayRule; use Carp; use Koha::Exceptions::MarcOverlayRule; use Try::Tiny; use Scalar::Util qw(looks_like_number); +use Clone qw(clone); use parent qw(Koha::Objects); @@ -46,6 +47,16 @@ sub operations { return ('add', 'append', 'remove', 'delete'); } +=head3 modules + +Returns a list of all modules in order of priority. + +=cut + +sub modules { + return ( 'userid', 'categorycode', 'source' ); +} + =head3 context_rules my $rules = Koha::MarcOverlayRules->context_rules($context); @@ -55,61 +66,120 @@ Gets all MARC overlay rules for the supplied C<$context> (hashref with { module =cut sub context_rules { - my ($self, $context) = @_; + my ( $self, $context ) = @_; return unless %{$context}; - my $rules = $cache->get_from_cache('marc_overlay_rules', { unsafe => 1 }); + my $rules = $cache->get_from_cache( 'marc_overlay_rules', { unsafe => 1 } ); - if (!$rules) { + if ( !$rules ) { $rules = {}; my @rules_rows = $self->_resultset()->search( undef, - { - order_by => { -desc => [qw/id/] } - } + { order_by => { -desc => [qw/id/] } } ); foreach my $rule_row (@rules_rows) { - my %rule = $rule_row->get_columns(); + my %rule = $rule_row->get_columns(); my $operations = {}; - foreach my $operation ($self->operations) { + foreach my $operation ( $self->operations ) { $operations->{$operation} = { allow => $rule{$operation}, rule => $rule{id} }; } # TODO: Remove unless check and validate on saving rules? - if ($rule{tag} eq '*') { - unless (exists $rules->{$rule{module}}->{$rule{filter}}->{'*'}) { - $rules->{$rule{module}}->{$rule{filter}}->{'*'} = $operations; + if ( $rule{tag} eq '*' ) { + unless ( exists $rules->{ $rule{module} }->{ $rule{filter} }->{'*'} ) { + $rules->{ $rule{module} }->{ $rule{filter} }->{'*'} = $operations; } - } - elsif ($rule{tag} =~ /^(\d{3})$/) { - unless (exists $rules->{$rule{module}}->{$rule{filter}}->{tags}->{$rule{tag}}) { - $rules->{$rule{module}}->{$rule{filter}}->{tags}->{$rule{tag}} = $operations; + } elsif ( $rule{tag} =~ /^(\d{3})$/ ) { + unless ( exists $rules->{ $rule{module} }->{ $rule{filter} }->{tags}->{ $rule{tag} } ) { + $rules->{ $rule{module} }->{ $rule{filter} }->{tags}->{ $rule{tag} } = $operations; } - } - else { - my $regexps = ($rules->{$rule{module}}->{$rule{filter}}->{regexps} //= []); - push @{$regexps}, [$rule{tag}, $operations]; + } else { + my $regexps = ( $rules->{ $rule{module} }->{ $rule{filter} }->{regexps} //= [] ); + push @{$regexps}, [ $rule{tag}, $operations ]; } } - $cache->set_in_cache('marc_overlay_rules', $rules); + $cache->set_in_cache( 'marc_overlay_rules', $rules ); } - my $context_rules = undef; - foreach my $module_name (keys %{$context}) { - if ( - exists $rules->{$module_name} && - exists $rules->{$module_name}->{$context->{$module_name}} - ) { - $context_rules = $rules->{$module_name}->{$context->{$module_name}}; + my $context_rules; + my @context_modules = grep { exists $context->{$_} } $self->modules(); + + foreach my $module_name (@context_modules) { + if ( exists $rules->{$module_name} + && exists $rules->{$module_name}->{ $context->{$module_name} } ) + { + $context_rules = $rules->{$module_name}->{ $context->{$module_name} }; + + unless ( exists $rules->{$module_name}->{'*'} ) { + + # No wildcard filter rules defined + last; + } + + # Merge in all wildcard filter rules which has not been overridden + # by the matching context + if ( $context_rules->{'*'} ) { + + # Wildcard tag for matching context will override all rules, + # nothing left to do + last; + } + + # Clone since we will potentially be modified cached value + # fetched with unsafe => 1 + $context_rules = clone($context_rules); + + if ( exists $rules->{$module_name}->{'*'}->{'*'} ) { + + # Merge in wildcard filter wildcard tag rule if exists + $context_rules->{'*'} = $rules->{$module_name}->{'*'}->{'*'}; + } + + if ( exists $rules->{$module_name}->{'*'}->{tags} ) { + if ( exists $context_rules->{regexps} ) { + + # If the current context has regexp rules, we have to make sure + # to not only to skip tags already present but also those matching + # any of those rules + + my @regexps = map { qr/^$_->[0]$/ } @{ $context_rules->{regexps} }; + + foreach my $tag ( keys %{ $rules->{$module_name}->{'*'}->{tags} } ) { + unless ( ( any { $tag =~ $_ } @regexps ) || exists $context_rules->{tags}->{$tag} ) { + $context_rules->{tags}->{$tag} = $rules->{$module_name}->{'*'}->{tags}->{$tag}; + } + } + } else { + + # Merge in wildcard filter tag rules not already present + # in the matching context + foreach my $tag ( keys %{ $rules->{$module_name}->{'*'}->{tags} } ) { + unless ( exists $context_rules->{tags}->{$tag} ) { + $context_rules->{tags}->{$tag} = $rules->{$module_name}->{'*'}->{tags}->{$tag}; + } + } + } + } + + if ( exists $rules->{$module_name}->{'*'}->{regexps} ) { + + # Merge in wildcard filter regexp rules last, making sure rules from the + # matching context have precedence + $context_rules->{regexps} //= []; + push @{ $context_rules->{regexps} }, @{ $rules->{$module_name}->{'*'}->{regexps} }; + } + last; } } - if (!$context_rules) { - # No perms matching specific context conditions found, try wildcard value for each active context - foreach my $module_name (keys %{$context}) { - if (exists $rules->{$module_name}->{'*'}) { + + if ( !$context_rules ) { + + # No rules matching specific context conditions found, try wildcard value for each active context + foreach my $module_name (@context_modules) { + if ( exists $rules->{$module_name}->{'*'} ) { $context_rules = $rules->{$module_name}->{'*'}; last; } diff --git a/t/db_dependent/Biblio/MarcOverlayRules.t b/t/db_dependent/Biblio/MarcOverlayRules.t index dc66860a8bb..4156907645a 100755 --- a/t/db_dependent/Biblio/MarcOverlayRules.t +++ b/t/db_dependent/Biblio/MarcOverlayRules.t @@ -64,19 +64,16 @@ my $orig_record = build_record([ [ '250', 'a', '250 bottles of beer on the wall' ], [ '250', 'a', '256 bottles of beer on the wall' ], [ '500', 'a', 'One bottle of beer in the fridge' ], -] -); +]); -my $incoming_record = build_record( - [ +my $incoming_record = build_record([ ['250', 'a', '256 bottles of beer on the wall'], # Unchanged ['250', 'a', '251 bottles of beer on the wall'], # Appended #['250', 'a', '250 bottles of beer on the wall'], # Removed #['500', 'a', 'One bottle of beer in the fridge'], # Deleted ['501', 'a', 'One cold bottle of beer in the fridge'], # Added ['501', 'a', 'Two cold bottles of beer in the fridge'], # Added -] -); +]); # Test default behavior when MARCOverlayRules is enabled, but no rules defined (overwrite) subtest 'No rule defined' => sub { @@ -685,24 +682,195 @@ subtest '"5XX" fields has been protected when rule matching on regexp "5\d{2}" i $skip_all_rule->delete(); -# Test module specificity, the 0 all rule should no longer be included in set of applied rules -subtest 'Record fields has been overwritten when non wild card rule with filter match is add = 1, append = 1, remove = 1, delete = 1' => sub { - plan tests => 1; +# Test module filter specificity +subtest 'Module filter precedence tests' => sub { + plan tests => 7; $rule->set( { - 'filter' => 'test', + tag => '*', + module => 'source', + filter => '*', + add => 0, + append => 0, + remove => 0, + delete => 0, } )->store(); + my $matching_filter_rule_overwrite = Koha::MarcOverlayRules->find_or_create( + { + tag => '*', + module => 'source', + filter => 'test', + add => 1, + append => 1, + remove => 1, + delete => 1 + } + ); + + # Current rules: + # source: *, tag: *, add: 0, append: 0, remove: 0, delete: 0 + # source: test, tag: *, add: 1, append: 1, remove: 1, delete: 1 my $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); is( $merged_record->as_formatted, $incoming_record->as_formatted, - 'Only existing rule is not for us, erasing' # FIXME Is this comment correct? + 'Non wildcard rule with wildcard tag matches and overrides wildcard rule with wildcard tag' + ); + + $rule->set( + { + tag => '5\d{2}' + } + )->store(); + + # Current rules: + # source: *, tag: 5\d{2}, add: 0, append: 0, remove: 0, delete: 0 + # source: test, tag: *, add: 1, append: 1, remove: 1, delete: 1 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + is( + $merged_record->as_formatted, + $incoming_record->as_formatted, + 'Non wildcard rule with wildcard tag matches and overrides wildcard rules with regexp tags' + ); + + $rule->set( + { + tag => '501' + } + )->store(); + + # Current rules: + # source: *, tag: 501, add: 0, append: 0, remove: 0, delete: 0 + # source: test, tag: *, add: 1, append: 1, remove: 1, delete: 1 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + is( + $merged_record->as_formatted, + $incoming_record->as_formatted, + 'Non wildcard rule with wildcard tag matches and overrides wildcard rules with specific tags' ); + $rule->set( + { + tag => '*' + } + )->store(); + + my $wildcard_filter_rule_overwrite = Koha::MarcOverlayRules->find_or_create( + { + tag => '501', + module => 'source', + filter => '*', + add => 1, + append => 1, + remove => 1, + delete => 1, + } + ); + + $matching_filter_rule_overwrite->set( + { + tag => '250' + } + )->store(); + + # Current rules: + # source: *, tag: *, add: 0, append: 0, remove: 0, delete: 0 + # source: *, tag: 501, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 250, add: 1, append: 1, remove: 1, delete: 1 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + my $expected_record = build_record([ + ['250', 'a', '256 bottles of beer on the wall'], # Unchanged + ['250', 'a', '251 bottles of beer on the wall'], # Appended + #['250', 'a', '250 bottles of beer on the wall'], # Removed + ['500', 'a', 'One bottle of beer in the fridge'], # Protected + ['501', 'a', 'One cold bottle of beer in the fridge'], # Added + ['501', 'a', 'Two cold bottles of beer in the fridge'], # Added + ]); + + is( + $merged_record->as_formatted, + $expected_record->as_formatted, + 'Rules have been merged when non wildcard filter context does not contain any wildcard tag rules' + ); + + my $matching_filter_rule_protect = Koha::MarcOverlayRules->find_or_create( + { + tag => '501', + module => 'source', + filter => 'test', + add => 0, + append => 0, + remove => 0, + delete => 0 + } + ); + + # Current rules: + # source: *, tag: *, add: 0, append: 0, remove: 0, delete: 0 + # source: *, tag: 501, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 250, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 501, add: 0, append: 0, remove: 0, delete: 0 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + $expected_record = build_record([ + ['250', 'a', '256 bottles of beer on the wall'], # Unchanged + ['250', 'a', '251 bottles of beer on the wall'], # Appended + #['250', 'a', '250 bottles of beer on the wall'], # Removed + ['500', 'a', 'One bottle of beer in the fridge'], # Protected + #['501', 'a', 'One cold bottle of beer in the fridge'], # Protected + #['501', 'a', 'Two cold bottles of beer in the fridge'], # Protected + ]); + + is( + $merged_record->as_formatted, + $expected_record->as_formatted, + 'Wildcard filter rule has been overridden by non wildcard filter rule with same tag' + ); + + $matching_filter_rule_protect->set({ + tag => '5\d{2}', + })->store(); + + # Current rules: + # source: *, tag: *, add: 0, append: 0, remove: 0, delete: 0 + # source: *, tag: 501, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 250, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 5\d{2}, add: 0, append: 0, remove: 0, delete: 0 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + is( + $merged_record->as_formatted, + $expected_record->as_formatted, + 'Wildcard filter rules with tags that matches tag regexps for non wildcard filter rules has been overridden' + ); + + $wildcard_filter_rule_overwrite->set({ + tag => '5\d{2}', + })->store(); + + # Current rules: + # source: *, tag: *, add: 0, append: 0, remove: 0, delete: 0 + # source: *, tag: 5\d{2}, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 250, add: 1, append: 1, remove: 1, delete: 1 + # source: test, tag: 5\d{2}, add: 0, append: 0, remove: 0, delete: 0 + $merged_record = Koha::MarcOverlayRules->merge_records($orig_record, $incoming_record, { 'source' => 'test' }); + + is( + $merged_record->as_formatted, + $expected_record->as_formatted, + 'Wildcard filter rules with tags with tag regexps matching the same tag as regexps for non wildcard filter rules has been overridden' + ); + + $wildcard_filter_rule_overwrite->delete(); + $matching_filter_rule_overwrite->delete(); + $matching_filter_rule_protect->delete(); }; subtest 'An exception is thrown when append = 1, remove = 0 is set for control field rule' => sub { @@ -765,7 +933,7 @@ subtest 'context option in ModBiblio is handled correctly' => sub { my $expected_record = build_record([ # "250" field has been appended [ '250', 'a', '250 bottles of beer on the wall' ], # original - [ '250', 'a', '256 bottles of beer on the wall' ], # incoming + [ '250', 'a', '256 bottles of beer on the wall' ], # original [ '500', 'a', 'One bottle of beer in the fridge' ], # original [ '999', 'c', $biblionumber, 'd', $biblioitemnumber ], # created by AddBiblio ]); @@ -788,10 +956,11 @@ subtest 'context option in ModBiblio is handled correctly' => sub { my $updated_record = $biblio->get_from_storage->metadata->record; $expected_record = build_record([ - # "250" field has been appended + # "250" field has been protected [ '250', 'a', '250 bottles of beer on the wall' ], [ '250', 'a', '256 bottles of beer on the wall' ], [ '500', 'a', 'One bottle of beer in the fridge' ], + # "500" field has been appended [ '500', 'a', 'One cold bottle of beer in the fridge' ], [ '999', 'c', $biblionumber, 'd', $biblioitemnumber ], # created by AddBiblio ]); -- 2.43.0