Created attachment 65781 [details] A single MARC record to use in order to test what I'm describing. When adding a new action in a MARCH modification template, the "doesn't match" option fails in places where there are multiple fields. I was importing MARC records with item holdings in 999 fields. Most of the records had multiple 999s. In most, there was one 999 field that had an $l field with the value "NASH". I set up the template to first get rid of all 999 fields where the l doeesn't match NASH. It didn't work. If I reverse the action, and write multiple actions that match on every other 999$1 field value, it works. I've tested several different ways, and the only conclusion I can come to is that the "doesn't match" option is failing. I've attached a sample MARC record.
Created attachment 67530 [details] [review] Bug 19069: Fix 'does not match' behaviour in MARC modification template The "does not match" condition does not behave as expected. We want it to process the action if the subfield exists and that the value does not match a given pattern. Test plan: Be creative and write different template actions using the "does not match" condition. Using the "Batch record modification" and the "Show MARC" popup, confirm that the processed record is the one you are expecting.
I first glance I would say the the "does not exist" condition is buggy as well.
Is there a reason for having "does not match" and "does not exist" considering there is the choice of "if" or "unless"? In other words isn't: "Delete field 999 unless 999$l matches NASH" equivalent to "Delete field 999 if 999$l does not match NASH" ?
Created attachment 67972 [details] [review] Bug 19069: Fix 'does not match' behaviour in MARC modification template The "does not match" condition does not behave as expected. We want it to process the action if the subfield exists and that the value does not match a given pattern. Test plan: Be creative and write different template actions using the "does not match" condition. Using the "Batch record modification" and the "Show MARC" popup, confirm that the processed record is the one you are expecting. Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Created attachment 67973 [details] [review] Bug 19069: Fix 'does not match' behaviour in MARC modification template The "does not match" condition does not behave as expected. We want it to process the action if the subfield exists and that the value does not match a given pattern. Test plan: Be creative and write different template actions using the "does not match" condition. Using the "Batch record modification" and the "Show MARC" popup, confirm that the processed record is the one you are expecting. Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
FAIL Koha/MoreUtils.pm FAIL critic Subroutine prototypes used at line 6, column 1. See page 194 of PBP. OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666 SKIP spelling OK valid
(In reply to Jon Knight from comment #3) > Is there a reason for having "does not match" and "does not exist" > considering there is the choice of "if" or "unless"? In other words isn't: > > "Delete field 999 unless 999$l matches NASH" > > equivalent to > > "Delete field 999 if 999$l does not match NASH" > > ? Yes, that sounds definitely valid. Marcel, what do you think? (In reply to Marcel de Rooy from comment #6) > FAIL Koha/MoreUtils.pm The thing is that this subroutine is a copy/paste of the List::MoreUtils subroutine, but in a newer version. I do not think we should modify it, even for trivial changes.
(In reply to Jon Knight from comment #3) > Is there a reason for having "does not match" and "does not exist" > considering there is the choice of "if" or "unless"? In other words isn't: > > "Delete field 999 unless 999$l matches NASH" > > equivalent to > > "Delete field 999 if 999$l does not match NASH" > > ? Your question and your example do not match. You are not comparing match and exist here. Testing for 245a does [not] exist is different from 245a does [not] match 'test'.
(In reply to Jonathan Druart from comment #7) > (In reply to Marcel de Rooy from comment #6) > > FAIL Koha/MoreUtils.pm > > The thing is that this subroutine is a copy/paste of the List::MoreUtils > subroutine, but in a newer version. I do not think we should modify it, even > for trivial changes. Good enough
(In reply to Marcel de Rooy from comment #8) > (In reply to Jon Knight from comment #3) > > Is there a reason for having "does not match" and "does not exist" > > considering there is the choice of "if" or "unless"? In other words isn't: > > > > "Delete field 999 unless 999$l matches NASH" > > > > equivalent to > > > > "Delete field 999 if 999$l does not match NASH" > > > > ? > > Your question and your example do not match. > You are not comparing match and exist here. > Testing for 245a does [not] exist is different from 245a does [not] match > 'test'. The example I used was "match"/"does not match", but the same holds true if we use "exists"/"does not exist" too: "Delete field 999 unless 999$l exists" equivalent to "Delete field 999 if 999$l does not exist"
(In reply to Jon Knight from comment #10) > The example I used was "match"/"does not match", but the same holds true if > we use "exists"/"does not exist" too: > > "Delete field 999 unless 999$l exists" > > equivalent to > > "Delete field 999 if 999$l does not exist" OK. That is of course true. Misunderstood.
Created attachment 68693 [details] [review] Bug 19069: Fix 'does not match' behaviour in MARC modification template The "does not match" condition does not behave as expected. We want it to process the action if the subfield exists and that the value does not match a given pattern. Test plan: Be creative and write different template actions using the "does not match" condition. Using the "Batch record modification" and the "Show MARC" popup, confirm that the processed record is the one you are expecting. Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 17.11, thanks to everybody involved!
Created attachment 68781 [details] [review] Bug 19069: (follow-up) Remove perlcritic error Subroutine prototypes used at line 6, column 1. See page 194 of PBP. (Severity: 5)
(In reply to Jonathan Druart from comment #14) > Created attachment 68781 [details] [review] [review] > Bug 19069: (follow-up) Remove perlcritic error > > Subroutine prototypes used at line 6, column 1. See page 194 of PBP. > (Severity: 5) Pushed to master