-
-
Notifications
You must be signed in to change notification settings - Fork 87
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
User permissions search #1853
User permissions search #1853
Conversation
…d users-permission-search
…d users-permission-search
Added an API that fetches all the admins
Redesign users screen 1751
Filtered permission 1754
Filtered permissions data comp
File in Frontend needs to adapt to manage an object that has |
Thanks for finding the issue, @trillium! |
@trillium, do you think we should include the cc: @JackHaeg |
for (const projectId of projectManagerObj.managedProjects) { | ||
const projectDetail = await Project.findById(projectId); | ||
if (projectDetail && projectDetail.name) { | ||
projectNames.push(projectDetail.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you return (push) the projectId
in addition to the name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jng34, The projectId is already included as part of the admins data. So, I think we don't need to push it.
The API for the admins is returning this data object
The API for the project leads is returning this data object
So, I want to check if we should include the isProjectLead
and managedProjectNames
properties in the API that fetches all the admin as well?
Thanks for taking a look at the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks for the screenshots.
Regarding the isProjectLead
and managedProjectNames
properties, I think including those in the API fetch for admins would be a good idea if the user wants to know that info about a specific admin when clicking on his/her profile.
I guess that depends on what accessLevel
('user', 'admin', 'superadmin') does the user have and what info do we want each to be able to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per discussion with @trillium, it should be okay to not include isProjectLead
and managedProjectNames
as part of the admins API.
@vorleakyek |
@vorleakyek @trillium |
@JackHaeg Oops, clicked the self-request by accident! |
@jng34, could you please approve the PR since you have already looked at the code? Thank you! |
@vorleakyek @trillium @JackHaeg |
Update - @ntrehan plans to review this PR this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vorleakyek and @jng34
Sorry for the delay in reviewing. It was a very large PR and have gone through the changes. The changes are working as expected
I am approving the PR from my end
Fixes #1801
Fixes #1836
What changes did you make and why did you make them ?
Screenshots of Proposed Changes
Visuals after changes are applied