Bugzilla – Attachment 159160 Details for
Bug 33268
Overlay rules don't work correctly when source is set to *
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[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), 14.71 KB, created by
David Nind
on 2023-11-21 13:41:01 UTC
(
hide
)
Description:
Bug 33268: Overlay rules don't work correctly when source is set to *
Filename:
MIME Type:
Creator:
David Nind
Created:
2023-11-21 13:41:01 UTC
Size:
14.71 KB
patch
obsolete
>From ff827ff4b3d05dbacaba21a2e8f808b217fbf909 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> >--- > Koha/MarcOverlayRules.pm | 80 +++++++++- > t/db_dependent/Biblio/MarcOverlayRules.t | 195 +++++++++++++++++++++-- > 2 files changed, 257 insertions(+), 18 deletions(-) > >diff --git a/Koha/MarcOverlayRules.pm b/Koha/MarcOverlayRules.pm >index b874f7cd07..caaa30340e 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,17 @@ 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); >@@ -96,19 +108,77 @@ sub context_rules { > $cache->set_in_cache('marc_overlay_rules', $rules); > } > >- my $context_rules = undef; >- foreach my $module_name (keys %{$context}) { >+ 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 overriden >+ # 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 precidence >+ $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}) { >+ # 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 dc66860a8b..4156907645 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.30.2
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 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
|
175659
|
175660
|
175661
|
175662
|
175663
|
175939
|
175940
|
175941
|
175942
|
175943
|
176201
|
176202
|
176203
|
176204
|
176205