-
Notifications
You must be signed in to change notification settings - Fork 14
feat: create teams #112
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
feat: create teams #112
Changes from all commits
706782e
9db5246
191735a
e02e1c8
013eea7
45d29fb
e614f3c
28cb7d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from pydantic import BaseModel | ||
from todo.dto.team_dto import TeamDTO | ||
|
||
|
||
class CreateTeamResponse(BaseModel): | ||
team: TeamDTO | ||
message: str | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
from pydantic import BaseModel, validator | ||
from typing import List, Optional | ||
from datetime import datetime | ||
from todo.repositories.user_repository import UserRepository | ||
|
||
|
||
class CreateTeamDTO(BaseModel): | ||
name: str | ||
description: Optional[str] = None | ||
member_ids: Optional[List[str]] = None | ||
poc_id: str | ||
|
||
@validator("member_ids") | ||
def validate_member_ids(cls, value): | ||
"""Validate that all member IDs exist in the database.""" | ||
if value is None: | ||
return value | ||
|
||
invalid_ids = [] | ||
for member_id in value: | ||
user = UserRepository.get_by_id(member_id) | ||
if not user: | ||
invalid_ids.append(member_id) | ||
|
||
if invalid_ids: | ||
raise ValueError(f"Invalid member IDs: {invalid_ids}") | ||
return value | ||
|
||
@validator("poc_id") | ||
def validate_poc_id(cls, value): | ||
"""Validate that the POC ID exists in the database.""" | ||
if value is None: | ||
return value | ||
|
||
user = UserRepository.get_by_id(value) | ||
if not user: | ||
raise ValueError(f"Invalid POC ID: {value}") | ||
return value | ||
|
||
|
||
class TeamDTO(BaseModel): | ||
id: str | ||
name: str | ||
description: Optional[str] = None | ||
poc_id: str | ||
created_by: str | ||
updated_by: str | ||
lakshayman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
created_at: datetime | ||
updated_at: datetime |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
from pydantic import Field, validator | ||
from typing import ClassVar | ||
from datetime import datetime, timezone | ||
|
||
from todo.models.common.document import Document | ||
from todo.models.common.pyobjectid import PyObjectId | ||
|
||
|
||
class TeamModel(Document): | ||
""" | ||
Model for teams. | ||
""" | ||
|
||
collection_name: ClassVar[str] = "teams" | ||
|
||
name: str = Field(..., min_length=1, max_length=100) | ||
description: str | None = None | ||
poc_id: PyObjectId | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing POC ID Validation
Tell me moreWhat is the issue?The poc_id field in TeamModel lacks validation unlike other ObjectId fields. Why this mattersInvalid poc_id values could be accepted, potentially causing runtime errors when interacting with the database. Suggested change ∙ Feature PreviewAdd poc_id to the validator decorator in TeamModel: @validator("created_by", "updated_by", "poc_id")
def validate_user_id(cls, v):
"""Validate that the user ID is a valid ObjectId format."""
if v is None:
raise ValueError("User ID cannot be None")
if not PyObjectId.is_valid(v):
raise ValueError(f"Invalid user ID format: {v}")
return v Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
created_by: PyObjectId | ||
updated_by: PyObjectId | ||
created_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) | ||
updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) | ||
is_deleted: bool = False | ||
|
||
@validator("created_by", "updated_by") | ||
def validate_user_id(cls, v): | ||
"""Validate that the user ID is a valid ObjectId format.""" | ||
if v is None: | ||
raise ValueError("User ID cannot be None") | ||
if not PyObjectId.is_valid(v): | ||
raise ValueError(f"Invalid user ID format: {v}") | ||
Comment on lines
+28
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Field Context in Validation Error
Tell me moreWhat is the issue?Error messages in the TeamModel validator lack context about which field failed validation Why this mattersWhen validation fails, the generic error message makes it harder to identify whether it was created_by or updated_by that caused the failure during debugging Suggested change ∙ Feature Preview@validator("created_by", "updated_by")
def validate_user_id(cls, v, field):
if v is None:
raise ValueError(f"{field.name} cannot be None")
if not PyObjectId.is_valid(v):
raise ValueError(f"Invalid {field.name} format: {v}")
return v Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
return v | ||
Comment on lines
+25
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate ObjectId Validation Logic
Tell me moreWhat is the issue?Duplicate validation logic between TeamModel.validate_user_id and UserTeamDetailsModel.validate_object_ids for ObjectId fields. Why this mattersCode duplication increases maintenance burden and risk of inconsistencies when changes are needed to the validation logic. Suggested change ∙ Feature PreviewExtract the common validation logic into a shared mixin class or utility function: class ObjectIdValidatorMixin:
@classmethod
def validate_object_id(cls, v, field_name="ObjectId"):
if v is None:
raise ValueError(f"{field_name} cannot be None")
if not PyObjectId.is_valid(v):
raise ValueError(f"Invalid {field_name} format: {v}")
return v
class TeamModel(Document, ObjectIdValidatorMixin):
@validator("created_by", "updated_by")
def validate_user_id(cls, v):
return cls.validate_object_id(v, "User ID") Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
|
||
|
||
class UserTeamDetailsModel(Document): | ||
""" | ||
Model for user-team relationships. | ||
""" | ||
|
||
collection_name: ClassVar[str] = "user_team_details" | ||
|
||
user_id: PyObjectId | ||
team_id: PyObjectId | ||
is_active: bool = True | ||
role_id: str | ||
lakshayman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
created_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) | ||
updated_at: datetime = Field(default_factory=lambda: datetime.now(timezone.utc)) | ||
created_by: PyObjectId | ||
updated_by: PyObjectId | ||
|
||
@validator("user_id", "team_id", "created_by", "updated_by") | ||
def validate_object_ids(cls, v): | ||
"""Validate that the ObjectId fields are in valid format.""" | ||
if v is None: | ||
raise ValueError("ObjectId cannot be None") | ||
if not PyObjectId.is_valid(v): | ||
raise ValueError(f"Invalid ObjectId format: {v}") | ||
return v |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
from datetime import datetime, timezone | ||
from typing import Optional | ||
from bson import ObjectId | ||
|
||
from todo.models.team import TeamModel, UserTeamDetailsModel | ||
from todo.repositories.common.mongo_repository import MongoRepository | ||
|
||
|
||
class TeamRepository(MongoRepository): | ||
collection_name = TeamModel.collection_name | ||
|
||
@classmethod | ||
def create(cls, team: TeamModel) -> TeamModel: | ||
""" | ||
Creates a new team in the repository. | ||
""" | ||
teams_collection = cls.get_collection() | ||
team.created_at = datetime.now(timezone.utc) | ||
team.updated_at = datetime.now(timezone.utc) | ||
|
||
team_dict = team.model_dump(mode="json", by_alias=True, exclude_none=True) | ||
insert_result = teams_collection.insert_one(team_dict) | ||
team.id = insert_result.inserted_id | ||
return team | ||
|
||
@classmethod | ||
def get_by_id(cls, team_id: str) -> Optional[TeamModel]: | ||
""" | ||
Get a team by its ID. | ||
""" | ||
teams_collection = cls.get_collection() | ||
try: | ||
team_data = teams_collection.find_one({"_id": ObjectId(team_id), "is_deleted": False}) | ||
if team_data: | ||
return TeamModel(**team_data) | ||
return None | ||
except Exception: | ||
return None | ||
lakshayman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class UserTeamDetailsRepository(MongoRepository): | ||
collection_name = UserTeamDetailsModel.collection_name | ||
|
||
@classmethod | ||
def create(cls, user_team: UserTeamDetailsModel) -> UserTeamDetailsModel: | ||
""" | ||
Creates a new user-team relationship. | ||
""" | ||
collection = cls.get_collection() | ||
user_team.created_at = datetime.now(timezone.utc) | ||
user_team.updated_at = datetime.now(timezone.utc) | ||
|
||
user_team_dict = user_team.model_dump(mode="json", by_alias=True, exclude_none=True) | ||
insert_result = collection.insert_one(user_team_dict) | ||
user_team.id = insert_result.inserted_id | ||
return user_team | ||
|
||
@classmethod | ||
def create_many(cls, user_teams: list[UserTeamDetailsModel]) -> list[UserTeamDetailsModel]: | ||
""" | ||
Creates multiple user-team relationships. | ||
""" | ||
collection = cls.get_collection() | ||
current_time = datetime.now(timezone.utc) | ||
|
||
for user_team in user_teams: | ||
user_team.created_at = current_time | ||
user_team.updated_at = current_time | ||
|
||
user_teams_dicts = [ | ||
user_team.model_dump(mode="json", by_alias=True, exclude_none=True) for user_team in user_teams | ||
] | ||
insert_result = collection.insert_many(user_teams_dicts) | ||
|
||
# Set the inserted IDs | ||
for i, user_team in enumerate(user_teams): | ||
user_team.id = insert_result.inserted_ids[i] | ||
|
||
return user_teams |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from rest_framework import serializers | ||
|
||
|
||
class CreateTeamSerializer(serializers.Serializer): | ||
name = serializers.CharField(max_length=100) | ||
description = serializers.CharField(max_length=500, required=False, allow_blank=True) | ||
member_ids = serializers.ListField(child=serializers.CharField(), required=False, default=list) | ||
poc_id = serializers.CharField(required=True, allow_null=False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unclear Abbreviation in Variable Name
Tell me moreWhat is the issue?The variable name 'poc_id' uses an unclear abbreviation that may not be immediately understood by all developers. Why this mattersUsing abbreviations in code reduces readability and requires developers to context switch or search for the meaning, slowing down code comprehension. Suggested change ∙ Feature Previewpoint_of_contact_id = serializers.CharField(required=True, allow_null=False) Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit.
Comment on lines
+4
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing critical field context in serializer
Tell me moreWhat is the issue?The serializer class lacks a docstring explaining its purpose and the significance of the poc_id field which appears to be a critical required field. Why this mattersWithout understanding what poc_id represents, future maintainers might incorrectly modify its requirements or misuse the serializer. Suggested change ∙ Feature Previewclass CreateTeamSerializer(serializers.Serializer):
Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
from todo.dto.team_dto import CreateTeamDTO, TeamDTO | ||
from todo.dto.responses.create_team_response import CreateTeamResponse | ||
from todo.models.team import TeamModel, UserTeamDetailsModel | ||
from todo.models.common.pyobjectid import PyObjectId | ||
from todo.repositories.team_repository import TeamRepository, UserTeamDetailsRepository | ||
from todo.constants.messages import AppMessages | ||
|
||
|
||
class TeamService: | ||
@classmethod | ||
def create_team(cls, dto: CreateTeamDTO, created_by_user_id: str) -> CreateTeamResponse: | ||
""" | ||
Create a new team with members and POC. | ||
""" | ||
Comment on lines
+10
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insufficient method documentation
Tell me moreWhat is the issue?The docstring only repeats what the method name already conveys and lacks critical information about parameters, return value, and key business logic decisions. Why this mattersMissing information about parameters and behavior makes it harder for other developers to understand important aspects like automatic team member additions and role assignments. Suggested change ∙ Feature Preview
Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
try: | ||
# Member IDs and POC ID validation is handled at DTO level | ||
member_ids = dto.member_ids or [] | ||
|
||
# Create team | ||
team = TeamModel( | ||
name=dto.name, | ||
description=dto.description, | ||
poc_id=PyObjectId(dto.poc_id) if dto.poc_id else None, | ||
created_by=PyObjectId(created_by_user_id), | ||
updated_by=PyObjectId(created_by_user_id), | ||
) | ||
|
||
created_team = TeamRepository.create(team) | ||
|
||
# Create user-team relationships | ||
user_teams = [] | ||
|
||
# Add members to the team | ||
if member_ids: | ||
for user_id in member_ids: | ||
user_team = UserTeamDetailsModel( | ||
user_id=PyObjectId(user_id), | ||
team_id=created_team.id, | ||
role_id="1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Consider making role_id configurable instead of hardcoded. The hardcoded +# At the top of the file
+DEFAULT_TEAM_ROLE_ID = "1"
# In the service method
- role_id="1",
+ role_id=DEFAULT_TEAM_ROLE_ID, Or consider making it a parameter of the service method if different roles should be supported. Also applies to: 60-60 🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Insecure Role Assignment Using Hardcoded Values
Tell me moreWhat is the issue?Hardcoded role IDs ('1' and '2') are used to assign user roles in teams without validation or constant definitions. Why this mattersUsing magic string values for role assignments could lead to authorization vulnerabilities if these values are mistyped or if the role system changes. An attacker could potentially gain elevated privileges if role IDs are not properly validated. Suggested change ∙ Feature PreviewDefine role IDs as validated enums or constants, and add role validation: from enum import Enum
class TeamRoles(Enum):
MEMBER = "1"
POC = "2"
# Then use:
role_id=TeamRoles.MEMBER.value
role_id=TeamRoles.POC.value Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
created_by=PyObjectId(created_by_user_id), | ||
updated_by=PyObjectId(created_by_user_id), | ||
) | ||
user_teams.append(user_team) | ||
|
||
# Add POC if not already in member_ids | ||
if dto.poc_id and dto.poc_id not in member_ids: | ||
poc_user_team = UserTeamDetailsModel( | ||
user_id=PyObjectId(dto.poc_id), | ||
team_id=created_team.id, | ||
role_id="2", # POC role | ||
created_by=PyObjectId(created_by_user_id), | ||
updated_by=PyObjectId(created_by_user_id), | ||
) | ||
user_teams.append(poc_user_team) | ||
|
||
# Add creator if not already in member_ids | ||
if created_by_user_id not in member_ids: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent ID Type Comparison
Tell me moreWhat is the issue?The code compares a string (created_by_user_id) with a list of strings (member_ids) directly, but earlier in the code PyObjectId is used for user IDs. Why this mattersThis comparison will always evaluate to True since string and PyObjectId types never match, causing the creator to be added as a duplicate team member. Suggested change ∙ Feature PreviewConvert created_by_user_id to str for comparison with member_ids: if str(created_by_user_id) not in member_ids: Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
creator_user_team = UserTeamDetailsModel( | ||
user_id=PyObjectId(created_by_user_id), | ||
team_id=created_team.id, | ||
role_id="1", | ||
created_by=PyObjectId(created_by_user_id), | ||
updated_by=PyObjectId(created_by_user_id), | ||
) | ||
user_teams.append(creator_user_team) | ||
|
||
# Create all user-team relationships | ||
if user_teams: | ||
UserTeamDetailsRepository.create_many(user_teams) | ||
|
||
# Convert to DTO | ||
team_dto = TeamDTO( | ||
id=str(created_team.id), | ||
name=created_team.name, | ||
description=created_team.description, | ||
poc_id=str(created_team.poc_id) if created_team.poc_id else None, | ||
created_by=str(created_team.created_by), | ||
updated_by=str(created_team.updated_by), | ||
created_at=created_team.created_at, | ||
updated_at=created_team.updated_at, | ||
) | ||
|
||
return CreateTeamResponse(team=team_dto, message=AppMessages.TEAM_CREATED) | ||
|
||
except Exception as e: | ||
raise ValueError(str(e)) | ||
Comment on lines
+85
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loss of Exception Context
Tell me moreWhat is the issue?The exception handling wraps all exceptions into ValueError, losing the original exception type and stack trace. Why this mattersThis makes debugging harder and masks important error information that could help identify the root cause of failures in team creation. Suggested change ∙ Feature PreviewEither handle specific exceptions or re-raise the original exception: except Exception as e:
raise # This preserves the original exception and stack trace Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
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.
Missing Response Model Documentation
Tell me more
What is the issue?
The response model lacks documentation explaining its purpose and the meaning of its fields.
Why this matters
Without clear documentation, developers integrating with this API response model will have to review implementation code to understand what values to expect in each field.
Suggested change ∙ Feature Preview
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.