From 2a07ad6b917f30d3c383b80aef378c785a06943b 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. Administration => record overlay rules 2. Add this: Module Filter Tag Preset Added Appended Removed Deleted Source Z39.50 245 Protect Skip Skip Skip Skip Source * 300 Protect Skip Skip Skip Skip Source Batch record modification 245 Protect Skip Skip Skip Skip 3. Add MARC modification templates: Update existing or add new field 245$a with value CATSCATSCATSCATS Update existing or add new field 300$a with value CATSCATSCATSCATS 4. Find a record that has those fields (likely any record would) 5. Use batch record modification on the record 6. 300$a should have been catified (expected, that's the bug) 7. 245$a is preserved 8. Change the value of the 300$a to something else. Temporarly disable MARCOverlayRules to be able to do so. And reenable it. 9. Apply the patch, restart services 10. Use batch record modification on the record 11. 300$a should be preserved, cat protection should have worked 12. Run tests: prove t/db_dependent/Biblio/MarcOverlayRules.t 13. Celebrate! :D 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 Signed-off-by: Victor Grousset/tuxayo --- Koha/MarcOverlayRules.pm | 134 +++++++++++++++++++++++++++++---------- 1 file changed, 102 insertions(+), 32 deletions(-) diff --git a/Koha/MarcOverlayRules.pm b/Koha/MarcOverlayRules.pm index b874f7cd07..26484723e8 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; } -- 2.44.0