-
Notifications
You must be signed in to change notification settings - Fork 14
Dev to Main Sync #277
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
Dev to Main Sync #277
Changes from all commits
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,22 @@ | ||
from todo.constants.messages import ApiErrors | ||
|
||
|
||
class BaseTeamException(Exception): | ||
def __init__(self, message: str): | ||
self.message = message | ||
super().__init__(self.message) | ||
|
||
|
||
class CannotRemoveOwnerException(BaseTeamException): | ||
def __init__(self, message=ApiErrors.CANNOT_REMOVE_OWNER): | ||
super().__init__(message) | ||
|
||
|
||
class NotTeamAdminException(BaseTeamException): | ||
def __init__(self, message=ApiErrors.UNAUTHORIZED_TITLE): | ||
super().__init__(message) | ||
|
||
|
||
class CannotRemoveTeamPOCException(BaseTeamException): | ||
def __init__(self, message=ApiErrors.CANNOT_REMOVE_POC): | ||
super().__init__(message) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
# Generated by Django 5.1.5 on 2025-09-09 14:49 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
dependencies = [ | ||
("todo", "0002_rename_postgres_ta_assignee_95ca3b_idx_postgres_ta_assigne_f1c6e7_idx_and_more"), | ||
] | ||
|
||
operations = [ | ||
migrations.AlterUniqueTogether( | ||
name="postgresuserrole", | ||
unique_together=set(), | ||
), | ||
migrations.AddConstraint( | ||
model_name="postgresuserrole", | ||
constraint=models.UniqueConstraint( | ||
condition=models.Q(("is_active", True)), | ||
fields=("user_id", "role_name", "scope", "team_id"), | ||
name="unique_active_user_team_role", | ||
), | ||
), | ||
Comment on lines
+16
to
+23
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 Migration Rationale
Tell me moreWhat is the issue?The migration lacks a comment explaining the business reason for replacing unique_together with a conditional unique constraint. Why this mattersFuture developers may not understand why this specific database constraint change was necessary, making it harder to maintain or modify the constraint logic. Suggested change ∙ Feature PreviewReplace unique_together with conditional unique constraint to allow multiple inactive roleswhile ensuring only one active role per user-team-role-scope combinationProvide 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 | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,7 +6,9 @@ | |||||||||
from todo.models.task_assignment import TaskAssignmentModel | ||||||||||
from todo.repositories.common.mongo_repository import MongoRepository | ||||||||||
from todo.models.common.pyobjectid import PyObjectId | ||||||||||
from todo.constants.task import TaskStatus | ||||||||||
from todo.services.enhanced_dual_write_service import EnhancedDualWriteService | ||||||||||
from todo.repositories.audit_log_repository import AuditLogRepository, AuditLogModel | ||||||||||
|
||||||||||
|
||||||||||
class TaskAssignmentRepository(MongoRepository): | ||||||||||
|
@@ -355,3 +357,167 @@ def deactivate_by_task_id(cls, task_id: str, user_id: str) -> bool: | |||||||||
return result.modified_count > 0 | ||||||||||
except Exception: | ||||||||||
return False | ||||||||||
|
||||||||||
@classmethod | ||||||||||
def reassign_tasks_from_user_to_team(cls, user_id: str, team_id: str, performed_by_user_id: str) -> bool: | ||||||||||
""" | ||||||||||
Reassign all tasks of user to team | ||||||||||
""" | ||||||||||
collection = cls.get_collection() | ||||||||||
client = cls.get_client() | ||||||||||
with client.start_session() as session: | ||||||||||
try: | ||||||||||
with session.start_transaction(): | ||||||||||
now = datetime.now(timezone.utc) | ||||||||||
user_task_assignments = list( | ||||||||||
collection.find( | ||||||||||
{ | ||||||||||
"$and": [ | ||||||||||
{"is_active": True}, | ||||||||||
{ | ||||||||||
"$or": [{"assignee_id": user_id}, {"assignee_id": ObjectId(user_id)}], | ||||||||||
}, | ||||||||||
{"$or": [{"team_id": team_id}, {"team_id": ObjectId(team_id)}]}, | ||||||||||
] | ||||||||||
}, | ||||||||||
session=session, | ||||||||||
) | ||||||||||
) | ||||||||||
if not user_task_assignments: | ||||||||||
return 0 | ||||||||||
Comment on lines
+386
to
+387
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. Bug: Function returns 0 when no assignments found, but the function signature indicates it should return bool. This type inconsistency will cause issues for callers expecting boolean. Should return False instead of 0.
Suggested change
Spotted by Diamond |
||||||||||
active_user_task_assignments_ids = [ | ||||||||||
ObjectId(assignment["task_id"]) for assignment in user_task_assignments | ||||||||||
] | ||||||||||
|
||||||||||
from todo.repositories.task_repository import TaskRepository | ||||||||||
|
||||||||||
tasks_collection = TaskRepository.get_collection() | ||||||||||
active_tasks = list( | ||||||||||
tasks_collection.find( | ||||||||||
{ | ||||||||||
"_id": {"$in": active_user_task_assignments_ids}, | ||||||||||
"status": {"$ne": TaskStatus.DONE.value}, | ||||||||||
}, | ||||||||||
session=session, | ||||||||||
) | ||||||||||
) | ||||||||||
not_done_tasks_ids = [str(tasks["_id"]) for tasks in active_tasks] | ||||||||||
tasks_to_reset_status_ids = [] | ||||||||||
tasks_to_clear_deferred_ids = [] | ||||||||||
for tasks in active_tasks: | ||||||||||
if tasks["status"] == TaskStatus.IN_PROGRESS.value: | ||||||||||
tasks_to_reset_status_ids.append(tasks["_id"]) | ||||||||||
elif tasks.get("deferredDetails") is not None: | ||||||||||
tasks_to_clear_deferred_ids.append(tasks["_id"]) | ||||||||||
Comment on lines
+405
to
+411
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. Misleading Iterator Variable Name
Tell me moreWhat is the issue?Variable name 'tasks' is used in singular form when iterating over 'active_tasks', suggesting it represents multiple tasks when it actually represents a single task. Why this mattersMisleading variable names can cause confusion about data structures and their contents. Suggested change ∙ Feature Previewtasks_to_reset_status_ids = []
tasks_to_clear_deferred_ids = []
for task in active_tasks:
if task["status"] == TaskStatus.IN_PROGRESS.value:
tasks_to_reset_status_ids.append(task["_id"])
elif task.get("deferredDetails") is not None:
tasks_to_clear_deferred_ids.append(task["_id"]) Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||||||||||
|
||||||||||
collection.update_many( | ||||||||||
{ | ||||||||||
"task_id": {"$in": not_done_tasks_ids}, | ||||||||||
}, | ||||||||||
{ | ||||||||||
"$set": { | ||||||||||
"assignee_id": team_id, | ||||||||||
"user_type": "team", | ||||||||||
"updated_at": now, | ||||||||||
"updated_by": ObjectId(performed_by_user_id), | ||||||||||
} | ||||||||||
}, | ||||||||||
session=session, | ||||||||||
) | ||||||||||
|
||||||||||
for assignment in user_task_assignments: | ||||||||||
AuditLogRepository.create( | ||||||||||
AuditLogModel( | ||||||||||
task_id=PyObjectId(assignment["task_id"]), | ||||||||||
team_id=PyObjectId(team_id), | ||||||||||
action="assigned_to_team", | ||||||||||
performed_by=PyObjectId(performed_by_user_id), | ||||||||||
) | ||||||||||
) | ||||||||||
|
||||||||||
tasks_collection.update_many( | ||||||||||
{"_id": {"$in": tasks_to_reset_status_ids}}, | ||||||||||
{ | ||||||||||
"$set": { | ||||||||||
"status": TaskStatus.TODO.value, | ||||||||||
"updated_at": now, | ||||||||||
"updated_by": ObjectId(performed_by_user_id), | ||||||||||
} | ||||||||||
}, | ||||||||||
session=session, | ||||||||||
) | ||||||||||
tasks_collection.update_many( | ||||||||||
{"_id": {"$in": tasks_to_clear_deferred_ids}}, | ||||||||||
{ | ||||||||||
"$set": { | ||||||||||
"status": TaskStatus.TODO.value, | ||||||||||
"deferredDetails": None, | ||||||||||
"updated_at": now, | ||||||||||
"updated_by": ObjectId(performed_by_user_id), | ||||||||||
} | ||||||||||
}, | ||||||||||
session=session, | ||||||||||
) | ||||||||||
|
||||||||||
tasks_by_id = {task["_id"]: task for task in active_tasks} | ||||||||||
operations = [] | ||||||||||
dual_write_service = EnhancedDualWriteService() | ||||||||||
for assignment in user_task_assignments: | ||||||||||
operations.append( | ||||||||||
{ | ||||||||||
"collection_name": "task_assignments", | ||||||||||
"operation": "update", | ||||||||||
"mongo_id": assignment["_id"], | ||||||||||
"data": { | ||||||||||
"task_mongo_id": str(assignment["task_id"]), | ||||||||||
"assignee_id": str(assignment["team_id"]), | ||||||||||
"user_type": "team", | ||||||||||
"team_id": str(assignment["team_id"]), | ||||||||||
"is_active": True, | ||||||||||
"created_at": assignment["created_at"], | ||||||||||
"created_by": str(assignment["created_by"]), | ||||||||||
"updated_at": datetime.now(timezone.utc), | ||||||||||
"updated_by": str(performed_by_user_id), | ||||||||||
}, | ||||||||||
} | ||||||||||
) | ||||||||||
if ( | ||||||||||
assignment["task_id"] in tasks_to_clear_deferred_ids | ||||||||||
or assignment["task_id"] in tasks_to_reset_status_ids | ||||||||||
): | ||||||||||
task = tasks_by_id[assignment["task_id"]] | ||||||||||
operations.append( | ||||||||||
{ | ||||||||||
"collection_name": "tasks", | ||||||||||
"operation": "update", | ||||||||||
"mongo_id": assignment["task_id"], | ||||||||||
"data": { | ||||||||||
"title": task.get("title"), | ||||||||||
"description": task.get("description"), | ||||||||||
"priority": task.get("priority"), | ||||||||||
"status": TaskStatus.TODO.value, | ||||||||||
"displayId": task.get("displayId"), | ||||||||||
"deferredDetails": None, | ||||||||||
"isAcknowledged": task.get("isAcknowledged", False), | ||||||||||
"isDeleted": task.get("isDeleted", False), | ||||||||||
"startedAt": task.get("startedAt"), | ||||||||||
"dueAt": task.get("dueAt"), | ||||||||||
"createdAt": task.get("createdAt"), | ||||||||||
"createdBy": str(task.get("createdBy")), | ||||||||||
"updatedAt": datetime.now(timezone.utc), | ||||||||||
"updated_by": str(performed_by_user_id), | ||||||||||
}, | ||||||||||
} | ||||||||||
) | ||||||||||
|
||||||||||
dual_write_success = dual_write_service.batch_operations(operations) | ||||||||||
if not dual_write_success: | ||||||||||
import logging | ||||||||||
|
||||||||||
logger = logging.getLogger(__name__) | ||||||||||
logger.warning("Failed to sync task reassignments to Postgres") | ||||||||||
|
||||||||||
return False | ||||||||||
return True | ||||||||||
except Exception: | ||||||||||
return False | ||||||||||
Comment on lines
+522
to
+523
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. Generic exception handling without context
Tell me moreWhat is the issue?Generic catch-all exception handling without logging or contextual information about what went wrong. Why this mattersWhen exceptions occur, no information is captured about the root cause, making debugging and monitoring extremely difficult. The method silently fails without any trace of what actually happened. Suggested change ∙ Feature PreviewAdd logging with contextual information about the error: except Exception as e:
import logging
logger = logging.getLogger(__name__)
logger.error(f"Failed to reassign tasks from user {user_id} to team {team_id}: {str(e)}", exc_info=True)
return False 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,18 @@ | ||
from rest_framework import serializers | ||
from bson import ObjectId | ||
from todo.constants.messages import ValidationErrors | ||
|
||
|
||
class RemoveFromTeamSerializer(serializers.Serializer): | ||
team_id = serializers.CharField() | ||
user_id = serializers.CharField() | ||
|
||
def validate_team_id(self, value): | ||
if not ObjectId.is_valid(value): | ||
raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) | ||
return value | ||
|
||
def validate_user_id(self, value): | ||
if not ObjectId.is_valid(value): | ||
raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) | ||
return value | ||
Comment on lines
+10
to
+18
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?The validation logic for ObjectId is duplicated between validate_team_id and validate_user_id methods. Why this mattersCode duplication increases maintenance burden and risk of inconsistencies when changes are needed. If the ObjectId validation logic needs to change, it would need to be updated in multiple places. Suggested change ∙ Feature PreviewExtract the common validation logic into a reusable method: def validate_object_id(self, value):
if not ObjectId.is_valid(value):
raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value))
return value
def validate_team_id(self, value):
return self.validate_object_id(value)
def validate_user_id(self, value):
return self.validate_object_id(value) 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 |
---|---|---|
|
@@ -150,3 +150,10 @@ def delete_task_assignment(cls, task_id: str, user_id: str) -> bool: | |
Delete task assignment by task ID. | ||
""" | ||
return TaskAssignmentRepository.delete_assignment(task_id, user_id) | ||
|
||
@classmethod | ||
def reassign_tasks_from_user_to_team(cls, user_id: str, team_id: str, performed_by_user_id: str): | ||
""" | ||
Reassign all tasks of user to team | ||
""" | ||
return TaskAssignmentRepository.reassign_tasks_from_user_to_team(user_id, team_id, performed_by_user_id) | ||
Comment on lines
+154
to
+159
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 audit logging for task reassignment
Tell me moreWhat is the issue?The method does not create audit logs for the reassignment operations, unlike other assignment operations in the service. Why this mattersThis breaks the audit trail consistency established by other methods in the service, making it impossible to track when and by whom task reassignments occurred during team member removal. Suggested change ∙ Feature PreviewAdd audit logging after successful reassignment: @classmethod
def reassign_tasks_from_user_to_team(cls, user_id: str, team_id: str, performed_by_user_id: str):
"""
Reassign all tasks of user to team
"""
# ... validation code ...
result = TaskAssignmentRepository.reassign_tasks_from_user_to_team(user_id, team_id, performed_by_user_id)
# Log the reassignment action
if result: # Assuming result indicates success
AuditLogRepository.create(
AuditLogModel(
task_id=None, # Multiple tasks, so no specific task_id
team_id=PyObjectId(team_id),
action="tasks_reassigned_from_user_to_team",
performed_by=PyObjectId(performed_by_user_id),
)
)
return result 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 |
---|---|---|
|
@@ -13,6 +13,15 @@ | |
from todo.models.audit_log import AuditLogModel | ||
from todo.repositories.audit_log_repository import AuditLogRepository | ||
from todo.dto.responses.error_response import ApiErrorResponse, ApiErrorDetail | ||
from todo.constants.role import RoleScope | ||
from todo.services.user_role_service import UserRoleService | ||
from todo.services.task_assignment_service import TaskAssignmentService | ||
from todo.services.user_service import UserService | ||
from todo.exceptions.team_exceptions import ( | ||
CannotRemoveOwnerException, | ||
NotTeamAdminException, | ||
CannotRemoveTeamPOCException, | ||
) | ||
|
||
DEFAULT_ROLE_ID = "1" | ||
|
||
|
@@ -483,7 +492,25 @@ class TeamOrUserNotFound(Exception): | |
pass | ||
|
||
@classmethod | ||
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str = None): | ||
def _validate_remove_member_permissions(cls, user_id: str, team_id: str, removed_by_user_id: str): | ||
team = TeamService.get_team_by_id(team_id) | ||
team_members = UserService.get_users_by_team_id(team_id) | ||
team_member_ids = [user.id for user in team_members] | ||
|
||
if user_id not in team_member_ids: | ||
raise cls.TeamOrUserNotFound | ||
if user_id == team.created_by: | ||
raise CannotRemoveOwnerException() | ||
if user_id == team.poc_id: | ||
raise CannotRemoveTeamPOCException() | ||
if user_id != removed_by_user_id: | ||
if not UserRoleService.has_role(removed_by_user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id): | ||
raise NotTeamAdminException() | ||
|
||
@classmethod | ||
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str): | ||
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. Breaking change in method signature
Tell me moreWhat is the issue?The method signature changed from optional removed_by_user_id to required, but the method doesn't handle the case where a user removes themselves. Why this mattersThis breaks backward compatibility and may cause issues where self-removal scenarios expect the parameter to be optional or the same as user_id. Suggested change ∙ Feature PreviewMake removed_by_user_id optional and default to user_id when not provided: def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str = None):
if removed_by_user_id is None:
removed_by_user_id = user_id
cls._validate_remove_member_permissions(user_id, team_id, removed_by_user_id) Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
cls._validate_remove_member_permissions(user_id, team_id, removed_by_user_id) | ||
|
||
from todo.repositories.user_team_details_repository import UserTeamDetailsRepository | ||
|
||
success = UserTeamDetailsRepository.remove_member_from_team(user_id=user_id, team_id=team_id) | ||
|
@@ -494,9 +521,12 @@ def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: | |
AuditLogRepository.create( | ||
AuditLogModel( | ||
team_id=PyObjectId(team_id), | ||
action="member_removed_from_team", | ||
action="member_removed_from_team" if user_id != removed_by_user_id else "member_left_team", | ||
performed_by=PyObjectId(removed_by_user_id) if removed_by_user_id else PyObjectId(user_id), | ||
) | ||
) | ||
|
||
UserRoleService.remove_all_user_roles_for_team(user_id, team_id) | ||
TaskAssignmentService.reassign_tasks_from_user_to_team(user_id, team_id, removed_by_user_id) | ||
|
||
return True |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -129,3 +129,16 @@ def get_team_users_with_roles(cls, team_id: str) -> List[Dict[str, Any]]: | |
except Exception as e: | ||
logger.error(f"Failed to get team users with roles: {str(e)}") | ||
return [] | ||
|
||
@classmethod | ||
def remove_all_user_roles_for_team(cls, user_id: str, team_id: str) -> bool: | ||
"""Remove all roles for a user within a specific team.""" | ||
try: | ||
user_roles = cls.get_user_roles(user_id, RoleScope.TEAM.value, team_id) | ||
user_role_ids = [roles["role_id"] for roles in user_roles] | ||
for role_id in user_role_ids: | ||
cls.remove_role_by_id(user_id, role_id, RoleScope.TEAM.value, team_id) | ||
Comment on lines
+139
to
+140
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. Ignoring individual role removal failures
Tell me moreWhat is the issue?The method does not check the return value of remove_role_by_id, which could fail silently for individual role removals while still returning True overall. Why this mattersIf any individual role removal fails, the method will still return True, giving a false indication of success. This could lead to inconsistent state where some roles are removed but others remain, without the caller being aware of the partial failure. Suggested change ∙ Feature PreviewCheck the return value of each remove_role_by_id call and track failures: @classmethod
def remove_all_user_roles_for_team(cls, user_id: str, team_id: str) -> bool:
"""Remove all roles for a user within a specific team."""
try:
user_roles = cls.get_user_roles(user_id, RoleScope.TEAM.value, team_id)
user_role_ids = [roles["role_id"] for roles in user_roles]
all_removed = True
for role_id in user_role_ids:
if not cls.remove_role_by_id(user_id, role_id, RoleScope.TEAM.value, team_id):
all_removed = False
logger.error(f"Failed to remove role {role_id} for user {user_id} in team {team_id}")
return all_removed
except Exception as e:
logger.error(f"Failed to remove roles of user: {str(e)}")
return False Provide feedback to improve future suggestions💬 Looking for more details? Reply to this comment to chat with Korbit. |
||
return True | ||
except Exception as e: | ||
logger.error(f"Failed to remove roles of user: {str(e)}") | ||
return False |
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.
Suboptimal Exception Hierarchy
Tell me more
What is the issue?
The exception hierarchy doesn't reflect the semantic relationship between exceptions. NotTeamAdminException is more of a permission/authorization exception.
Why this matters
Current design makes it harder to catch and handle related exceptions together. A better hierarchy would group exceptions by their semantic meaning (e.g., all permission-related exceptions together).
Suggested change ∙ Feature Preview
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.