Bug 19069 - "Doesn't match" option fails in MARC Modification Templates
Summary: "Doesn't match" option fails in MARC Modification Templates
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-09 20:52 UTC by jfowler
Modified: 2017-10-27 17:59 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
A single MARC record to use in order to test what I'm describing. (1.10 KB, text/plain)
2017-08-09 20:52 UTC, jfowler
Details
Bug 19069: Fix 'does not match' behaviour in MARC modification template (5.50 KB, patch)
2017-10-02 19:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19069: Fix 'does not match' behaviour in MARC modification template (5.54 KB, patch)
2017-10-11 18:58 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19069: Fix 'does not match' behaviour in MARC modification template (5.54 KB, patch)
2017-10-11 19:00 UTC, Jon Knight
Details | Diff | Splinter Review
Bug 19069: Fix 'does not match' behaviour in MARC modification template (5.59 KB, patch)
2017-10-27 13:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 19069: (follow-up) Remove perlcritic error (731 bytes, patch)
2017-10-27 17:58 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description jfowler 2017-08-09 20:52:19 UTC
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.
Comment 1 Jonathan Druart 2017-10-02 19:51:14 UTC
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.
Comment 2 Jonathan Druart 2017-10-02 19:52:15 UTC
I first glance I would say the the "does not exist" condition is buggy as well.
Comment 3 Jon Knight 2017-10-11 15:19:57 UTC
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"

?
Comment 4 Jon Knight 2017-10-11 18:58:41 UTC
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>
Comment 5 Jon Knight 2017-10-11 19:00:24 UTC
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>
Comment 6 Marcel de Rooy 2017-10-13 09:16:51 UTC
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
Comment 7 Jonathan Druart 2017-10-13 14:19:52 UTC
(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.
Comment 8 Marcel de Rooy 2017-10-16 07:24:10 UTC
(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'.
Comment 9 Marcel de Rooy 2017-10-16 07:24:47 UTC
(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
Comment 10 Jon Knight 2017-10-16 09:21:18 UTC
(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"
Comment 11 Marcel de Rooy 2017-10-16 09:44:06 UTC
(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.
Comment 12 Kyle M Hall 2017-10-27 13:02:00 UTC
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>
Comment 13 Jonathan Druart 2017-10-27 17:15:25 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 14 Jonathan Druart 2017-10-27 17:58:47 UTC
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)
Comment 15 Jonathan Druart 2017-10-27 17:59:51 UTC
(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