Some libraries do a lot of copy pasting when cataloguing, which means occasionally copying over whitespace characters including line breaks and carriage breaks. It would be good if Koha stripped these.
Created attachment 132202 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing WIP
Created attachment 132275 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS
*** Bug 30443 has been marked as a duplicate of this bug. ***
Line breaks and carriage returns are known to break legacy library system record imports and, text area fields in MARC records are not designed to carry formatting, therefore it is implied that the data would be saved as a plain text string with no added formatting. For users who follow these cataloguing standards, there should be a setting to ask Koha to strip these. Planning to amend StripWhitespaceChars to also strip inner line breaks and carriage returns.
Created attachment 132919 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled.
This seems to work but the test does pass: t/db_dependent/Biblio/ModBiblioMarc.t .. 1/2 # Failed test 'Title still has whitespace characters because StripWhitespaceChars is disabled' # at t/db_dependent/Biblio/ModBiblioMarc.t line 67. # got: 'My title' # expected: ' My title # ' # Looks like you failed 1 test of 3. # Failed test 'StripWhitespaceChars tests' # at t/db_dependent/Biblio/ModBiblioMarc.t line 75. # Looks like you failed 1 test of 2. t/db_dependent/Biblio/ModBiblioMarc.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/2 subtests
Created attachment 133055 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space
Created attachment 133056 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines
Created attachment 133140 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines
(In reply to Aleisha Amohia from comment #4) > text area fields in MARC records are not designed to > carry formatting, therefore it is implied that the data would be saved as a > plain text string with no added formatting. As far as I know, there is no prohibition against using carriage returns and line feeds in MARC records. The MARCXML schema seems to indicate that all whitespace is allowed: https://www.loc.gov/standards/marcxml/schema/MARC21slim.xsd. That said, I haven't ever seen an example that includes them either. > For users who follow these cataloguing standards, there should be a setting > to ask Koha to strip these. > > Planning to amend StripWhitespaceChars to also strip inner line breaks and > carriage returns. It would probably be a good idea to indicate that it's data fields and not the leader and control fields that are affected. -- I'd also suggest adding an additional option for inner newlines. While I can see the utility of trimming whitespace off the beginning and end, I imagine that many librarians would be frustrated by inner whitespace being trimmed (at least if there wasn't a clear warning about it).
As this is going to be tied to a new pref StripWhitespaceChars I think it will be ok. We had the same discussion I feel over adding HTML tags to records before. Ok, maybe that was a bit more extreme. I feel why some libraries won't want this, others will be happy as it helps when you copy&paste a lot of content from other sources.
(In reply to Katrin Fischer from comment #11) > As this is going to be tied to a new pref StripWhitespaceChars I think it > will be ok. We had the same discussion I feel over adding HTML tags to > records before. Ok, maybe that was a bit more extreme. I feel why some > libraries won't want this, others will be happy as it helps when you > copy&paste a lot of content from other sources. Yeah, the system preference default looks fine. I think adding clear description in the system preference is a good idea though. The current wording is misleading.
Created attachment 133224 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines
Created attachment 133225 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected
Created attachment 134195 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped.
Created attachment 134453 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side
Created attachment 137451 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137452 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137453 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137454 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137455 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137456 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 137457 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com>
Looking for QA. Code added in C4/AuthoritiesMarc.pm and C4/Biblio.pm is exactly the same. Please move to a common method, maybe in Koha::Util::MARC. And add a unit test on this method. It will make a solid code ;)
Created attachment 139110 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT
Patch no longer applies... git bz apply 30358 Bug 30358 - Strip leading/trailing whitespace characters from input fields when cataloguing 137451 - Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing 137452 - Bug 30358: (follow-up) Also strip inner newlines 137453 - Bug 30358: (follow-up) Inner newlines should be replaced with a space 137454 - Bug 30358: (follow-up) Fixing tests and including for inner newlines 137455 - Bug 30358: (follow-up) Clarify syspref wording about fields affected 137456 - Bug 30358: (follow-up) Consider field has multiple subfields of same key 137457 - Bug 30358: (follow-up) Put multiple subfields fix on auth side 139110 - Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing .git/rebase-apply/patch:16: trailing whitespace. $record->add_fields($auth_type_tag,'','', $auth_type_subfield=>$authtypecode); warning: 1 line adds whitespace errors. Using index info to reconstruct a base tree... M C4/AuthoritiesMarc.pm M C4/Biblio.pm M installer/data/mysql/mandatory/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref M t/db_dependent/Biblio/ModBiblioMarc.t .git/rebase-apply/patch:16: trailing whitespace. $record->add_fields($auth_type_tag,'','', $auth_type_subfield=>$authtypecode); warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Biblio/ModBiblioMarc.t Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging installer/data/mysql/mandatory/sysprefs.sql Auto-merging C4/Biblio.pm Auto-merging C4/AuthoritiesMarc.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing
Created attachment 142905 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142906 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142907 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142908 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142909 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142910 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142911 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com>
Created attachment 142912 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT
Note that line feed in title or author breaks items search because of Ajax via JSON
Everything works as per the test plan, except for tests failing for t/db_dependent/Biblio/ModBiblioMarc.t
Created attachment 143053 [details] [review] Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests
Created attachment 143057 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143058 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143059 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143060 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143061 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143062 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143063 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143064 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Created attachment 143065 [details] [review] Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests Signed-off-by: David Nind <david@davidnind.com>
Wondering if this is a bit too aggressively cleaning up? E.g. field 008 (and maybe more control fields) could end with a few spaces. You will remove them, which is not correct.
About the regex: $value =~ s/^\s+|\s+$|^\n+|\n+$|^\r+|\r+$|^\t+|\t+$//g; Is the following not the same: $value =~ s/^\s+|\s+$//g; ?
(In reply to Marcel de Rooy from comment #47) > Wondering if this is a bit too aggressively cleaning up? > E.g. field 008 (and maybe more control fields) could end with a few spaces. > You will remove them, which is not correct. Should I write a follow-up to exclude fields < 010?
(In reply to Aleisha Amohia from comment #49) > (In reply to Marcel de Rooy from comment #47) > > Wondering if this is a bit too aggressively cleaning up? > > E.g. field 008 (and maybe more control fields) could end with a few spaces. > > You will remove them, which is not correct. > > Should I write a follow-up to exclude fields < 010? Sounds reasonable to me.
Created attachment 144120 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144121 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144122 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144123 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144124 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144125 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144126 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144127 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144128 [details] [review] Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 144129 [details] [review] Bug 30358 (follow-up): Do not strip whitespace from control fields
Created attachment 144130 [details] [review] Bug 30358 (follow-up) Simplify regex The regex does the following: 1. Replace newlines and carriage returns with a space 2. Replace leading and trailing whitespace with nothing (strip)
Created attachment 144171 [details] [review] Bug 30358 (follow-up) Simplify regex The regex does the following: 1. Replace newlines and carriage returns with a space 2. Replace leading and trailing whitespace with nothing (strip) Signed-off-by: Hammat Wele <hammat.wele@inlibro.com>
Created attachment 145308 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145309 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145310 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145311 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145312 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145313 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145314 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145315 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145316 [details] [review] Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145317 [details] [review] Bug 30358 (follow-up): Do not strip whitespace from control fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 145318 [details] [review] Bug 30358 (follow-up) Simplify regex The regex does the following: 1. Replace newlines and carriage returns with a space 2. Replace leading and trailing whitespace with nothing (strip) Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151202 [details] [review] Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing This enhancement adds a system preference StripWhitespaceChars which, when enabled, will strip leading and trailing whitespace characters from all fields when cataloguing both bibliographic records and authority records. Whitespace characters that will be stripped are: - spaces - newlines - carriage returns - tabs To test: 1. Apply patch and install database updates 2. Go to Administration, system preferences, find the new StripWhitespaceChars preference. It should be "Don't strip" by default. Change it to "Strip". 3. Search for a biblio record and edit it. Put some leading or trailing whitespace characters in input fields and textarea fields and save. 4. Confirm these characters are removed when you save the record. 5. Repeat steps 3 and 4 for authority records. 6. Confirm tests pass t/db_dependent/Biblio/ModBiblioMarc.t Sponsored-by: Educational Services Australia SCIS Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151203 [details] [review] Bug 30358: (follow-up) Also strip inner newlines This patch amends the StripWhitespaceChars system preference to also strip inner newlines (line breaks and carriage returns) when enabled. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151204 [details] [review] Bug 30358: (follow-up) Inner newlines should be replaced with a space Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151205 [details] [review] Bug 30358: (follow-up) Fixing tests and including for inner newlines Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151206 [details] [review] Bug 30358: (follow-up) Clarify syspref wording about fields affected Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151207 [details] [review] Bug 30358: (follow-up) Consider field has multiple subfields of same key To test: 1) Click the clone subfield button to make multiple subfields with the same key, i.e. 500$a$a$a 2) Save the record and confirm that the fields contain the correct data after whitespaces are stripped. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151208 [details] [review] Bug 30358: (follow-up) Put multiple subfields fix on auth side Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151210 [details] [review] Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests To test: Confirm test plan above still works as expected and tests pass in t/Koha_MetadataRecord.t Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151212 [details] [review] Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151214 [details] [review] Bug 30358: (follow-up) Do not strip whitespace from control fields Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 151215 [details] [review] Bug 30358: (follow-up) Simplify regex The regex does the following: 1. Replace newlines and carriage returns with a space 2. Replace leading and trailing whitespace with nothing (strip) Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
* Rebased, only conflict was in the .pref file * Fixed commit messages on the last 2 patches (QA script is happy now) * Ran the unit tests. All green * Verified controlfields are not affected, but other fields are cleaned up Back to Tomas :)
Hi all. I'm willing to push a squashed version of this, because it's been hanging around for a while and it has an on/off switch and also (when squashed) seems like a small thing. I would've prefered more communication about this and a more QA eyes, because the chosen pattern is hard to accept in the current codebase (a static method on a not so related class). This should've been a Koha::RecordProcessor filter instead, as (a) This is not about Koha::Metadata::Record objects, but MARC::Record (b) that's what the Koha::RecordProcessor pattern is designed for. I will file a separate follow-up bug, and would like the authors to consider movign this code into a Koha::recordProcessor::Filter class. Happy to guide/help.
Pushed to master for 23.05. Nice work everyone, thanks!
Enhancement. Not pushing to 22.11.x.
Documentation already submitted
Is the removal of inner newline chars on purpose? Doesn't it break note fields for some people
(In reply to Tomás Cohen Arazi from comment #90) > Is the removal of inner newline chars on purpose? Doesn't it break note > fields for some people Yeah I think it's on purpose, but I have the same criticism. I think it would break note fields for people. I raised some concerns back on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30358#c10 I also suggested adding different options for trimming start/end and start/inner/end. I wouldn't recommend this system preference to anyone as is, but it should default to off, so caveat emptor...