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: Prevent form reset on submit when errors are present #3952

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

rumzledz
Copy link
Contributor

Description

The problem is that the form was being reset when submitted even though errors are present. We don't want that to happen.

form-error-reset

Testing

  1. Open the UserAccountPage/hooks.tsx file and update line 31 to:
return { canChangeUsername: true, daysTillUsernameChange };
  1. Connect Amy's wallet (Wallet 2)
  2. Go to the profile page: http://localhost:9091/account/profile
  3. Enter "leela" in the Username field
  4. Click the Save changes button repeatedly
  5. Verify that the form doesn't submit

Resolves #3730

@rumzledz rumzledz self-assigned this Dec 16, 2024
@rumzledz rumzledz marked this pull request as ready for review December 16, 2024 18:47
@rumzledz rumzledz requested a review from a team as a code owner December 16, 2024 18:47
Copy link
Contributor

@mmioana mmioana left a comment

Choose a reason for hiding this comment

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

Nice work on this one @rumzledz 🙌

Can confirm the form doesn't get submitted if there are errors present

Screen.Recording.2024-12-18.at.22.52.52.mov

I left a small refactoring comment, but it shouldn't block this PR getting merged 💯

Copy link
Contributor

Choose a reason for hiding this comment

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

@rumzledz would it make sense to have a variable const hasErrors = !Object.keys(formErrors) and use that in the useEffect?

  useEffect(() => {
    if (isSubmitting && resetOnSubmit && !hasErrors) {
      reset(values);
    }
  }, [isSubmitting, values, resetOnSubmit, reset, hasErrors]);

Copy link
Contributor

@bassgeta bassgeta left a comment

Choose a reason for hiding this comment

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

Nice catch, this is quite a silly bug honestly, glad it wasn't a pain to solve it 👍
image
On the right you can see an empty console, since nothing was submitted after spamming the Save changes button :D

Nice one 💪

Copy link
Contributor

@iamsamgibbs iamsamgibbs left a comment

Choose a reason for hiding this comment

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

Nice fix! I agree with @mmioana 's code suggestion.

Screenshot 2025-01-06 at 11 40 59

I think we should probably add a check to auth proxy to determine if the username is actually available as we're only doing a frontend check here which would be possible to bypass. I know we try to avoid adding anything to auth proxy unless it's really necessary, but I think we can check if the username is actually changing and only query the database in that case and it would probably be okay. (Also aware that at some point the user profile will be split into public / private, but seeing as the username is public I don't think this would be affected by that change).

@rumzledz rumzledz merged commit e5dcd41 into master Jan 9, 2025
2 checks passed
@rumzledz rumzledz deleted the fix/3730-do-not-reset-form-on-error branch January 9, 2025 19:14
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.

[Manage account] Issue with username Validation in Manage Profile page
4 participants