Bug 24900 - Fix 'MARC modification templates' to not assume that 'from field' will match 'conditional field'
Summary: Fix 'MARC modification templates' to not assume that 'from field' will match ...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Aleisha Amohia
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 19069
Blocks:
  Show dependency treegraph
 
Reported: 2020-03-19 04:48 UTC by Aleisha Amohia
Modified: 2020-04-28 20:03 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
This patch ensures MARC modification template actions work as expected when the 'from field' doesn't match the 'conditional field'.
Version(s) released in:
20.05.00


Attachments
test record (2.68 KB, text/plain)
2020-03-19 04:48 UTC, Aleisha Amohia
Details
Bug 24900: If condition in template, check for existence of from fields in MARC record (1.56 KB, patch)
2020-03-19 04:55 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 24900: Some fixes to how MARC modification template actions work (3.29 KB, patch)
2020-03-25 02:04 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field (8.10 KB, patch)
2020-03-25 05:00 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field (8.16 KB, patch)
2020-03-27 09:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field (8.22 KB, patch)
2020-03-31 11:06 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 Aleisha Amohia 2020-03-19 04:48:51 UTC
Created attachment 100992 [details]
test record

To reproduce:

Download the attached biblio record (try right-clicking -> save link as)
Create a MARC modification template with the following action:
Delete field 020 if field 020$z exists
Import the record (Tools -> Stage MARC record for import)
Under Use MARC modification template, select your template to modify the record
Stage for import
Once complete, go to Manage staged records
Click the staged record to view the MARC
Confirm only the 020$z field was deleted, even though we wanted all 020 fields to be deleted
Comment 1 Aleisha Amohia 2020-03-19 04:55:10 UTC
Created attachment 100993 [details] [review]
Bug 24900: If condition in template, check for existence of from fields in MARC record

This patch checks the existence of 'from fields' in the record, as
defined in the template, if the template includes a condition.

To test:
Follow plan to reproduce the bug
Apply the patch and go back to import the record (Tools -> Stage MARC record for import)
Under Use MARC modification template, select your template to modify the record
Stage for import
Once complete, go to Manage staged records
Click the staged record to view the MARC
Confirm all 020 fields were deleted, as expected

Sponsored-by: Catalyst IT
Comment 2 Aleisha Amohia 2020-03-19 04:55:34 UTC
Tests coming
Comment 3 Aleisha Amohia 2020-03-25 02:04:44 UTC
Created attachment 101665 [details] [review]
Bug 24900: Some fixes to how MARC modification template actions work

CASE 1: Delete field 020 if field 020$z exists
BROKEN BEHAVIOUR (before patch): 020$z field was deleted, even though we
                                 wanted all 020 fields to be deleted
EXPECTED BEHAVIOUR (corrected by patch): all 020 fields are deleted

CASE 2: Delete 1st field 020 if 651$z exists
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020 instead
                                 of 1st
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 3: Delete 1st field 020 if 020$z exists
BROKEN BEHAVIOUR (before patch): deletes the 3rd instance of 020 which
                                 contains subfield z
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 4: Delete 1st field 020 if 651$z matches Berlin. (must include '.')
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 5: Delete 1st field 020 if 020$z matches 9781460708385
BROKEN BEHAVIOUR (before patch): deletes the 3rd instance of 020 which
                                 contains subfield z
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

Sponsored-by: Catalyst IT
Comment 4 Aleisha Amohia 2020-03-25 05:00:23 UTC
Created attachment 101666 [details] [review]
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field

When MARC modification template actions are applied, they assume that
the from field is the same as the conditional field. This patch adds
checks for this, as well as tests to confirm the behaviour is correct.

CASE 1: Delete 1st field 020 if 651$z exists
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020 instead
                                 of 1st
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 2: Delete 1st field 020 if 651$z matches Berlin. (must include '.')
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 3: Delete field 020 if 650$2 does not match fast
BROKEN BEHAVIOUR (before patch): deletes all 020 fields even though
                                 650$2 does match fast
EXPECTED BEHAVIOUR (corrected by patch): does not delete 020 fields

Confirm tests pass: t/db_dependent/MarcModificationTemplates.t

Sponsored-by: Catalyst IT
Comment 5 Frank Hansen 2020-03-27 09:07:39 UTC
(In reply to Aleisha Amohia from comment #4)
> Created attachment 101666 [details] [review] [review]
> Bug 24900: Checks in MARC mod templates for when from field does not equal
> conditional field
> 
> When MARC modification template actions are applied, they assume that
> the from field is the same as the conditional field. This patch adds
> checks for this, as well as tests to confirm the behaviour is correct.
> 
> CASE 1: Delete 1st field 020 if 651$z exists
> BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020 instead
>                                  of 1st
> EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020
> 
> CASE 2: Delete 1st field 020 if 651$z matches Berlin. (must include '.')
> BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020
> EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020
> 
> CASE 3: Delete field 020 if 650$2 does not match fast
> BROKEN BEHAVIOUR (before patch): deletes all 020 fields even though
>                                  650$2 does match fast
> EXPECTED BEHAVIOUR (corrected by patch): does not delete 020 fields
> 
> Confirm tests pass: t/db_dependent/MarcModificationTemplates.t
> 
> Sponsored-by: Catalyst IT

What I can see, this patch does as expected.
Comment 6 ByWater Sandboxes 2020-03-27 09:09:11 UTC
Created attachment 101970 [details] [review]
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field

When MARC modification template actions are applied, they assume that
the from field is the same as the conditional field. This patch adds
checks for this, as well as tests to confirm the behaviour is correct.

CASE 1: Delete 1st field 020 if 651$z exists
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020 instead
                                 of 1st
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 2: Delete 1st field 020 if 651$z matches Berlin. (must include '.')
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 3: Delete field 020 if 650$2 does not match fast
BROKEN BEHAVIOUR (before patch): deletes all 020 fields even though
                                 650$2 does match fast
EXPECTED BEHAVIOUR (corrected by patch): does not delete 020 fields

Confirm tests pass: t/db_dependent/MarcModificationTemplates.t

Sponsored-by: Catalyst IT

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 7 David Nind 2020-03-29 09:22:06 UTC
Changed status to signed off - looks like Frank Hansen signed this on off to me!
Comment 8 Jonathan Druart 2020-03-31 11:06:20 UTC
Great fix Aleisha!
Comment 9 Jonathan Druart 2020-03-31 11:06:44 UTC
Created attachment 102121 [details] [review]
Bug 24900: Checks in MARC mod templates for when from field does not equal conditional field

When MARC modification template actions are applied, they assume that
the from field is the same as the conditional field. This patch adds
checks for this, as well as tests to confirm the behaviour is correct.

CASE 1: Delete 1st field 020 if 651$z exists
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020 instead
                                 of 1st
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 2: Delete 1st field 020 if 651$z matches Berlin. (must include '.')
BROKEN BEHAVIOUR (before patch): deletes the 2nd instance of 020
EXPECTED BEHAVIOUR (corrected by patch): deletes the 1st instance of 020

CASE 3: Delete field 020 if 650$2 does not match fast
BROKEN BEHAVIOUR (before patch): deletes all 020 fields even though
                                 650$2 does match fast
EXPECTED BEHAVIOUR (corrected by patch): does not delete 020 fields

Confirm tests pass: t/db_dependent/MarcModificationTemplates.t

Sponsored-by: Catalyst IT

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Martin Renvoize 2020-04-03 13:24:13 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 11 Joy Nelson 2020-04-28 20:03:29 UTC
Tests don't apply cleanly to 19.11.x branch.  Please rebase if needed in 19.11.x