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

Create separate user endpoints for different purposes (admin, org, project) #2238

Merged
merged 5 commits into from
Mar 4, 2025

Conversation

Sujanadh
Copy link
Collaborator

What type of PR is this? (check all applicable)

  • πŸ• Feature
  • πŸ› Bug Fix
  • πŸ“ Documentation
  • πŸ§‘β€πŸ’» Refactor
  • βœ… Test
  • πŸ€– Build or CI
  • ❓ Other (please specify)

Related Issue

  • Previously, the project admin role was used to fetch the users list, thinking that one project admin can add another project admin but during project creation we won't have project id to check if the user is project admin or not since to create project they need to be org admin

Describe this PR

  • Update role dependency to org_admin in backend to fetch users list
  • Passed org id as a query param from frontend

Screenshots

N/A

Alternative Approaches Considered

Did you attempt any other approaches that are not documented in code?

Review Guide

Notes for the reviewer. How to test this change?

Checklist before requesting a review

[optional] What gif best describes this PR or how it makes you feel?

@Sujanadh Sujanadh requested a review from spwoodcock February 28, 2025 12:12
@Sujanadh Sujanadh self-assigned this Feb 28, 2025
@github-actions github-actions bot added bug Something isn't working frontend Related to frontend code backend Related to backend code labels Feb 28, 2025
Copy link
Member

@spwoodcock spwoodcock left a comment

Choose a reason for hiding this comment

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

We need both the capability to access all users on FMTM (admin only), plus also users per project (project manager) I think?

So either we need a separate endpoint for these cases, or we need another way to handle this with one endpoint.

I don't think this change will work for both πŸ˜…

@Sujanadh
Copy link
Collaborator Author

Sujanadh commented Mar 3, 2025

We also need to fetch all the users while assigning during project creation. So that would need org admin access. Like you said to fetch all the users by admin, our role management doesn't work even if I am super admin; I would need an org id or project id if the roles are org admin or project admin. I think we need to work around this if he is super admin; he shouldn't have any dependency on org and projects. That being said, it will check for all the users if they are super admin or not, which is not great. Need to rethink a way to allow that.

@spwoodcock
Copy link
Member

spwoodcock commented Mar 3, 2025

I would propose we have three endpoints:

  • / the GET for the root of the users router makes sense being admin only. Return all user details.
  • /usernames?org_id=1 - org admin permission for project creation. Update the response model to only return user ids and usernames to display.
  • /roles?project_id=1 - project management, return user ids, usernames, and role (a different response model again)

We can keep the role management the same this way πŸ‘

@Sujanadh
Copy link
Collaborator Author

Sujanadh commented Mar 4, 2025

Updates:

  • changed user role back to super admin in /users endpoint
  • created new endpoint users/usernames to fetch user id and username to assign project admin : role -> org_admin
  • created new endpoint /users/{project_id}/project-users to fetch users from the project and their roles

Response:

/users/usernames

[
  {
    "id": 1,
    "username": "localadmin"
  },
  {
    "id": 20386219,
    "username": "svcfmtm"
  }
]

/users/{project_id}/project-users/

[
  {
    "user_id": 1,
    "project_id": 143,
    "role": "PROJECT_MANAGER"
  }
]

@@ -64,7 +79,7 @@ async def get_user_roles(current_user: Annotated[DbUser, Depends(mapper)]):
async def update_existing_user(
user_id: int,
new_user_data: user_schemas.UserUpdate,
current_user: Annotated[DbUser, Depends(super_admin)],
_: Annotated[DbUser, Depends(super_admin)],
Copy link
Member

Choose a reason for hiding this comment

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

I like this approach for showing unused vars, similar to other languages

@spwoodcock spwoodcock changed the title Change user role to org admin in user list endpoint Create separate user endpoints for different purposes (admin, org, project) Mar 4, 2025
@spwoodcock spwoodcock merged commit 2039176 into development Mar 4, 2025
8 of 9 checks passed
@spwoodcock spwoodcock deleted the fix/assign-prj-admin branch March 4, 2025 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Related to backend code bug Something isn't working frontend Related to frontend code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants