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: saving user account info #4121

Merged
merged 6 commits into from
Jun 6, 2024
Merged

fix: saving user account info #4121

merged 6 commits into from
Jun 6, 2024

Conversation

emilyjablonski
Copy link
Collaborator

@emilyjablonski emilyjablonski commented Jun 6, 2024

This PR addresses metrotranscom#631

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

This PR fixes two bugs on the user account screen:

  1. Edit a field on the user account page, visit another page like the dashboard, go back in the browser and your change was not there
  2. Edit a field on the user account page, edit another field, refresh, and only the second change persisted

It seemed like the problem with the first bug was that we are setting the form field defaults if the user profile is present, and when you go back in the browser, the profile is present and wasn't refetching, so I am fetching the user on page load, and after every user update I am re-setting the user object. Because I'm fetching on page load and that data is required in the UI, I pulled in our loading overlay over the card.

We were also having validation issues because we have separate forms on the page but we were not actually setting up the forms as separate as react-hook-forms expects - validation for all fields was triggering on submitting just one of them - so I separated these out into four separate forms as well.

How Can This Be Tested/Reviewed?

Follow the steps above and ensure they work!

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

Copy link

netlify bot commented Jun 6, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 03bc89d
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/666234d3b848d00008962a7f
😎 Deploy Preview https://deploy-preview-4121--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@emilyjablonski emilyjablonski added the 1 review needed Requires 1 more review before ready to merge label Jun 6, 2024
@emilyjablonski emilyjablonski marked this pull request as ready for review June 6, 2024 05:52
@emilyjablonski emilyjablonski added 2 reviews needed Requires 2 more review before ready to merge and removed 1 review needed Requires 1 more review before ready to merge labels Jun 6, 2024
Copy link
Collaborator

@mcgarrye mcgarrye 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!

sites/public/src/pages/account/edit.tsx Show resolved Hide resolved
@mcgarrye mcgarrye added 1 review needed Requires 1 more review before ready to merge and removed 2 reviews needed Requires 2 more review before ready to merge labels Jun 6, 2024
Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

Love the new test! The change looks good. Just have one question on the password min-length

sites/public/src/pages/account/edit.tsx Show resolved Hide resolved
@ludtkemorgan ludtkemorgan added ready to merge Should be applied when a PR has been reviewed and approved and removed 1 review needed Requires 1 more review before ready to merge labels Jun 6, 2024
@emilyjablonski emilyjablonski merged commit fa2f6c4 into main Jun 6, 2024
7 of 12 checks passed
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 6, 2024
emilyjablonski added a commit to housingbayarea/bloom that referenced this pull request Jun 6, 2024
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 11, 2024
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 13, 2024
emilyjablonski added a commit to metrotranscom/doorway that referenced this pull request Jun 13, 2024
fix: change type of income voucher

fix: test issue

fix: test issue

fix: build issue

fix: cypress tests

fix: env vars

fix: rebase issues

fix: rebase issues
ludtkemorgan

This comment was marked as off-topic.

emilyjablonski added a commit to housingbayarea/bloom that referenced this pull request Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Should be applied when a PR has been reviewed and approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants