Bug 19466 - Cardnumber auto calc is broken because field is required
Summary: Cardnumber auto calc is broken because field is required
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: David Bourgault
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 6782
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-13 19:26 UTC by David Bourgault
Modified: 2018-05-03 14:04 UTC (History)
6 users (show)

See Also:
Change 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
Cardnumber cannot be blank (16.88 KB, image/png)
2017-10-13 19:26 UTC, David Bourgault
Details
Bug 19466 - Check if cardnumber mandatory (1.79 KB, patch)
2017-11-22 22:00 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19466 - Check if cardnumber mandatory (1.28 KB, patch)
2017-11-22 23:19 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 19466: Check if cardnumber is mandatory (6.13 KB, patch)
2017-12-21 15:37 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19466: Check if cardnumber is mandatory (6.19 KB, patch)
2018-02-12 14:04 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 19466: Warn about conflicting settings of autoMemberNum and BorrowerMandatoryField for cardnumber (6.30 KB, patch)
2018-02-13 06:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19466: Restore original sentence to avoid additional translation (1.23 KB, patch)
2018-02-13 16:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19466: AutoCalc is off for new installs (2.06 KB, patch)
2018-05-01 19:12 UTC, David Bourgault
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Bourgault 2017-10-13 19:26:21 UTC
Created attachment 68099 [details]
Cardnumber cannot be blank

When trying to register a new user, it says Cardnumber is a required field, but it also says to "leave blank for auto calc during registration".
Comment 1 David Bourgault 2017-11-22 22:00:04 UTC
Created attachment 69298 [details] [review]
Bug 19466 - Check if cardnumber mandatory

Turns out this isn't a bug, since it is caused by 'cardnumber' being in the 'BorrowerMandatoryField' syspref.

It is however confusing for the user that it suggested to leave the field blank when the field is required. To alleviate this I added a condition to the code; it will not suggest leaving the field blank if 'BorrowerMandatoryField' contains 'cardnumber'.

To test:

Before:
0) Enable autoMemberNum
1) Add 'cardnumber' to BorrowerMandatoryField
2) Go to create a user: a message will appear suggesting you leave the cardnumber field blank for autocalc, but when submitting the form it will show as required.
Apply patch
2) Go to create a user: if 'cardnumber' is in BorrowerMandatoryField, the suggestion will not be shown.
Comment 2 Dilan Johnpullé 2017-11-22 23:19:00 UTC
Created attachment 69299 [details] [review]
Bug 19466 - Check if cardnumber mandatory

Followed test plan and patch applies and functions as expected.
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 3 Jonathan Druart 2017-12-19 15:49:13 UTC
David, do not forget to set the different fields of the bug report (at least severity and assignee).
You should also start your commit messages with 'Bug XXXXX: ' instead of 'Bug XXXXX - '
Comment 4 Jonathan Druart 2017-12-19 15:51:19 UTC
From bug 6782, Marc Véron raised this problem:
"""
    Note: Syspref PorrowerMandatoryField must not include cardnumber, otherwise
          you can not save. Maybe that should be mentioned in the comment for
          syspref autoMemberNum.
"""
Comment 5 Kyle M Hall 2017-12-21 11:29:22 UTC
I don't think this is a complete enough solution. We have two choices:
A) Hide auto-calc if cardnumber is mandatory
B) Strip cardnumber from mandatory fields if auto calc is enabled

This patch does A, which is fine. But what I think either solution needs is
to display a warning to the library that autocalc and BorrowerMandatoryField are in conflict with each other. Otherwise libraries won't understand why autocalc isn't working even though it's enabled.
Comment 6 David Bourgault 2017-12-21 15:37:10 UTC
Created attachment 70056 [details] [review]
Bug 19466: Check if cardnumber is mandatory

Changes the appearance of the cardnumber entry field in memberentrygen.tt
The "Leave empty for autocalc" message has been moved to the hint div
under the input field. If AutoMemberNum and BorrowerMandatoryField
interfere with each other, the auto calc hint is replace by a warning
telling the user that auto calc has been disabled.

Cardnumber should now correctly appear as mandatory if marked as such
in BorrowerMandatoryField.

Test plan:
0] Apply patch
1} Disable AutoMemberNum, remove cardnumber from BorrowerMandatoryField
2) Edit or create a patron, scroll down to cardnumber input field
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is not marked as required
    There is no mention of auto calc

3> Enable AutoMemberNum
    Hint includes "Leave empty for auto calc" message

4~ Add cardnumber to BorrowerMandatoryField
    Hint warns you that your sysprefs are conflicting.
    Cardnumber input is marked as required

5: Disable AutoMemberNum
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is marked as required
Comment 7 David Bourgault 2017-12-21 15:40:12 UTC
(In reply to Kyle M Hall from comment #5)
> I don't think this is a complete enough solution. We have two choices:
> A) Hide auto-calc if cardnumber is mandatory
> B) Strip cardnumber from mandatory fields if auto calc is enabled

I don't think we should be changing SysPrefs without explicitly telling the user, that would simply lead to more confusion.

I changed my patch to make the behavior more explicit to the user by warning him if syspref are interfering with each other, and correctly marking the input field as required. I think this is the most simple option for this problem.
Comment 8 Claire Gravely 2018-02-12 14:04:19 UTC
Created attachment 71473 [details] [review]
Bug 19466: Check if cardnumber is mandatory

Changes the appearance of the cardnumber entry field in memberentrygen.tt
The "Leave empty for autocalc" message has been moved to the hint div
under the input field. If AutoMemberNum and BorrowerMandatoryField
interfere with each other, the auto calc hint is replace by a warning
telling the user that auto calc has been disabled.

Cardnumber should now correctly appear as mandatory if marked as such
in BorrowerMandatoryField.

Test plan:
0] Apply patch
1} Disable AutoMemberNum, remove cardnumber from BorrowerMandatoryField
2) Edit or create a patron, scroll down to cardnumber input field
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is not marked as required
    There is no mention of auto calc

3> Enable AutoMemberNum
    Hint includes "Leave empty for auto calc" message

4~ Add cardnumber to BorrowerMandatoryField
    Hint warns you that your sysprefs are conflicting.
    Cardnumber input is marked as required

5: Disable AutoMemberNum
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is marked as required

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 9 Katrin Fischer 2018-02-13 06:56:52 UTC
Created attachment 71534 [details] [review]
Bug 19466: Warn about conflicting settings of autoMemberNum and BorrowerMandatoryField for cardnumber

Changes the appearance of the cardnumber entry field in memberentrygen.tt
The "Leave empty for autocalc" message has been moved to the hint div
under the input field. If AutoMemberNum and BorrowerMandatoryField
interfere with each other, the auto calc hint is replace by a warning
telling the user that auto calc has been disabled.

Cardnumber should now correctly appear as mandatory if marked as such
in BorrowerMandatoryField.

Test plan:
0] Apply patch
1} Disable AutoMemberNum, remove cardnumber from BorrowerMandatoryField
2) Edit or create a patron, scroll down to cardnumber input field
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is not marked as required
    There is no mention of auto calc

3> Enable AutoMemberNum
    Hint includes "Leave empty for auto calc" message

4~ Add cardnumber to BorrowerMandatoryField
    Hint warns you that your sysprefs are conflicting.
    Cardnumber input is marked as required

5: Disable AutoMemberNum
    Hint is some form of "Cardnumber must be this long"
    Cardnumber input is marked as required

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Katrin Fischer 2018-02-13 06:57:50 UTC
Please remember that the commit message subject should describe what has been fixed.
Comment 11 Jonathan Druart 2018-02-13 16:24:35 UTC
"Leave blank for auto calc during registration"
vs
"Leave empty for auto calc during registration"

Is it really needed?
Comment 12 Jonathan Druart 2018-02-13 16:33:16 UTC
13:31 < kidclamp_dnd> I would lean towards 'blank' over 'empty' - makes more sense for a form, but either works
Comment 13 Jonathan Druart 2018-02-13 16:34:44 UTC
Created attachment 71558 [details] [review]
Bug 19466: Restore original sentence to avoid additional translation
Comment 14 Jonathan Druart 2018-02-13 17:17:49 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 15 Jonathan Druart 2018-02-13 19:01:10 UTC
On a fresh install, I edit a patron and see: "AutoMemberNum is set to enabled, but cardnumber is marked as mandatory in BorrowerMandatoryField: auto calc has been disabled."
That sounds wrong, what can we do?
Comment 16 Katrin Fischer 2018-02-13 19:52:23 UTC
Because it's on a fresh installation? I think either remove autocalc... or mandatory. I'd lean towards removing the autocalc.
Comment 17 Jonathan Druart 2018-02-19 18:53:15 UTC
David, can you have a look?
Comment 18 Jonathan Druart 2018-02-21 20:02:12 UTC
Can someone explain me the point of the pref autoMemberNum?
With BorrowerMandatoryField, it's like we could remove the autoMemberNum: If the cardnumber should not be auto calculated you mark the field as mandatory
If it is not mandatory, then we autocalculate it

Does it make sense?
Comment 19 David Bourgault 2018-02-21 20:04:00 UTC
That makes sense to me. Would that be a new bug or keep it here?
Comment 20 Jonathan Druart 2018-02-21 20:53:56 UTC
(In reply to David Bourgault from comment #19)
> That makes sense to me. Would that be a new bug or keep it here?

Actually I got the answer on IRC: some libraries do not use it and they are not defined in DB.
Comment 21 Barton Chittenden 2018-03-06 19:10:11 UTC
I would like to see this back-ported, if possible.
Comment 22 Jonathan Druart 2018-04-11 17:49:17 UTC
(In reply to Jonathan Druart from comment #15)
> On a fresh install, I edit a patron and see: "AutoMemberNum is set to
> enabled, but cardnumber is marked as mandatory in BorrowerMandatoryField:
> auto calc has been disabled."
> That sounds wrong, what can we do?

Still valid, please fix for 18.05.
Comment 23 David Bourgault 2018-04-11 18:30:49 UTC
The default value (in syspref.sql) is to set AutoCalc on and have cardnumber be mandatory. Which one should be turned off by default?
Comment 24 Katrin Fischer 2018-04-13 16:18:00 UTC
I think turn off autocalc, it seems the less likely use case and it doesn't work well when you are using your own numbers.
Comment 25 David Bourgault 2018-05-01 19:12:27 UTC
Created attachment 74957 [details] [review]
Bug 19466: AutoCalc is off for new installs

Default value of AutoMemberNum is now 0.

TEST PLAN:
0. Do a fresh install of Koha
1. In system preferences AutoMemberNum should be off
2. No warning should be shown next to the cardnumber field in the patron creation form.

Sorry for the late patch. Also I'm not sure which status to use here.
Comment 26 Jonathan Druart 2018-05-03 14:04:40 UTC
Last patch pushed to master, thanks David