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

refactor(organizations): code cleanup and patch data cleanup in account settings and tos view TASK-1292 #5290

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Nov 21, 2024

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

👀 Preview steps

  1. ℹ️ have an account and a project
  2. Go into account settings view
  3. Change information on some field(s)
  4. Save the changes
  5. Check in the network tab the call to the /me endpoint. The payload should only contain the changed fields.
  6. Enable a TOS view by following this document
  7. In the TOS view, fill in the information and save (you may want to preserve log before saving data)
  8. Check the network tab. Payload should contain only the changed field.

💭 Notes

We're refactoring accountDetails and TOS view on the way of implementing some changes on displayed fields:

  • Removed the "global" observer from mob-x which was causing linting issues
  • Cleaned up the code
  • Changed in a way that only changed fields are sent to the PATCH request to update data

@pauloamorimbr pauloamorimbr changed the title refactor(organizations): refactor account settings ans tos pages TASK-1292 refactor(organizations): cleanup and cleanup patch data in account settings and tos view TASK-1292 Nov 21, 2024
@pauloamorimbr pauloamorimbr changed the title refactor(organizations): cleanup and cleanup patch data in account settings and tos view TASK-1292 refactor(organizations): code cleanup and patch data cleanup in account settings and tos view TASK-1292 Nov 21, 2024
// We're verifying that the user is logged in so we can consider the current account is from a valid logged user
const {currentAccount} = useLocalObservable(
() => {
return {currentAccount: sessionStore.isLoggedIn && (sessionStore.currentAccount as AccountResponse)};
Copy link
Contributor

Choose a reason for hiding this comment

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

This might just be a style preference, but it seems a little strange to me to have currentAccount here defined as false | AccountRespone. Could we make it null instead of false?

Copy link
Contributor

@jamesrkiger jamesrkiger left a comment

Choose a reason for hiding this comment

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

I left one comment, but otherwise lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants