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

feat(organizations): hide organization fields from settings for mmo organizations - TASK-978 #5280

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

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Nov 19, 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

📣 Summary

Organization fields are not displayed anymore in setting screens for MMOs.

👀 Preview steps

Feature/no-change template:

  1. ℹ️ have account
  2. Go to "settings" view at /#/account/settings
  3. Use the feature flag ?ff_mmoEnabled=true
  4. Set the organization as MMO
  5. 🟢 Fields should not be displayed
  6. Set the feature flag to false as in ?ff_mmoEnabled=false
  7. 🟢 Fields should be displayed again

💭 Notes

The 'organization', 'organization_type' and 'organization_website' are now being hidden when the current organization has the is_mmo flag.

@jamesrkiger
Copy link
Contributor

@pauloamorimbr This isn't an issue with your code, but I am concerned about the way we are submitting the PATCH request in this form. It looks to me like we are basically just always sending the data for every form field. It's a PATCH request but we're essentially treating it like a PUT. This gets uglier as a result of the changes we are making here: Now we are submitting the org detail fields even though we aren't displaying them.

At the very least, I believe we need to stop submitting org data this way. If it didn't take too long, I would say it would be better if we only submitted data for "dirty" form fields (ones that have been edited by the user). But if that second goal is going to take too long perhaps we can ignore that for the moment. Can you spend 20 minutes or so to determine what the best approach is here?

@pauloamorimbr pauloamorimbr marked this pull request as draft November 21, 2024 19:15
@pauloamorimbr pauloamorimbr force-pushed the task-978-update-profile-and-security-pages branch from 634da71 to aa22083 Compare November 28, 2024 16:30
@pauloamorimbr
Copy link
Contributor Author

pauloamorimbr commented Nov 28, 2024

@pauloamorimbr This isn't an issue with your code, but I am concerned about the way we are submitting the PATCH request in this form. It looks to me like we are basically just always sending the data for every form field. It's a PATCH request but we're essentially treating it like a PUT. This gets uglier as a result of the changes we are making here: Now we are submitting the org detail fields even though we aren't displaying them.

At the very least, I believe we need to stop submitting org data this way. If it didn't take too long, I would say it would be better if we only submitted data for "dirty" form fields (ones that have been edited by the user). But if that second goal is going to take too long perhaps we can ignore that for the moment. Can you spend 20 minutes or so to determine what the best approach is here?

After the refactors in #5303 and #5290, I'm back here. Now this behaves the proper way, like you commented, sending only the edited fields to the PATCH call, thus not sending organization data when the organization is MMO since the fields are not visible for editing.

@pauloamorimbr pauloamorimbr marked this pull request as ready for review November 28, 2024 20:19

const organization = orgQuery.data;

// We will not display organization fields if it's and MMO organization
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: should be "an MMO organization".

It might be more accurate to say "We will not display organization fields if user is a member of an MMO ..."

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.

Left one minor comment. Other than that, 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.

2 participants