feat: add functionality to retrieve users by team ID and update team …#151
feat: add functionality to retrieve users by team ID and update team …#151iamitprakash merged 1 commit intoRealDevSquad:developfrom amit-flx:teamdetails
Conversation
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
WalkthroughA method to retrieve users by team ID was added to the team repository, and a method to fetch user details by a list of IDs was added to the user service. The team detail API endpoint now supports an optional query parameter to return team members instead of team details, with corresponding schema updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TeamDetailView
participant UserTeamDetailsRepository
participant UserService
participant UserRepository
Client->>TeamDetailView: GET /team/{team_id}?member=true
TeamDetailView->>UserTeamDetailsRepository: get_users_by_team_id(team_id)
UserTeamDetailsRepository-->>TeamDetailView: [user_id_1, user_id_2, ...]
TeamDetailView->>UserService: get_users_by_ids([user_id_1, user_id_2, ...])
UserService->>UserRepository: get_by_id(user_id)
UserRepository-->>UserService: UserModel
UserService-->>TeamDetailView: [UserDTO, ...]
TeamDetailView-->>Client: [UserDTO, ...]
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Missing Input Validation ▹ view | |
 |
Inefficient Memory Usage in Result Processing ▹ view | |
 |
Missing Authorization Check in Bulk User Data Retrieval ▹ view | |
 |
Non-Pythonic iteration pattern ▹ view | |
 |
Silent Failure in Bulk User Retrieval ▹ view | |
 |
Hidden Repository Dependency ▹ view | |
|
Missing team existence validation ▹ view | |
|
Silent Exception Handling ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/repositories/team_repository.py | ✅ |
| todo/services/user_service.py | ✅ |
| todo/views/team.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| user = UserRepository.get_by_id(user_id) | ||
| if user: | ||
| users.append(UserDTO( | ||
| id=str(user.id), | ||
| name=user.name, | ||
| email_id=user.email_id, | ||
| created_at=user.created_at, | ||
| updated_at=user.updated_at, | ||
| )) |
There was a problem hiding this comment.
Missing Authorization Check in Bulk User Data Retrieval 
Tell me more
What is the issue?
The get_users_by_ids method exposes user data without any authentication or authorization checks
Why this matters
Without proper access control checks, sensitive user information like email addresses could be exposed to unauthorized users, leading to potential privacy violations and data breaches
Suggested change ∙ Feature Preview
@classmethod
def get_users_by_ids(cls, user_ids: list[str], requesting_user: UserModel) -> list[UserDTO]:
if not requesting_user.has_permission('view_user_details'):
raise PermissionDenied()
# ... rest of the methodProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| @classmethod | ||
| def get_users_by_ids(cls, user_ids: list[str]) -> list[UserDTO]: | ||
| users = [] | ||
| for user_id in user_ids: | ||
| user = UserRepository.get_by_id(user_id) | ||
| if user: | ||
| users.append(UserDTO( | ||
| id=str(user.id), | ||
| name=user.name, | ||
| email_id=user.email_id, | ||
| created_at=user.created_at, | ||
| updated_at=user.updated_at, | ||
| )) |
There was a problem hiding this comment.
Non-Pythonic iteration pattern 
Tell me more
What is the issue?
The method performs unnecessary sequential database queries in a loop, which could be converted to a clearer list comprehension or filter pattern.
Why this matters
The current implementation is harder to read and understand at a glance. A list comprehension would make the intent clearer and follow Python idioms better.
Suggested change ∙ Feature Preview
@classmethod
def get_users_by_ids(cls, user_ids: list[str]) -> list[UserDTO]:
return [
UserDTO(
id=str(user.id),
name=user.name,
email_id=user.email_id,
created_at=user.created_at,
updated_at=user.updated_at
)
for user_id in user_ids
if (user := UserRepository.get_by_id(user_id))
]Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| @classmethod | ||
| def get_users_by_ids(cls, user_ids: list[str]) -> list[UserDTO]: | ||
| users = [] | ||
| for user_id in user_ids: | ||
| user = UserRepository.get_by_id(user_id) | ||
| if user: | ||
| users.append(UserDTO( | ||
| id=str(user.id), | ||
| name=user.name, | ||
| email_id=user.email_id, | ||
| created_at=user.created_at, | ||
| updated_at=user.updated_at, | ||
| )) | ||
| return users |
There was a problem hiding this comment.
Silent Failure in Bulk User Retrieval 
Tell me more
What is the issue?
The method silently skips invalid or non-existent user IDs without any indication to the caller.
Why this matters
The caller won't know which user IDs failed to be retrieved, making it difficult to handle partial failures or debug issues in production.
Suggested change ∙ Feature Preview
Modify the method to either raise an exception for invalid IDs or return both successful and failed results:
@classmethod
def get_users_by_ids(cls, user_ids: list[str]) -> tuple[list[UserDTO], list[str]]:
users = []
failed_ids = []
for user_id in user_ids:
user = UserRepository.get_by_id(user_id)
if user:
users.append(UserDTO(
id=str(user.id),
name=user.name,
email_id=user.email_id,
created_at=user.created_at,
updated_at=user.updated_at,
))
else:
failed_ids.append(user_id)
return users, failed_idsProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| @classmethod | ||
| def get_users_by_ids(cls, user_ids: list[str]) -> list[UserDTO]: |
There was a problem hiding this comment.
Missing Input Validation 
Tell me more
What is the issue?
The method lacks input validation for the user_ids parameter.
Why this matters
Invalid input like empty lists, None values, or invalid ID formats could cause runtime errors or unexpected behavior.
Suggested change ∙ Feature Preview
Add input validation at the start of the method:
@classmethod
def get_users_by_ids(cls, user_ids: list[str]) -> list[UserDTO]:
if not user_ids:
raise DRFValidationError({'user_ids': 'User IDs list cannot be empty'})
if not isinstance(user_ids, list):
raise DRFValidationError({'user_ids': 'User IDs must be provided as a list'})
if not all(isinstance(uid, str) for uid in user_ids):
raise DRFValidationError({'user_ids': 'All user IDs must be strings'})Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| try: | ||
| team_dto: TeamDTO = TeamService.get_team_by_id(team_id) | ||
| return Response(data=team_dto.model_dump(mode="json"), status=status.HTTP_200_OK) | ||
| member = request.query_params.get("member", "false").lower() == "true" |
There was a problem hiding this comment.
Hidden Repository Dependency 
Tell me more
What is the issue?
The code uses a local import for UserTeamDetailsRepository inside the if condition, which could lead to circular import issues and make the dependency unclear.
Why this matters
Local imports can mask dependency issues and make the code harder to maintain. If there's a circular import, it might only be discovered at runtime when that specific code path is executed.
Suggested change ∙ Feature Preview
Move the import to the top of the file with other imports and explicitly handle any circular dependencies:
from todo.repositories.team_repository import UserTeamDetailsRepository
class TeamDetailView(APIView):
def get(self, request: Request, team_id: str):
try:
member = request.query_params.get("member", "false").lower() == "true"
if member:
user_ids = UserTeamDetailsRepository.get_users_by_team_id(team_id)
users = UserService.get_users_by_ids(user_ids)
return Response(data=[user.model_dump(mode="json") for user in users], status=status.HTTP_200_OK)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| collection = cls.get_collection() | ||
| try: | ||
| user_teams_data = collection.find({"team_id": ObjectId(team_id), "is_active": True}) | ||
| return [str(data["user_id"]) for data in user_teams_data] |
There was a problem hiding this comment.
Inefficient Memory Usage in Result Processing 
Tell me more
What is the issue?
List comprehension materializes entire result set in memory before processing.
Why this matters
For large teams, this approach loads all user records into memory at once, which could cause memory spikes.
Suggested change ∙ Feature Preview
Use a generator expression to process results lazily:
return (str(data["user_id"]) for data in user_teams_data)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| user_teams_data = collection.find({"team_id": ObjectId(team_id), "is_active": True}) | ||
| return [str(data["user_id"]) for data in user_teams_data] |
There was a problem hiding this comment.
Missing team existence validation
Tell me more
What is the issue?
The get_users_by_team_id method does not validate if the team_id exists before attempting to fetch users
Why this matters
If a non-existent team_id is provided, the method will still return an empty list instead of indicating the error condition, potentially masking invalid team queries from the caller
Suggested change ∙ Feature Preview
Add team existence check before fetching users:
@classmethod
def get_users_by_team_id(cls, team_id: str) -> list[str]:
"""
Get all user IDs for a specific team.
"""
collection = cls.get_collection()
try:
# First check if team exists
team = TeamRepository.get_by_id(team_id)
if not team:
return [] # or raise a specific exception
user_teams_data = collection.find({"team_id": ObjectId(team_id), "is_active": True})
return [str(data["user_id"]) for data in user_teams_data]
except Exception:
return []Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| try: | ||
| user_teams_data = collection.find({"team_id": ObjectId(team_id), "is_active": True}) | ||
| return [str(data["user_id"]) for data in user_teams_data] | ||
| except Exception: | ||
| return [] |
There was a problem hiding this comment.
Silent Exception Handling
Tell me more
What is the issue?
Using a bare except Exception clause without logging or providing context about what went wrong
Why this matters
Silent failure makes debugging production issues extremely difficult as there's no trace of what caused the error (invalid team_id, database connection issues, etc.)
Suggested change ∙ Feature Preview
try:
user_teams_data = collection.find({"team_id": ObjectId(team_id), "is_active": True})
return [str(data["user_id"]) for data in user_teams_data]
except Exception as e:
logger.error(f"Failed to get users for team {team_id}: {str(e)}")
return []Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
2 participants
Description by Korbit AI
What change is being made?
Add functionality to retrieve users by team ID in the
team_repositoryand update theteamview to include an optional query parameter for returning team members.Why are these changes being made?
These changes improve the ability to manage team-related data by enabling the backend to fetch user IDs associated with a given team and provide this information through the API. This approach allows for more flexible client requests which can optionally return either team details or the list of team members, enhancing the user's ability to manage and access team-specific data as needed.