Skip to content

Conversation

@mwtrew
Copy link
Contributor

@mwtrew mwtrew commented Oct 6, 2025

The bug that necessitated this PR was due to our "general error" logic from Redux; we want to block submission if the password reset token from the URL is invalid, but other server-side errors (including submitting an invalid password!) also manifest as a general error which blocked submission. There's another card to move away from this general error business, and at that point I'll revisit the todos in this PR.

I also noticed this page was fairly hideous, so I mostly rebuilt it using SetPasswordInput. I've also refactored SetPasswordInput to rely less on state from the parent so it's easier to reuse - for example, the parent no longer needs to track what's in the confirmation field or perform that equality check.

@codecov
Copy link

codecov bot commented Oct 6, 2025

Codecov Report

❌ Patch coverage is 76.47059% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.58%. Comparing base (14a07fb) to head (a4a5527).
⚠️ Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/test/pages/PasswordResetHandler.cy.tsx 0.00% 5 Missing ⚠️
...pp/components/elements/inputs/SetPasswordInput.tsx 85.00% 3 Missing ⚠️
...rc/app/components/pages/RegistrationSetDetails.tsx 0.00% 3 Missing ⚠️
...c/app/components/handlers/PasswordResetHandler.tsx 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1764      +/-   ##
==========================================
+ Coverage   41.26%   41.58%   +0.32%     
==========================================
  Files         534      535       +1     
  Lines       23506    23509       +3     
  Branches     6940     7753     +813     
==========================================
+ Hits         9699     9776      +77     
+ Misses      13766    13096     -670     
- Partials       41      637     +596     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mwtrew mwtrew marked this pull request as ready for review October 8, 2025 14:04
@barna-isaac
Copy link
Contributor

barna-isaac commented Oct 9, 2025

If I understand correctly, the original bug was because the event handler on the button didn't do anything as long as there was an "error", and the fix is that we now only skip the request if there are validation errors. Also, the client-side validation now correctly prevents empty passwords from being submitted. Looks good to me.

I'm glad the PasswordResetHandler now uses the same SetPasswordInput as the RegistrationSetDetails component. To me, the fact that we could move some of the validation logic down to this component from PasswordResetHandler shows this is a good approach.

Physics notes:

  • The old wording, which was "New password" instead of now just "Password", made it clear to users that we're not expecting them to remember their previous password. I think this was a nice thing for users, even though it's kind of obvious on a password reset page. I suspect this was removed because we now use the same component from the registration page. I think it's okay to pass down the label for that component.
  • This is a bit subjective, but to me it looks like the new design has unnecessary padding above the "password" header, and is missing some padding beneath the button, where the container ends.
  • Shouldn't error messages like "invalid password reset token" use red (for error) instead of yellow (which is usually for warnings)?
  • I like that the new page provides the option to show the password, and uses new black page headers

Ada notes:

  • would prefer "New password" here as well
  • paddings (top as well as bottom) look good on this page
  • question about error message background colour applies here as well
  • what triggered the decision to hide the "confirm password" field? It was shown on Ada as well as Sci before this PR, but this PR removes it from the Ada page but leaves it on Sci. Update: I now see that Ada does not request password confirmation on the initial sign-up form, so it is more consistent not to request password confirmation when resetting the password either.
  • The new page is prettier and makes use of standard components (like the breadcrumbs for navigation)^^

I also want to thank you, from the bottom of my heart, for creating test cases for the password reset page. It's been such a relief I didn't need to try all the error cases by hand. I still manually tested the happy path and some error cases (including on the registration page).

Overall, this PR is very thoughtfully considered and makes great changes! I left a few more comments -- not so much because there was anything wrong with the original versions from the PR, but because I wanted to take the opportunity to discuss these. They're just suggestions, please feel free to ignore any of them!

@mwtrew
Copy link
Contributor Author

mwtrew commented Oct 13, 2025

Thanks @barna-isaac, I've updated the margin and added a label prop so we can override that in different contexts.

Shouldn't error messages like "invalid password reset token" use red (for error) instead of yellow (which is usually for warnings)?

We're not particularly consistent with how we show server-side errors like this, but we use this approach in the registration flow so I'll leave it like this for now. We could probably use a design review of how we do this in general.

@mwtrew mwtrew merged commit 4bc22fc into main Oct 13, 2025
10 checks passed
@mwtrew mwtrew deleted the bug/re-submit-password-form branch October 13, 2025 15:42
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