Bug 30358 - Strip leading/trailing whitespace characters from input fields when cataloguing
Summary: Strip leading/trailing whitespace characters from input fields when cataloguing
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
: 30443 (view as bug list)
Depends on:
Blocks: 33749
  Show dependency treegraph
 
Reported: 2022-03-25 01:41 UTC by Aleisha Amohia
Modified: 2023-06-07 23:45 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/692
Text to go in the release notes:
This adds a new 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
Version(s) released in:
23.05.00


Attachments
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (4.59 KB, patch)
2022-03-25 03:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.18 KB, patch)
2022-03-28 03:10 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.42 KB, patch)
2022-04-04 02:36 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.48 KB, patch)
2022-04-07 06:04 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (2.11 KB, patch)
2022-04-07 06:04 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (2.73 KB, patch)
2022-04-11 03:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.39 KB, patch)
2022-04-12 22:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.39 KB, patch)
2022-04-12 22:53 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.11 KB, patch)
2022-04-27 22:56 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (962 bytes, patch)
2022-05-02 04:58 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.18 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.42 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.53 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.44 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.43 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.16 KB, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1008 bytes, patch)
2022-07-09 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.47 KB, patch)
2022-08-15 04:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.33 KB, patch)
2022-11-02 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.46 KB, patch)
2022-11-02 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.53 KB, patch)
2022-11-02 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.44 KB, patch)
2022-11-02 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.44 KB, patch)
2022-11-02 02:08 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.16 KB, patch)
2022-11-02 02:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1008 bytes, patch)
2022-11-02 02:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.47 KB, patch)
2022-11-02 02:09 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests (1.91 KB, patch)
2022-11-03 01:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.29 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.42 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.53 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.44 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.43 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.16 KB, patch)
2022-11-03 06:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1008 bytes, patch)
2022-11-03 06:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.52 KB, patch)
2022-11-03 06:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests (1.96 KB, patch)
2022-11-03 06:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.36 KB, patch)
2022-11-21 21:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.43 KB, patch)
2022-11-21 21:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.53 KB, patch)
2022-11-21 21:39 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.45 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.43 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.16 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1010 bytes, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.52 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests (1.96 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358 (follow-up): Do not strip whitespace from control fields (2.89 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358 (follow-up) Simplify regex (1.09 KB, patch)
2022-11-21 21:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30358 (follow-up) Simplify regex (1.14 KB, patch)
2022-11-22 17:31 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.40 KB, patch)
2023-01-13 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.48 KB, patch)
2023-01-13 20:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.58 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.49 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.48 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.21 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1.04 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.57 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests (2.01 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358 (follow-up): Do not strip whitespace from control fields (2.94 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358 (follow-up) Simplify regex (1.19 KB, patch)
2023-01-13 20:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing (8.37 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Also strip inner newlines (5.48 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Inner newlines should be replaced with a space (1.58 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing tests and including for inner newlines (3.49 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Clarify syspref wording about fields affected (1.48 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Consider field has multiple subfields of same key (1.21 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Put multiple subfields fix on auth side (1.04 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) stripWhitespaceChars subroutine and tests (4.57 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests (2.01 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Do not strip whitespace from control fields (2.94 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30358: (follow-up) Simplify regex (1.19 KB, patch)
2023-05-15 18:05 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-03-25 01:41:03 UTC
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.
Comment 1 Aleisha Amohia 2022-03-25 03:13:30 UTC
Created attachment 132202 [details] [review]
Bug 30358: Strip leading/trailing whitespace characters from input fields when cataloguing

WIP
Comment 2 Aleisha Amohia 2022-03-28 03:10:27 UTC
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
Comment 3 Aleisha Amohia 2022-04-04 01:45:07 UTC
*** Bug 30443 has been marked as a duplicate of this bug. ***
Comment 4 Aleisha Amohia 2022-04-04 01:46:00 UTC
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.
Comment 5 Aleisha Amohia 2022-04-04 02:36:24 UTC
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.
Comment 6 Owen Leonard 2022-04-04 14:02:45 UTC
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
Comment 7 Aleisha Amohia 2022-04-07 06:04:08 UTC
Created attachment 133055 [details] [review]
Bug 30358: (follow-up) Inner newlines should be replaced with a space
Comment 8 Aleisha Amohia 2022-04-07 06:04:12 UTC
Created attachment 133056 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines
Comment 9 Aleisha Amohia 2022-04-11 03:11:35 UTC
Created attachment 133140 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines
Comment 10 David Cook 2022-04-11 05:27:32 UTC
(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).
Comment 11 Katrin Fischer 2022-04-11 06:10:11 UTC
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.
Comment 12 David Cook 2022-04-11 07:09:38 UTC
(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.
Comment 13 Aleisha Amohia 2022-04-12 22:53:21 UTC
Created attachment 133224 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines
Comment 14 Aleisha Amohia 2022-04-12 22:53:25 UTC
Created attachment 133225 [details] [review]
Bug 30358: (follow-up) Clarify syspref wording about fields affected
Comment 15 Aleisha Amohia 2022-04-27 22:56:44 UTC
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.
Comment 16 Aleisha Amohia 2022-05-02 04:58:05 UTC
Created attachment 134453 [details] [review]
Bug 30358: (follow-up) Put multiple subfields fix on auth side
Comment 17 David Nind 2022-07-09 22:29:13 UTC
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>
Comment 18 David Nind 2022-07-09 22:29:18 UTC
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>
Comment 19 David Nind 2022-07-09 22:29:23 UTC
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>
Comment 20 David Nind 2022-07-09 22:29:27 UTC
Created attachment 137454 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2022-07-09 22:29:31 UTC
Created attachment 137455 [details] [review]
Bug 30358: (follow-up) Clarify syspref wording about fields affected

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2022-07-09 22:29:36 UTC
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>
Comment 23 David Nind 2022-07-09 22:29:40 UTC
Created attachment 137457 [details] [review]
Bug 30358: (follow-up) Put multiple subfields fix on auth side

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 Fridolin Somers 2022-08-13 07:54:49 UTC
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 ;)
Comment 25 Aleisha Amohia 2022-08-15 04:50:36 UTC
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
Comment 26 David Nind 2022-10-28 20:58:46 UTC
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
Comment 27 Aleisha Amohia 2022-11-02 02:08:33 UTC Comment hidden (obsolete)
Comment 28 Aleisha Amohia 2022-11-02 02:08:38 UTC Comment hidden (obsolete)
Comment 29 Aleisha Amohia 2022-11-02 02:08:44 UTC Comment hidden (obsolete)
Comment 30 Aleisha Amohia 2022-11-02 02:08:49 UTC Comment hidden (obsolete)
Comment 31 Aleisha Amohia 2022-11-02 02:08:55 UTC Comment hidden (obsolete)
Comment 32 Aleisha Amohia 2022-11-02 02:09:02 UTC Comment hidden (obsolete)
Comment 33 Aleisha Amohia 2022-11-02 02:09:07 UTC Comment hidden (obsolete)
Comment 34 Aleisha Amohia 2022-11-02 02:09:14 UTC Comment hidden (obsolete)
Comment 35 Fridolin Somers 2022-11-02 07:33:01 UTC
Note that line feed in title or author breaks items search because of Ajax via JSON
Comment 36 David Nind 2022-11-02 13:36:56 UTC
Everything works as per the test plan, except for tests failing for t/db_dependent/Biblio/ModBiblioMarc.t
Comment 37 Aleisha Amohia 2022-11-03 01:13:02 UTC Comment hidden (obsolete)
Comment 38 David Nind 2022-11-03 06:34:31 UTC
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>
Comment 39 David Nind 2022-11-03 06:34:37 UTC
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>
Comment 40 David Nind 2022-11-03 06:34:42 UTC
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>
Comment 41 David Nind 2022-11-03 06:34:47 UTC
Created attachment 143060 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 David Nind 2022-11-03 06:34:52 UTC
Created attachment 143061 [details] [review]
Bug 30358: (follow-up) Clarify syspref wording about fields affected

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2022-11-03 06:34:58 UTC
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>
Comment 44 David Nind 2022-11-03 06:35:04 UTC
Created attachment 143063 [details] [review]
Bug 30358: (follow-up) Put multiple subfields fix on auth side

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2022-11-03 06:35:09 UTC
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>
Comment 46 David Nind 2022-11-03 06:35:15 UTC
Created attachment 143065 [details] [review]
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 47 Marcel de Rooy 2022-11-03 12:54:04 UTC
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.
Comment 48 Marcel de Rooy 2022-11-03 12:59:11 UTC
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;

?
Comment 49 Aleisha Amohia 2022-11-21 03:25:20 UTC
(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?
Comment 50 Marcel de Rooy 2022-11-21 07:49:13 UTC
(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.
Comment 51 Aleisha Amohia 2022-11-21 21:39:43 UTC
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>
Comment 52 Aleisha Amohia 2022-11-21 21:39:51 UTC
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>
Comment 53 Aleisha Amohia 2022-11-21 21:39:58 UTC
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>
Comment 54 Aleisha Amohia 2022-11-21 21:40:06 UTC
Created attachment 144123 [details] [review]
Bug 30358: (follow-up) Fixing tests and including for inner newlines

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 Aleisha Amohia 2022-11-21 21:40:13 UTC
Created attachment 144124 [details] [review]
Bug 30358: (follow-up) Clarify syspref wording about fields affected

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 Aleisha Amohia 2022-11-21 21:40:21 UTC
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>
Comment 57 Aleisha Amohia 2022-11-21 21:40:29 UTC
Created attachment 144126 [details] [review]
Bug 30358: (follow-up) Put multiple subfields fix on auth side

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 Aleisha Amohia 2022-11-21 21:40:36 UTC
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>
Comment 59 Aleisha Amohia 2022-11-21 21:40:43 UTC
Created attachment 144128 [details] [review]
Bug 30358: (follow-up) Fixing ModBiblioMarc.t tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 60 Aleisha Amohia 2022-11-21 21:40:51 UTC
Created attachment 144129 [details] [review]
Bug 30358 (follow-up): Do not strip whitespace from control fields
Comment 61 Aleisha Amohia 2022-11-21 21:40:59 UTC
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)
Comment 62 Hammat wele 2022-11-22 17:31:17 UTC
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>
Comment 63 Kyle M Hall 2023-01-13 20:00:40 UTC
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>
Comment 64 Kyle M Hall 2023-01-13 20:00:56 UTC
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>
Comment 65 Kyle M Hall 2023-01-13 20:01:01 UTC
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>
Comment 66 Kyle M Hall 2023-01-13 20:01:07 UTC
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>
Comment 67 Kyle M Hall 2023-01-13 20:01:12 UTC
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>
Comment 68 Kyle M Hall 2023-01-13 20:01:17 UTC
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>
Comment 69 Kyle M Hall 2023-01-13 20:01:22 UTC
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>
Comment 70 Kyle M Hall 2023-01-13 20:01:27 UTC
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>
Comment 71 Kyle M Hall 2023-01-13 20:01:33 UTC
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>
Comment 72 Kyle M Hall 2023-01-13 20:01:38 UTC
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>
Comment 73 Kyle M Hall 2023-01-13 20:01:43 UTC
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>
Comment 74 Katrin Fischer 2023-05-15 18:05:02 UTC
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>
Comment 75 Katrin Fischer 2023-05-15 18:05:07 UTC
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>
Comment 76 Katrin Fischer 2023-05-15 18:05:11 UTC
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>
Comment 77 Katrin Fischer 2023-05-15 18:05:15 UTC
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>
Comment 78 Katrin Fischer 2023-05-15 18:05:19 UTC
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>
Comment 79 Katrin Fischer 2023-05-15 18:05:23 UTC
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>
Comment 80 Katrin Fischer 2023-05-15 18:05:27 UTC
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>
Comment 81 Katrin Fischer 2023-05-15 18:05:31 UTC
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>
Comment 82 Katrin Fischer 2023-05-15 18:05:35 UTC
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>
Comment 83 Katrin Fischer 2023-05-15 18:05:39 UTC
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>
Comment 84 Katrin Fischer 2023-05-15 18:05:42 UTC
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>
Comment 85 Katrin Fischer 2023-05-15 18:09:36 UTC
* 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 :)
Comment 86 Tomás Cohen Arazi 2023-05-16 17:39:51 UTC
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.
Comment 87 Tomás Cohen Arazi 2023-05-16 18:19:07 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 88 Pedro Amorim 2023-06-07 12:01:06 UTC
Enhancement. Not pushing to 22.11.x.
Comment 89 Caroline Cyr La Rose 2023-06-07 13:00:26 UTC
Documentation already submitted
Comment 90 Tomás Cohen Arazi 2023-06-07 17:34:13 UTC
Is the removal of inner newline chars on purpose? Doesn't it break note fields for some people
Comment 91 David Cook 2023-06-07 23:45:00 UTC
(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...