Some libraries would like to add a check on the cardnumber length.
Created attachment 20950 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute)
Applying: Bug 10861: Add a check on cardnumber length Using index info to reconstruct a base tree... M installer/data/mysql/updatedatabase.pl Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Patch failed at 0001 Bug 10861: Add a check on cardnumber length The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 21196 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute)
Applying: Bug 10861: Add a check on cardnumber length Using index info to reconstruct a base tree... M installer/data/mysql/updatedatabase.pl M koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Patch failed at 0001 Bug 10861: Add a check on cardnumber length The copy of the patch that failed is found in: /home/christopher/git/koha/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Created attachment 21586 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute)
Patch applied cleanly, go forth and signoff
Created attachment 23387 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested adding, updating; importing and ran unit test. Preliminary QA comments on Bugzilla
Created attachment 23388 [details] [review] Bug 10861: Follow-up for typo in patrons.pref I'd rather have a comma than a coma :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Just some preliminary QA comments while signing off (for the QAer on duty): Cardnumber already in use or not in a good format: I would personally like to differentiate between the two messages. Is it in use? Or is just the length incorrect? The min and maxlength logic in memberentry.pl should be in a module? Isn't there a way to check field length earlier with javascript than waiting for Save? I am not a fan of my $qr = qr|^\d{$length}$|; and just pasting the pref inside the regex. You just assume that it is of the form m,n but you do not really know. Why not just pull them out of the pref with a regex like (\d+),(\d+) or similar?
(In reply to M. de Rooy from comment #9) > Just some preliminary QA comments while signing off (for the QAer on duty): Thanks Marcel, yours remarks allowed me to improve the code. > Cardnumber already in use or not in a good format: I would personally like > to differentiate between the two messages. Is it in use? Or is just the > length incorrect? Will be fixed > The min and maxlength logic in memberentry.pl should be in a module? New routine and UT will be provided. > Isn't there a way to check field length earlier with javascript than waiting > for Save? It is already done. I use the pattern attribute on the input html tag. > I am not a fan of my $qr = qr|^\d{$length}$|; and just pasting the pref > inside the regex. > You just assume that it is of the form m,n but you do not really know. > Why not just pull them out of the pref with a regex like (\d+),(\d+) or > similar? I use the code which was in the pl file from the pm. It avoids that.
Created attachment 23439 [details] [review] Bug 10861: Followup Add a check on cardnumber length This patch refactors the previous code and moves the logic from the pl to a new routine. Same test plan as previous patch. /!\ new unit test filename.
Created attachment 23476 [details] [review] Bug 10861: Followup Add a check on cardnumber length This patch refactors the previous code and moves the logic from the pl to a new routine. Same test plan as previous patch. /!\ new unit test filename. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>> Isn't there a way to check field length earlier with javascript than waiting >> for Save? > It is already done. I use the pattern attribute on the input html tag. Note that you removed the pattern attribute now. But it is no problem. It seems that this attribute does not work like it should. (I tested adding it again with a new Firefox 26 version, but it did not work..) Signed off
(In reply to M. de Rooy from comment #13) > Note that you removed the pattern attribute now. But it is no problem. > It seems that this attribute does not work like it should. (I tested adding > it again with a new Firefox 26 version, but it did not work..) Arf, it is not needed! I removed it for testing the server side error. I will provide a followup to reintroduce the test in the client side.
Created attachment 23477 [details] [review] Bug 10861: Reintroduced the cardnumber length check (client side) Previous patches has removed the pattern attribute of the input, it was not needed. This patch reintroduces it. It will only work for new browser version. Moreover, it manages with the ',XX' format (see UT).
Sorry Marcel but could you please have a look at this last (I hope!) followup please?
(In reply to Jonathan Druart from comment #16) > Sorry Marcel but could you please have a look at this last (I hope!) > followup please? Well I did. But it does not work (see earlier comment)
(In reply to M. de Rooy from comment #17) > Well I did. But it does not work (see earlier comment) If I clear the field, the title flashes about what I should enter. But it does not force anything. Moreover, if I do not clear the field, I can type whatever I want without any warning or so..
Using Iceweasel 19.0, I set the pref to '5', add a new patron, fill the cardnumber field with "1234", click on "save" and the field is highlighted with a message "Please fill this field: Exactly 5 characters" (something like that).
Created attachment 23563 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested adding, updating; importing and ran unit test. Preliminary QA comments on Bugzilla
Created attachment 23564 [details] [review] Bug 10861: Follow-up for typo in patrons.pref I'd rather have a comma than a coma :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 23565 [details] [review] Bug 10861: Followup Add a check on cardnumber length This patch refactors the previous code and moves the logic from the pl to a new routine. Same test plan as previous patch. /!\ new unit test filename. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 10861: Reintroduced the cardnumber length check (client side) Previous patches has removed the pattern attribute of the input, it was not needed. This patch reintroduces it. It will only work for new browser version. Moreover, it manages with the ',XX' format (see UT). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Squashed the last two follow-ups. The pattern test did not work fully for me in Firefox 26 (very recent). But I see the message when I clear the field.
Created attachment 23732 [details] [review] Bug 10861: Add a check on cardnumber length Some libraries would like to add a check on the cardnumber length. This patch adds the ability to restrict the cardnumber to a specific length (strictly equal to XX, or length > XX or min < length < max). This restriction is checked on inserting/updating a patron or on importing patrons. This patch adds: - 1 new syspref CardnumberLength. 2 formats: a number or a range (xx,yy). - 1 new unit test file t/Members/checkcardnumber.t for the C4::Members::checkcardnumber routine. Test plan: 1/ Fill the pref CardnumberLength with '5,8' 2/ Create a new patron with an invalid cardnumber (123456789) 3/ Check that you cannot save 4/ With Firebug, replace the pattern attribute value (for the cardnumber input) with ".{5,10}" 5/ You are allowed to save but an error occurred. 6/ Try the same steps for update. 7/ Go to the import borrowers tool. 8/ Play with the import borrowers tool. We must test add/update patrons and the "record matching" field (cardnumber or a uniq patron attribute) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested adding, updating; importing and ran unit test. Preliminary QA comments on Bugzilla Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 23733 [details] [review] Bug 10861: Follow-up for typo in patrons.pref I'd rather have a comma than a coma :) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 23734 [details] [review] Bug 10861: Followup Add a check on cardnumber length This patch refactors the previous code and moves the logic from the pl to a new routine. Same test plan as previous patch. /!\ new unit test filename. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Bug 10861: Reintroduced the cardnumber length check (client side) Previous patches has removed the pattern attribute of the input, it was not needed. This patch reintroduces it. It will only work for new browser version. Moreover, it manages with the ',XX' format (see UT). Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Squashed the last two follow-ups. The pattern test did not work fully for me in Firefox 26 (very recent). But I see the message when I clear the field. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 23735 [details] [review] Bug 10861 - QA Followup - Clarify preference description
Created attachment 23736 [details] [review] Bug 10861 - QA Followup - Clarify preference description
Pushed to master, along with follow-ups that improve client-side validation, fully implement the ",$max" option, and improve the test cases. Thanks, Jonathan!