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

CRDCDH-2123 Support "All" studies for Federal Leads #566

Merged
merged 13 commits into from
Dec 19, 2024
Merged

Conversation

amattu2
Copy link
Member

@amattu2 amattu2 commented Dec 17, 2024

Overview

This PR adds the "All" studies option for Federal Leads on the Edit User tool.

Warning

When switching roles from anything (e.g. Submitter) to Federal Lead, MUI throws a warning because the "All" option is programmatically selected, but not available in the study options during the first render. The only solution I could find that worked is not very clean. I'm open to any alternatives.

Change Details (Specifics)

  • Add All option to the Studies list for Federal Leads, selected by default
  • Migrate VisibleFieldState out of ProfileView into the useProfileFields hook

Related Ticket(s)

CRDCDH-2123 (Task)

@coveralls
Copy link
Collaborator

coveralls commented Dec 17, 2024

Pull Request Test Coverage Report for Build 12418541811

Details

  • 1 of 38 (2.63%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 56.529%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/content/users/ProfileView.tsx 0 37 0.0%
Files with Coverage Reduction New Missed Lines %
src/content/users/ProfileView.tsx 3 0.0%
Totals Coverage Status
Change from base Build 12401702046: -0.2%
Covered Lines: 3805
Relevant Lines: 6274

💛 - Coveralls

@amattu2 amattu2 changed the title CRDCDH-2123 CRDCDH-2123 Support "All" studies for Federal Leads Dec 17, 2024
@amattu2 amattu2 marked this pull request as ready for review December 17, 2024 20:25
@amattu2 amattu2 added this to the 3.2.0 (PMVP-M3) milestone Dec 17, 2024
@amattu2
Copy link
Member Author

amattu2 commented Dec 17, 2024

Coveralls is rejecting this PR since it has a significant coverage drop (not really seeing why). Regardless, that can be ignored.

Copy link
Collaborator

@Alejandro-Vega Alejandro-Vega left a comment

Choose a reason for hiding this comment

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

Everything LGTM and works great! I left a potential fix for the timing issue regarding the "All" option below. As well as found one potential bug or just a MUI thing. Feel free to disregard both if needed.

src/content/users/ProfileView.tsx Show resolved Hide resolved
src/content/users/ProfileView.tsx Outdated Show resolved Hide resolved
@Alejandro-Vega Alejandro-Vega added the 📝 Change Requested This PR has requested changes label Dec 19, 2024
@amattu2 amattu2 removed the 📝 Change Requested This PR has requested changes label Dec 19, 2024
@Alejandro-Vega Alejandro-Vega added the 📝 Change Requested This PR has requested changes label Dec 19, 2024
@amattu2 amattu2 removed the 📝 Change Requested This PR has requested changes label Dec 19, 2024
Copy link
Collaborator

@Alejandro-Vega Alejandro-Vega left a comment

Choose a reason for hiding this comment

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

LGTM! Working well with updated changes.

@Alejandro-Vega Alejandro-Vega merged commit effdb42 into 3.2.0 Dec 19, 2024
7 checks passed
@Alejandro-Vega Alejandro-Vega deleted the CRDCDH-2123 branch December 19, 2024 18:29
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.

3 participants