-
Notifications
You must be signed in to change notification settings - Fork 14
tests: add tests for remove from team API #271
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
Changes from 8 commits
13e97ad
ad26f3a
f25bc28
8ef56b8
3f0bd93
7877f29
f075980
13485ff
6bb20aa
7713420
b0e48d3
482f7f0
f4ca977
f96e4c4
42c9485
3b50a66
4b4ae55
bd88687
3d20bfb
d567b3a
01e59b8
bbc2132
3bf3c2a
3c6f245
8bcea47
2059402
918cd79
8027660
d156c32
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) | ||
Hariom01010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
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) | ||
Hariom01010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class CannotRemoveTeamPOCException(BaseTeamException): | ||
def __init__(self, message=ApiErrors.CANNOT_REMOVE_POC): | ||
super().__init__(message) | ||
Hariom01010 marked this conversation as resolved.
Show resolved
Hide resolved
|
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"), | ||
|
||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name="postgresauditlog", | ||
name="task_count", | ||
field=models.IntegerField(blank=True, null=True), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||
|
# 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.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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) | ||||||||||||||||||||
|
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.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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, | ||||||
|
"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.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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}}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
# 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.
Outdated
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.
🧹 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.
Outdated
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.
💡 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.
{ | |
"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.
Outdated
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.
🛠️ 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.
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.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -12,6 +12,13 @@ | |||||||||||||||||||||||||||||||||||||||
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, RoleName | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
from todo.exceptions.team_exceptions import ( | ||||||||||||||||||||||||||||||||||||||||
CannotRemoveOwnerException, | ||||||||||||||||||||||||||||||||||||||||
NotTeamAdminException, | ||||||||||||||||||||||||||||||||||||||||
CannotRemoveTeamPOCException, | ||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
DEFAULT_ROLE_ID = "1" | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
|
@@ -477,7 +484,31 @@ 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 | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
# Authentication Checks | ||||||||||||||||||||||||||||||||||||||||
if user_id == team.created_by: | ||||||||||||||||||||||||||||||||||||||||
Hariom01010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||
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() | ||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||
|
# 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.
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.
🧹 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.
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.
Uh oh!
There was an error while loading. Please reload this page.