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

fix: Add client side validation to public forms #4529

Merged
merged 10 commits into from
Oct 31, 2024

Conversation

dsamojlenko
Copy link
Member

@dsamojlenko dsamojlenko commented Oct 30, 2024

Summary | Résumé

Related: #4519
While investigating the recent memory leak issues, there seems to be some correlation to Contact form submissions with large amounts of invalid data.

Noted that this form (along with Support and Unlock publishing) rely exclusively on server-side validation of form inputs.

Given that Contact and Support are public-facing unauthenticated forms, it's good practice to validate both client-side and server-side. Catching validation errors client-side will prevent unnecessary round trips to the server.

Unclear if this is the specific cause of the memory leak issue, but given the correlation between Contact form submits and the recent spikes, it's a possibility to be considered, and as mentioned is just good practice.

Testing

This change affects the Contact, Support, and Unlock publishing form.
To test, visit the forms and submit some information.
The form should behave as normal.

To get a better sense of what's changed, open the Network tab of the Developer tools in Chrome.

  • Fill the form with some invalid data (bad email, missing required input, etc)
  • When you submit, you should see the validation errors appear, but you should not see any activity in the Network tab.
  • If you fix the errors in the form and submit again, assuming you've fixed all the errors you will see the submission to the server in the Network tab.
  • If you were to compare against the current version of the forms, you would see the form submission sent to the server on every submit valid or not.

@dsamojlenko dsamojlenko marked this pull request as draft October 30, 2024 19:57
Copy link
Contributor

@dsamojlenko dsamojlenko marked this pull request as ready for review October 30, 2024 20:40
@anikbrazeau
Copy link
Member

anikbrazeau commented Oct 30, 2024

Tested and seeing some activity is it what's expected?
Screenshot 2024-10-30 at 5 21 56 PM
Screenshot 2024-10-30 at 5 25 33 PM
Screenshot 2024-10-30 at 5 22 40 PM
Screenshot 2024-10-30 at 5 22 04 PM

@dsamojlenko
Copy link
Member Author

Tested and seeing some activity is it what's expected? Screenshot 2024-10-30 at 5 21 56 PM Screenshot 2024-10-30 at 5 25 33 PM Screenshot 2024-10-30 at 5 22 40 PM Screenshot 2024-10-30 at 5 22 04 PM

yes, that's unrelated.

@dsamojlenko dsamojlenko reopened this Oct 31, 2024
Copy link
Contributor

craigzour
craigzour previously approved these changes Oct 31, 2024
Copy link
Contributor

@craigzour craigzour left a comment

Choose a reason for hiding this comment

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

LGTM! I tested it and it does work as expected :)

Copy link
Contributor

@bryan-robitaille bryan-robitaille left a comment

Choose a reason for hiding this comment

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

Looks good.

@dsamojlenko dsamojlenko enabled auto-merge (squash) October 31, 2024 14:53
@dsamojlenko dsamojlenko merged commit 75b06d5 into main Oct 31, 2024
13 checks passed
@dsamojlenko dsamojlenko deleted the fix/add_client_side_validation_to_forms branch October 31, 2024 14:56
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.

4 participants