Skip to content

[Bugfix:System] Add signup validation#12679

Closed
Asterisk-Hunter wants to merge 18 commits into
Submitty:mainfrom
Asterisk-Hunter:bugfix/signup-form-validation
Closed

[Bugfix:System] Add signup validation#12679
Asterisk-Hunter wants to merge 18 commits into
Submitty:mainfrom
Asterisk-Hunter:bugfix/signup-form-validation

Conversation

@Asterisk-Hunter

Copy link
Copy Markdown
Contributor

Why is this Change Important & Necessary?

Closes #12490 (taking over from abandoned PR #12491)

Right now when you fill out the Sign Up form and hit submit with an invalid email domain or a user ID that's too short/long, the server validates it, redirects back, and all your form fields get wiped clean. Pretty annoying if you've already typed in your name, password, etc.

What is the New Behavior?

Added client-side validation that checks the email domain and user ID length before the form submits. If something's wrong, you get an error toast and your form fields stay intact so you can just fix the one field that was wrong.

Validations added:

  • Email must have a valid format (something@domain)
  • Email domain must be in the accepted list
  • User ID must meet min/max length requirements
  • Passwords must match (catches this before submit too)

What steps should a reviewer take to test this?

  1. Go to /authentication/create_account
  2. Fill in all the fields with valid data except use an unaccepted email like [email protected]
  3. Click Sign Up - you should see an error toast and all fields should stay filled
  4. Fix the email to use an accepted domain
  5. Make the user ID too short (like 2 characters) and submit - error toast, fields preserved
  6. Fix everything and submit - should work normally

Automated Testing & Documentation

This builds on existing patterns in the codebase - similar to how checkPasswordsMatch() already works. The server-side validation still runs as a backup.

Other information

I noticed the abandoned PR had a similar approach. Made a few small tweaks like case-insensitive email domain matching and clearer error messages that tell you exactly what's wrong.

@codecov

codecov Bot commented Mar 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 21.45%. Comparing base (2d738b8) to head (126a664).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##               main   #12679   +/-   ##
=========================================
  Coverage     21.45%   21.45%           
  Complexity     9896     9896           
=========================================
  Files           268      268           
  Lines         36967    36967           
  Branches        495      495           
=========================================
  Hits           7933     7933           
  Misses        28543    28543           
  Partials        491      491           
Flag Coverage Δ
autograder 21.32% <ø> (ø)
js 2.01% <ø> (ø)
migrator 100.00% <ø> (ø)
php 20.49% <ø> (ø)
python_submitty_utils 80.08% <ø> (ø)
submitty_daemon_jobs 91.13% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Asterisk-Hunter Asterisk-Hunter changed the title [Bugfix:UI] Add client-side validation to Sign Up form [Bugfix:System] Add signup validation Mar 29, 2026

@williamjallen williamjallen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a Cypress test for this in site/cypress/e2e/Cypress-System/self_account_creation.spec.js.

@github-project-automation github-project-automation Bot moved this from Seeking Reviewer to Work in Progress in Submitty Development Mar 29, 2026
@Asterisk-Hunter

Copy link
Copy Markdown
Contributor Author

Thanks for the review! The test in self_account_creation.spec.js (lines 22-41) already covers the validations we're adding:

  • Not accepted email domain (lines 22-25)
  • User ID too short (lines 26-29)
  • User ID too long (lines 30-33)
  • Password too short (lines 34-37)
  • Passwords don't match (lines 38-41)

Is there a specific additional test case you'd like added, or should I verify the existing test passes? I can see Cypress (System) is showing red - is that related to our change or is there a separate flaky test issue there?

@automateprojectmangement automateprojectmangement Bot moved this from Work in Progress to In Review in Submitty Development Mar 30, 2026
@williamjallen

Copy link
Copy Markdown
Member

@Asterisk-Hunter Can't you test that the fields remain filled out and that the validation is performed client-side?

Test verifies that:
- Invalid email triggers client-side validation (not server round-trip)
- Form fields remain filled after validation error
- This proves the fix actually prevents form submission on client errors
@Asterisk-Hunter

Copy link
Copy Markdown
Contributor Author

Hey! i added the tests please review it.

@IDzyre

IDzyre commented Mar 31, 2026

Copy link
Copy Markdown
Member

If you click on the cypress-system tests, it shows which of the tests failed. The tests that failed were the Self account creation tests.

@Asterisk-Hunter

Copy link
Copy Markdown
Contributor Author

Yes! I fixed them.

@Asterisk-Hunter

Copy link
Copy Markdown
Contributor Author

Added the Cypress test as requested in site/cypress/e2e/Cypress-System/self_account_creation.spec.js. The test verifies that form fields are preserved after client-side validation fails.

Also fixed the ESLint errors (brace-style in authentication.js and unsafe-to-chain-command in the test file).

Note: The Cypress (System) check has been failing with a Postgres infrastructure error (role "root" does not exist) that occurs during test setup, before my test runs. This appears to be a CI environment issue unrelated to my changes.

@github-actions github-actions Bot added the Abandoned PR - Needs New Owner No activity on PR for more than 2 weeks -- seeking new owner to complete label Apr 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These tests currently dont pass, so its hard to do a review of this PR.

@github-project-automation github-project-automation Bot moved this from In Review to Work in Progress in Submitty Development May 1, 2026
@dandrecollins07-ctrl dandrecollins07-ctrl self-assigned this Jun 8, 2026
@dandrecollins07-ctrl
dandrecollins07-ctrl force-pushed the bugfix/signup-form-validation branch from aa6c0f9 to d34f6fd Compare June 11, 2026 15:08

@dandrecollins07-ctrl dandrecollins07-ctrl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After fixing minor CI testing issues, the code passes Cypress System test. All left is common CI failings. Testing works locally, approving.

@github-actions github-actions Bot removed the Abandoned PR - Needs New Owner No activity on PR for more than 2 weeks -- seeking new owner to complete label Jun 12, 2026
@bmcutler

Copy link
Copy Markdown
Member

@dandrecollins07-ctrl please fix merge conflicts. thanks :)

bmcutler added a commit that referenced this pull request Jun 26, 2026
### What is the current behavior?

When server-side validation rejects the self-account creation form, the
redirect back to the signup page clears every field. This includes
failures that cannot be prevented by client-side validation, such as
duplicate user IDs or email addresses.

### What is the new behavior?

The controller stores only the submitted email, user ID, given name, and
family name for the redirect request. The signup form consumes those
values once and repopulates the corresponding fields.

Password and confirmation values are intentionally never stored or
repopulated.

This keeps server-side validation authoritative and covers every
existing validation/error redirect rather than only rules that can be
duplicated in JavaScript.

### Testing

The self-account-creation Cypress flow now verifies that:

- all four non-password fields remain populated after an invalid
email-domain response;
- password and confirmation fields are empty after the redirect;
- subsequent validation paths can replace the preserved values cleanly.

Local checks: `git diff --check` and JavaScript syntax validation
passed. Full PHP lint and Cypress execution require the Submitty
development environment and are left to CI.

Closes #12490

This is a server-side replacement for the incomplete client-side
approach in #12679.

Co-authored-by: Barb Cutler <[email protected]>
jndlansh pushed a commit to jndlansh/Submitty that referenced this pull request Jul 1, 2026
### What is the current behavior?

When server-side validation rejects the self-account creation form, the
redirect back to the signup page clears every field. This includes
failures that cannot be prevented by client-side validation, such as
duplicate user IDs or email addresses.

### What is the new behavior?

The controller stores only the submitted email, user ID, given name, and
family name for the redirect request. The signup form consumes those
values once and repopulates the corresponding fields.

Password and confirmation values are intentionally never stored or
repopulated.

This keeps server-side validation authoritative and covers every
existing validation/error redirect rather than only rules that can be
duplicated in JavaScript.

### Testing

The self-account-creation Cypress flow now verifies that:

- all four non-password fields remain populated after an invalid
email-domain response;
- password and confirmation fields are empty after the redirect;
- subsequent validation paths can replace the preserved values cleanly.

Local checks: `git diff --check` and JavaScript syntax validation
passed. Full PHP lint and Cypress execution require the Submitty
development environment and are left to CI.

Closes Submitty#12490

This is a server-side replacement for the incomplete client-side
approach in Submitty#12679.

Co-authored-by: Barb Cutler <[email protected]>
@dandrecollins07-ctrl

Copy link
Copy Markdown
Contributor

Closing as it was fixed through PR #12939.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Fix Sign Up form clearing fields on validation error

5 participants