It would be great if this could get some attention as it can create data loss.

*** Bug 33975 has been marked as a duplicate of this bug. ***

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?

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.

Hm, If they were intended as a fallback, why does my second rule in the example above not work? 

What is context here?

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.

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.

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.

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.

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.

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

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.

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.

Created attachment 159058 [details] [review]
Bug 33268: Add tests for context module priority

Added tests for module priority when multiple module filters matches, and ran qa-script. Should now be ready for signoff.

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>

Created attachment 159161 [details] [review]
Bug 33268: Add tests for context module priority

Signed-off-by: David Nind <david@davidnind.com>

 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.

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..

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.

pending comment 18 (perltidy unhappy) and comment 20 (test plan needed)

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)

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)

(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.

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>

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>

Great teamwork here.. QA scripts happy and functionality now works more as described.

Passing QA

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.

*ping* 

This is waiting now waiting for feedback for more than 2 weeks.

I am sad to see this has dropped off the radar.

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.

@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.

Created attachment 162764 [details] [review]
Bug 33268: If rules exists add rules sure to preserve current behaviour

Created attachment 162766 [details] [review]
Bug 33268: If rules exists add rules sure to preserve current behaviour

Created attachment 162767 [details] [review]
Bug 33268: If rules exists add rules to preserve current behaviour

2) Should now be fixed

Discovered an issue with the previous version of the update script which should now be fixed.

Created attachment 162785 [details] [review]
Bug 33268: If rules exists add rules to preserve current behaviour

Created attachment 162786 [details] [review]
Bug 33268: If rules exists add rules to preserve current behavior

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 :)

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>

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>

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>

Created attachment 162804 [details] [review]
Bug 33268: If rules exists add rules to preserve current behavior

Created attachment 162805 [details] [review]
Bug 33268: (QA follow-up): tidy up code

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.

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.

Works for me also with the latest version of the patch, hope I'm not missing something, but setting back to Needs Signoff.

> 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)

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.

(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.

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>

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>

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>

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>

Created attachment 162940 [details] [review]
Bug 33268: (QA follow-up): tidy up code

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.

@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.

(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?

Hi! No problem. I think Victor Grousset/tuxayo removed the release note, perhaps by mistake, but now re-added it again.

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 ^^"

Thanks, no problem!

Bug was filed a year ago and poses a real risk of data loss. Can we move this forward please?

I didn't get the expected results (mostly) following the test plan

Instead of disabling overlay rules to change the record - I updated the MARC modificatoin template for the 300 to set value 'FAILFAILFAILFAIL'

After applying, updating, restarting all, I batch modified and the 300 was still updated.

My rules match what Victor posts after the update

> 6. 300$a should have been catified (expected, that's the bug)
> 7. 245$a is preserved

There has been something up in main since last time I tested this. So, without patches, both subfields are changed by the batch record modification. I checked that the overlay rules are the same with a diff between the test plan a selecting the rules tables in the page.

Same with the patches, overlay is not protecting either fields.

UI: The MARCOverlayRules preference is not set, don't forget to enable it for rules to take effect.

No, wait, I just missed that. Retesting. (good, bisecting that would have been tedious)

> Instead of disabling overlay rules to change the record - I updated the MARC modificatoin template for the 300 to set value 'FAILFAILFAILFAIL'
> After applying, updating, restarting all, I batch modified and the 300 was still updated.

Did the same, 300$a wasn't replaced with 'FAILFAILFAILFAIL'.

Any more details to reproduce?

Can't believe we still haven't fixed this :(