Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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)
17 changes: 17 additions & 0 deletions todo/migrations/0003_postgresauditlog_task_count.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Generated by Django 5.1.5 on 2025-09-02 15:07

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"),
Copy link

Choose a reason for hiding this comment

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

Unclear migration dependency name category Readability

Tell me more
What is the issue?

The migration dependency name is excessively long and contains truncated terms that make it difficult to understand the context.

Why this matters

Long, truncated migration names make it hard to track and understand the history of database changes. Future developers will struggle to locate and reference specific migrations.

Suggested change ∙ Feature Preview

Consider breaking down database changes into smaller, more focused migrations with clearer names like:

"todo", "0002_rename_task_assignee_index"
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

]

operations = [
migrations.AddField(
model_name="postgresauditlog",
name="task_count",
field=models.IntegerField(blank=True, null=True),
),
]
2 changes: 2 additions & 0 deletions todo/models/audit_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,5 @@ class AuditLogModel(Document):
assignee_to: PyObjectId | None = None
# For general user reference (who performed the action)
performed_by: PyObjectId | None = None
# For multiple task reassignments
task_count: int | None = None
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Enforce non-negative task_count at the model layer

Apply a lower bound; keeps data clean across services.

-    # For multiple task reassignments
-    task_count: int | None = None
+    # For multiple task reassignments
+    task_count: int | None = Field(default=None, ge=0)
📝 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
# For multiple task reassignments
task_count: int | None = None
# For multiple task reassignments
task_count: int | None = Field(default=None, ge=0)
🤖 Prompt for AI Agents
In todo/models/audit_log.py around lines 26-27, the task_count field currently
allows negative values; enforce a non-negative lower bound by changing the
declaration to include validation (e.g., if using Pydantic, replace or augment
the field with Field(default=None, ge=0) or add a validator that raises
ValueError when task_count is not None and task_count < 0) so the model layer
rejects negative counts.

1 change: 1 addition & 0 deletions todo/models/postgres/audit_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class PostgresAuditLog(models.Model):
assignee_from = models.CharField(max_length=24, null=True, blank=True)
assignee_to = models.CharField(max_length=24, null=True, blank=True)
performed_by = models.CharField(max_length=24, null=True, blank=True)
task_count = models.IntegerField(null=True, blank=True)
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Constrain task_count to non-negative values

Add a validator to prevent accidental negatives without requiring a migration.

-from django.db import models
+from django.db import models
+from django.core.validators import MinValueValidator
@@
-    task_count = models.IntegerField(null=True, blank=True)
+    task_count = models.IntegerField(null=True, blank=True, validators=[MinValueValidator(0)])
📝 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
task_count = models.IntegerField(null=True, blank=True)
from django.db import models
from django.core.validators import MinValueValidator
task_count = models.IntegerField(
null=True,
blank=True,
validators=[MinValueValidator(0)],
)
🤖 Prompt for AI Agents
In todo/models/postgres/audit_log.py around line 20, the IntegerField task_count
should be constrained to non-negative values; add Django's MinValueValidator(0)
to the field's validators (and import it from django.core.validators at the top)
so negatives are rejected at the model/validation layer without requiring a DB
migration; keep null=True/blank=True if you still want to allow missing values.


last_sync_at = models.DateTimeField(auto_now=True)
sync_status = models.CharField(
Expand Down
1 change: 1 addition & 0 deletions todo/repositories/audit_log_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def create(cls, audit_log: AuditLogModel) -> AuditLogModel:
"assignee_from": str(audit_log.assignee_from) if audit_log.assignee_from else None,
"assignee_to": str(audit_log.assignee_to) if audit_log.assignee_to else None,
"performed_by": str(audit_log.performed_by) if audit_log.performed_by else None,
"task_count": int(audit_log.task_count) if audit_log.task_count else None,
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Preserve zero and avoid unnecessary cast for task_count

Truthiness drops 0; Pydantic already types this as int|None, so casting is unnecessary.

-            "task_count": int(audit_log.task_count) if audit_log.task_count else None,
+            "task_count": audit_log.task_count if audit_log.task_count is not None else None,
📝 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
"task_count": int(audit_log.task_count) if audit_log.task_count else None,
"task_count": audit_log.task_count if audit_log.task_count is not None else None,
🤖 Prompt for AI Agents
In todo/repositories/audit_log_repository.py around line 32, the current
expression "int(audit_log.task_count) if audit_log.task_count else None"
incorrectly treats 0 as falsy and performs an unnecessary cast; replace it with
the raw value audit_log.task_count (or audit_log.task_count if
audit_log.task_count is not None else None) so zero is preserved and no cast is
done.

}

dual_write_success = dual_write_service.create_document(
Expand Down
108 changes: 108 additions & 0 deletions todo/repositories/task_assignment_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
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


Expand Down Expand Up @@ -355,3 +356,110 @@ 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):
"""
Reassign all tasks of user to team
"""
try:
collection = cls.get_collection()
now = datetime.now(timezone.utc)
pipeline = [
{
"$match": {
"team_id": {"$in": [team_id, ObjectId(team_id)]},
"assignee_id": {"$in": [user_id, ObjectId(user_id)]},
"user_type": "user",
"is_active": True,
}
},
{
"$addFields": {
"task_id_obj": {
"$convert": {"input": "$task_id", "to": "objectId", "onError": "$task_id", "onNull": None}
}
}
},
{"$lookup": {"from": "tasks", "localField": "task_id_obj", "foreignField": "_id", "as": "task_info"}},
{"$unwind": "$task_info"},
{"$match": {"task_info.status": {"$ne": TaskStatus.DONE.value}}},
]
user_task_assignments = list(collection.aggregate(pipeline))
if not user_task_assignments:
return 0

# Deactivate current assignment (try using both ObjectId and string)
collection.update_many(
{"assignee_id": ObjectId(user_id), "team_id": ObjectId(team_id), "is_active": True},
{"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
)

collection.update_many(
{"assignee_id": user_id, "team_id": team_id, "is_active": True},
{"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
)

This comment was marked as resolved.

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Bug: deactivates ALL assignments for the user/team, including DONE tasks.

You filtered out DONE in the aggregation, but the deactivation query is broad and will also deactivate DONE-task assignments. Restrict the deactivation to the matched task_ids only.

Apply:

-            # Deactivate current assignment (try using both ObjectId and string)
-            collection.update_many(
-                {"assignee_id": ObjectId(user_id), "team_id": ObjectId(team_id), "is_active": True},
-                {"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
-            )
-
-            collection.update_many(
-                {"assignee_id": user_id, "team_id": team_id, "is_active": True},
-                {"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
-            )
+            # Deactivate only the assignments we will reassign (handle both ObjectId and string task_id forms)
+            task_ids_obj = [t.get("task_id_obj") for t in user_task_assignments if isinstance(t.get("task_id_obj"), ObjectId)]
+            task_ids_str = [t.get("task_id") for t in user_task_assignments if isinstance(t.get("task_id"), (str, bytes))]
+
+            if task_ids_obj:
+                collection.update_many(
+                    {
+                        "assignee_id": ObjectId(user_id),
+                        "team_id": ObjectId(team_id),
+                        "task_id": {"$in": task_ids_obj},
+                        "is_active": True,
+                    },
+                    {"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
+                )
+            if task_ids_str:
+                collection.update_many(
+                    {
+                        "assignee_id": user_id,
+                        "team_id": team_id,
+                        "task_id": {"$in": task_ids_str},
+                        "is_active": True,
+                    },
+                    {"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
+                )
📝 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
# Deactivate current assignment (try using both ObjectId and string)
collection.update_many(
{"assignee_id": ObjectId(user_id), "team_id": ObjectId(team_id), "is_active": True},
{"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
)
collection.update_many(
{"assignee_id": user_id, "team_id": team_id, "is_active": True},
{"$set": {"is_active": False, "updated_by": ObjectId(performed_by_user_id), "updated_at": now}},
)
# Deactivate only the assignments we will reassign (handle both ObjectId and string task_id forms)
task_ids_obj = [
t.get("task_id_obj")
for t in user_task_assignments
if isinstance(t.get("task_id_obj"), ObjectId)
]
task_ids_str = [
t.get("task_id")
for t in user_task_assignments
if isinstance(t.get("task_id"), (str, bytes))
]
if task_ids_obj:
collection.update_many(
{
"assignee_id": ObjectId(user_id),
"team_id": ObjectId(team_id),
"task_id": {"$in": task_ids_obj},
"is_active": True,
},
{
"$set": {
"is_active": False,
"updated_by": ObjectId(performed_by_user_id),
"updated_at": now,
}
},
)
if task_ids_str:
collection.update_many(
{
"assignee_id": user_id,
"team_id": team_id,
"task_id": {"$in": task_ids_str},
"is_active": True,
},
{
"$set": {
"is_active": False,
"updated_by": ObjectId(performed_by_user_id),
"updated_at": now,
}
},
)
🤖 Prompt for AI Agents
In todo/repositories/task_assignment_repository.py around lines 392 to 401, the
update_many calls currently deactivate all active assignments for the user/team
(including DONE tasks); restrict the deactivation to only the task_ids returned
by the preceding aggregation by adding a task_id filter (e.g. {"task_id":
{"$in": task_ids}}) to both update queries, ensure you convert task_ids to the
correct stored type (ObjectId or string) before using them in the $in, and only
run the updates when task_ids is non-empty to avoid accidentally deactivating
unrelated assignments.


new_assignments = []
operations = []
dual_write_service = EnhancedDualWriteService()
for task in user_task_assignments:
operations.append(
{
"collection_name": "task_assignments",
"operation": "update",
"mongo_id": task["_id"],
"data": {
"task_mongo_id": str(task["task_id"]),
"assignee_id": str(task["assignee_id"]),
"user_type": task["user_type"],
"team_id": str(task["team_id"]),
"is_active": False,
"created_at": task["created_at"],
"updated_at": datetime.now(timezone.utc),
"created_by": str(task["created_by"]),
"updated_by": str(performed_by_user_id),
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consistency: reuse the computed now and avoid setting updated_by on creates.

Use the single captured timestamp and set updated_by=None on create to match create() semantics.

-                            "updated_at": datetime.now(timezone.utc),
+                            "updated_at": now,
...
-                            "created_at": datetime.now(timezone.utc),
-                            "updated_at": None,
-                            "created_by": str(performed_by_user_id),
-                            "updated_by": str(performed_by_user_id),
+                            "created_at": now,
+                            "updated_at": None,
+                            "created_by": str(performed_by_user_id),
+                            "updated_by": None,

Also applies to: 448-452

🤖 Prompt for AI Agents
In todo/repositories/task_assignment_repository.py around lines 419-421 (and
similarly lines 448-452), the code uses datetime.now(timezone.utc) inline and
sets updated_by to the performer on create; change to reuse a single captured
timestamp variable (e.g., now = datetime.now(timezone.utc)) for all timestamp
fields in the create path and set "updated_by": None (instead of
str(performed_by_user_id)) when inserting a new record so create() semantics
match existing behavior.

},
}
)
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Bug: Postgres dual-write uses raw ObjectId for mongo_id. Must be string.

EnhancedDualWriteService expects string mongo_id; passing ObjectId/PyObjectId will break lookups.

Apply:

-                        "mongo_id": task["_id"],
+                        "mongo_id": str(task["_id"]),
...
-                        "mongo_id": new_assignment_id,
+                        "mongo_id": str(new_assignment_id),

Also applies to: 438-454


🏁 Script executed:

#!/bin/bash
set -eux
# locate EnhancedDualWriteService class definition
rg -n "class EnhancedDualWriteService" -C3
# preview task_assignment_repository.py around the dual-write snippet
sed -n '370,450p' todo/repositories/task_assignment_repository.py

Length of output: 4656


🏁 Script executed:

#!/bin/bash
set -eux
# Preview the first 200 lines of EnhancedDualWriteService to inspect mongo_id handling
sed -n '1,200p' todo/services/enhanced_dual_write_service.py

Length of output: 6451


Convert mongo_id to string in dual-write operations
In todo/repositories/task_assignment_repository.py (around lines 408–424 and 438–454), wrap both task["_id"] and new_assignment_id in str(), e.g.:

- "mongo_id": task["_id"],
+ "mongo_id": str(task["_id"]),- "mongo_id": new_assignment_id,
+ "mongo_id": str(new_assignment_id),

so that EnhancedDualWriteService receives string IDs.

📝 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
{
"collection_name": "task_assignments",
"operation": "update",
"mongo_id": task["_id"],
"data": {
"task_mongo_id": str(task["task_id"]),
"assignee_id": str(task["assignee_id"]),
"user_type": task["user_type"],
"team_id": str(task["team_id"]),
"is_active": False,
"created_at": task["created_at"],
"updated_at": datetime.now(timezone.utc),
"created_by": str(task["created_by"]),
"updated_by": str(performed_by_user_id),
},
}
)
{
"collection_name": "task_assignments",
"operation": "update",
"mongo_id": str(task["_id"]),
"data": {
"task_mongo_id": str(task["task_id"]),
"assignee_id": str(task["assignee_id"]),
"user_type": task["user_type"],
"team_id": str(task["team_id"]),
"is_active": False,
"created_at": task["created_at"],
"updated_at": datetime.now(timezone.utc),
"created_by": str(task["created_by"]),
"updated_by": str(performed_by_user_id),
},
}
)
🤖 Prompt for AI Agents
In todo/repositories/task_assignment_repository.py around lines 408–424 and
438–454, the dual-write payloads pass Mongo IDs as raw ObjectId values; convert
task["_id"] and new_assignment_id to strings (wrap them with str()) where
assigned to "mongo_id" (and any other places sending IDs such as "task_mongo_id"
or similar) so the EnhancedDualWriteService receives string IDs consistently.

new_assignment_id = PyObjectId()
new_assignments.append(
TaskAssignmentModel(
_id=new_assignment_id,
task_id=task["task_id_obj"],
assignee_id=PyObjectId(team_id),
user_type="team",
created_by=PyObjectId(performed_by_user_id),
updated_by=None,
team_id=PyObjectId(team_id),
).model_dump(mode="json", by_alias=True, exclude_none=True)
)
operations.append(
{
"collection_name": "task_assignments",
"operation": "create",
"mongo_id": new_assignment_id,
"data": {
"task_mongo_id": str(task["task_id"]),
"assignee_id": str(team_id),
"user_type": "team",
"team_id": str(team_id),
"is_active": True,
"created_at": datetime.now(timezone.utc),
"updated_at": None,
"created_by": str(performed_by_user_id),
"updated_by": str(performed_by_user_id),
},
}
)
if new_assignments:
collection.insert_many(new_assignments)
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 len(new_assignments)
except Exception:
return 0
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Don’t swallow exceptions silently; log with context.

Return 0 is fine, but log the exception for diagnostics.

-        except Exception:
-            return 0
+        except Exception as e:
+            import logging
+            logging.getLogger(__name__).exception(
+                "Failed reassigning tasks from user %s to team %s", user_id, team_id
+            )
+            return 0
📝 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
except Exception:
return 0
except Exception as e:
import logging
logging.getLogger(__name__).exception(
"Failed reassigning tasks from user %s to team %s", user_id, team_id
)
return 0
🤖 Prompt for AI Agents
In todo/repositories/task_assignment_repository.py around lines 464-465 the
except block swallows all exceptions and just returns 0; change it to catch the
exception as a variable, log the error with contextual details (e.g., which
task/operation failed) and include the exception message/traceback via the
module logger (or import and use an existing logger), then return 0 as before so
diagnostics are available without changing behavior.

19 changes: 19 additions & 0 deletions todo/services/task_assignment_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -150,3 +150,22 @@ 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
"""
reassigned_tasks_count = TaskAssignmentRepository.reassign_tasks_from_user_to_team(
user_id, team_id, performed_by_user_id
)
if reassigned_tasks_count > 0:
AuditLogRepository.create(
AuditLogModel(
team_id=PyObjectId(team_id),
performed_by=PyObjectId(performed_by_user_id),
task_count=reassigned_tasks_count,
action="tasks_reassigned_to_team",
)
)
return reassigned_tasks_count
36 changes: 34 additions & 2 deletions todo/services/team_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@
from todo.repositories.audit_log_repository import AuditLogRepository
from todo.dto.responses.error_response import ApiErrorResponse, ApiErrorDetail


from todo.exceptions.team_exceptions import (
CannotRemoveOwnerException,
NotTeamAdminException,
CannotRemoveTeamPOCException,
)

DEFAULT_ROLE_ID = "1"


Expand Down Expand Up @@ -477,7 +484,32 @@ class TeamOrUserNotFound(Exception):
pass

@classmethod
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str = None):
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str):
team = TeamService.get_team_by_id(team_id)
from todo.services.user_role_service import UserRoleService
from todo.constants.role import RoleScope, RoleName

# Authentication Checks
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()

Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Owner-protection should be role-based, not creator-based.

Using created_by blocks the original creator even if ownership has been transferred. Check the OWNER role instead and guard POC None.

-        # Authentication Checks
-        if user_id == team.created_by:
-            raise CannotRemoveOwnerException()
-        if user_id == team.poc_id:
+        # Authorization checks
+        from todo.services.user_role_service import UserRoleService  # already imported above
+        if UserRoleService.has_role(user_id, TeamRoleName.OWNER.value, RoleScope.TEAM.value, team_id):
+            raise CannotRemoveOwnerException()
+        if team.poc_id and 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):
+        if user_id != removed_by_user_id:
+            if not UserRoleService.has_role(
+                removed_by_user_id, TeamRoleName.ADMIN.value, RoleScope.TEAM.value, team_id
+            ):
                 raise NotTeamAdminException()
📝 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
# Authentication Checks
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()
# Authorization checks
from todo.services.user_role_service import UserRoleService # already imported above
if UserRoleService.has_role(user_id, TeamRoleName.OWNER.value, RoleScope.TEAM.value, team_id):
raise CannotRemoveOwnerException()
if team.poc_id and user_id == team.poc_id:
raise CannotRemoveTeamPOCException()
if user_id != removed_by_user_id:
if not UserRoleService.has_role(
removed_by_user_id, TeamRoleName.ADMIN.value, RoleScope.TEAM.value, team_id
):
raise NotTeamAdminException()
🤖 Prompt for AI Agents
In todo/services/team_service.py around lines 491 to 499, the current checks
block the original creator via team.created_by and unconditionally compare POC;
change to role-based owner protection and guard POC None: replace the created_by
check with a call to UserRoleService.has_role(user_id, RoleName.OWNER.value,
RoleScope.TEAM.value, team_id) and raise CannotRemoveOwnerException() if true,
and only compare POC when team.poc_id is set (if team.poc_id and user_id ==
team.poc_id: raise CannotRemoveTeamPOCException()); keep the admin check for
removed_by_user_id as-is.

# Remove User Roles
user_roles = UserRoleService.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:
UserRoleService.remove_role_by_id(user_id, role_id, RoleScope.TEAM.value, team_id)

# Reassign Tasks:
from todo.services.task_assignment_service import TaskAssignmentService

TaskAssignmentService.reassign_tasks_from_user_to_team(user_id, team_id, removed_by_user_id)

# Remove User
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 @@ -488,7 +520,7 @@ 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.

Expand Down
Loading