Bug 15702 - Trim whitespace from patron details upon submission
Summary: Trim whitespace from patron details upon submission
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 18494
  Show dependency treegraph
 
Reported: 2016-01-29 08:04 UTC by Marc Véron
Modified: 2017-04-26 08:38 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Leading / trailing blanks in patron information (screenshots) (140.32 KB, image/png)
2016-01-29 08:06 UTC, Marc Véron
Details
Bug 15702: Trim whitespace from fields with non-whitespace data (1.47 KB, patch)
2017-01-10 00:16 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15702: Trim whitespace from fields with non-whitespace data (1.53 KB, patch)
2017-01-10 17:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 15702: Add test cases for modified code (1.07 KB, patch)
2017-04-21 13:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 15702: Recommended Counter-patch (1.65 KB, patch)
2017-04-21 13:12 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 15702: Add test cases for modified code (1.20 KB, patch)
2017-04-21 14:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15702: Recommended Counter-patch (1.79 KB, patch)
2017-04-21 14:14 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15702: Add test cases for modified code (1.29 KB, patch)
2017-04-24 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15702: Recommended Counter-patch (1.86 KB, patch)
2017-04-24 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 M. 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 M. Tompsett 2017-04-21 13:12:37 UTC
Created attachment 62519 [details] [review]
Bug 15702: Add test cases for modified code
Comment 9 M. 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 M. 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 M. 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 M. 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!