Description
Marc Véron
2016-01-29 08:04:30 UTC
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 |