Description
Katrin Fischer
2023-03-17 16:00:17 UTC
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 Created attachment 162796 [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 162797 [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 162798 [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> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 162799 [details] [review] Bug 33268: If rules exists add rules to preserve current behavior Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 162800 [details] [review] Bug 33268: (QA follow-up): tidy up code 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 :( Created attachment 175659 [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> Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Created attachment 175660 [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> Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Created attachment 175661 [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> Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Created attachment 175662 [details] [review] Bug 33268: If rules exists add rules to preserve current behavior Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Created attachment 175663 [details] [review] Bug 33268: (QA follow-up): tidy up code Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> I used the same overlay rules +----+-----+--------+----------+---------+ | id | tag | module | filter | preset | +----+-----+--------+----------+---------+ | 2 | 245 | source | z3950 | protect | | 3 | 300 | source | * | protect | | 4 | 245 | source | batchmod | protect | +----+-----+--------+----------+---------+ And tested according to the test plan - applying the patch - batch modification (245 -> CATS and 300 -> CATS) - 245a and 300a remained the same as before So I would confirm, that rule id=3 acts as expected even when doing a batch modification Tested further with other rules applied: +----+-----+--------+----------+-----------+ | id | tag | module | filter | preset | +----+-----+--------+----------+-----------+ | 1 | 245 | source | * | protect | | 2 | 245 | source | batchmod | overwrite | +----+-----+--------+----------+-----------+ Changing 245a from marc editor in staff was not possible (fallback to rule #1), on the other hand a batchmod was able to change it. As I see the patch works as expected and also tests pass. Therefore I would change status to at least 'needs signoff' again. Created attachment 175939 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175940 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175941 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175942 [details] [review] Bug 33268: If rules exists add rules to preserve current behavior Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Created attachment 175943 [details] [review] Bug 33268: (QA follow-up): tidy up code Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Test plan still works for me. Created attachment 176201 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 176202 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 176203 [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: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 176204 [details] [review] Bug 33268: If rules exists add rules to preserve current behavior Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 176205 [details] [review] Bug 33268: (QA follow-up): tidy up code Signed-off-by: Jan Kissig <bibliothek@th-wildau.de> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Martin beat me by a few minutes, I also tested, works as expected, the DB upgrade preserves the existing behavior, seems great (In reply to Nick Clemens (kidclamp) from comment #92) > Martin beat me by a few minutes, I also tested, works as expected, the DB > upgrade preserves the existing behavior, seems great Thanks Nick! If you want to add another sign-off, please do. I'll start working on pushes soon. Pushed for 25.05! Well done everyone, thank you! Nice work everyone! Pushed to 24.11.x for 24.11.04 *** Bug 34191 has been marked as a duplicate of this bug. *** |