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.
Created attachment 47424 [details] Leading / trailing blanks in patron information (screenshots)
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?
(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.
+1. No reason not to
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.
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>
I think this should be done in ModMember and AddMember, covered by tests.
Created attachment 62519 [details] [review] Bug 15702: Add test cases for modified code
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.
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
Created attachment 62527 [details] [review] Bug 15702: Add test cases for modified code Signed-off-by: Marc Véron <veron@veron.ch>
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>
(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. :)
(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?
(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. :)
(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. :-)
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>
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>
Pushed to master for 17.05, thanks Mark!
These patches have been pushed to 16.11.x and will be in 16.11.08.
Pushed to 16.05.x, for 16.05.13 release