Attachment 159518 Details for Bug 33268 – Bug 33268: Overlay rules don't work correctly when source is set to *

[patch] Bug 33268: Overlay rules don't work correctly when source is set to *

Bug-33268-Overlay-rules-dont-work-correctly-when-s.patch (text/plain), 17.67 KB, created by Tomás Cohen Arazi (tcohen) on 2023-12-04 11:35:09 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2023-12-04 11:35:09 UTC

Size: 17.67 KB

patch

obsolete

>From 73c9933e51daf11526cd1fe30fdf831e6c11b148 Mon Sep 17 00:00:00 2001
>From: David Gustafsson <david.gustafsson@ub.gu.se>
>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 <david@davidnind.com>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>Edit: tidied inline (tcohen)
>---
> 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 <http://www.gnu.org/licenses>.
> 
> 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

Actions: View | Diff | Splinter Review

Attachments on bug 33268: 158956 | 159004 | 159058 | 159160 | 159161 | 159518 | 159519 | 160017 | 160018 | 162764 | 162766 | 162767 | 162785 | 162786 | 162796 | 162797 | 162798 | 162799 | 162800 | 162801 | 162802 | 162803 | 162804 | 162805 | 162936 | 162937 | 162938 | 162939 | 162940