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.
Created attachment 115247 [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>
NOTE FOR QA: Using repeat is nicer code but is not available in IE. Although we actually should not want that anyway ;)
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>
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.
(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.
Created attachment 123973 [details] [review] Bug 27461: Move hardcoded value to module Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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>
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>
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 ;)
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>
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>
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>
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>
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>
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>
Nice clear code, works as expected and no regressions found. Tests all passing. Passing QA, thanks guys.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
Pushed to 20.11.x for 20.11.11
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.