Bug 19069

Summary: "Doesn't match" option fails in MARC Modification Templates
Product: Koha Reporter: jfowler
Component: MARC Bibliographic record staging/importAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: normal    
Priority: P5 - low CC: J.P.Knight, jfowler, jonathan.druart, kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 24900    
Attachments: A single MARC record to use in order to test what I'm describing.
Bug 19069: Fix 'does not match' behaviour in MARC modification template
Bug 19069: Fix 'does not match' behaviour in MARC modification template
Bug 19069: Fix 'does not match' behaviour in MARC modification template
Bug 19069: Fix 'does not match' behaviour in MARC modification template
Bug 19069: (follow-up) Remove perlcritic error

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