Bug 27461

Summary: Fix field 008 length below 40 positions in cataloguing plugin
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: CatalogingAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, kyle, m.de.rooy, martin.renvoize, phil, victor
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.11
Attachments: Bug 27461: Fix field 008 length below 40 positions in cataloguing plugin
Bug 27461: Fix field 008 length below 40 positions in cataloguing plugin
Bug 27461: Move hardcoded value to module
Bug 27461: Add tests for biblio_008
Bug 27461: Right pad with default elements for too short 008s
Bug 27461: Move hardcoded value to module
Bug 27461: Add tests for biblio_008
Bug 27461: Right pad with default elements for too short 008s
Bug 27461: Move hardcoded value to module
Bug 27461: Add tests for biblio_008
Bug 27461: Right pad with default elements for too short 008s

Description Marcel de Rooy 2021-01-18 13:48:06 UTC
If the length is below 40 chars, the string is not extended to 40 positions. I am not completely sure if there is more going on, but this could be a first fix anyway. I came across shorter fields in our data.
Comment 1 Marcel de Rooy 2021-01-18 13:54:23 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2021-01-18 13:56:19 UTC
NOTE FOR QA: Using repeat is nicer code but is not available in IE. Although we actually should not want that anyway ;)
Comment 3 Marcel de Rooy 2021-01-18 13:58:03 UTC
Created attachment 115248 [details] [review]
Bug 27461: Fix field 008 length below 40 positions in cataloguing plugin

Test plan:
Make contents of field 008 shorter in editor before clicking plugin.
Save in plugin.
Verify that character length in field 008 is now 40 positions.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Phil Ringnalda 2021-02-21 09:07:39 UTC
A space is an invalid character in some positions, a character with a particular meaning in some positions, and only rarely a synonym for "dunno what should be here." | is closer to the mark, since it's defined as "no attempt to code" for every position except 0-5.

However, just pipe-padding instead of space-padding won't work except in the case where value.length going into the plugin was 39, because if it's 38 or less, the plugin is *broken*. If the length going in is 37 (delete everything back to and including position 37, the last letter of the language) and you then use the plugin to change the value for one of the last two positions, your value will actually be put into position 37 rather than where it belongs.

Do what seems reasonable until you try it, type 210220s in the text input and then trigger the plugin, and fill things in in random order, first setting the Cataloging source so you don't forget, then seeing that Index and Literary form and Biography are wrong and changing them, then going back up to put in your single date without realizing you're also supposed to put in spaces for the second date, then go back down and put in the language, then back up for the place of publication. You now have a random-length 008 with random letters in random spots.

So, three cases:

The value going into the plugin is 0 characters long. That is already handled by creating a value with some imperfect default values

The value going into the plugin is 1 to 5 characters long. That's a broken date-entered, which can't be fixed with pipes, it can only be deleted and treated the same as 0 characters.

The value going into the plugin is 6-39 characters long, which can be fixed by either applying our somewhat imperfect default values to any missing characters, or less-imperfectly by pipe-filling.

Inconveniently, you can't just pad with either pipes or the remaining characters from our default blindly, since filling position 35-39 with pipes is just fine, but filling position 36-39 is invalid, since "e||" is not a valid language code. So it's an awkward case of length 6-7 you can pad, 8-11 you have to truncate to 7 and pad, 12 you can pad, 13-15 you have to truncate to 12 and pad, etc.
Comment 5 Marcel de Rooy 2021-08-19 13:55:39 UTC
(In reply to Phil Ringnalda from comment #4)

> Inconveniently, you can't just pad with either pipes or the remaining
> characters from our default blindly, since filling position 35-39 with pipes
> is just fine, but filling position 36-39 is invalid, since "e||" is not a
> valid language code. So it's an awkward case of length 6-7 you can pad, 8-11
> you have to truncate to 7 and pad, 12 you can pad, 13-15 you have to
> truncate to 12 and pad, etc.

Phil.
I added a proposal to right pad with characters from the default from the last complete data element onwards.
Viewing block 18-34 as one element, since it depends on material type.
Comment 6 Marcel de Rooy 2021-08-19 13:56:25 UTC
Created attachment 123973 [details] [review]
Bug 27461: Move hardcoded value to module

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2021-08-19 13:56:28 UTC
Created attachment 123974 [details] [review]
Bug 27461: Add tests for biblio_008

Test plan:
Run  t/db_dependent/FrameworkPlugin.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2021-08-19 13:56:32 UTC
Created attachment 123975 [details] [review]
Bug 27461: Right pad with default elements for too short 008s

Note that we treat the elements 18-34 as one block of elements,
since its subdivision may vary.
E.g. if you pass 24 characters to the form, it will use the first
18 chars (until the last complete element) and pad with default
elements from position 18-39.

Test plan:
[1] Go to addbiblio. Make sure that 008 is connected to the plugin.
[2] Backspace field 008 a bit, click on the plugin button.
[3] Verify that the last elements come from the default.
[4] Repeat for a few different lengths.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2021-08-19 14:29:41 UTC
For the record noting here that we are using as default pipe chars in the country field.

MARC specs say:

Three fill characters (|||) may be used in place of a valid code, but their use in 008/15-17 is discouraged.

If we want to change that too, please open up a new report ;)
Comment 10 Phil Ringnalda 2021-09-02 18:12:48 UTC
Created attachment 124433 [details] [review]
Bug 27461: Move hardcoded value to module

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 11 Phil Ringnalda 2021-09-02 18:12:52 UTC
Created attachment 124434 [details] [review]
Bug 27461: Add tests for biblio_008

Test plan:
Run  t/db_dependent/FrameworkPlugin.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 12 Phil Ringnalda 2021-09-02 18:12:56 UTC
Created attachment 124435 [details] [review]
Bug 27461: Right pad with default elements for too short 008s

Note that we treat the elements 18-34 as one block of elements,
since its subdivision may vary.
E.g. if you pass 24 characters to the form, it will use the first
18 chars (until the last complete element) and pad with default
elements from position 18-39.

Test plan:
[1] Go to addbiblio. Make sure that 008 is connected to the plugin.
[2] Backspace field 008 a bit, click on the plugin button.
[3] Verify that the last elements come from the default.
[4] Repeat for a few different lengths.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 13 Martin Renvoize 2021-09-27 09:51:21 UTC
Created attachment 125349 [details] [review]
Bug 27461: Move hardcoded value to module

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2021-09-27 09:51:25 UTC
Created attachment 125350 [details] [review]
Bug 27461: Add tests for biblio_008

Test plan:
Run  t/db_dependent/FrameworkPlugin.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2021-09-27 09:51:29 UTC
Created attachment 125351 [details] [review]
Bug 27461: Right pad with default elements for too short 008s

Note that we treat the elements 18-34 as one block of elements,
since its subdivision may vary.
E.g. if you pass 24 characters to the form, it will use the first
18 chars (until the last complete element) and pad with default
elements from position 18-39.

Test plan:
[1] Go to addbiblio. Make sure that 008 is connected to the plugin.
[2] Backspace field 008 a bit, click on the plugin button.
[3] Verify that the last elements come from the default.
[4] Repeat for a few different lengths.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-09-27 09:52:13 UTC
Nice clear code, works as expected and no regressions found. Tests all passing.

Passing QA, thanks guys.
Comment 17 Jonathan Druart 2021-09-28 13:38:09 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 18 Kyle M Hall 2021-10-08 11:59:50 UTC
Pushed to 21.05.x for 21.05.05
Comment 19 Fridolin Somers 2021-10-09 02:30:56 UTC
Pushed to 20.11.x for 20.11.11
Comment 20 Victor Grousset/tuxayo 2021-10-17 01:16:32 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.