Bug 10861 - Add a check on cardnumber length
Summary: Add a check on cardnumber length
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13984
  Show dependency treegraph
 
Reported: 2013-09-10 11:57 UTC by Jonathan Druart
Modified: 2015-04-13 11:08 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10861: Add a check on cardnumber length (12.36 KB, patch)
2013-09-10 12:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10861: Add a check on cardnumber length (12.35 KB, patch)
2013-09-18 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10861: Add a check on cardnumber length (12.33 KB, patch)
2013-09-29 06:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10861: Add a check on cardnumber length (12.50 KB, patch)
2013-12-11 12:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Follow-up for typo in patrons.pref (1.13 KB, patch)
2013-12-11 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Followup Add a check on cardnumber length (9.28 KB, patch)
2013-12-11 18:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10861: Followup Add a check on cardnumber length (9.37 KB, patch)
2013-12-12 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Reintroduced the cardnumber length check (client side) (4.39 KB, patch)
2013-12-12 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10861: Add a check on cardnumber length (12.53 KB, patch)
2013-12-16 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Follow-up for typo in patrons.pref (1.13 KB, patch)
2013-12-16 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Followup Add a check on cardnumber length (13.03 KB, patch)
2013-12-16 09:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10861: Add a check on cardnumber length (12.54 KB, patch)
2013-12-20 19:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10861: Follow-up for typo in patrons.pref (1.15 KB, patch)
2013-12-20 19:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10861: Followup Add a check on cardnumber length (13.05 KB, patch)
2013-12-20 19:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10861 - QA Followup - Clarify preference description (1.02 KB, patch)
2013-12-20 19:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10861 - QA Followup - Clarify preference description (1.02 KB, patch)
2013-12-20 19:23 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-10 11:57:52 UTC
Some libraries would like to add a check on the cardnumber length.
Comment 1 Jonathan Druart 2013-09-10 12:56:22 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-09-18 10:48:14 UTC
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".
Comment 3 Jonathan Druart 2013-09-18 12:19:23 UTC Comment hidden (obsolete)
Comment 4 I'm just a bot 2013-09-29 06:07:17 UTC
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".
Comment 5 Chris Cormack 2013-09-29 06:58:11 UTC Comment hidden (obsolete)
Comment 6 I'm just a bot 2013-10-27 06:31:16 UTC
Patch applied cleanly, go forth and signoff
Comment 7 Marcel de Rooy 2013-12-11 12:48:02 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2013-12-11 12:49:06 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2013-12-11 12:53:47 UTC
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?
Comment 10 Jonathan Druart 2013-12-11 18:10:48 UTC
(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.
Comment 11 Jonathan Druart 2013-12-11 18:13:51 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2013-12-12 08:04:33 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-12-12 08:08:23 UTC
>> 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
Comment 14 Jonathan Druart 2013-12-12 09:22:25 UTC
(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.
Comment 15 Jonathan Druart 2013-12-12 09:45:52 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-12-12 09:47:01 UTC
Sorry Marcel but could you please have a look at this last (I hope!) followup please?
Comment 17 Marcel de Rooy 2013-12-12 11:11:05 UTC
(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)
Comment 18 Marcel de Rooy 2013-12-12 11:12:28 UTC
(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..
Comment 19 Jonathan Druart 2013-12-12 11:32:26 UTC
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).
Comment 20 Marcel de Rooy 2013-12-16 09:17:33 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2013-12-16 09:17:48 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2013-12-16 09:17:58 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-12-20 19:20:45 UTC
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>
Comment 24 Kyle M Hall 2013-12-20 19:20:54 UTC
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>
Comment 25 Kyle M Hall 2013-12-20 19:21:09 UTC
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>
Comment 26 Kyle M Hall 2013-12-20 19:22:03 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2013-12-20 19:23:18 UTC
Created attachment 23736 [details] [review]
Bug 10861 - QA Followup - Clarify preference description
Comment 28 Galen Charlton 2014-03-12 04:09:35 UTC
Pushed to master, along with follow-ups that improve client-side validation, fully implement the ",$max" option, and improve the test cases.

Thanks, Jonathan!