When using the self-registration, "email" is a mandatory field no matter what, if the "PatronSelfRegistrationVerifyByEmail" system preference is true. However, you can still hide the "email" field, which means the form will refuse to submit (saying that you haven't filled in all mandatory fields) even though you've filled out every mandatory field on the screen.
Actually, now that I think about it, any mandatory field can be hidden... and that would cause trouble. So really no mandatory field should be able to be hidden. That's easy enough to do...
Created attachment 34991 [details] [review] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration Currently, mandatory fields in the borrower self-registration can be hidden. This causes problems since the validator rejects the registration, even though all required fields on the screen have been filled out. This is especially a problem when using the system preference "PatronSelfRegistrationVerifyByEmail", since it automatically makes "email" a mandatory field. This patch makes it so that a mandatory field cannot be hidden on the self-registration page. _TEST PLAN_ Before applying 1) Hide the "email" and the "emailpro" fields using the "PatronSelfRegistrationBorrowerUnwantedField" system preference. 2) Make the "email" and the "emailpro" fields mandatory using the "PatronSelfRegistrationBorrowerMandatoryField" system preference. 3) Note that you cannot see "Primary email" or "Secondary email" on the self registration page. (The registration page which can be found by turning on "PatronSelfRegistration" and filling in "PatronSelfRegistrationDefaultCategory". The link will be on opac-main.pl on the right side of the screen under the login box.) 4) Note that you cannot submit a self-registration request as the system tells you that you have not filled in all the mandatory fields. 5) Apply the patch && refresh the self registration page 6) Note that you can now see "Primary email" and "Secondary email" on the self-registration screen".
This might be a useful one for Academy members to take a look at :)
Comment on attachment 34991 [details] [review] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration Review of attachment 34991 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-memberentry.pl @@ +235,5 @@ > my $BorrowerUnwantedField = > C4::Context->preference("PatronSelfRegistrationBorrowerUnwantedField"); > > + #Fetch the mandatory fields to compare against hidden fields > + my @mandatory_fields = keys %{GetMandatoryFields($action)}; The GetMandatoryFields function is already called on line 62. This calls it again. Could this be optimized?
(In reply to M. Tompsett from comment #4) > Comment on attachment 34991 [details] [review] [review] > Bug 13526 - Mandatory fields should not be able to be hidden in borrower > self-registration > > Review of attachment 34991 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: opac/opac-memberentry.pl > @@ +235,5 @@ > > my $BorrowerUnwantedField = > > C4::Context->preference("PatronSelfRegistrationBorrowerUnwantedField"); > > > > + #Fetch the mandatory fields to compare against hidden fields > > + my @mandatory_fields = keys %{GetMandatoryFields($action)}; > > The GetMandatoryFields function is already called on line 62. This calls it > again. Could this be optimized? Probably. I was thinking that it's pointless doing the grep as well as a person could just use the hash... I'll take another look at this pronto.
Created attachment 35029 [details] [review] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration Currently, mandatory fields in the borrower self-registration can be hidden. This causes problems since the validator rejects the registration, even though all required fields on the screen have been filled out. This is especially a problem when using the system preference "PatronSelfRegistrationVerifyByEmail", since it automatically makes "email" a mandatory field. This patch makes it so that a mandatory field cannot be hidden on the self-registration page. _TEST PLAN_ Before applying 1) Hide the "email" and the "emailpro" fields using the "PatronSelfRegistrationBorrowerUnwantedField" system preference. 2) Make the "email" and the "emailpro" fields mandatory using the "PatronSelfRegistrationBorrowerMandatoryField" system preference. 3) Note that you cannot see "Primary email" or "Secondary email" on the self registration page. (The registration page which can be found by turning on "PatronSelfRegistration" and filling in "PatronSelfRegistrationDefaultCategory". The link will be on opac-main.pl on the right side of the screen under the login box.) 4) Note that you cannot submit a self-registration request as the system tells you that you have not filled in all the mandatory fields. 5) Apply the patch && refresh the self registration page 6) Note that you can now see "Primary email" and "Secondary email" on the self-registration screen".
(In reply to M. Tompsett from comment #4) > The GetMandatoryFields function is already called on line 62. This calls it > again. Could this be optimized? There we go. That should be much better! A fair number of my changes turned out to be needless, so I simplified it and it works a treat. Cheers!
Created attachment 35126 [details] [review] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration Currently, mandatory fields in the borrower self-registration can be hidden. This causes problems since the validator rejects the registration, even though all required fields on the screen have been filled out. This is especially a problem when using the system preference "PatronSelfRegistrationVerifyByEmail", since it automatically makes "email" a mandatory field. This patch makes it so that a mandatory field cannot be hidden on the self-registration page. _TEST PLAN_ Before applying 1) Hide the "email" and the "emailpro" fields using the "PatronSelfRegistrationBorrowerUnwantedField" system preference. 2) Make the "email" and the "emailpro" fields mandatory using the "PatronSelfRegistrationBorrowerMandatoryField" system preference. 3) Note that you cannot see "Primary email" or "Secondary email" on the self registration page. (The registration page which can be found by turning on "PatronSelfRegistration" and filling in "PatronSelfRegistrationDefaultCategory". The link will be on opac-main.pl on the right side of the screen under the login box.) 4) Note that you cannot submit a self-registration request as the system tells you that you have not filled in all the mandatory fields. 5) Apply the patch && refresh the self registration page 6) Note that you can now see "Primary email" and "Secondary email" on the self-registration screen". Signed-off-by: Abby <abbyrobnz@gmail.com>
I am wondering if we need to take into account this kind of configuration problems. Maybe a check in the about page is more appropriate?
Created attachment 35331 [details] [review] [PASSED QA] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration Currently, mandatory fields in the borrower self-registration can be hidden. This causes problems since the validator rejects the registration, even though all required fields on the screen have been filled out. This is especially a problem when using the system preference "PatronSelfRegistrationVerifyByEmail", since it automatically makes "email" a mandatory field. This patch makes it so that a mandatory field cannot be hidden on the self-registration page. _TEST PLAN_ Before applying 1) Hide the "email" and the "emailpro" fields using the "PatronSelfRegistrationBorrowerUnwantedField" system preference. 2) Make the "email" and the "emailpro" fields mandatory using the "PatronSelfRegistrationBorrowerMandatoryField" system preference. 3) Note that you cannot see "Primary email" or "Secondary email" on the self registration page. (The registration page which can be found by turning on "PatronSelfRegistration" and filling in "PatronSelfRegistrationDefaultCategory". The link will be on opac-main.pl on the right side of the screen under the login box.) 4) Note that you cannot submit a self-registration request as the system tells you that you have not filled in all the mandatory fields. 5) Apply the patch && refresh the self registration page 6) Note that you can now see "Primary email" and "Secondary email" on the self-registration screen". Signed-off-by: Abby <abbyrobnz@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I think both would be good! I know I'd welcome and additional report and patch to add a warning for these conflicts to the about page. (In reply to Jonathan Druart from comment #9) > I am wondering if we need to take into account this kind of configuration > problems. > Maybe a check in the about page is more appropriate?
Patch pushed to master. Thanks David!
Pushed to 3.18.x will be in 3.18.3