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".
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.
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>
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 - '
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. """
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.
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
(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.
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>
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>
Please remember that the commit message subject should describe what has been fixed.
"Leave blank for auto calc during registration" vs "Leave empty for auto calc during registration" Is it really needed?
13:31 < kidclamp_dnd> I would lean towards 'blank' over 'empty' - makes more sense for a form, but either works
Created attachment 71558 [details] [review] Bug 19466: Restore original sentence to avoid additional translation
Pushed to master for 18.05, thanks to everybody involved!
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?
Because it's on a fresh installation? I think either remove autocalc... or mandatory. I'd lean towards removing the autocalc.
David, can you have a look?
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?
That makes sense to me. Would that be a new bug or keep it here?
(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.
I would like to see this back-ported, if possible.
(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.
The default value (in syspref.sql) is to set AutoCalc on and have cardnumber be mandatory. Which one should be turned off by default?
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.
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.
Last patch pushed to master, thanks David
*** Bug 19225 has been marked as a duplicate of this bug. ***