Bug 21520

Summary: More complex OAI sets mappings
Product: Koha Reporter: Radek Šiman (R-Bit Technology, s.r.o.) <rbit>
Component: System AdministrationAssignee: Matthias Meusburger <matthias.meusburger>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, black23, gmcharlt, jonathan.druart, josef.moravec, martin.renvoize, matthias.meusburger, matti.lassila, mist
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Prior to this patchset, 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.
Version(s) released in:
20.05.00
Circulation function:
Attachments: Bug 21520: More complex OAI sets mappings
Bug 21520: More complex OAI sets mappings
Bug 21520: QA Follow-up: do not build html tags with TT conditions
Bug 21520: (QA follow-up): Updatedatabase
Bug 21520: More complex OAI sets mappings
Bug 21520: QA Follow-up: do not build html tags with TT conditions
Bug 21520: (QA follow-up): Updatedatabase

Description Radek Šiman (R-Bit Technology, s.r.o.) 2018-10-09 14:06:49 UTC
OAI sets mappings should allow "AND" operator as well as more complex condition defintion.
Comment 1 matti.lassila 2019-10-15 10:21:33 UTC
Support for regular expressions in set selection operator in particular would be useful. It is common for free text fields to have slight variation depending when bibliographic record has been created, so exact match doesn't work very well for creating sets.
Comment 2 Matthias Meusburger 2019-12-05 14:26:55 UTC
Created attachment 96041 [details] [review]
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
Comment 3 Michal Denar 2019-12-05 20:53:33 UTC
Hello Mathias,
thank you for patch. I tested it but if I add AND operator, whole updated set gone from OAI:

vagrant@kohadevbox:kohaclone(bug_21520)$ sudo koha-foreach --enabled /usr/share/koha/bin/migration_tools/build_oai_sets.pl -i -r
Summary:
Set 'test': 303 biblios
Set 'NKP': 303 biblios
Updating database... done.
vagrant@kohadevbox:kohaclone(bug_21520)$ sudo koha-foreach --enabled /usr/share/koha/bin/migration_tools/build_oai_sets.pl -i -r
Summary:
Set 'NKP': 303 biblios
Updating database... done.

Can you review code?

Thank you.
Comment 4 Michal Denar 2019-12-05 20:56:37 UTC
Sorry, my fault. Wrong boolean operators.
Comment 5 Michal Denar 2019-12-05 21:02:12 UTC
Created attachment 96052 [details] [review]
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

Great work!

 Signed-off-by: Michal Denar <black23@gmail.com>
Comment 6 Jonathan Druart 2020-01-02 13:58:55 UTC
Hi Matts,
A couple QA remarks:

1. FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt
   FAIL   tt_valid
                lines 49, 50, 77, 78

You are not allowed to build a HTML tag using TT conditional as it breaks the translator script.

2. update database:
 a. rule_order is always null, is that expected?
 b. first rule should have a null for "rule_operator", isn't it?

Before:
+--------+-----------+--------------+----------+-----------+
| set_id | marcfield | marcsubfield | operator | marcvalue |
+--------+-----------+--------------+----------+-----------+
|      3 | 100       | a            | equal    | pouet     |
|      3 | 200       | a            | equal    | pouet     |
|      3 | 300       | a            | equal    | pouet     |
+--------+-----------+--------------+----------+-----------+

After the update:
+--------+------------+---------------+-----------+--------------+----------+-----------+
| set_id | rule_order | rule_operator | marcfield | marcsubfield | operator | marcvalue |
+--------+------------+---------------+-----------+--------------+----------+-----------+
|      3 |       NULL | or            | 100       | a            | equal    | pouet     |
|      3 |       NULL | or            | 200       | a            | equal    | pouet     |
|      3 |       NULL | or            | 300       | a            | equal    | pouet     |
+--------+------------+---------------+-----------+--------------+----------+-----------+
Comment 7 Matthias Meusburger 2020-01-03 09:43:28 UTC
Hi Joubu, thanks for the feedback.

1. I will attach a QA Follow-up

2.
 a. rule_order is always null, is that expected?

=> Kind of :) In fact, rule_order will be populated when a rule is modified/created in the GUI. The updatedatabase doesn't need to do it, because at this point, we only have rules with 'or' operator, and the order is not important.

b. first rule should have a null for "rule_operator", isn't it?

=> Same as 2.a. The first rule_operator could have been set to null by the updatedatabase, but it doesn't need to, since it will always be ignored in the code. However, when a rule is modified/created is the GUI, the first rule_operator will be set to null (just for better readability in the database).
Comment 8 Matthias Meusburger 2020-01-03 10:54:58 UTC
Created attachment 96790 [details] [review]
Bug 21520: QA Follow-up: do not build html tags with TT conditions
Comment 9 Jonathan Druart 2020-01-06 21:39:26 UTC
(In reply to Matthias Meusburger from comment #7)
Why not fix the existing data then? That seems quite easy to do.
Comment 10 Matthias Meusburger 2020-01-07 10:50:34 UTC
Well, I hesitated, but I chose not to, having those two things in mind:

 - Keep It Simple, Stupid.
 - If it's not broken, don't fix it.

That being said, I can come up with a follow-up for the updatedatabase if you want me to.
Comment 11 Matthias Meusburger 2020-01-13 15:29:00 UTC
Created attachment 97319 [details] [review]
Bug 21520: (QA follow-up): Updatedatabase
Comment 12 Matthias Meusburger 2020-01-13 15:30:28 UTC
rule_order is now set and the first rule_operator of each mapping is set to null.
Comment 13 Jonathan Druart 2020-01-13 15:46:09 UTC
Created attachment 97320 [details] [review]
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>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Jonathan Druart 2020-01-13 15:46:13 UTC
Created attachment 97321 [details] [review]
Bug 21520: QA Follow-up: do not build html tags with TT conditions

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Jonathan Druart 2020-01-13 15:46:17 UTC
Created attachment 97322 [details] [review]
Bug 21520: (QA follow-up): Updatedatabase

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Martin Renvoize (ashimema) 2020-01-15 13:56:56 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 17 Martin Renvoize (ashimema) 2020-01-15 16:21:34 UTC
OK, I added the kohastructure update inline during push.. and made the DB update idempotent
Comment 18 Martin Renvoize (ashimema) 2020-01-20 14:04:18 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 19 Joy Nelson 2020-01-31 23:20:24 UTC
Enhancement not backported to 19.11.x