Bug 15702

Summary: Trim whitespace from patron details upon submission
Product: Koha Reporter: Marc Véron <veron>
Component: PatronsAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: chris, gmcharlt, jonathan.druart, katrin.fischer, kyle.m.hall, kyle, martin.renvoize, mtj, mtompset
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10177
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14509
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18494    
Attachments: Leading / trailing blanks in patron information (screenshots)
Bug 15702: Trim whitespace from fields with non-whitespace data
[SIGNED-OFF] Bug 15702: Trim whitespace from fields with non-whitespace data
Bug 15702: Add test cases for modified code
Bug 15702: Recommended Counter-patch
Bug 15702: Add test cases for modified code
Bug 15702: Recommended Counter-patch
Bug 15702: Add test cases for modified code
Bug 15702: Recommended Counter-patch

Description Marc Véron 2016-01-29 08:04:30 UTC
It is possible to enter leading and trailing blanks in patron's surname, firstname (and in other fields as well). This leads to visible and invisible symptoms like wrong sorting, name not findable if you browse by last name, display of blanks where you do not expect them.
Comment 1 Marc Véron 2016-01-29 08:06:06 UTC
Created attachment 47424 [details]
Leading / trailing blanks in patron information (screenshots)
Comment 2 Owen Leonard 2016-01-29 14:03:55 UTC
Thanks for filing this bug--I should have done so ages ago. This is enough of a problem for us that I regularly run a report to catch these records and correct them. I'm not sure why librarians here are so trigger-happy on the keyboard.

We addressed this issue a long time ago with barcodes. See in returns.pl, line 253: $barcode =~ s/^\s*|\s*$//g; # remove leading/trailing whitespace

Is there any reason not to trim leading and trailing whitespace from all the data submitted during patron entry?
Comment 3 Marc Véron 2016-01-30 00:52:15 UTC
(In reply to Owen Leonard from comment #2)
> Thanks for filing this bug--I should have done so ages ago. This is enough
> of a problem for us that I regularly run a report to catch these records and
> correct them. I'm not sure why librarians here are so trigger-happy on the
> keyboard.
> 
> We addressed this issue a long time ago with barcodes. See in returns.pl,
> line 253: $barcode =~ s/^\s*|\s*$//g; # remove leading/trailing whitespace
> 
> Is there any reason not to trim leading and trailing whitespace from all the
> data submitted during patron entry?

I think there is no any reason to not trim trailin and leading whte spaces. Please go ahead and I will be to signoff.
Comment 4 Martin Renvoize 2016-05-26 20:53:35 UTC
+1. No reason not to
Comment 5 Mark Tompsett 2017-01-10 00:16:55 UTC
Created attachment 58717 [details] [review]
Bug 15702: Trim whitespace from fields with non-whitespace data

Since space may be a valid value for something, trimming what has
non-whitespace was done.

TEST PLAN
---------
1) Enter patron with spaces before, after, and both.
2) Apply patch.
3) Attempt to enter another patron with spaces before, after, and both.
   -- attempting to edit after saving will demonstrate the spaces
      were trimmed.
4) Attempt to edit the patron initially entered.
   -- while there may be spaces before, after, and both initially,
      after saving and trying to edit again, there will not be.
5) run koha qa test tools.
Comment 6 Owen Leonard 2017-01-10 17:02:35 UTC
Created attachment 58768 [details] [review]
[SIGNED-OFF] Bug 15702: Trim whitespace from fields with non-whitespace data

Since space may be a valid value for something, trimming what has
non-whitespace was done.

TEST PLAN
---------
1) Enter patron with spaces before, after, and both.
2) Apply patch.
3) Attempt to enter another patron with spaces before, after, and both.
   -- attempting to edit after saving will demonstrate the spaces
      were trimmed.
4) Attempt to edit the patron initially entered.
   -- while there may be spaces before, after, and both initially,
      after saving and trying to edit again, there will not be.
5) run koha qa test tools.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Jonathan Druart 2017-01-11 08:20:33 UTC
I think this should be done in ModMember and AddMember, covered by tests.
Comment 8 Mark Tompsett 2017-04-21 13:12:37 UTC
Created attachment 62519 [details] [review]
Bug 15702: Add test cases for modified code
Comment 9 Mark Tompsett 2017-04-21 13:12:49 UTC
Created attachment 62520 [details] [review]
Bug 15702: Recommended Counter-patch

As per comment #7, this patch affects AddMember and ModMember.
The test plan should be the same as comment #6.
Secondary patch with tests still to come.
Comment 10 Mark Tompsett 2017-04-21 13:15:06 UTC
TEST PLAN
----------
1) Apply first patch
2) Run prove -v t/db_dependent/Members.t
   -- expecting failures
3) Apply second patch
4) Run prove -v t/db_dependent/Members.t
   -- expecting success
5) run koha qa test tools
   -- two files modified with no issues
Comment 11 Marc Véron 2017-04-21 14:13:19 UTC
Created attachment 62527 [details] [review]
Bug 15702: Add test cases for modified code

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2017-04-21 14:14:22 UTC
Created attachment 62528 [details] [review]
Bug 15702: Recommended Counter-patch

As per comment #7, this patch affects AddMember and ModMember.
The test plan should be the same as comment #6.
Secondary patch with tests still to come.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Mark Tompsett 2017-04-21 15:42:26 UTC
(In reply to Marc Véron from comment #12)
> As per comment #7, this patch affects AddMember and ModMember.

Yes. :)


> The test plan should be the same as comment #6.

No, the test plan is comment #10.

TEST PLAN
----------
1) Apply first patch
2) Run prove -v t/db_dependent/Members.t
   -- expecting failures
3) Apply second patch
4) Run prove -v t/db_dependent/Members.t
   -- expecting success
5) run koha qa test tools
   -- two files modified with no issues


> Secondary patch with tests still to come.

Because I purposefully changed two tests, there are no additional tests. The initial failures are because I purposefully changed the add and mod to include an extra space (. q{ }) at the end in two of the tests. With the new code, that will not change anything, because they are filtered. :)
Comment 14 Marc Véron 2017-04-21 17:38:31 UTC
(In reply to M. Tompsett from comment #13)
> (In reply to Marc Véron from comment #12)
> > As per comment #7, this patch affects AddMember and ModMember.
> 
> Yes. :)
> 
> 
> > The test plan should be the same as comment #6.
> 
> No, the test plan is comment #10.

Sorry, I did not edit the original commit messages. I expected them to be up to date.

> 
> TEST PLAN
> ----------
> 1) Apply first patch
> 2) Run prove -v t/db_dependent/Members.t
>    -- expecting failures
> 3) Apply second patch
> 4) Run prove -v t/db_dependent/Members.t
>    -- expecting success
> 5) run koha qa test tools
>    -- two files modified with no issues
> 
> 
> > Secondary patch with tests still to come.
> 
> Because I purposefully changed two tests, there are no additional tests. The
> initial failures are because I purposefully changed the add and mod to
> include an extra space (. q{ }) at the end in two of the tests. With the new
> code, that will not change anything, because they are filtered. :)

Not sure, is my Sign-off OK or should I do more testing?
Comment 15 Mark Tompsett 2017-04-21 18:49:22 UTC
(In reply to Marc Véron from comment #14)
> Sorry, I did not edit the original commit messages. I expected them to be up
> to date.

Sorry, I failed to update them.
That's my fault.


> Not sure, is my Sign-off OK or should I do more testing?

If you ran the prove, your sign off is okay. If not, run the prove. :)
And feel free to update the test plan. :)
Comment 16 Marc Véron 2017-04-21 20:42:50 UTC
(In reply to M. Tompsett from comment #15)
> (In reply to Marc Véron from comment #14)
> > Sorry, I did not edit the original commit messages. I expected them to be up
> > to date.
> 
> Sorry, I failed to update them.
> That's my fault.
> 
> 
> > Not sure, is my Sign-off OK or should I do more testing?
> 
> If you ran the prove, your sign off is okay. If not, run the prove. :)
> And feel free to update the test plan. :)

No problem, proves were OK, QA tools as well. :-)
Comment 17 Jonathan Druart 2017-04-24 16:15:08 UTC
Created attachment 62613 [details] [review]
Bug 15702: Add test cases for modified code

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2017-04-24 16:15:16 UTC
Created attachment 62614 [details] [review]
Bug 15702: Recommended Counter-patch

As per comment #7, this patch affects AddMember and ModMember.
The test plan should be the same as comment #6.
Secondary patch with tests still to come.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Kyle M Hall 2017-04-24 17:24:30 UTC
Pushed to master for 17.05, thanks Mark!
Comment 20 Katrin Fischer 2017-05-13 11:49:32 UTC
These patches have been pushed to 16.11.x and will be in 16.11.08.
Comment 21 Mason James 2017-05-24 02:05:04 UTC
Pushed to 16.05.x, for 16.05.13 release