Bug 31644 - MARCModification template fails to copy to/from subfield 0
Summary: MARCModification template fails to copy to/from subfield 0
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-09-28 18:53 UTC by Nick Clemens
Modified: 2023-12-28 20:44 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.08, 21.11.15


Attachments
Bug 31644: Check if subfield defined, not for truth (1.50 KB, patch)
2022-09-28 18:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31644: Unit test (2.79 KB, patch)
2022-09-29 16:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31644: Check if subfield defined, not for truth (1.50 KB, patch)
2022-09-29 16:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31644: Add copy test and clarify some functions (4.70 KB, patch)
2022-09-29 16:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31644: Unit test (2.84 KB, patch)
2022-10-16 05:30 UTC, David Nind
Details | Diff | Splinter Review
Bug 31644: Check if subfield defined, not for truth (1.55 KB, patch)
2022-10-16 05:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 31644: Add copy test and clarify some functions (4.75 KB, patch)
2022-10-16 05:31 UTC, David Nind
Details | Diff | Splinter Review
Bug 31644: Unit test (2.94 KB, patch)
2022-10-21 07:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31644: Check if subfield defined, not for truth (1.65 KB, patch)
2022-10-21 07:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31644: Add copy test and clarify some functions (4.84 KB, patch)
2022-10-21 07:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31644: (QA follow-up) Add two missing shebang lines (1008 bytes, patch)
2022-10-21 07:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-09-28 18:53:13 UTC
Like 26781 or 26894 - but for copy action which appears to have been missed
Comment 1 Nick Clemens 2022-09-28 18:56:06 UTC
Created attachment 141066 [details] [review]
Bug 31644: Check if subfield defined, not for truth

To test:
1 - Define a new MARC Modification template with actions:
        Copy field 600$a to 942$0
2 - Define a new record like:
    LDR 00334nam a22001217a 4500
    003 ff
    005 20201102111604.0
    008 201102b        xxu||||| |||| 00| 0 eng d
    040 _ _ ‡cvsd
    100 1 _ ‡012345‡aKnuth, Donal Ervin‡d1938
    245 _ _ ‡012345‡aThe aty of computer programming‡cDonald E. Knuth
    600 _ 0 ‡042‡aComputer programming‡9462
3 - Modify this record using the template above
4 - Note that entire  field is copied to 942
5 - Apply patch
6 - Now only subfield 0 is copied
Comment 2 Nick Clemens 2022-09-29 16:20:14 UTC
Created attachment 141091 [details] [review]
Bug 31644: Unit test
Comment 3 Nick Clemens 2022-09-29 16:20:19 UTC
Created attachment 141092 [details] [review]
Bug 31644: Check if subfield defined, not for truth

To test:
1 - Define a new MARC Modification template with actions:
        Copy field 600$a to 942$0
2 - Define a new record like:
    LDR 00334nam a22001217a 4500
    003 ff
    005 20201102111604.0
    008 201102b        xxu||||| |||| 00| 0 eng d
    040 _ _ ‡cvsd
    100 1 _ ‡012345‡aKnuth, Donal Ervin‡d1938
    245 _ _ ‡012345‡aThe aty of computer programming‡cDonald E. Knuth
    600 _ 0 ‡042‡aComputer programming‡9462
3 - Modify this record using the template above
4 - Note that entire  field is copied to 942
5 - Apply patch
6 - Now only subfield 0 is copied
Comment 4 Nick Clemens 2022-09-29 16:20:23 UTC
Created attachment 141093 [details] [review]
Bug 31644: Add copy test and clarify some functions

This patch adds a tst for copy without subfields. I also clarify what eachstep does
so the next user/coder understands current behaviour

Update existing or add new: In the case where the field/subfield exists
we update, if we have two fields - one with the subfield, and one without, we
add the subfield to the one without

Copy field:
 - If given a subfield - we will add to existing fields in the record
 - If not given a subfield - we create an entirely new field

The logic of all of this is tricky, makes sense in a certain light, any complaints
are for a new bug :-)
Comment 5 David Nind 2022-10-15 20:44:25 UTC
I attempted to test.

The record is updated, however there is this message in the job list after running the batch modification:

Bibliographic record 439 has not been modified. An error occurred on modifying it. (C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception: DBD::mysql::st execute failed: Incorrect integer value: 'Computer programming' for column `koha_kohadev`.`biblioitems`.`totalissues` at row 1 at /kohadevbox/koha/C4/Biblio.pm line 425 ). 

Here is the record after the batch record modification is run.

000      nam a22     7a 4500
003 ff
005 20221015202837.0
008 201102b        xxu||||| |||| 00| 0 eng d
040 _ _ ‡cvsd
100 1 _ ‡012345‡9102‡aKnuth, Donald Ervin,‡d1938-
245 _ _ ‡012345‡aThe aty of computer programming‡cDonald E. Knuth
600 _ 0 ‡042‡aComputer programming‡9462
999 _ _ ‡c439‡d439
942 _ _ ‡0Computer programming
Comment 6 Nick Clemens 2022-10-15 21:23:40 UTC
(In reply to David Nind from comment #5)
> I attempted to test.
> 
> The record is updated, however there is this message in the job list after
> running the batch modification:
> 
> Bibliographic record 439 has not been modified. An error occurred on
> modifying it. (C4::Biblio::_koha_modify_biblioitem_nonmarc(): DBI Exception:
> DBD::mysql::st execute failed: Incorrect integer value: 'Computer
> programming' for column `koha_kohadev`.`biblioitems`.`totalissues` at row 1
> at /kohadevbox/koha/C4/Biblio.pm line 425 ). 
> 
>

That's my fault, the test plan has you copy text to a numeric field. If you try with a field that has a number is suspect you won't get that error
Comment 7 David Nind 2022-10-16 05:30:58 UTC
Created attachment 141936 [details] [review]
Bug 31644: Unit test

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-10-16 05:31:03 UTC
Created attachment 141937 [details] [review]
Bug 31644: Check if subfield defined, not for truth

To test:
1 - Define a new MARC Modification template with actions:
        Copy field 600$a to 942$0
2 - Define a new record like:
    LDR 00334nam a22001217a 4500
    003 ff
    005 20201102111604.0
    008 201102b        xxu||||| |||| 00| 0 eng d
    040 _ _ ‡cvsd
    100 1 _ ‡012345‡aKnuth, Donal Ervin‡d1938
    245 _ _ ‡012345‡aThe aty of computer programming‡cDonald E. Knuth
    600 _ 0 ‡042‡aComputer programming‡9462
3 - Modify this record using the template above
4 - Note that entire  field is copied to 942
5 - Apply patch
6 - Now only subfield 0 is copied

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2022-10-16 05:31:07 UTC
Created attachment 141938 [details] [review]
Bug 31644: Add copy test and clarify some functions

This patch adds a tst for copy without subfields. I also clarify what eachstep does
so the next user/coder understands current behaviour

Update existing or add new: In the case where the field/subfield exists
we update, if we have two fields - one with the subfield, and one without, we
add the subfield to the one without

Copy field:
 - If given a subfield - we will add to existing fields in the record
 - If not given a subfield - we create an entirely new field

The logic of all of this is tricky, makes sense in a certain light, any complaints
are for a new bug :-)

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-10-16 05:34:59 UTC
(In reply to Nick Clemens from comment #6)

> That's my fault, the test plan has you copy text to a numeric field. If you
> try with a field that has a number is suspect you won't get that error

I ended up using this MARC modification template for step 1:
   Copy field 600$0 to 611$0
Comment 11 Marcel de Rooy 2022-10-21 07:43:02 UTC
> The logic of all of this is tricky, makes sense in a certain light, any complaints are for a new bug :-)

LOL
The module change makes sense to me, reading the tests is hard, you need to make a study of the logic in order to grasp whats going on there..
Comment 12 Marcel de Rooy 2022-10-21 07:46:14 UTC
Created attachment 142298 [details] [review]
Bug 31644: Unit test

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-10-21 07:46:20 UTC
Created attachment 142299 [details] [review]
Bug 31644: Check if subfield defined, not for truth

To test:
1 - Define a new MARC Modification template with actions:
        Copy field 600$a to 942$0
2 - Define a new record like:
    LDR 00334nam a22001217a 4500
    003 ff
    005 20201102111604.0
    008 201102b        xxu||||| |||| 00| 0 eng d
    040 _ _ ‡cvsd
    100 1 _ ‡012345‡aKnuth, Donal Ervin‡d1938
    245 _ _ ‡012345‡aThe aty of computer programming‡cDonald E. Knuth
    600 _ 0 ‡042‡aComputer programming‡9462
3 - Modify this record using the template above
4 - Note that entire  field is copied to 942
5 - Apply patch
6 - Now only subfield 0 is copied

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2022-10-21 07:46:25 UTC
Created attachment 142300 [details] [review]
Bug 31644: Add copy test and clarify some functions

This patch adds a tst for copy without subfields. I also clarify what eachstep does
so the next user/coder understands current behaviour

Update existing or add new: In the case where the field/subfield exists
we update, if we have two fields - one with the subfield, and one without, we
add the subfield to the one without

Copy field:
 - If given a subfield - we will add to existing fields in the record
 - If not given a subfield - we create an entirely new field

The logic of all of this is tricky, makes sense in a certain light, any complaints
are for a new bug :-)

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2022-10-21 07:46:30 UTC
Created attachment 142301 [details] [review]
Bug 31644: (QA follow-up) Add two missing shebang lines

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Tomás Cohen Arazi 2022-10-24 14:55:39 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 17 Arthur Suzuki 2022-12-16 14:33:35 UTC
Lucas did not update that one but it's applied to 22.05 for (I guess) 22.05.08.
I applied to 21.11 for 21.11.15
Comment 18 wainuiwitikapark 2023-03-15 01:02:45 UTC
Not backported to 21.05.x