Attachment 162938 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), 9.12 KB, created by Victor Grousset/tuxayo on 2024-03-08 04:05:29 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Victor Grousset/tuxayo

Created: 2024-03-08 04:05:29 UTC

Size: 9.12 KB

patch

obsolete

>From 2a07ad6b917f30d3c383b80aef378c785a06943b 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. 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 <david@davidnind.com>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>Edit: tidied inline (tcohen)
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
>---
> 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 <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;
>             }
>-- 
>2.44.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