Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip required validation when field is hidden by 'hide if exist' and has existing value. #2622

Merged
merged 2 commits into from
Oct 11, 2016

Conversation

Yame-
Copy link
Contributor

@Yame- Yame- commented Oct 2, 2016

Q A
Bug fix? x
Issues addressed (#s or URLs) #2612

Description:

Fixes form submit when previously a required field with the behaviour "Hide if value exist" was filled in. Now the form submits without return an error for the required field.

Steps to test this PR:

  1. Create a form
  2. Add a field that is required
  3. Set the option "Hide if value exist"
  4. Fill in the form
  5. Fill in the form again ( the field should be hidden now )
  6. Form should now send without any problems.

Steps to reproduce the bug:

  1. Create a form
  2. Add a field that is required
  3. Set the option "Hide if value exist"
  4. Fill in the form
  5. Fill in the form again ( the field should be hidden now )
  6. When submitting it will not send.

Fixes form submit when required field gets hidden because of the ‘hide if value exist’ behaviour. Else form will halt and returns that x field is required, but this field is not shown on the frontend.
@escopecz escopecz added T1 Low difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Oct 3, 2016
@escopecz escopecz added this to the 2.2.1 milestone Oct 3, 2016
@mqueme
Copy link
Contributor

mqueme commented Oct 11, 2016

??

@mqueme mqueme added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Oct 11, 2016
@@ -214,6 +214,12 @@ public function saveSubmission($post, $server, Form $form, Request $request = nu
}

if ($f->isRequired() && empty($value)) {

//field is required, but hidden from form because of 'ShowWhenValueExists'
if( null !== $f->getShowWhenValueExists() && $f->getShowWhenValueExists() === false ){
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this line be just:

if ($f->getShowWhenValueExists() === false ) {

because if something === false, the !== null part must be true as well, mustn't it?

@alanhartless
Copy link
Contributor

Won't this skip validation for all fields set to show when value exists? Shouldn't we do a check on $post as well to see if the field was included in the request?

@alanhartless alanhartless added the code-review-needed PR's that require a code review before merging label Oct 11, 2016
…ow a field if the lead data is an empty string and not just null
@alanhartless alanhartless removed the code-review-needed PR's that require a code review before merging label Oct 11, 2016
@alanhartless
Copy link
Contributor

I updated to only bypass validation if the field is not part of $post

@escopecz
Copy link
Sponsor Member

It seems to be working well 👍

@alanhartless alanhartless merged commit 04b7690 into mautic:staging Oct 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants