Attachment 96052 Details for Bug 21520 – Bug 21520: More complex OAI sets mappings

[patch] Bug 21520: More complex OAI sets mappings

Bug-21520-More-complex-OAI-sets-mappings.patch (text/plain), 19.97 KB, created by Michal Denar on 2019-12-05 21:02:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Michal Denar

Created: 2019-12-05 21:02:12 UTC

Size: 19.97 KB

patch

obsolete

>From 2f79711d3c518d59a4fe70029505cb1c15368c5e Mon Sep 17 00:00:00 2001
>From: Matthias Meusburger <matthias.meusburger@biblibre.com>
>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 <black23@gmail.com>
>---
> C4/OAI/Sets.pm                                     | 100 ++++++++----
> admin/oai_set_mappings.pl                          |  13 +-
> installer/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 @@
>         <table id="mappings">
>           <thead>
>             <tr>
>+              <th>Rule operator</th>
>               <th>Field</th>
>               <th>Subfield</th>
>               <th>Operator</th>
>@@ -43,6 +44,12 @@
>             [% IF ( mappings ) %]
>               [% FOREACH mapping IN mappings %]
>                 <tr>
>+                  <td>
>+                  <select name="rule_operator">
>+                    <option value="and" [% IF (mapping.rule_operator == 'and') %] selected="selected" [% END %]>and</option>
>+                    <option value="or" [% IF (mapping.rule_operator == 'or') %] selected="selected" [% END %]>or</option>
>+                  </select>
>+                  </td>
>                   <td><input type="text" name="marcfield" size="3" value="[% mapping.marcfield | html %]" /></td>
>                   <td style="text-align:center"><input type="text" name="marcsubfield" size="1" value="[% mapping.marcsubfield | html %]" /></td>
>                   <td><select name=operator>
>@@ -57,9 +64,7 @@
>                   <td><input type="text" name="marcvalue" value="[% mapping.marcvalue | html %]" /></td>
>                   <td style="text-align:center">
>                     [% IF ( loop.last ) %]
>-                      <input type="button" id="ORbutton" value="OR" title="Add another condition" />
>-                    [% ELSE %]
>-                      OR
>+                      <input type="button" id="new_rule_button" value="add" title="Add another condition" />
>                     [% END %]
>                   </td>
>                   <td><button class="btn btn-default btn-xs clear-field" type="button"><i class="fa fa-trash"></i> Delete</button></td>
>@@ -67,6 +72,12 @@
>               [% END %]
>             [% ELSE %]
>               <tr>
>+                <td>
>+                  <select name="rule_operator">
>+                    <option value="and" [% IF (mapping.rule_operator == 'and') %] selected="selected" [% END %]>and</option>
>+                    <option value="or" [% IF (mapping.rule_operator == 'or') %] selected="selected" [% END %]>or</option>
>+                  </select>
>+                </td>
>                 <td><input type="text" name="marcfield" size="3" /></td>
>                 <td style="text-align:center"><input type="text" name="marcsubfield" size="1" /></td>
>                 <td><select name=operator>
>@@ -74,7 +85,7 @@
>                     <option value="notequal">not equal to</option>
>                     </select></td>
>                 <td><input type="text" name="marcvalue" /></td>
>-                <td><input type="button" id="ORbutton" value="OR" /></td>
>+                <td><input type="button" id="new_rule_button" value="Add" /></td>
>                 <td><button class="btn btn-default btn-xs clear-field" type="button"><i class="fa fa-trash"></i> Delete</button></td>
>               </tr>
>             [% 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('<td style="text-align:center">OR</td>');
>+            $("#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 <http://www.gnu.org/licenses>.
>+
>+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.11.0

Actions: View | Diff | Splinter Review

Attachments on bug 21520: 96041 | 96052 | 96790 | 97319 | 97320 | 97321 | 97322