[Bugfix:System] Add signup validation#12679
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
williamjallen
left a comment
There was a problem hiding this comment.
Please add a Cypress test for this in site/cypress/e2e/Cypress-System/self_account_creation.spec.js.
|
Thanks for the review! The test in
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? |
|
@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
|
Hey! i added the tests please review it. |
|
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. |
|
Yes! I fixed them. |
|
Added the Cypress test as requested in Also fixed the ESLint errors (brace-style in Note: The Cypress (System) check has been failing with a Postgres infrastructure error ( |
There was a problem hiding this comment.
These tests currently dont pass, so its hard to do a review of this PR.
aa6c0f9 to
d34f6fd
Compare
dandrecollins07-ctrl
left a comment
There was a problem hiding this comment.
After fixing minor CI testing issues, the code passes Cypress System test. All left is common CI failings. Testing works locally, approving.
|
@dandrecollins07-ctrl please fix merge conflicts. thanks :) |
### 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]>
### 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]>
|
Closing as it was fixed through PR #12939. |
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:
What steps should a reviewer take to test this?
/authentication/create_account[email protected]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.