Bug 33268 - Overlay rules don't work correctly when source is set to *
Summary: Overlay rules don't work correctly when source is set to *
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords: release-notes-needed
: 33975 (view as bug list)
Depends on: 14957
Blocks:
  Show dependency treegraph
 
Reported: 2023-03-17 16:00 UTC by Katrin Fischer
Modified: 2024-04-03 05:36 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement changes how marc overlay rules are evaluated. Before the change rules with filter set to '*' (wildcard) where only applied when no other rules had been defined for a specific filter value, regardless of if any of the rules with that filter value matched or not. With this change we fallback to the wildcard filter rules even though rules with a matching filter value do exists, if no rule for that filter value matches. This resolves the issue of having to repeat the same default rules for each filter value rule set. If for some filter value the wildcard filter rules should be overridden, a wildcard tag rule for that filter value can be defined which will have higher precedence and override all the filter wildcard rules. In summary, the rules will applied as follows: - A matching rule is looked for based on context (module and filter value), if multiple contexts matches rules for the module with highest priority are used. The module priority is (from highest to lowest) "User name", "Patron category" and "Source". - If no matching rule is found, we fallback to the wildcard filter rules of the current context module. For Koha installations where marc overlay rules is in use a database migration will run that creates new rules if necessary to preserve the current behavior, so no user action needs to be taken in order to correct for this change.
Version(s) released in:


Attachments
Bug 33268: Overlay rules don't work correctly when source is set to * (4.90 KB, patch)
2023-11-14 19:01 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (14.67 KB, patch)
2023-11-15 19:02 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (3.44 KB, patch)
2023-11-16 17:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (14.71 KB, patch)
2023-11-21 13:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (3.48 KB, patch)
2023-11-21 13:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (17.67 KB, patch)
2023-12-04 11:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (13.61 KB, patch)
2023-12-04 11:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (17.73 KB, patch)
2023-12-19 15:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (13.67 KB, patch)
2023-12-19 15:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules sure to preserve current behaviour (2.30 KB, patch)
2024-03-05 15:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules sure to preserve current behaviour (2.16 KB, patch)
2024-03-05 15:51 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behaviour (2.16 KB, patch)
2024-03-05 15:58 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behaviour (2.18 KB, patch)
2024-03-05 17:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behavior (2.18 KB, patch)
2024-03-05 17:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 33268: Add tests (10.19 KB, patch)
2024-03-05 22:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (13.73 KB, patch)
2024-03-05 22:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (8.12 KB, patch)
2024-03-05 22:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behavior (2.24 KB, patch)
2024-03-05 22:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: (QA follow-up): tidy up code (1.74 KB, patch)
2024-03-05 22:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Add tests (10.14 KB, patch)
2024-03-05 23:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (13.67 KB, patch)
2024-03-05 23:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (8.06 KB, patch)
2024-03-05 23:33 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behavior (2.18 KB, patch)
2024-03-05 23:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: (QA follow-up): tidy up code (1.74 KB, patch)
2024-03-05 23:34 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Add tests (10.19 KB, patch)
2024-03-08 04:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Add tests for context module priority (13.73 KB, patch)
2024-03-08 04:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: Overlay rules don't work correctly when source is set to * (9.12 KB, patch)
2024-03-08 04:05 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: If rules exists add rules to preserve current behavior (2.24 KB, patch)
2024-03-08 04:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 33268: (QA follow-up): tidy up code (1.74 KB, patch)
2024-03-08 04:06 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-03-17 16:00:17 UTC
Testing bug 30966 I found a general issue with * rules:

My rules:
+----+-----+--------+----------+-----+--------+--------+--------+
| id | tag | module | filter   | add | append | remove | delete |
+----+-----+--------+----------+-----+--------+--------+--------+
|  3 | 245 | source | z3950    |   0 |      0 |      0 |      0 |
|  4 | 300 | source | *        |   0 |      0 |      0 |      0 |
|  5 | 245 | source | batchmod |   0 |      0 |      0 |      0 |
+----+-----+--------+----------+-----+--------+--------+--------+

I tested both batch record modification and Z39.50 overlays and found that in both cases:

* 245 was protected/unchanged = correct
* 300 was unprotected/overwritten = incorrect
Comment 1 Katrin Fischer 2023-06-10 19:08:08 UTC
It would be great if this could get some attention as it can create data loss.
Comment 2 Katrin Fischer 2023-06-10 19:08:24 UTC
*** Bug 33975 has been marked as a duplicate of this bug. ***
Comment 3 Nick Clemens 2023-06-29 16:26:27 UTC
So, the way it seems to work is, if there are any rules for the context, then we don't get the '*' rules

i.e. if you are going to define rules for z3950, then you must define all the rules you want - but if you don't define any rules for a specific filter, then the '*' rules are used

Is this the intention, Tomas?
Comment 4 David Gustafsson 2023-11-13 16:01:46 UTC
Yes, even though this behaviour seems be confusing users, that was the intention. '*' rules are never "merged in" into another rule set, they just serve as defaults where no rules matching the current context are found.
Comment 5 Katrin Fischer 2023-11-13 16:04:56 UTC
Hm, If they were intended as a fallback, why does my second rule in the example above not work? 

What is context here?
Comment 6 David Gustafsson 2023-11-13 16:41:02 UTC
Context is module and filter, so if rules are applied with either z3950 or batchmod, '*' rules are ignored. '*' rules are only applied if there are no rules defined for the current context (module + filter). I think I decided on this so that each context would have a clean slate and though that would be easier to maintain, not having to undo wildcard rules for a new context, but I can see how it's not the most intuitive. For our library for example, we want to protect quite a lot of fields on imports, but want everything to be overwritable in the staff interface. So we define the "import" rules in the '*' context, and can just add one tag wildcard rule with for the staff interface to make everything overwritable again. I think I assumed that would be the most common use case. Merging in '*' would complicate things for us (but perhaps make things easier for others). Another reason avoided this was that it complicates the code quite a bit, but I think it would still be doable.
Comment 7 David Gustafsson 2023-11-14 15:19:26 UTC
I have to correct my previous comment, if context + filter (context) has higher priority, but default rules are still merged in (but only if a rule with an exact match on context already does already match the tag condition) our current rules would work the same (as we have a wildcard rule matching the exact context (source => staff interface). I gave it some thought, and I think it would be possible to merge in the '*' with a few simple rules:

If there are rules for the context with an exact filter match with a wildcard tag, discard all wildcard filter rules.

If there are one or more regexp rules for the context with exact match, loop through all wildcard filter rules and throw away rules with tags matching this context.

Else merge in all wildcard filter rules with tags not already present in current matching context.
Comment 8 David Gustafsson 2023-11-14 15:41:27 UTC
To summarize how the rules would be applied in a less convoluted way, for a specific tag and context:

First all the rules for the matching module filter combination (context) are evaluated, if there is a match, that rule applies.

If no rule was matched then rules for the wildcard filter are evaluated.

There is another issue to address, and that is module priority. Say we have two or more matching context with different modules, which one applies? I think this is an oversight in the current implementation, and we have to make sure we loop through modules in a specific predefined order. Right now we just loop though the hash keys of "context" provided to the function merge_record in effect resulting in random order. I will have a go at patch addressing both the merging issue, and priority of modules.
Comment 9 David Gustafsson 2023-11-14 19:01:54 UTC
Created attachment 158956 [details] [review]
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.
Comment 10 David Gustafsson 2023-11-14 19:03:42 UTC
The attached patch needs some more work. Have done some manual testing and I think it's probably ok, but need to add tests before ready for review.
Comment 11 David Gustafsson 2023-11-15 19:02:00 UTC
Created attachment 159004 [details] [review]
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
Comment 12 David Gustafsson 2023-11-15 19:06:19 UTC
Now added some tests, it would probably be a good idea to add a test for multiple contexts with regards to module precedence. Besides from that it should be ready for review. Also have not run the qa-script yet. Don't know how much more time I currently have to spend on this though, it should more or less be ready for review.
Comment 13 David Gustafsson 2023-11-15 19:09:44 UTC
Would be nice with some feedback whether this is preferable to the current way rules are evaluated, will also need to update documentation in the manual.
Comment 14 David Gustafsson 2023-11-16 17:24:56 UTC
Created attachment 159058 [details] [review]
Bug 33268: Add tests for context module priority
Comment 15 David Gustafsson 2023-11-16 17:55:31 UTC
Added tests for module priority when multiple module filters matches, and ran qa-script. Should now be ready for signoff.
Comment 16 David Nind 2023-11-21 13:41:01 UTC
Created attachment 159160 [details] [review]
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>
Comment 17 David Nind 2023-11-21 13:41:04 UTC
Created attachment 159161 [details] [review]
Bug 33268: Add tests for context module priority

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Marcel de Rooy 2023-11-24 08:03:32 UTC
 WARN   Koha/MarcOverlayRules.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 114, now: 128)

 WARN   t/db_dependent/Biblio/MarcOverlayRules.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 226, now: 285)

Please tidy inline.
Comment 19 Marcel de Rooy 2023-11-24 09:00:08 UTC
It would be nice to know how many libraries have enabled OverlayRules btw. Unfortunately this pref is not sent to HEA ;)

I need to study the manual what is actually meant by Delete and Remove etc. I think that the whole feature is very confusing and unclear.

Not sure if this is a major. Not even sure if we should call this an Enh.

Just my opinion..
Comment 20 Katrin Fischer 2023-11-24 09:54:05 UTC
We are keen on the feature, but not on the right version yet. I know of at least one big library that will be using it in production very soon.

I found the current behaviour with * confusing and we also had others reporting about it.

But I haven't been able to follow the latest developments here to give an opinion about the planned changes yet. 

It looks like we are missing a test plan that highlights the change.
Comment 21 Victor Grousset/tuxayo 2023-12-03 15:43:01 UTC
pending comment 18 (perltidy unhappy) and comment 20 (test plan needed)
Comment 22 Tomás Cohen Arazi 2023-12-04 11:35:09 UTC
Created attachment 159518 [details] [review]
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>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tidied inline (tcohen)
Comment 23 Tomás Cohen Arazi 2023-12-04 11:35:12 UTC
Created attachment 159519 [details] [review]
Bug 33268: Add tests for context module priority

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tidied inline (tcohen)
Comment 24 Tomás Cohen Arazi 2023-12-04 11:38:43 UTC
(In reply to Marcel de Rooy from comment #18)
> 
> Please tidy inline.

Done!

I think the fact '*' rules don't stack up is a bit confusing, but it makes sense. We should (maybe) name it 'fallback' or similar.
Comment 25 Martin Renvoize 2023-12-19 15:18:35 UTC
Created attachment 160017 [details] [review]
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>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tidied inline (tcohen)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2023-12-19 15:18:38 UTC
Created attachment 160018 [details] [review]
Bug 33268: Add tests for context module priority

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>
Comment 27 Martin Renvoize 2023-12-19 15:19:09 UTC
Great teamwork here.. QA scripts happy and functionality now works more as described.

Passing QA
Comment 28 Katrin Fischer 2023-12-28 09:26:13 UTC
Hi all,

I have tried to make sense of the notes here, but I am not confident I fully understand the consequences of this change yet.

I'll try to summarize what I got so far:

* The problem is about how we determine which rules apply in a situation.
* context = filter + module
* In my example context source-z3950 deactivated source-*, as they had the same context.

+----+-----+--------+----------+-----+--------+--------+--------+
| id | tag | module | filter   | add | append | remove | delete |
+----+-----+--------+----------+-----+--------+--------+--------+
|  3 | 245 | source | z3950    |   0 |      0 |      0 |      0 |
|  4 | 300 | source | *        |   0 |      0 |      0 |      0 |
|  5 | 245 | source | batchmod |   0 |      0 |      0 |      0 |
+----+-----+--------+----------+-----+--------+--------+--------+

The patch states:

  "Fallback to overlay rules with wildcard filter if no match
   found for exact filter match."

Does an exact match comprise of context + tag and now as there is no exact match for 
300-source-z3950 we will also check for 300-source-*?

Please:

1) Update commit message to include a test plan that highlights the change
2) Please explain: How will this change affect existing installations? Do we need to provide any information? Can we rule out data loss? 
3) Please add release notes.
Comment 29 Katrin Fischer 2024-01-16 14:45:10 UTC
*ping* 

This is waiting now waiting for feedback for more than 2 weeks.
Comment 30 Katrin Fischer 2024-01-28 13:25:43 UTC
I am sad to see this has dropped off the radar.
Comment 31 Ray Delahunty 2024-02-28 14:04:02 UTC
I've just set up my first overlay rule in our test server and my clumsy workaround to protect 506, 541, 561 and 562 (set up 4 sets of rules, for each filter of Staff interface MARC editor, 39.50 import, Staged MARC import and Batch record modification) seems to work in the meantime, while a fix is being worked on.
Comment 32 David Gustafsson 2024-03-05 14:30:13 UTC
@Katrin Fischer Sorry about the late reply.

1) I don't know if I can summarize it better than the current commit message, perhaps I could add "instead of falling back to the default rule (overwrite), which was the previous behavior. Previously the wildcard filter rules where only applied if no other filter value would match. The added tests should also be equivalent to performing manual testing, so don't really see why we would need both, but I guess I could add some simple case to be performed in the staff interface.

2) Gave this some thought and I think there is a relatively simple way to add rules depending on existing rules that would result in this patch not at all affecting behavior for current used defined rules. Will add a database migration  for this.

3) Haven't done this for any other patch, so a little bit uncertain what it should contain.

About your rules the wildcard filter rule will be merged in to the rule-sets of both the other rules. If the filter value is z3950 rule 2 and 4 will be applied, if batchmod 4 and five. If instead of 245 you had *, the wildcard filter rule (for tag 300) would not be included since the rule for z3950 and batchmod would always match, and rules with a specific filter value take precedence over wildcard filter rules if they match. (As summarized with "Fallback to overlay rules with wildcard filter if no match found for exact filter match." in the commit message.
Comment 33 David Gustafsson 2024-03-05 15:49:21 UTC
Created attachment 162764 [details] [review]
Bug 33268: If rules exists add rules sure to preserve current behaviour
Comment 34 David Gustafsson 2024-03-05 15:51:39 UTC
Created attachment 162766 [details] [review]
Bug 33268: If rules exists add rules sure to preserve current behaviour
Comment 35 David Gustafsson 2024-03-05 15:58:39 UTC
Created attachment 162767 [details] [review]
Bug 33268: If rules exists add rules to preserve current behaviour
Comment 36 David Gustafsson 2024-03-05 15:59:02 UTC
2) Should now be fixed
Comment 37 David Gustafsson 2024-03-05 17:19:15 UTC
Discovered an issue with the previous version of the update script which should now be fixed.
Comment 38 David Gustafsson 2024-03-05 17:19:38 UTC
Created attachment 162785 [details] [review]
Bug 33268: If rules exists add rules to preserve current behaviour
Comment 39 David Gustafsson 2024-03-05 17:20:38 UTC
Created attachment 162786 [details] [review]
Bug 33268: If rules exists add rules to preserve current behavior
Comment 40 Victor Grousset/tuxayo 2024-03-05 22:33:26 UTC Comment hidden (obsolete)
Comment 41 Victor Grousset/tuxayo 2024-03-05 22:33:29 UTC Comment hidden (obsolete)
Comment 42 Victor Grousset/tuxayo 2024-03-05 22:33:32 UTC Comment hidden (obsolete)
Comment 43 Victor Grousset/tuxayo 2024-03-05 22:33:35 UTC Comment hidden (obsolete)
Comment 44 Victor Grousset/tuxayo 2024-03-05 22:33:38 UTC Comment hidden (obsolete)
Comment 45 Victor Grousset/tuxayo 2024-03-05 23:33:45 UTC
Thanks for the follow-up David.
I moved the tests to their own commit (with a generic commit message that would benefit some completion) and reordered so we have all the test changes first.
That allowed to run the test without implementation to confirm that they indeed fail without it. So that's great :)
Comment 46 Victor Grousset/tuxayo 2024-03-05 23:33:51 UTC
Created attachment 162801 [details] [review]
Bug 33268: Add tests

Sponsored-by: Gothenburg University Library
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Victor Grousset/tuxayo 2024-03-05 23:33:55 UTC
Created attachment 162802 [details] [review]
Bug 33268: Add tests for context module priority

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>
Comment 48 Victor Grousset/tuxayo 2024-03-05 23:33:58 UTC
Created attachment 162803 [details] [review]
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>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: tidied inline (tcohen)
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Victor Grousset/tuxayo 2024-03-05 23:34:01 UTC
Created attachment 162804 [details] [review]
Bug 33268: If rules exists add rules to preserve current behavior
Comment 50 Victor Grousset/tuxayo 2024-03-05 23:34:06 UTC
Created attachment 162805 [details] [review]
Bug 33268: (QA follow-up): tidy up code
Comment 51 Victor Grousset/tuxayo 2024-03-05 23:52:36 UTC
I was a bit too quick to signoff the patches. That indeed doesn't look like a change that can only have automated tests (thanks for these though) without some basic manual testing.

So I tried something based on comment 0

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 field (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 values of the two subfields to something else
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

But no it still got changed :(

It's the first time I'm using record overlay rules so there is likely a mistake there.
Either that or there is still a bug.
So in the end, test plan needed or bug found.
Comment 52 David Gustafsson 2024-03-06 15:50:58 UTC
About the tests there is no difference testing manually vs the test cases with the exception if there is an incorrect overlay_context parameter for ModBiblio somewhere, and that has already been manually testing and would also probably have been discovered in production if where the case. Of course there could also be some testing scenario not covered by the tests (and most should be), but in that case we should just add that test in the test suite instead. I'm not able to reproduce your test case, for me it seems to work. Tried both with the same rules, and some different permutations. I will try again with your version of the patch. Also made a script to print out the resulting rule-set, and it produces the expected result.
Comment 53 David Gustafsson 2024-03-06 16:21:10 UTC
Works for me also with the latest version of the patch, hope I'm not missing something, but setting back to Needs Signoff.
Comment 54 Victor Grousset/tuxayo 2024-03-06 18:54:41 UTC
> I'm not able to reproduce your test case, for me it seems to work.
> [...]
> Works for me also with the latest version of the patch

So you followed the steps and at the 11th you got the subfield protected as expected? :D
(vs at the 6th were it got changed)
Comment 55 David Gustafsson 2024-03-07 15:15:40 UTC
Yes, exactly. You could try this script (run with koha-shell <koha-instance> -c "perl <script-name>":

use Koha::MarcOverlayRules;
use Data::Dumper;
my $rules = Koha::MarcOverlayRules->context_rules({ source => 'batchmod' });
print Dumper($rules);

And review the resulting rule-set. Could be useful for debugging.
Comment 56 Victor Grousset/tuxayo 2024-03-08 03:59:25 UTC
(In reply to David Gustafsson from comment #55)
> Yes, exactly.

I retried and it works!!! :D

I might have messed something because I should have been blocked at step:
> 8. Change the values of the two subfields to something else

Because the * overlay rules also include the editor. And back then I was able to change the values with the editor and now as expected, I can't. So likely had mess the rules.
(You likely hit that too with my bogus test plan and that was an hint that I did something wrong)

Anyway, I'm fixing the test plan and putting it in the patch commit message.
Comment 57 Victor Grousset/tuxayo 2024-03-08 04:04:13 UTC
Created attachment 162936 [details] [review]
Bug 33268: Add tests

Sponsored-by: Gothenburg University Library
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 58 Victor Grousset/tuxayo 2024-03-08 04:04:50 UTC
Created attachment 162937 [details] [review]
Bug 33268: Add tests for context module priority

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>
Comment 59 Victor Grousset/tuxayo 2024-03-08 04:05:29 UTC
Created attachment 162938 [details] [review]
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>
Comment 60 Victor Grousset/tuxayo 2024-03-08 04:06:08 UTC
Created attachment 162939 [details] [review]
Bug 33268: If rules exists add rules to preserve current behavior

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 61 Victor Grousset/tuxayo 2024-03-08 04:06:44 UTC
Created attachment 162940 [details] [review]
Bug 33268: (QA follow-up): tidy up code
Comment 62 Victor Grousset/tuxayo 2024-03-08 05:06:32 UTC
It works :)

--

@QA here is an example of the changes from the DB upgrade on the data used in test plan:

+----+-----+--------+----------+-----+--------+--------+--------+
| id | tag | module | filter   | add | append | remove | delete |
+----+-----+--------+----------+-----+--------+--------+--------+
|  2 | 245 | source | z3950    |   0 |      0 |      0 |      0 |
|  3 | 300 | source | *        |   0 |      0 |      0 |      0 |
|  4 | 245 | source | batchmod |   0 |      0 |      0 |      0 |
+----+-----+--------+----------+-----+--------+--------+--------+
↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓
+----+-----+--------+----------+-----+--------+--------+--------+
| id | tag | module | filter   | add | append | remove | delete |
+----+-----+--------+----------+-----+--------+--------+--------+
|  2 | 245 | source | z3950    |   0 |      0 |      0 |      0 |
|  3 | 300 | source | *        |   0 |      0 |      0 |      0 |
|  4 | 245 | source | batchmod |   0 |      0 |      0 |      0 |
| 12 | *   | source | z3950    |   1 |      1 |      1 |      1 |
| 13 | *   | source | batchmod |   1 |      1 |      1 |      1 |
+----+-----+--------+----------+-----+--------+--------+--------+


---

@David G. to address  Comment 28 's point 3 (release notes) you can look at latest release announcement: https://koha-community.org/koha-23-11-released/
Some tickets have grey boxes with text. That's from the "Text to go in the release notes" field here.
Comment 63 David Gustafsson 2024-03-11 17:15:33 UTC
@Victor Grousset/tuxayo

Ok, thanks for the feedback and test plan. I now submitted a release note. It's not that easy to explain the change in clear terms, so suggestions are welcome if there is room for improvements.
Comment 64 Katrin Fischer 2024-04-01 12:51:20 UTC
(In reply to David Gustafsson from comment #63)
> @Victor Grousset/tuxayo
> 
> Ok, thanks for the feedback and test plan. I now submitted a release note.
> It's not that easy to explain the change in clear terms, so suggestions are
> welcome if there is room for improvements.

Hi David, sorry if my comments where not clear enough and thanks for your perseverance. 

With release-notes-needed or "add to release notes" we usually mean the bugzilla field "Text to go in the release notes:". This is used when the release notes are auto generated.

Not sure if yours got lost somehow as the field is now empty, can you please double check?
Comment 65 David Gustafsson 2024-04-02 09:50:22 UTC
Hi! No problem. I think Victor Grousset/tuxayo removed the release note, perhaps by mistake, but now re-added it again.
Comment 66 Victor Grousset/tuxayo 2024-04-03 04:55:49 UTC
Oh sorry, no idea what happened! :o
I didn't even do another real change that day on the ticket.

The "Removed" column for my action on the 2024-03-12 in the ticket history makes absolutely no sense with what was previously in the field that got emptied. Like all the lines are in random order.

Anyway, thanks for reverting. In the process, the line breaks were lost, I reintroduced them, hopefully nothing weird will happen again ^^"
Comment 67 David Gustafsson 2024-04-03 05:36:19 UTC
Thanks, no problem!