Bug 10267 - No error message when entering an invalid cardnumber
Summary: No error message when entering an invalid cardnumber
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-17 08:04 UTC by Katrin Fischer
Modified: 2017-10-27 17:12 UTC (History)
9 users (show)

See Also:
Change sponsored?: 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
Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.39 KB, patch)
2017-04-20 03:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.56 KB, patch)
2017-04-20 06:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.16 KB, patch)
2017-04-28 01:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.23 KB, patch)
2017-05-01 22:20 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.28 KB, patch)
2017-08-08 16:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.33 KB, patch)
2017-09-01 13:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10267: Display the message only if the user enters too many characters (2.25 KB, patch)
2017-10-24 21:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10267: Display the message only if the user enters too many characters (2.30 KB, patch)
2017-10-24 21:22 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 10267: Show error message if user tries to enter too many characters for cardnumber (4.33 KB, patch)
2017-10-27 14:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10267: Display the message only if the user enters too many characters (2.35 KB, patch)
2017-10-27 14:24 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 Katrin Fischer 2013-05-17 08:04:49 UTC
The cardnumber field is limited to 16 in Koha. When you enter a longer cardnumber the number is cut off. When you try to change it adding the missing part, there is no error message shown and your change is just not saved.

Koha should give a useful error message.
Comment 1 Magnus Enger 2013-05-19 21:52:40 UTC
16 sounds quite restrictive - could it be made longer too?
Comment 2 Owen Leonard 2013-05-21 13:02:07 UTC
Here are some steps we could take, individually or in combination:

1. Add a "maxlenth" attribute to the input field.
2. Add some JavaScript validation to warn the user when the limit is reached.
3. Increase the length which can be stored.

If we increase the possible length of a card number what should it be? 50? 100?
Comment 3 Katrin Fischer 2013-05-22 07:56:10 UTC
I think I'd like to see a combination of JavaScript warning and maxlength.

16 seems like a reasonable size for a cardnumber, but not sure if there are use cases where a longer number would be needed.
Comment 4 Aleisha Amohia 2016-01-25 22:58:17 UTC
I don't think this is valid anymore - I am unable to type anything once the card number has reached 16 characters in the input box. Please confirm if this is still valid?
Comment 5 Owen Leonard 2016-01-27 14:37:50 UTC
I can confirm this bug by doing this:

1. Bring up the new patron form.
2. Along with any other required fields, enter '23467890758428435' as the barcode (17 digits).
3. Save the patron record. The barcode will be saved as '2346789075842843' (16 digits, the last digit missing).
Comment 6 Aleisha Amohia 2016-01-27 22:23:26 UTC
Oh okay I see! So this only (In reply to Owen Leonard from comment #5)
> I can confirm this bug by doing this:
> 
> 1. Bring up the new patron form.
> 2. Along with any other required fields, enter '23467890758428435' as the
> barcode (17 digits).
> 3. Save the patron record. The barcode will be saved as '2346789075842843'
> (16 digits, the last digit missing).

I don't get your problem, when I paste that number it removes the 5 straight away from the text box.
Comment 7 Owen Leonard 2016-01-28 13:13:10 UTC
(In reply to Aleisha Amohia from comment #6)

> when I paste that number it removes the 5 straight
> away from the text box.

Okay, my mistake. When I tested it I could have sworn the field let me put in a longer number. 

Either way, if the user isn't carefully watching the number go in they will not be aware that something is wrong. Imagine scanning a barcode -- will the user visually confirm that the number matches what they scanned?

This isn't something which is necessarily badly broken, it's just something we can do better with.
Comment 8 Aleisha Amohia 2017-04-20 03:40:59 UTC
Created attachment 62440 [details] [review]
Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) If you try to type again, the error message will show up.
5) if you backspace some characters, the message should disappear

Sponsored-by: Catalyst IT
Comment 9 Marc Véron 2017-04-20 06:16:04 UTC
Created attachment 62442 [details] [review]
Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) If you try to type again, the error message will show up.
5) if you backspace some characters, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Marc Véron 2017-04-20 20:51:10 UTC
Oh, I forgot to sitsch status...
Comment 11 Jonathan Druart 2017-04-24 15:33:53 UTC
Aleisha, the last case is wrong, maxlength_cardnumber is evaluated false and the message will be "Card number must not be more than  characters". That should not be possible (because C4::Members::get_cardnumber_length returns max=16), but the code does not make sense as it is.

Moreover you should not use the keypress event but change, that way a paste or delete in the input will update the message as well.
Comment 12 Aleisha Amohia 2017-04-28 01:21:16 UTC
Created attachment 62806 [details] [review]
Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) Click out of the text field (so it loses focus), the error message will show up.
5) if you backspace some characters and click out of the text field
again, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Katrin Fischer 2017-05-01 22:20:58 UTC
Created attachment 62934 [details] [review]
[SIGNED OFF] Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) Click out of the text field (so it loses focus), the error message will show up.
5) if you backspace some characters and click out of the text field
again, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Marcel de Rooy 2017-05-12 06:35:57 UTC
Hmm I do not really understand why we need this message.

You cannot type more than 16 chars or paste them.
There is a hint that it should be between 1 and 16.
Currently, the message shows up at 16 chars. This makes me think that I entered too much chars, but I did not.

I think that if we want to do something on this matter, we need to search for another angle. But I am not convinced that we should; the hint is clear to me. We could allow more than 16 chars and then mark it red or something ?
Comment 15 Aleisha Amohia 2017-06-27 04:42:48 UTC
(In reply to Marcel de Rooy from comment #14)
> Hmm I do not really understand why we need this message.
> 
> You cannot type more than 16 chars or paste them.
> There is a hint that it should be between 1 and 16.
> Currently, the message shows up at 16 chars. This makes me think that I
> entered too much chars, but I did not.
> 
> I think that if we want to do something on this matter, we need to search
> for another angle. But I am not convinced that we should; the hint is clear
> to me. We could allow more than 16 chars and then mark it red or something ?

Owen explains the benefit of a patch like this in Comment 7. It may not be broken and seems very obvious to us, but the point is to make things as clear as possible for the user.
Comment 16 Marc Véron 2017-06-27 07:54:03 UTC
Related bug, signed off:

Bug 13178 - cardnumber field length is too short
It expands the card number length to 32.
Comment 17 Séverine Queune 2017-06-28 12:35:50 UTC
Tested on BibLibre's sandbox #2

The patch applies as described in Comment #13.
However, now there is a new patch for bug 13178 and it is not taken in count in the current bug, I don't know if I'm supposed to sign it :/
Comment 18 M. Tompsett 2017-07-04 00:21:52 UTC
Comment on attachment 62934 [details] [review]
[SIGNED OFF] Bug 10267: Show error message if user tries to enter too many characters for cardnumber

Review of attachment 62934 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +621,4 @@
>            [% END %]
>            [% IF minlength_cardnumber == maxlength_cardnumber %]
>                  <input type="text" id="cardnumber" name="cardnumber" size="20" value="[% cardnumber | html %]" minlength="[% minlength_cardnumber %]" maxlength="[% maxlength_cardnumber %]" />
> +                [% IF mandatorycardnumber and not autoMemberNum %]<span class="required">Required</span>[% END %]<span id="cn_max" class="required">Card number must not be more than [% maxlength_cardnumber %] characters.</span>

It is impossible to enter more than maxlength_cardnumber characters because the maxlength attribute of the input line makes it impossible. I believe bug 13178, providing flexibility of the max size based on the schema which should be regenerated as needed to match the table's field size, makes this bug obsolete.
Comment 19 Katrin Fischer 2017-08-08 16:04:37 UTC
It's impossible to enter them, but scanning from an ID or c&p from another system you might not know that it has been automatically cut off. This was just thought to provide some help for this case. 

I am signing off.
Comment 20 Katrin Fischer 2017-08-08 16:05:19 UTC
Created attachment 65657 [details] [review]
[SIGNED OFF] Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) Click out of the text field (so it loses focus), the error message will show up.
5) if you backspace some characters and click out of the text field
again, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Jonathan Druart 2017-08-09 17:49:25 UTC
If we increase the size of borrowers.cardnumber to 32 (bug 13178), do we still need this?
Comment 22 Katrin Fischer 2017-08-09 18:44:07 UTC
I haven't tested the other bug, but this is not dependent on the size I think. They still might conflict.
Comment 23 Kyle M Hall 2017-09-01 13:05:56 UTC
On master I cannot input more than the max number of characters for cardnumber. Is this patch still necessary?
Comment 24 Kyle M Hall 2017-09-01 13:06:46 UTC
Created attachment 66732 [details] [review]
Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) Click out of the text field (so it loses focus), the error message will show up.
5) if you backspace some characters and click out of the text field
again, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Katrin Fischer 2017-09-01 18:10:12 UTC
The problem is not input - try and copy a longer string in. You won't notice that it has been cut off - with the patch you get a note.
Comment 26 Jonathan Druart 2017-10-24 17:32:38 UTC
With CardnumberLength=16, if I enter a 16 chars length string, I get the warning.
It should display only if > 16, right?
Comment 27 Aleisha Amohia 2017-10-24 20:01:20 UTC
(In reply to Jonathan Druart from comment #26)
> With CardnumberLength=16, if I enter a 16 chars length string, I get the
> warning.
> It should display only if > 16, right?

Not quite - you can't actually input more than 16 characters so no warning would show if we made that change, which is the problem already. Having a warning at 16 characters (rather than > 16 characters) just lets the user know that there is a limit in place and that they've reached the max. Does that make sense?
Comment 28 Jonathan Druart 2017-10-24 21:13:06 UTC
Created attachment 68503 [details] [review]
Bug 10267: Display the message only if the user enters too many characters

It seems better to display the warning if the user tries to enter too
many characters in the input.

Test plan:
With max=16
1. Copy/paste a string with 15, 16 and 17 characters
2. Enter a cardnumber of 15, 16, 17 characters
The warning should be displayed only the input overflows
Comment 29 Jonathan Druart 2017-10-24 21:13:29 UTC
Aleisha, can you take a look at this try please?
Comment 30 Aleisha Amohia 2017-10-24 21:21:52 UTC
(In reply to Jonathan Druart from comment #29)
> Aleisha, can you take a look at this try please?

Ah yes I see how your change works, I like it and it makes sense. Will sign off on it.
Comment 31 Aleisha Amohia 2017-10-24 21:22:28 UTC
Created attachment 68504 [details] [review]
Bug 10267: Display the message only if the user enters too many characters

It seems better to display the warning if the user tries to enter too
many characters in the input.

Test plan:
With max=16
1. Copy/paste a string with 15, 16 and 17 characters
2. Enter a cardnumber of 15, 16, 17 characters
The warning should be displayed only the input overflows

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>
Comment 32 Kyle M Hall 2017-10-27 14:24:12 UTC
Created attachment 68724 [details] [review]
Bug 10267: Show error message if user tries to enter too many characters for cardnumber

This patch displays a message if the user tries to enter more than the
max number of characters for a cardnumber.

To test:
1) Apply patch and go to Patrons -> New patron
2) Scroll down to Card number
3) Put in any characters. Notice that when you have entered the max
number of characters, you are unable to type any more.
4) Click out of the text field (so it loses focus), the error message will show up.
5) if you backspace some characters and click out of the text field
again, the message should disappear

Sponsored-by: Catalyst IT

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2017-10-27 14:24:19 UTC
Created attachment 68725 [details] [review]
Bug 10267: Display the message only if the user enters too many characters

It seems better to display the warning if the user tries to enter too
many characters in the input.

Test plan:
With max=16
1. Copy/paste a string with 15, 16 and 17 characters
2. Enter a cardnumber of 15, 16, 17 characters
The warning should be displayed only the input overflows

Signed-off-by: Aleisha Amohia <aleishaamohia@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Jonathan Druart 2017-10-27 17:12:09 UTC
Pushed to master for 17.11, thanks to everybody involved!