|
Description
Nick Clemens (kidclamp)
2022-09-28 18:53:13 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 Created attachment 141091 [details] [review] Bug 31644: Unit test 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 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 :-) 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 (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 Created attachment 141936 [details] [review] Bug 31644: Unit test Signed-off-by: David Nind <david@davidnind.com> 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> 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> (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 > 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..
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> 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> 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! 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 Not backported to 21.05.x |