Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration
Summary: Mandatory fields should not be able to be hidden in borrower self-registration
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-07 05:46 UTC by David Cook
Modified: 2016-12-05 21:22 UTC (History)
8 users (show)

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


Attachments
Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration (2.99 KB, patch)
2015-01-07 06:33 UTC, David Cook
Details | Diff | Splinter Review
Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration (2.85 KB, patch)
2015-01-08 06:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration (2.89 KB, patch)
2015-01-12 23:17 UTC, Abby
Details | Diff | Splinter Review
[PASSED QA] Bug 13526 - Mandatory fields should not be able to be hidden in borrower self-registration (2.96 KB, patch)
2015-01-16 14:59 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 David Cook 2015-01-07 05:46:20 UTC
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.
Comment 1 David Cook 2015-01-07 06:04:13 UTC
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...
Comment 2 David Cook 2015-01-07 06:33:44 UTC Comment hidden (obsolete)
Comment 3 David Cook 2015-01-07 06:34:47 UTC
This might be a useful one for Academy members to take a look at :)
Comment 4 Mark Tompsett 2015-01-07 06:41:53 UTC
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?
Comment 5 David Cook 2015-01-08 00:21:39 UTC
(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.
Comment 6 David Cook 2015-01-08 06:04:56 UTC Comment hidden (obsolete)
Comment 7 David Cook 2015-01-08 06:06:43 UTC
(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!
Comment 8 Abby 2015-01-12 23:17:01 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-01-16 10:49:10 UTC
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?
Comment 10 Kyle M Hall 2015-01-16 14:59:44 UTC
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>
Comment 11 Kyle M Hall 2015-01-16 15:00:45 UTC
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?
Comment 12 Tomás Cohen Arazi 2015-01-18 13:56:44 UTC
Patch pushed to master.

Thanks David!
Comment 13 Chris Cormack 2015-01-19 19:08:18 UTC
Pushed to 3.18.x will be in 3.18.3