Bug 34671 - Collapsing fields in patron form allows saving without mandatory fields
Summary: Collapsing fields in patron form allows saving without mandatory fields
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-08-31 06:17 UTC by Emmi Takkinen
Modified: 2023-11-17 10:08 UTC (History)
5 users (show)

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


Attachments
Bug 34671: Show error message if fieldset has invalid input (3.80 KB, patch)
2023-09-15 08:21 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Ignore new_guarantor_relationship input if it's hidden (3.43 KB, patch)
2023-09-25 08:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Show error message if fieldset has invalid input (3.81 KB, patch)
2023-09-27 09:29 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Ignore new_guarantor_relationship input if it's hidden (3.69 KB, patch)
2023-09-27 09:30 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Show error message if fieldset has invalid input (3.82 KB, patch)
2023-10-02 10:23 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Ignore new_guarantor_relationship input if it's hidden (3.69 KB, patch)
2023-10-02 10:24 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Ignore validation if patron attribute field is hidden (2.17 KB, patch)
2023-11-01 11:39 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Ignore validation if patron attribute field is hidden (5.35 KB, patch)
2023-11-03 07:28 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34671: Show error message if fieldset has invalid input (3.86 KB, patch)
2023-11-06 14:15 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34671: Ignore new_guarantor_relationship input if it's hidden (3.74 KB, patch)
2023-11-06 14:15 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 34671: Ignore validation if patron attribute field is hidden (5.40 KB, patch)
2023-11-06 14:15 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emmi Takkinen 2023-08-31 06:17:38 UTC
We've had cases where user has been able to save patron without address. It turned out that if one collapses patron entry fields validating those fields no longer works even if they are marked as mandatory.

To reproduce:
1. Make sure you have "Main address" fields set as mandatory with BorrowerMandatoryField syspref (address, city, zipcode)
2. Add new patron, fill in some info, but leave "Main address" fields empty.
3. Attempt to save.
=> "This field is mandatory" should be displayed next to the address fields.
4. Collapse "Main address" section.
5. Attempt to save again.
=> Patron is saved successfully without address information.  

I'm using address section as an example here, but this happens with every other fields as well. I was able to save patron without surname and date of birth this way.
Comment 1 Emmi Takkinen 2023-09-01 06:27:52 UTC
Adding "ignore" rule as "" for entry forms validation (members.js line 242) does the trick. However "This field is required." errors aren't visible when fields are collapsed. Either there should be some error displayed next to field sets name or some sort of "There are some required fields missing. Please check form." text displayed. Or maybe collapsed fields should be opened if they contain required fields.
Comment 2 Emmi Takkinen 2023-09-15 08:21:32 UTC
Created attachment 155643 [details] [review]
Bug 34671: Show error message if fieldset has invalid input

If fieldset is collapsed and it contains required fields
patron can be saved without filling these fields. This patch adds
rule ignore as "" in form validation and displayes error message
in fieldsets containing missing required inputs.

To test:
1. Make sure you have "Main address" fields set as mandatory
with BorrowerMandatoryField syspref (address, city, zipcode)
2. Add new patron, fill in some info, but leave "Main address"
fields empty.
3. Attempt to save.
=> "This field is mandatory" should be displayed next to the
address fields.
4. Collapse "Main address" fieldset.
5. Attempt to save again.
=> Patron is saved successfully without address information.
6. Apply this patch.
7. Edit patron and attempt to save again with "Main address"
fieldset collapsed.
=> Error message "There are some required fields missing from this field set. Please fill them." should be displayed
and patron isn't saved.
8. Fill missing fields, attempt to save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Comment 3 Emmi Takkinen 2023-09-22 05:23:51 UTC
Currently this breaks child patron modification. Relationship input is always mandatory if borrowerRelationship syspref is set and it's also always hidden.
Comment 4 Emmi Takkinen 2023-09-25 08:56:07 UTC Comment hidden (obsolete)
Comment 5 Kelly McElligott 2023-09-25 16:16:32 UTC
Upon testing this, I did see the error message under the collapsed address field, so that works successfully. However, I did not see the same error on the Guarantor Collapsed field.  I can not save the form without the relationship of the guarantor, but that does not show if the field is collapsed.
Comment 6 Emmi Takkinen 2023-09-27 09:29:46 UTC
Created attachment 156263 [details] [review]
Bug 34671: Show error message if fieldset has invalid input

If fieldset is collapsed and it contains required fields
patron can be saved without filling these fields. This patch adds
rule ignore as "" in form validation and displayes error message
in fieldsets containing missing required inputs.

To test:
1. Make sure you have "Main address" fields set as mandatory
with BorrowerMandatoryField syspref (address, city, zipcode)
2. Add new patron, fill in some info, but leave "Main address"
fields empty.
3. Attempt to save.
=> "This field is mandatory" should be displayed next to the
address fields.
4. Collapse "Main address" fieldset.
5. Attempt to save again.
=> Patron is saved successfully without address information.
6. Apply this patch.
7. Edit patron and attempt to save again with "Main address"
fieldset collapsed.
=> Error message "There are some required fields missing from this field set. Please fill them." should be displayed
and patron isn't saved.
8. Fill missing fields, attempt to save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Comment 7 Emmi Takkinen 2023-09-27 09:30:19 UTC
Created attachment 156264 [details] [review]
Bug 34671: Ignore new_guarantor_relationship input if it's hidden

Validating patron forms hidden fields causes problems when
adding/modifying guarantee patrons since class
new_guarantor_relationship is always hidden and mandatory
if syspref "borrowerRelationship" is set. This patch adds class
ignore_validation to relationship input if it's hidden to ignore
validatation and removes it when input is displayed, so it is
validated.

To test:
1. Apply previous patch.
2. Add or modify guarantee patron.
3. Do not add or modify guarantor.
4. Attempt to save patron.
=> Pressing save button does nothing and no error is
displayed.
5. Apply this patch.
6. Again add or modify guarantee patron and don't add or
modify guarantor.
7. Attempt to save.
=> Saving should now be successfull.
8. Again, add or modify guarantee patron, but this time also
add or modify guarantor.
9. Leave relationship input empty.
10. Attempt to save.
=> Saving should fail and error "There are some required
fields missing..." should be displayed on top of "Patron
guarantor" field set.
11. Add relationship for guarantor and save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Comment 8 Emmi Takkinen 2023-09-27 09:32:53 UTC
Fixed issue Kelly pointed out. There was also problem when adding guarantee via "Add guarantee" option. 

Just a node, I made these two patches for mainly testing purposes. They can be squashed together if needed.
Comment 9 Owen Leonard 2023-09-27 12:21:08 UTC
I would suggest this change to the text:

"There are some required fields missing from this section. Please complete them."

And I think it looks better when the new message appears after the <legend> not before it.
Comment 10 Emmi Takkinen 2023-10-02 10:23:42 UTC
Created attachment 156453 [details] [review]
Bug 34671: Show error message if fieldset has invalid input

If fieldset is collapsed and it contains required fields
patron can be saved without filling these fields. This patch adds
rule ignore as "" in form validation and displayes error message
in fieldsets containing missing required inputs.

To test:
1. Make sure you have "Main address" fields set as mandatory
with BorrowerMandatoryField syspref (address, city, zipcode)
2. Add new patron, fill in some info, but leave "Main address"
fields empty.
3. Attempt to save.
=> "This field is mandatory" should be displayed next to the
address fields.
4. Collapse "Main address" fieldset.
5. Attempt to save again.
=> Patron is saved successfully without address information.
6. Apply this patch.
7. Edit patron and attempt to save again with "Main address"
fieldset collapsed.
=> Error message "There are some required fields missing from this field set. Please fill them." should be displayed
and patron isn't saved.
8. Fill missing fields, attempt to save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Comment 11 Emmi Takkinen 2023-10-02 10:24:15 UTC
Created attachment 156454 [details] [review]
Bug 34671: Ignore new_guarantor_relationship input if it's hidden

adding/modifying guarantee patrons since class
new_guarantor_relationship is always hidden and mandatory
if syspref "borrowerRelationship" is set. This patch adds class
ignore_validation to relationship input if it's hidden to ignore
validatation and removes it when input is displayed, so it is
validated.

To test:
1. Apply previous patch.
2. Add or modify guarantee patron.
3. Do not add or modify guarantor.
4. Attempt to save patron.
=> Pressing save button does nothing and no error is
displayed.
5. Apply this patch.
6. Again add or modify guarantee patron and don't add or
modify guarantor.
7. Attempt to save.
=> Saving should now be successfull.
8. Again, add or modify guarantee patron, but this time also
add or modify guarantor.
9. Leave relationship input empty.
10. Attempt to save.
=> Saving should fail and error "There are some required
fields missing..." should be displayed on top of "Patron
guarantor" field set.
11. Add relationship for guarantor and save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Comment 12 Emmi Takkinen 2023-10-02 10:31:27 UTC
(In reply to Owen Leonard from comment #9)
> I would suggest this change to the text:
> 
> "There are some required fields missing from this section. Please complete
> them."
> 
> And I think it looks better when the new message appears after the <legend>
> not before it.

Adjusted as suggested. And it does look better now :D
Comment 13 Emmi Takkinen 2023-10-31 11:04:07 UTC
There's still a problem with how this behaves with patron attribute types set as required. Even if they aren't needed for certain type of patron and are hidden from form, they're still required and prevent save.
Comment 14 Emmi Takkinen 2023-11-01 11:39:11 UTC
Created attachment 158168 [details] [review]
Bug 34671: Ignore validation if patron attribute field is hidden

Patron attributes not used by patron category are still present in
form as hidden and marked as required if they are set as
mandatory. This prevents saving patron when validation is
required also for hidden fields. This patch adds and removes class "ignore_validation" for these fields based on their visibility.

To test:
1. Add patron attribute for patron category A and set it as mandatory.
2. Add patron with category B and fill in required fields.
3. Attempt to save.
=> Error is displayed above "Additional attributes and identifiers"
field set.
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Save successfully.
6. Add new patron with category B, but this time change patrons
category as A from patron form.
=> Confirm that madatory patron attribute is displayed correctly
and without it patron cannot be saved.

Sponsored-by: Koha-Suomi Oy
Comment 15 Emmi Takkinen 2023-11-02 09:36:39 UTC
This still needs some work.
Comment 16 Emmi Takkinen 2023-11-03 07:28:45 UTC
Created attachment 158299 [details] [review]
Bug 34671: Ignore validation if patron attribute field is hidden

Patron attributes not used by patron category are still present in
form as hidden and marked as required if they are set as
mandatory. This prevents saving patron when validation is
required also for hidden fields. This patch adds and removes class "ignore_validation" for these fields based on their visibility.

To test:
1. Add patron attribute for patron category A and set it as mandatory.
2. Add patron with category B and fill in required fields.
3. Attempt to save.
=> Error is displayed above "Additional attributes and identifiers"
field set.
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Save successfully.
6. Add new patron with category B, but this time change patrons
category as A from patron form.
=> Confirm that madatory patron attribute is displayed correctly
and without it patron cannot be saved.

Sponsored-by: Koha-Suomi Oy
Comment 17 ByWater Sandboxes 2023-11-06 14:15:47 UTC
Created attachment 158524 [details] [review]
Bug 34671: Show error message if fieldset has invalid input

If fieldset is collapsed and it contains required fields
patron can be saved without filling these fields. This patch adds
rule ignore as "" in form validation and displayes error message
in fieldsets containing missing required inputs.

To test:
1. Make sure you have "Main address" fields set as mandatory
with BorrowerMandatoryField syspref (address, city, zipcode)
2. Add new patron, fill in some info, but leave "Main address"
fields empty.
3. Attempt to save.
=> "This field is mandatory" should be displayed next to the
address fields.
4. Collapse "Main address" fieldset.
5. Attempt to save again.
=> Patron is saved successfully without address information.
6. Apply this patch.
7. Edit patron and attempt to save again with "Main address"
fieldset collapsed.
=> Error message "There are some required fields missing from this field set. Please fill them." should be displayed
and patron isn't saved.
8. Fill missing fields, attempt to save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 18 ByWater Sandboxes 2023-11-06 14:15:49 UTC
Created attachment 158525 [details] [review]
Bug 34671: Ignore new_guarantor_relationship input if it's hidden

Validating patron forms hidden fields causes problems when
adding/modifying guarantee patrons since class
new_guarantor_relationship is always hidden and mandatory
if syspref "borrowerRelationship" is set. This patch adds class
ignore_validation to relationship input if it's hidden to ignore
validatation and removes it when input is displayed, so it is
validated.

To test:
1. Apply previous patch.
2. Add or modify guarantee patron.
3. Do not add or modify guarantor.
4. Attempt to save patron.
=> Pressing save button does nothing and no error is
displayed.
5. Apply this patch.
6. Again add or modify guarantee patron and don't add or
modify guarantor.
7. Attempt to save.
=> Saving should now be successfull.
8. Again, add or modify guarantee patron, but this time also
add or modify guarantor.
9. Leave relationship input empty.
10. Attempt to save.
=> Saving should fail and error "There are some required
fields missing..." should be displayed on top of "Patron
guarantor" field set.
11. Add relationship for guarantor and save.
=> Saving should now be successfull.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 19 ByWater Sandboxes 2023-11-06 14:15:51 UTC
Created attachment 158526 [details] [review]
Bug 34671: Ignore validation if patron attribute field is hidden

Patron attributes not used by patron category are still present in
form as hidden and marked as required if they are set as
mandatory. This prevents saving patron when validation is
required also for hidden fields. This patch adds and removes class "ignore_validation" for these fields based on their visibility.

To test:
1. Add patron attribute for patron category A and set it as mandatory.
2. Add patron with category B and fill in required fields.
3. Attempt to save.
=> Error is displayed above "Additional attributes and identifiers"
field set.
4. Apply this patch.
5. Repeat steps 2 and 3.
=> Save successfully.
6. Add new patron with category B, but this time change patrons
category as A from patron form.
=> Confirm that madatory patron attribute is displayed correctly
and without it patron cannot be saved.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Kelly <kelly@bywatersolutions.com>
Comment 20 Marcel de Rooy 2023-11-10 10:58:40 UTC
+        ignore: "",
Default should be :hidden. Does this make a difference?

Comparing with
+        if($(".new_guarantor_relationship").is(":hidden")){
+            $(".new_guarantor_relationship").addClass("ignore_validation");
+        }

-        ignore: "",
+        ignore: ".ignore_validation",

Also third patch.
My first thought here is that we are doing things too complicated?
Comment 21 Katrin Fischer 2023-11-10 20:08:15 UTC
Failing QA for feedback on comment#20.
Comment 22 Emmi Takkinen 2023-11-17 10:08:48 UTC
(In reply to Marcel de Rooy from comment #20)
> +        ignore: "",
> Default should be :hidden. Does this make a difference?
Hmm, if ignore setting isn't set then yes, default is ":hidden". That's why it has to be set as ignore: "" so that all fields are validated no matter if they're hidden or not. If we add class to this setting, only those fields are ignored. 

> Comparing with
> +        if($(".new_guarantor_relationship").is(":hidden")){
> +            $(".new_guarantor_relationship").addClass("ignore_validation");
> +        }
> 
> -        ignore: "",
> +        ignore: ".ignore_validation",
> 
> Also third patch.
> My first thought here is that we are doing things too complicated?
How form validation works does make things complicated because the way it works (as stated above). I did at one point test if we could leave class "required" out if field is hidden but I can't remember if it's even possible or less complicated to achieve than new class.