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

Issue/2159 validate settings #2233

Merged
merged 18 commits into from
Dec 19, 2024
Merged

Issue/2159 validate settings #2233

merged 18 commits into from
Dec 19, 2024

Conversation

zahardev
Copy link
Contributor

@zahardev zahardev commented Dec 6, 2024

Adds a possibility to validate setting fields. Supports the following rules:
required, max, min, email, integer, matches.

Rules example:

'validation' => [
			[
				'rule'    => 'required',
				'message' => __( 'Field is required', 'gk-gravityview' ),
			],
			[
				'rule'    => 'max:12',
				'message' => __( 'Should be less or equal 12', 'gk-gravityview' ),
			],
		];

Currently supports only frontend validation. We can improve it with a backend validation using the same validation config.

Added a "Validate URL with tags" rule for "'No Entries Redirect URL'" and "Edit Entry Redirect URL" using the "required" and "matches" rules. Here is the URL rule Regex - https://regex101.com/r/I2QSEI/1

💾 Build file (619e85c).

@zahardev zahardev requested a review from mrcasual December 6, 2024 16:35
@zahardev zahardev self-assigned this Dec 6, 2024
@mrcasual mrcasual requested a review from Mwalek December 6, 2024 17:45
@mrcasual
Copy link
Collaborator

mrcasual commented Dec 6, 2024

@Mwalek, once the unit tests are fixed and you see the build file, please test.

@Mwalek
Copy link

Mwalek commented Dec 9, 2024

@zahardev @mrcasual the validation now works with merge tags, but I identified a few issues.

  1. It is not possible to click the View update button when an invalid setting was added to the URL. Ideally, it should be possible to click this button even if an invalid setting has been added so that the user can be taken to the setting that is invalid.
  2. The layout breaks when the invalid URL message is shown.
Screenshot 2024-12-09 at 1 31 20 PM

@zahardev
Copy link
Contributor Author

zahardev commented Dec 9, 2024

Thanks, @Mwalek!

Regarding 2 - thanks, will be fixed.
Regarding 1 - @mrcasual Could you please confirm if users should be able to submit the form with invalid values? If yes, should we try to prevent saving the invalid fields? Possible solutions:

  1. Automatically clean up invalid values before submission.
  2. Display a confirmation message "Your settings contain invalid values. Are you sure you want to proceed?" and save invalid values.

@mrcasual
Copy link
Collaborator

mrcasual commented Dec 9, 2024

@zahardev, I don't think we should allow saving the View w/invalid fields. I haven't tested this PR yet and you may have already implemented this, but if there are invalid fields that are hidden (e.g., when switching from one tab to another), we should display a warning/clearly inform the user about where to find the errors.

@zahardev
Copy link
Contributor Author

@mrcasual Thanks, I’ll update the UX to enable the "Update" button but prevent form submission and show a warning if invalid fields are found.

@zahardev
Copy link
Contributor Author

@Mwalek Could you please check if my recent changes resolve the issues?

@Mwalek
Copy link

Mwalek commented Dec 12, 2024

@zahardev thanks! Issue 2 is resolved. However, I now see the following warning when GV is activated:

Warning: Undefined variable $version in /...wp-content/plugins/gravityview/includes/class-admin-views.php on line 1769

Additionally, validation does not work at all for the "Delete Entry Redirect URL" setting.

@mrcasual
Copy link
Collaborator

mrcasual commented Dec 12, 2024

@zahardev, in addition to @Mwalek's comment ☝️, please update the entry slug-related validation logic to use your new approach.

CleanShot 2024-12-12 at 10 18 32@2x

@zahardev
Copy link
Contributor Author

@Mwalek Fixed the warning issue, thanks for pointing it out.

@mrcasual Updated the Entry Slug field with the new validation logic. By the way, should we allow {entry_url} as an alternative to {entry_id}?

@zahardev
Copy link
Contributor Author

Added validation for the "Delete Entry Redirect URL" setting.

@Mwalek
Copy link

Mwalek commented Dec 18, 2024

@zahardev the earlier issues are solved, thank you!

As I was testing this time around, I noticed that the validation does not recognize merge tags in the format {x} only those in the format {X:d} (see images below). Not sure if this is intended. If not, please make the necessary changes.
Recognized
Screenshot 2024-12-18 at 2 14 15 PM
Unrecognized
Screenshot 2024-12-18 at 2 14 33 PM

Additionally, the "Field is required" error message does not disappear immediately when a merge tag is selected from the dropdown. For example, if I select "Book Title" from the dropdown, it adds {Book Title:3}, but the required error is still shown until I delete some of the text and retype it.

@zahardev
Copy link
Contributor Author

@Mwalek

Regarding the {x} format validation - it was actually working correctly. I believe the problem occurred because the validation event wasn't triggered when a Merge Tag was selected from the dropdown (issue number two). This should be fixed now - could you please check?

@Mwalek
Copy link

Mwalek commented Dec 19, 2024

@zahardev great, thanks for explaining! Looks good now.

cc. @mrcasual

@mrcasual mrcasual merged commit 89fa414 into develop Dec 19, 2024
1 check passed
@mrcasual mrcasual deleted the issue/2159-validate-settings branch December 19, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants