From de0161db1ecbf2106bdf9fbbcf5a4cd9e36a192b Mon Sep 17 00:00:00 2001 From: Matthias Meusburger Date: Wed, 20 Nov 2019 15:59:28 +0100 Subject: [PATCH] Bug 21520: More complex OAI sets mappings Currently, the rules used to create OAI sets are processed with the 'or' boolean operator between each rule. This patch allows to use 'or' or 'and' between the rules. The evaluation of the rules is done according to the boolean operators precedence: AND has a higher precedence than OR. For example: A and B or C and D will be evaluated as follow: (A and B) or (C and D) Test plan: - Apply the patch - Apply the atomicupdate - Prove t/db_dependant/OAI/AndSets.t - Check that existing mappings still work - Try modifying existing mappings - Try creating new mappings - Check that the boolean operators precedence is correctly taken into account Signed-off-by: Michal Denar Signed-off-by: Jonathan Druart Signed-off-by: Martin Renvoize --- C4/OAI/Sets.pm | 100 +++++++--- admin/oai_set_mappings.pl | 13 +- .../data/mysql/atomicupdate/bug_21520.perl | 9 + .../prog/en/modules/admin/oai_set_mappings.tt | 32 +++- t/db_dependent/OAI/AndSets.t | 181 ++++++++++++++++++ 5 files changed, 292 insertions(+), 43 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_21520.perl create mode 100644 t/db_dependent/OAI/AndSets.t diff --git a/C4/OAI/Sets.pm b/C4/OAI/Sets.pm index 62c98d8801..44100e8794 100644 --- a/C4/OAI/Sets.pm +++ b/C4/OAI/Sets.pm @@ -290,19 +290,19 @@ sub AddOAISet { GetOAISetsMappings returns mappings for all OAI Sets. Mappings define how biblios are categorized in sets. -A mapping is defined by four properties: +A mapping is defined by six properties: { - marcfield => 'XXX', # the MARC field to check - marcsubfield => 'Y', # the MARC subfield to check - operator => 'A', # the operator 'equal' or 'notequal'; 'equal' if '' - marcvalue => 'zzzz', # the value to check + marcfield => 'XXX', # the MARC field to check + marcsubfield => 'Y', # the MARC subfield to check + operator => 'A', # the operator 'equal' or 'notequal'; 'equal' if '' + marcvalue => 'zzzz', # the value to check + rule_operator => 'and|or|undef', # the operator between the rules + rule_order => 'n' # the order of the rule for the mapping } If defined in a set mapping, a biblio which have at least one 'Y' subfield of one 'XXX' field equal to 'zzzz' will belong to this set. -If multiple mappings are defined in a set, the biblio will belong to this set -if at least one condition is matched. GetOAISetsMappings returns a hashref of arrayrefs of hashrefs. The first hashref keys are the sets IDs, so it looks like this: @@ -313,7 +313,9 @@ The first hashref keys are the sets IDs, so it looks like this: marcfield => 'XXX', marcsubfield => 'Y', operator => 'A', - marcvalue => 'zzzz' + marcvalue => 'zzzz', + rule_operator => 'and|or|undef', + rule_order => 'n' }, { ... @@ -329,7 +331,7 @@ The first hashref keys are the sets IDs, so it looks like this: sub GetOAISetsMappings { my $dbh = C4::Context->dbh; my $query = qq{ - SELECT * FROM oai_sets_mappings + SELECT * FROM oai_sets_mappings ORDER BY set_id, rule_order }; my $sth = $dbh->prepare($query); $sth->execute; @@ -340,7 +342,9 @@ sub GetOAISetsMappings { marcfield => $result->{'marcfield'}, marcsubfield => $result->{'marcsubfield'}, operator => $result->{'operator'}, - marcvalue => $result->{'marcvalue'} + marcvalue => $result->{'marcvalue'}, + rule_operator => $result->{'rule_operator'}, + rule_order => $result->{'rule_order'} }; } @@ -365,6 +369,7 @@ sub GetOAISetMappings { SELECT * FROM oai_sets_mappings WHERE set_id = ? + ORDER BY rule_order }; my $sth = $dbh->prepare($query); $sth->execute($set_id); @@ -375,7 +380,9 @@ sub GetOAISetMappings { marcfield => $result->{'marcfield'}, marcsubfield => $result->{'marcsubfield'}, operator => $result->{'operator'}, - marcvalue => $result->{'marcvalue'} + marcvalue => $result->{'marcvalue'}, + rule_operator => $result->{'rule_operator'}, + rule_order => $result->{'rule_order'} }; } @@ -411,15 +418,14 @@ sub ModOAISetMappings { }; my $sth = $dbh->prepare($query); $sth->execute($set_id); - if(scalar @$mappings > 0) { $query = qq{ - INSERT INTO oai_sets_mappings (set_id, marcfield, marcsubfield, operator, marcvalue) - VALUES (?,?,?,?,?) + INSERT INTO oai_sets_mappings (set_id, marcfield, marcsubfield, operator, marcvalue, rule_operator, rule_order) + VALUES (?,?,?,?,?,?,?) }; $sth = $dbh->prepare($query); foreach (@$mappings) { - $sth->execute($set_id, $_->{'marcfield'}, $_->{'marcsubfield'}, $_->{'operator'}, $_->{'marcvalue'}); + $sth->execute($set_id, $_->{'marcfield'}, $_->{'marcsubfield'}, $_->{'operator'}, $_->{'marcvalue'}, $_->{'rule_operator'}, $_->{'rule_order'}); } } } @@ -492,30 +498,64 @@ sub CalcOAISetsBiblio { my @biblio_sets; foreach my $set_id (keys %$oai_sets_mappings) { + + my $rules = []; foreach my $mapping (@{ $oai_sets_mappings->{$set_id} }) { next if not $mapping; - my $field = $mapping->{'marcfield'}; - my $subfield = $mapping->{'marcsubfield'}; - my $operator = $mapping->{'operator'}; - my $value = $mapping->{'marcvalue'}; - my @subfield_values = $record->subfield($field, $subfield); - if ($operator eq 'notequal') { - if(0 == grep /^$value$/, @subfield_values) { - push @biblio_sets, $set_id; - last; - } + my $rule_operator = $mapping->{'rule_operator'}; + my $result = _evalRule($record, $mapping); + + # First rule or 'or' rule is always pushed + if (!@$rules || $rule_operator eq 'or') { + push @$rules, [$result]; + next; } - else { - if(0 < grep /^$value$/, @subfield_values) { - push @biblio_sets, $set_id; - last; - } + + # 'and' rule is pushed in the last 'or' rule + push @{$rules->[-1]}, $result; + } + + my @evaluated_and; + foreach my $ruleset (@$rules) { + if (0 < grep /0/, @{$ruleset}) { + push @evaluated_and, 0; + } else { + push @evaluated_and, 1; } } + + if (grep /1/, @evaluated_and) { + push @biblio_sets, $set_id; + } + } return @biblio_sets; } +# Does the record match a given mapping rule? +sub _evalRule { + my $record = shift; + my $mapping = shift; + + my $field = $mapping->{'marcfield'}; + my $subfield = $mapping->{'marcsubfield'}; + my $operator = $mapping->{'operator'}; + my $value = $mapping->{'marcvalue'}; + my @subfield_values = $record->subfield($field, $subfield); + if ($operator eq 'notequal') { + if(0 == grep /^$value$/, @subfield_values) { + return 1; + } + } + else { + if(0 < grep /^$value$/, @subfield_values) { + return 1; + } + } + return 0; +} + + =head2 ModOAISetsBiblios my $oai_sets_biblios = { diff --git a/admin/oai_set_mappings.pl b/admin/oai_set_mappings.pl index af956583d1..f6477943ab 100755 --- a/admin/oai_set_mappings.pl +++ b/admin/oai_set_mappings.pl @@ -57,20 +57,25 @@ if($op && $op eq "save") { my @marcsubfields = $input->multi_param('marcsubfield'); my @operators = $input->multi_param('operator'); my @marcvalues = $input->multi_param('marcvalue'); + my @ruleoperators = $input->multi_param('rule_operator'); + my @ruleorders = $input->multi_param('rule_order'); my @mappings; my $i = 0; while($i < @marcfields and $i < @marcsubfields and $i < @marcvalues) { if($marcfields[$i] and $marcsubfields[$i]) { push @mappings, { - marcfield => $marcfields[$i], - marcsubfield => $marcsubfields[$i], - operator => $operators[$i], - marcvalue => $marcvalues[$i] + marcfield => $marcfields[$i], + marcsubfield => $marcsubfields[$i], + operator => $operators[$i], + marcvalue => $marcvalues[$i], + rule_operator => $ruleoperators[$i], + rule_order => $i }; } $i++; } + $mappings[0]{'rule_operator'} = undef if (@mappings); ModOAISetMappings($id, \@mappings); $template->param(mappings_saved => 1); } diff --git a/installer/data/mysql/atomicupdate/bug_21520.perl b/installer/data/mysql/atomicupdate/bug_21520.perl new file mode 100644 index 0000000000..6b5a56e797 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_21520.perl @@ -0,0 +1,9 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + $dbh->do( "ALTER TABLE oai_sets_mappings ADD COLUMN rule_order INT AFTER set_id, ADD COLUMN rule_operator VARCHAR(3) AFTER rule_order" ); + $dbh->do( "UPDATE oai_sets_mappings SET rule_operator='or'" ); + + # Always end with this (adjust the bug info) + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 21520 - Add rule_order and rule_operator fields to oai_sets_mappings table)\n"; +} diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt index 7314e911c8..6b9f995862 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt @@ -31,6 +31,7 @@ + @@ -43,6 +44,12 @@ [% IF ( mappings ) %] [% FOREACH mapping IN mappings %] + @@ -67,6 +72,12 @@ [% END %] [% ELSE %] + - + [% END %] @@ -104,7 +115,7 @@ $("#mappingform").submit(function(){ hideDialogBox(); }); - $("body").on("click","#ORbutton", function(e){ + $("body").on("click","#new_rule_button", function(e){ e.preventDefault(); newCondition(); }); @@ -112,22 +123,25 @@ e.preventDefault(); clearRow(e.target); }); + $("#mappings tbody tr:first-child td:first-child select").hide(); }); function newCondition() { - var tr = $('#ORbutton').parents('tr'); + var tr = $('#new_rule_button').parents('tr'); var clone = $(tr).clone(); - $("#ORbutton").parent('td').replaceWith(''); + $("#new_rule_button").parent('td').find("#new_rule_button").remove(); + $(clone).find("select").show(); $(tr).parent('tbody').append(clone); } function clearRow(link){ var tr = $(link).parent().parent(); - var found = tr.find('#ORbutton'); + var found = tr.find('#new_rule_button'); if( found.length ){ tr.find('input[type="text"]').attr("value",""); } else { - tr.find('input[type="text"]').attr("value","").end().hide(); + tr.remove(); } + $("#mappings tbody tr:first-child td:first-child select").hide(); } function hideDialogBox() { $('div.dialog').remove(); diff --git a/t/db_dependent/OAI/AndSets.t b/t/db_dependent/OAI/AndSets.t new file mode 100644 index 0000000000..d58959520b --- /dev/null +++ b/t/db_dependent/OAI/AndSets.t @@ -0,0 +1,181 @@ +#!/usr/bin/perl + +# Copyright 2019 BibLibre +# +# This file is part of Koha. +# +# Koha is free software; you can redistribute it and/or modify it under the +# terms of the GNU General Public License as published by the Free Software +# Foundation; either version 3 of the License, or (at your option) any later +# version. +# +# Koha is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License along +# with Koha; if not, see . + +use Modern::Perl; + +use Test::More tests => 4; +use Test::MockModule; +use Test::Warn; +use MARC::Record; +use Data::Dumper; + +use Koha::Database; +use C4::Biblio; +use C4::OAI::Sets; + +use t::lib::TestBuilder; + +my $schema = Koha::Database->new->schema; +$schema->storage->txn_begin; +my $dbh = C4::Context->dbh; + +$dbh->do('DELETE FROM oai_sets'); +$dbh->do('DELETE FROM oai_sets_descriptions'); +$dbh->do('DELETE FROM oai_sets_mappings'); +$dbh->do('DELETE FROM oai_sets_biblios'); + +my $builder = t::lib::TestBuilder->new; + +my $set1 = { + 'spec' => 'specSet1', + 'name' => 'nameSet1', +}; +my $set1_id = AddOAISet($set1); + +my $marcflavour = C4::Context->preference('marcflavour'); +my $mapping1; + +if ($marcflavour eq 'UNIMARC' ){ + $mapping1 = [ + { + rule_order => 1, + marcfield => '200', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myTitle' + }, + { + rule_order => 2, + rule_operator => 'and', + marcfield => '200', + marcsubfield => 'f', + operator => 'equal', + marcvalue => 'myAuthor' + }, + ]; +} else { + $mapping1 = [ + { + rule_order => 1, + marcfield => '245', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myTitle' + }, + { + rule_order => 2, + rule_operator => 'and', + marcfield => '100', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myAuthor' + }, + ]; +} + +#Add 1st mapping for set1 +ModOAISetMappings($set1_id, $mapping1); + +my $biblio_1 = $builder->build_sample_biblio({ title => 'myTitle' }); +my $biblio_2 = $builder->build_sample_biblio({ title => 'myTitle', author => 'myAuthor' }); + +my $biblionumber1 = $biblio_1->biblionumber; +my $biblionumber2 = $biblio_2->biblionumber; + + +my $record = GetMarcBiblio({ biblionumber => $biblionumber1 }); +my @setsEq = CalcOAISetsBiblio($record); +ok(!@setsEq, 'If only one condition is true, the record does not belong to the set'); + +$record = GetMarcBiblio({ biblionumber => $biblionumber2 }); +@setsEq = CalcOAISetsBiblio($record); +is_deeply(@setsEq, $set1_id, 'If all conditions are true, the record belongs to the set'); + +if ($marcflavour eq 'UNIMARC' ){ + $mapping1 = [ + { + rule_order => 1, + marcfield => '200', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myTitle' + }, + { + rule_order => 2, + rule_operator => 'or', + marcfield => '200', + marcsubfield => 'f', + operator => 'equal', + marcvalue => 'myAuthor' + }, + { + rule_order => 3, + rule_operator => 'and', + marcfield => '995', + marcsubfield => 'r', + operator => 'equal', + marcvalue => 'myItemType' + }, + + ]; +} else { + $mapping1 = [ + { + rule_order => 1, + marcfield => '245', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myTitle' + }, + { + rule_order => 2, + rule_operator => 'or', + marcfield => '100', + marcsubfield => 'a', + operator => 'equal', + marcvalue => 'myAuthor' + }, + { + rule_order => 3, + rule_operator => 'and', + marcfield => '942', + marcsubfield => 'c', + operator => 'equal', + marcvalue => 'myItemType' + }, + ]; +} + +ModOAISetMappings($set1_id, $mapping1); + +$biblio_1 = $builder->build_sample_biblio({ title => 'myTitle' }); +$biblio_2 = $builder->build_sample_biblio({ author => 'myAuthor', itemtype => 'myItemType' }); + +$biblionumber1 = $biblio_1->biblionumber; +$biblionumber2 = $biblio_2->biblionumber; + +$record = GetMarcBiblio({ biblionumber => $biblionumber1 }); +@setsEq = CalcOAISetsBiblio($record); + +is_deeply(@setsEq, $set1_id, 'Boolean operators precedence is respected, the record with only the title belongs to the set'); + +$record = GetMarcBiblio({ biblionumber => $biblionumber2 }); +@setsEq = CalcOAISetsBiblio($record); +is_deeply(@setsEq, $set1_id, 'Boolean operators precedence is respected, the record with author and itemtype belongs to the set'); + +$schema->storage->txn_rollback; -- 2.39.5
Rule operator Field Subfield Operator
+ + [% IF ( loop.last ) %] - - [% ELSE %] - OR + [% END %]
+ +
OR