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
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
Tests coming
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
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
(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.
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>
Changed status to signed off - looks like Frank Hansen signed this on off to me!
Great fix Aleisha!
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>
Nice work everyone! Pushed to master for 20.05
Tests don't apply cleanly to 19.11.x branch. Please rebase if needed in 19.11.x