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

Feature/account page #900

Closed
wants to merge 32 commits into from
Closed

Conversation

anmol-fzr
Copy link
Contributor

Issue

Issue Number: 865

Issue Link: #865

What changes did you make?

Added Account Page with UI + Functionality + Translations. Delete Account button is intentionally left disabled reason being API to delete account by user isn't ready

Why did you make the changes?

Changes are made to fix the issue

Did you run tests?

Added a test named user-profile-update that ran successfully

anmol-fzr and others added 15 commits May 8, 2024 22:53
Removed unused variable,functions and hooks calls from LoginForm
Added Only UI part of the Account Page described in issue chaynHQ#865
Refactored Signout logic code across 3 files using custom hook useAuth
Added Functionality for profile data updates
Added Functionality to update email preferences in account settings
Added Caution modal for account deletion
Disabled delete Account Button as API is not made to delete user on its own
Copy link

vercel bot commented May 10, 2024

@anmol-fzr is attempting to deploy a commit to the Chayn Team on Vercel.

A member of the Team first needs to authorize it.

Fixed Strings must use Single Quote Error by elint
@eleanorreem
Copy link
Contributor

eleanorreem commented May 11, 2024

Thanks so much for this, loads of work 🎉

I know that this PR is WIP so apologies for all the comments. I had a look through the code in combination with the current functionality that we had in the backend. These are my comments:

  • The backend for updating an email isn't implemented so for now it is best just to have the Profile details form as per the designs (i.e. add the link in the issue to the getting in touch text and remove the button). If you would like to contribute this work at a later date, I can tag you in the issue when it comes up and you can simply implement the work you have already done. Let me know if that works for you!
  • The backend for deleting an account isn't ready yet and is going to be a bit complicated. For the sake of getting this merged soon, I would remove the Account Actions section for the PR and save it for the next issue that I will write up in the near future.
  • The email subscription form works well. I am happy with it. We will need to have some error / success feedback for the user. Please see the bloom-frontend/components/forms/CreateAccessCodeForm.tsx for some input on how you might implement that. For now the success can just be "Your email subscription preferences have been updated"
  • There are some console logs and commented out code that will need tidying up.
  • As you have included more functionality in this PR than in the original ticket, you would need to add more functionality to the cypress test. In the new cypress test, you will need to check the profile details and that updating your subscription preferences works.
  • The header h1 and description isn't quite aligned with the middle of the image. You might need to add a marginTop: auto and a marginBottom: auto the top and bottom of the div that contains the header and description. I hope that makes sense.
  • There are errors when I run yarn build and yarn lint so it would be great if you could sort the single quotes.
  • I don't know if you experience this but the page flashes on load which doesn't seem to happen to other pages.I think its because some code has been copied over from the disable service emails page that shouldn't be there. The whole use effect can go.
  • When you have made all the changes, it would be awesome if you could soft reset the branch and redo the commits, excluding the features we aren't going to include in this PR otherwise it might get confusing that we are adding features and then taking them away.

Sorry for the long list of comments! What you have done so far is great. Thank you for all your work. Please do keep the work that isn't going to be included in this PR and we can use it in the very near future.

const [updateUser, { isLoading: updateUserIsLoading }] = useUpdateUserMutation();

useEffect(() => {
if (userCreatedAt && userServiceEmailsPermission === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I see where you have copied this from. This shouldn't be there, that functionality is only for the disable-service-email.tsx page.

@eleanorreem
Copy link
Contributor

eleanorreem commented May 12, 2024

Thanks for your changes! They look great. There were a few more changes that were on PR. It would be great if they could be resolved. These are the ones outstanding:

  • removing account actions from the card
  • success/ error handling for saving profile information
  • The cypress test doesn't test whether the correct email is displaying, it just tests that the box is disabled. Ideally you would be checking against the email address and also checking that when you update your subscription preferences, it works.
  • Header alignment

removed account action from settings page
@kyleecodes kyleecodes self-assigned this May 15, 2024
@eleanorreem
Copy link
Contributor

@anmol-fzr, thanks for all your work so far! Just checking in here to make sure you have everything you need to finish off this ticket. Absolutely no pressure, it would just be great to get this contribution merged!

@anmol-fzr
Copy link
Contributor Author

@eleanorreem , I made the changes to the code suggest in this review please check.

@eleanorreem
Copy link
Contributor

Hi thank you for your quick reply and all the work so far! There were a few more things that I commented on your pull request - take a look through the pull request files. There are some unresolved comments that need to be addressed before I can merge.

@anmol-fzr
Copy link
Contributor Author

@eleanorreem I made change told last time about removing account action card to header alignment. It would be great if you review the code for merge or we resolve anything that needs to be done for this PR

@eleanorreem
Copy link
Contributor

Hiya @anmol-fzr, Thanks for your response 😄

There are these open comments on your PR. Let me know if you have any questions. I linked to the most of the comments here.

Please check through the open comments and make sure they are all resolved.

Thanks in advance for everything !

@anmol-fzr
Copy link
Contributor Author

@eleanorreem i made suggested changes, please check

@anmol-fzr
Copy link
Contributor Author

hope we can merge this now

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