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

User permissions search #1853

Merged
merged 37 commits into from
Mar 4, 2025
Merged

User permissions search #1853

merged 37 commits into from
Mar 4, 2025

Conversation

vorleakyek
Copy link
Member

@vorleakyek vorleakyek commented Feb 6, 2025

Fixes #1801
Fixes #1836

What changes did you make and why did you make them ?

  • Identified and removed all instances of dummy/placeholder data from the "User Permission Search" screen and filtered permissions data component.
  • Updated MongoDB query to return a list of projectLead to match the frontend required data as suggested by Trillium
  • Hook up the previously created APIs from
  • Ensure the API responses are correctly mapped to the UI elements and behave as expected.
  • Note: Unit test will be considered later once these changes are approved

Screenshots of Proposed Changes

Visuals after changes are applied

user-permission-search

ntrehan and others added 29 commits October 8, 2024 00:48
Added an API that fetches all the admins
@vorleakyek
Copy link
Member Author

vorleakyek commented Feb 6, 2025

@ntrehan, @jng34, and @trillium, please take a look at the changes when you have some time and let me know if there is anything I should change in the code. Thank you!
cc: @JackHaeg

@trillium
Copy link
Member

Looking into this, I think I've found the problem:

The API is returning project names, not project IDs in UserPermissionSearch

image

But in the user window that renders projects it returns project IDs

image

UserToEdit expects an array of projectId and is getting an array of projectName so it isn't displaying any of the projects

@trillium
Copy link
Member

File in backend/controllers/user.controller.js at UserController.projectLead_list needs to return both name and projectId

Frontend needs to adapt to manage an object that has { name: string, projectId: string } format.

@vorleakyek
Copy link
Member Author

Thanks for finding the issue, @trillium!
I'll update the data returned from the API and adjust the frontend accordingly.

@vorleakyek
Copy link
Member Author

@trillium, do you think we should include the isProjectLead and managedProjectNames properties in the API that fetches all the admin as well? Or it's okay to only return them in the project lead API? I was able to make it work on the frontend. But, I want to check if both APIs should return the same data structure by having all the same properties. Please take a look at the current changes. Thanks!

cc: @JackHaeg

@JackHaeg
Copy link
Member

Per @trillium - Based on your recent Slack conversation, please reach out to Nikhil (@ntrehan) for additional information. Trillium stated that he trusts your decision to get the API working and moving forward.

@vorleakyek
Copy link
Member Author

@ntrehan, @jng34, could you please take a look at the latest changes in the PR? Thanks!

@vorleakyek vorleakyek requested a review from jng34 February 20, 2025 22:16
for (const projectId of projectManagerObj.managedProjects) {
const projectDetail = await Project.findById(projectId);
if (projectDetail && projectDetail.name) {
projectNames.push(projectDetail.name);
Copy link
Member

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?

Copy link
Member Author

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
image

The API for the project leads is returning this data object
image

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.

Copy link
Member

Choose a reason for hiding this comment

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

@vorleakyek

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your input @jng34!
@ntrehan, could you tell us what you think since you worked on the fetch admin API ticket?

Copy link
Member Author

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.

@jng34
Copy link
Member

jng34 commented Feb 22, 2025

@vorleakyek
I'm having trouble running the changes locally on my end.
However, the new code addressed the proposed changes as far as I can tell!

@jng34
Copy link
Member

jng34 commented Feb 25, 2025

@vorleakyek
I was able to pull down the changes and run them locally.
Everything works as discussed. Looks merge-ready to me!

@trillium
Thank you for the git pointers.

@jng34 jng34 self-requested a review February 25, 2025 04:54
@jng34
Copy link
Member

jng34 commented Feb 25, 2025

@JackHaeg Oops, clicked the self-request by accident!

@vorleakyek
Copy link
Member Author

@jng34, could you please approve the PR since you have already looked at the code? Thank you!

@jng34
Copy link
Member

jng34 commented Feb 25, 2025

@vorleakyek @trillium @JackHaeg
Merging is blocked due to the pending requested changes by Trillium.

@vorleakyek
Copy link
Member Author

@trillium, @ntrehan, can one of you take a look at the PR since it requires 2 approvals to merge? Thanks!
cc: @JackHaeg

@JackHaeg
Copy link
Member

Update - @ntrehan plans to review this PR this week.

@ntrehan ntrehan self-requested a review March 3, 2025 19:18
Copy link
Member

@ntrehan ntrehan left a 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

@JackHaeg
Copy link
Member

JackHaeg commented Mar 3, 2025

@trillium Looks like we have 2 approvals for this, including @ntrehan on this issue. I see that you still have requested changes on this PR. Please let us know if we can go ahead and merge this PR, or if requested changes still need to be implemented.

@trillium trillium dismissed their stale review March 4, 2025 03:23

Fixed

@vorleakyek vorleakyek merged commit 0eba86d into development Mar 4, 2025
2 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants