Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
13e97ad
feat(team): restrict member removal to admins and reassign tasks
Hariom01010 Aug 23, 2025
ad26f3a
feat(remove-member): resolve comments by bot
Hariom01010 Aug 24, 2025
f25bc28
feat(remove-member): instantiate the exception raised
Hariom01010 Aug 24, 2025
8ef56b8
Merge branch 'develop' of https://github.com/Hariom01010/todo-backend…
Hariom01010 Aug 29, 2025
3f0bd93
Merge branch 'develop' of https://github.com/Hariom01010/todo-backend…
Hariom01010 Aug 31, 2025
7877f29
feat(remove-from-team): sync changes for task assignment to postgres
Hariom01010 Sep 2, 2025
f075980
feat(remove-member): update cannot remove POC exception message
Hariom01010 Sep 2, 2025
13485ff
Merge branch 'develop' of https://github.com/Hariom01010/todo-backend…
Hariom01010 Sep 3, 2025
6bb20aa
feat(remove-member-from-team): handle mixed ObjectId and string types…
Hariom01010 Sep 3, 2025
7713420
feat(remove-member-from-team): move UserTeamDetailsRepository to the
Hariom01010 Sep 3, 2025
b0e48d3
feat(remove-from-team): set updated_by to None in the ops for task
Hariom01010 Sep 3, 2025
482f7f0
test(remove-member): add tests for remove_member_from_team
Hariom01010 Sep 3, 2025
f4ca977
test(remove-member): move import to function
Hariom01010 Sep 3, 2025
f96e4c4
feat(remove-member): move task_reassignment function in transaction
Hariom01010 Sep 4, 2025
42c9485
feat(remove-member): fix formatting issues
Hariom01010 Sep 4, 2025
3b50a66
feat(remove-from-team): include is_active in unique constraint to pre…
Hariom01010 Sep 5, 2025
4b4ae55
feat(remove-from-team): remove unecessary comments
Hariom01010 Sep 9, 2025
bd88687
feat(remove-from-member): add serializer for path params validation
Hariom01010 Sep 9, 2025
3d20bfb
Merge branch 'develop' of https://github.com/Hariom01010/todo-backend…
Hariom01010 Sep 12, 2025
d567b3a
feat(remove-from-team): removed aggregation pipeline and use normal db
Hariom01010 Sep 16, 2025
01e59b8
feat(remove-from-todo): remove unecessary migration files
Hariom01010 Sep 16, 2025
bbc2132
feat(remove-from-team): remove task_count field from audit log model
Hariom01010 Sep 16, 2025
3bf3c2a
feat(remove-from-team): fix bug where reassigned todos not showing on
Hariom01010 Sep 17, 2025
3c6f245
feat(remove-from-team): move remove role and validation to individual
Hariom01010 Sep 17, 2025
8bcea47
feat(remove-from-team): fix formatting issues
Hariom01010 Sep 17, 2025
2059402
feat(remove-from-team): use bool return value instead of count for ta…
Hariom01010 Sep 17, 2025
918cd79
Merge branch 'remove-from-team-api' of https://github.com/Hariom01010…
Hariom01010 Sep 17, 2025
8027660
feat(remove-from-team): update test cases for changed implementation
Hariom01010 Sep 17, 2025
d156c32
test(remove-from-team): remove unnecssary setup values
Hariom01010 Sep 17, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions todo/constants/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ class ApiErrors:
USER_NOT_FOUND_GENERIC = "User not found."
SEARCH_QUERY_EMPTY = "Search query cannot be empty"
TASK_ALREADY_IN_WATCHLIST = "Task is already in the watchlist"
CANNOT_REMOVE_OWNER = "Owner cannot be removed from the team"
CANNOT_REMOVE_POC = "POC cannot be removed from a team. Reassign the POC first."


# Validation error messages
Expand Down
22 changes: 22 additions & 0 deletions todo/exceptions/team_exceptions.py
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",
),
),
]
8 changes: 7 additions & 1 deletion todo/models/postgres/user_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ class PostgresUserRole(models.Model):

class Meta:
db_table = "postgres_user_roles"
unique_together = ["user_id", "role_name", "scope", "team_id"]
constraints = [
models.UniqueConstraint(
fields=["user_id", "role_name", "scope", "team_id"],
condition=models.Q(is_active=True),
name="unique_active_user_team_role",
)
]
indexes = [
models.Index(fields=["mongo_id"]),
models.Index(fields=["user_id"]),
Expand Down
166 changes: 166 additions & 0 deletions todo/repositories/task_assignment_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic error: function returns 0 instead of boolean. The function signature and other return statements suggest it should return a boolean (True/False), but line 387 returns 0 when no task assignments are found. This inconsistency could cause issues for callers expecting a boolean. Change return 0 to return True or return False depending on the intended behavior.

Suggested change
return 0
return False

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

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"])

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
Comment on lines +484 to +486
Copy link
Contributor

Choose a reason for hiding this comment

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

Logic error in task ID comparison. The code compares assignment["task_id"] in tasks_to_clear_deferred_ids where assignment["task_id"] is an ObjectId but tasks_to_clear_deferred_ids contains ObjectId instances. However, tasks_to_reset_status_ids also contains ObjectId instances. This comparison may fail due to type mismatch. Convert to string for consistent comparison: str(assignment["task_id"]) in [str(id) for id in tasks_to_clear_deferred_ids]

Suggested change
if (
assignment["task_id"] in tasks_to_clear_deferred_ids
or assignment["task_id"] in tasks_to_reset_status_ids
if (
str(assignment["task_id"]) in [str(id) for id in tasks_to_clear_deferred_ids]
or str(assignment["task_id"]) in [str(id) for id in tasks_to_reset_status_ids]

Spotted by Diamond

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

):
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
18 changes: 18 additions & 0 deletions todo/serializers/remove_from_team_serializer.py
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
7 changes: 7 additions & 0 deletions todo/services/task_assignment_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
34 changes: 32 additions & 2 deletions todo/services/team_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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):
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)
Expand All @@ -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),
)
)
Comment on lines 521 to 527
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Minor: simplify performed_by (param is required).

removed_by_user_id is mandatory; the conditional is unnecessary.

-                performed_by=PyObjectId(removed_by_user_id) if removed_by_user_id else PyObjectId(user_id),
+                performed_by=PyObjectId(removed_by_user_id),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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),
)
)
AuditLogRepository.create(
AuditLogModel(
team_id=PyObjectId(team_id),
action="member_removed_from_team" if user_id != removed_by_user_id else "member_left_team",
performed_by=PyObjectId(removed_by_user_id),
)
)
🤖 Prompt for AI Agents
In todo/services/team_service.py around lines 519 to 525, the conditional used
for the performed_by field is unnecessary because removed_by_user_id is
required; replace the ternary with a direct use of
PyObjectId(removed_by_user_id) (i.e., set
performed_by=PyObjectId(removed_by_user_id)) and remove the conditional logic so
the AuditLogModel is constructed with the required performed_by value.


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
13 changes: 13 additions & 0 deletions todo/services/user_role_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
return True
except Exception as e:
logger.error(f"Failed to remove roles of user: {str(e)}")
return False
Loading