tests: add tests for remove from team API #271
Conversation
- enforce auth so only admins can remove another member - automatically remove user roles on removal - reassign all non-DONE tasks of the removed user to the team - add audit log entry for task reassignment
- fix cannot remove error being shown for correct exception
…into remove-from-team-api
…into remove-from-team-api
- update postgres model for audit log to include task_count
- fix formatting
…into remove-from-team-api
… in task reassignment - Moves service imports to the top level in `team_service.py` for better code organization. - Modifies the `update_many` call in `task_assignment_repository.py` to use the `$in` operator. This ensures that `assignee_id` and `team_id` can be matched regardless of whether they are stored as ObjectIds or strings, preventing potential issues when deactivating task assignments during member removal.
assignment creation in reassign_tasks_from_user_to_team - fix formatting issues
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
WalkthroughAdds new team-related exceptions and API error messages; enforces role checks in member removal; reassigns a removed user’s active tasks to the team; removes the user’s team-specific roles; records audit logs including task_count; introduces task_count field at model, migration, repository, and view layers; updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant TeamService
participant UserRoleService
participant TaskAssignmentService
participant AuditLogRepository
participant UserTeamDetailsRepo
Client->>TeamService: remove_member_from_team(user_id, team_id, removed_by_user_id)
TeamService->>TeamService: get_team_by_id(team_id)
alt Target is Owner
TeamService-->>Client: raise CannotRemoveOwnerException
else Target is POC
TeamService-->>Client: raise CannotRemoveTeamPOCException
else Removing someone else and not ADMIN
TeamService->>UserRoleService: has_role(removed_by_user_id, ADMIN scoped to team)
alt Not ADMIN
TeamService-->>Client: raise NotTeamAdminException
end
end
TeamService->>UserRoleService: get_user_roles(user_id, team_id)
loop For each role
TeamService->>UserRoleService: remove_role_by_id(role_id)
end
TeamService->>TaskAssignmentService: reassign_tasks_from_user_to_team(user_id, team_id, performed_by_user_id)
TaskAssignmentService->>AuditLogRepository: create(action="tasks_reassigned_to_team", task_count, team_id, performed_by)
TeamService->>UserTeamDetailsRepo: remove_member_from_team(team_id, user_id)
alt Self-removal
TeamService->>AuditLogRepository: create(action="member_left_team", team_id, performed_by=user_id)
else Admin-initiated
TeamService->>AuditLogRepository: create(action="member_removed_from_team", team_id, performed_by=removed_by_user_id)
end
TeamService-->>Client: 200 OK
sequenceDiagram
autonumber
participant TaskAssignmentService
participant TaskAssignmentRepository
participant MongoDB
participant EnhancedDualWriteService
participant PostgreSQL
TaskAssignmentService->>TaskAssignmentRepository: reassign_tasks_from_user_to_team(user_id, team_id, performed_by)
TaskAssignmentRepository->>MongoDB: aggregate active, non-DONE tasks for user+team
alt No tasks
TaskAssignmentRepository-->>TaskAssignmentService: 0
else Tasks found
TaskAssignmentRepository->>MongoDB: deactivate existing user assignments
TaskAssignmentRepository->>MongoDB: create team assignments
TaskAssignmentRepository->>EnhancedDualWriteService: batch dual-write inserts/updates
EnhancedDualWriteService->>PostgreSQL: apply changes
TaskAssignmentRepository-->>TaskAssignmentService: new_assignment_count
end
TaskAssignmentService->>TaskAssignmentService: if count>0 create audit log (separate call)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Unclear migration dependency name ▹ view | |
 |
Redundant message storage in exception class ▹ view | |
|
Mismatched Exception Name and Message ▹ view | |
 |
POC Exception Missing Context ▹ view | |
|
Unclear ID Type Handling ▹ view | ✅ Fix detected |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/migrations/0003_postgresauditlog_task_count.py | ✅ |
| todo/exceptions/team_exceptions.py | ✅ |
| todo/models/audit_log.py | ✅ |
| todo/models/postgres/audit_log.py | ✅ |
| todo/repositories/audit_log_repository.py | ✅ |
| todo/constants/messages.py | ✅ |
| todo/services/task_assignment_service.py | ✅ |
| todo/views/team.py | ✅ |
| todo/services/team_service.py | ✅ |
| todo/repositories/task_assignment_repository.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("todo", "0002_rename_postgres_ta_assignee_95ca3b_idx_postgres_ta_assigne_f1c6e7_idx_and_more"), |
There was a problem hiding this comment.
Unclear migration dependency name 
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
💬 Looking for more details? Reply to this comment to chat with Korbit.
| 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.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
todo/tests/unit/services/test_team_service.py (1)
342-359: Strengthen self-removal test: explicit returns and audit action.mock_get_by_team_id.return_value = self.team_model - mock_has_role.return_value = True mock_get_user_roles.return_value = self.member_roles + mock_remove_member_from_team.return_value = True + mock_reassign_tasks_from_user_to_team.return_value = 1 result = TeamService.remove_member_from_team(self.member_id, self.team_id, self.member_id) self.assertTrue(result) + mock_reassign_tasks_from_user_to_team.assert_called_once_with(self.member_id, self.team_id, self.member_id) + # Verify audit action differs for self-removal + args, _ = mock_audit_log_create.call_args + self.assertEqual(getattr(args[0], "action"), "member_left_team")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
todo/constants/messages.py(1 hunks)todo/exceptions/team_exceptions.py(1 hunks)todo/migrations/0003_postgresauditlog_task_count.py(1 hunks)todo/models/audit_log.py(1 hunks)todo/models/postgres/audit_log.py(1 hunks)todo/repositories/audit_log_repository.py(1 hunks)todo/repositories/task_assignment_repository.py(2 hunks)todo/services/task_assignment_service.py(1 hunks)todo/services/team_service.py(3 hunks)todo/tests/unit/services/test_team_service.py(3 hunks)todo/views/team.py(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: Issue #215 in the Real-Dev-Squad/todo-backend repository addresses the problem where tasks assigned to team members disappear from the team's todo list. The expected behavior is that tasks assigned to individual team members should still be visible in the team's todo list, which is implemented by aggregating both direct team assignments and member assignments in the _get_assigned_task_ids_for_team method.
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: In the todo-backend project, tasks can only be assigned to either a team (user_type = "team") or an individual user (user_type = "user"), never both simultaneously. When a POC reassigns a task from a team to an individual team member, the old team assignment is deactivated and a new user assignment is created, ensuring no overlapping assignments exist.
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: In the todo-backend project's TaskAssignmentRepository, the update_assignment method ensures exclusive task assignments by deactivating all current active assignments for a task before creating a new assignment. This prevents any task from being simultaneously assigned to both a team and individual team members.
📚 Learning: 2025-07-25T20:12:36.483Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: In the todo-backend project, tasks can only be assigned to either a team (user_type = "team") or an individual user (user_type = "user"), never both simultaneously. When a POC reassigns a task from a team to an individual team member, the old team assignment is deactivated and a new user assignment is created, ensuring no overlapping assignments exist.
Applied to files:
todo/services/task_assignment_service.pytodo/repositories/task_assignment_repository.py
📚 Learning: 2025-07-25T20:12:36.483Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: In the todo-backend project's TaskAssignmentRepository, the update_assignment method ensures exclusive task assignments by deactivating all current active assignments for a task before creating a new assignment. This prevents any task from being simultaneously assigned to both a team and individual team members.
Applied to files:
todo/repositories/task_assignment_repository.py
📚 Learning: 2025-07-25T20:12:36.483Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#231
File: todo/repositories/task_repository.py:93-109
Timestamp: 2025-07-25T20:12:36.483Z
Learning: Issue #215 in the Real-Dev-Squad/todo-backend repository addresses the problem where tasks assigned to team members disappear from the team's todo list. The expected behavior is that tasks assigned to individual team members should still be visible in the team's todo list, which is implemented by aggregating both direct team assignments and member assignments in the _get_assigned_task_ids_for_team method.
Applied to files:
todo/repositories/task_assignment_repository.py
📚 Learning: 2025-07-23T19:26:43.747Z
Learnt from: Achintya-Chatterjee
PR: Real-Dev-Squad/todo-backend#227
File: todo/repositories/task_repository.py:0-0
Timestamp: 2025-07-23T19:26:43.747Z
Learning: In the todo-backend project, the get_tasks_for_user method in TaskRepository is intentionally designed to return only tasks assigned to the user (not tasks created by them), while the count method includes both tasks created by and assigned to the user. This behavioral difference is by design to serve different use cases.
Applied to files:
todo/repositories/task_assignment_repository.py
🧬 Code graph analysis (5)
todo/exceptions/team_exceptions.py (1)
todo/constants/messages.py (1)
ApiErrors(25-58)
todo/services/task_assignment_service.py (4)
todo/repositories/task_assignment_repository.py (2)
reassign_tasks_from_user_to_team(361-465)create(17-49)todo/repositories/audit_log_repository.py (2)
AuditLogRepository(7-51)create(11-45)todo/models/audit_log.py (1)
AuditLogModel(8-27)todo/models/common/pyobjectid.py (1)
PyObjectId(4-15)
todo/services/team_service.py (7)
todo/constants/role.py (2)
RoleScope(4-6)RoleName(9-13)todo/exceptions/team_exceptions.py (3)
CannotRemoveOwnerException(10-12)NotTeamAdminException(15-17)CannotRemoveTeamPOCException(20-22)todo/repositories/user_team_details_repository.py (1)
remove_member_from_team(35-68)todo/services/user_role_service.py (4)
UserRoleService(10-131)has_role(82-87)get_user_roles(54-79)remove_role_by_id(46-51)todo/repositories/user_role_repository.py (2)
get_user_roles(67-89)remove_role_by_id(121-165)todo/services/task_assignment_service.py (2)
TaskAssignmentService(18-171)reassign_tasks_from_user_to_team(155-171)todo/repositories/task_assignment_repository.py (1)
reassign_tasks_from_user_to_team(361-465)
todo/repositories/task_assignment_repository.py (5)
todo/constants/task.py (1)
TaskStatus(4-9)todo/services/task_assignment_service.py (1)
reassign_tasks_from_user_to_team(155-171)todo/services/enhanced_dual_write_service.py (2)
EnhancedDualWriteService(10-179)batch_operations(50-58)todo/models/common/pyobjectid.py (1)
PyObjectId(4-15)todo/models/task_assignment.py (1)
TaskAssignmentModel(10-41)
todo/tests/unit/services/test_team_service.py (6)
todo/exceptions/team_exceptions.py (3)
CannotRemoveOwnerException(10-12)CannotRemoveTeamPOCException(20-22)NotTeamAdminException(15-17)todo/services/team_service.py (3)
TeamService(26-527)remove_member_from_team(487-527)TeamOrUserNotFound(483-484)todo/models/team.py (2)
TeamModel(20-43)UserTeamDetailsModel(46-63)todo/models/user_role.py (1)
UserRoleModel(10-45)todo/models/common/pyobjectid.py (1)
PyObjectId(4-15)todo/constants/role.py (2)
RoleName(9-13)RoleScope(4-6)
🪛 GitHub Actions: Tests
todo/services/team_service.py
[error] 16-16: Ruff check failed: F811 - Redefinition of unused RoleName from line 10 in 'todo/services/team_service.py'. Remove definition: RoleName.
🔇 Additional comments (11)
todo/services/task_assignment_service.py (1)
154-171: Reassignment + audit flow looks correct.Delegates to the repository, excludes DONE tasks indirectly, and emits a single audit with task_count. ID wrapping is consistent. No issues.
todo/migrations/0003_postgresauditlog_task_count.py (1)
11-17: LGTM: adds nullable task_count to PostgresAuditLog.Migration wiring and nullability look fine.
todo/services/team_service.py (3)
501-505: LGTM: removing all TEAM-scope roles for the user.Simple, idempotent deactivation via repository looks fine.
507-510: LGTM: task reassignment hook is correctly placed.This ensures active (non-DONE) tasks move to the team before membership deletion.
487-487: remove_member_from_team call sites updated to include removed_by_user_id. No usages remain with only two arguments.todo/repositories/task_assignment_repository.py (1)
9-9: LGTM: importing TaskStatus is correct and used appropriately.todo/exceptions/team_exceptions.py (2)
1-23: LGTM: focused, explicit exception types with sensible default messages.
4-7: Optional: provide a str for clearer logs/tracebacks.class BaseTeamException(Exception): def __init__(self, message: str): self.message = message super().__init__(self.message) + def __str__(self): + return self.messageLikely an incorrect or invalid review comment.
todo/tests/unit/services/test_team_service.py (3)
276-283: LGTM: owner removal path correctly guarded and asserted.
284-291: LGTM: POC removal path covered.
292-300: LGTM: non-admin cannot remove another member.
todo/models/audit_log.py
Outdated
| # For multiple task reassignments | ||
| task_count: int | None = None |
There was a problem hiding this comment.
🧹 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.
| # 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.
todo/models/postgres/audit_log.py
Outdated
| 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) |
There was a problem hiding this comment.
🧹 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.
| 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.
| "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, |
There was a problem hiding this comment.
🛠️ 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.
| "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.
| # 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}}, | ||
| ) |
There was a problem hiding this comment.
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.
| # 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.
| 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), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧹 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.
| # Mock user roles | ||
| self.owner_roles = [ | ||
| UserRoleModel( | ||
| id=PyObjectId("507f1f77bcf86cd799439021"), | ||
| user_id=self.user_id, | ||
| role_name=RoleName.ADMIN, | ||
| scope=RoleScope.TEAM, | ||
| team_id=self.team_id, | ||
| is_active=True, | ||
| created_at=datetime.now(timezone.utc), | ||
| created_by="system", | ||
| ), | ||
| UserRoleModel( | ||
| id=PyObjectId("507f1f77bcf86cd799439022"), | ||
| user_id=self.user_id, | ||
| role_name=RoleName.OWNER, | ||
| scope=RoleScope.TEAM, | ||
| team_id=self.team_id, | ||
| is_active=True, | ||
| created_at=datetime.now(timezone.utc), | ||
| created_by="system", | ||
| ), | ||
| UserRoleModel( | ||
| id=PyObjectId("507f1f77bcf86cd799439023"), | ||
| user_id=self.user_id, | ||
| role_name=RoleName.MEMBER, | ||
| scope=RoleScope.TEAM, | ||
| team_id=self.team_id, | ||
| is_active=True, | ||
| created_at=datetime.now(timezone.utc), | ||
| created_by="system", | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Tidy: large role fixtures are unused in tests.
owner_roles, poc_roles, and admin_roles are not referenced; keep only what’s used to reduce noise.
Also applies to: 85-118, 119-127
🤖 Prompt for AI Agents
In todo/tests/unit/services/test_team_service.py around lines 51-83 (and
similarly for ranges 85-118 and 119-127), the large role fixture arrays
(owner_roles, poc_roles, admin_roles) are declared but unused; remove these
unused fixtures and keep only the role fixtures that the tests actually
reference to reduce noise and fixture maintenance, updating any test setup
references to use the reduced fixtures or create minimal inline role objects
where needed.
| @patch("todo.services.user_role_service.UserRoleService.has_role") | ||
| @patch("todo.services.team_service.TeamService.get_team_by_id") | ||
| def test_team_or_user_not_found(self, mock_get_by_team_id, mock_has_role): | ||
| mock_get_by_team_id.return_value = self.team_model | ||
| mock_has_role.return_value = False | ||
|
|
||
| with self.assertRaises(TeamService.TeamOrUserNotFound): | ||
| TeamService.remove_member_from_team(self.member_id, "not-teamid-exist", self.member_id) | ||
|
|
There was a problem hiding this comment.
Fix: test hits real repository; patch and force failure deterministically.
Add patch for remove_member_from_team and set return False.
- @patch("todo.services.user_role_service.UserRoleService.has_role")
- @patch("todo.services.team_service.TeamService.get_team_by_id")
- def test_team_or_user_not_found(self, mock_get_by_team_id, mock_has_role):
+ @patch("todo.repositories.user_team_details_repository.UserTeamDetailsRepository.remove_member_from_team")
+ @patch("todo.services.user_role_service.UserRoleService.has_role")
+ @patch("todo.services.team_service.TeamService.get_team_by_id")
+ def test_team_or_user_not_found(self, mock_get_by_team_id, mock_has_role, mock_remove_member_from_team):
mock_get_by_team_id.return_value = self.team_model
mock_has_role.return_value = False
+ mock_remove_member_from_team.return_value = False📝 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.
| @patch("todo.services.user_role_service.UserRoleService.has_role") | |
| @patch("todo.services.team_service.TeamService.get_team_by_id") | |
| def test_team_or_user_not_found(self, mock_get_by_team_id, mock_has_role): | |
| mock_get_by_team_id.return_value = self.team_model | |
| mock_has_role.return_value = False | |
| with self.assertRaises(TeamService.TeamOrUserNotFound): | |
| TeamService.remove_member_from_team(self.member_id, "not-teamid-exist", self.member_id) | |
| @patch("todo.repositories.user_team_details_repository.UserTeamDetailsRepository.remove_member_from_team") | |
| @patch("todo.services.user_role_service.UserRoleService.has_role") | |
| @patch("todo.services.team_service.TeamService.get_team_by_id") | |
| def test_team_or_user_not_found(self, mock_get_by_team_id, mock_has_role, mock_remove_member_from_team): | |
| mock_get_by_team_id.return_value = self.team_model | |
| mock_has_role.return_value = False | |
| mock_remove_member_from_team.return_value = False | |
| with self.assertRaises(TeamService.TeamOrUserNotFound): | |
| TeamService.remove_member_from_team(self.member_id, "not-teamid-exist", self.member_id) |
🤖 Prompt for AI Agents
In todo/tests/unit/services/test_team_service.py around lines 301 to 309, the
test still calls into the real repository because remove_member is not patched;
update the test to patch the repository remove method (e.g., patch
"todo.services.team_service.TeamRepository.remove_member_from_team" or the
TeamService._team_repo.remove_member_from_team used by the service) and set its
return value to False so the service deterministically fails and raises
TeamService.TeamOrUserNotFound.
| def test_admin_can_remove_member_successfully( | ||
| self, | ||
| mock_get_by_team_id, | ||
| mock_has_role, | ||
| mock_get_user_roles, | ||
| mock_remove_member_from_team, | ||
| mock_remove_role_by_id, | ||
| mock_reassign_tasks_from_user_to_team, | ||
| mock_audit_log_create, | ||
| ): | ||
| mock_get_by_team_id.return_value = self.team_model | ||
| mock_has_role.return_value = True | ||
| mock_get_user_roles.return_value = self.member_roles | ||
|
|
||
| result = TeamService.remove_member_from_team(self.member_id, self.team_id, self.admin_id) | ||
|
|
||
| self.assertTrue(result) | ||
|
|
||
| @patch("todo.repositories.audit_log_repository.AuditLogRepository.create") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Strengthen assertions and set explicit return values for mocks.
Ensure deterministic behavior and verify key side effects.
mock_get_by_team_id.return_value = self.team_model
mock_has_role.return_value = True
mock_get_user_roles.return_value = self.member_roles
+ mock_remove_member_from_team.return_value = True
+ mock_reassign_tasks_from_user_to_team.return_value = 2
result = TeamService.remove_member_from_team(self.member_id, self.team_id, self.admin_id)
self.assertTrue(result)
+ mock_reassign_tasks_from_user_to_team.assert_called_once_with(self.member_id, self.team_id, self.admin_id)
+ self.assertEqual(mock_remove_role_by_id.call_count, len(self.member_roles))
+ mock_remove_member_from_team.assert_called_once_with(user_id=self.member_id, team_id=self.team_id)
+ # Verify audit action
+ args, _ = mock_audit_log_create.call_args
+ self.assertEqual(getattr(args[0], "action"), "member_removed_from_team")🏁 Script executed:
#!/bin/bash
set -e
file=$(find . -type f -path "*todo/tests/unit/services/test_team_service.py" | head -n 1)
rg -n "def test_admin_can_remove_member_successfully" -C5 "$file"Length of output: 892
🏁 Script executed:
#!/bin/bash
file=$(find . -type f -path "*todo/tests/unit/services/test_team_service.py" | head -n 1)
sed -n '312,350p' "$file"Length of output: 2084
Add missing mocks and strengthen assertions in test_admin_can_remove_member_successfully
- Prepend these patch decorators above
def test_admin_can_remove_member_successfullyto match its signature:@patch("todo.services.task_assignment_service.TaskAssignmentService.reassign_tasks_from_user_to_team") @patch("todo.repositories.audit_log_repository.AuditLogRepository.create")
- Inside the test, after existing return‐value setups, add:
mock_remove_member_from_team.return_value = True mock_reassign_tasks_from_user_to_team.return_value = 2
- After
self.assertTrue(result), verify all side effects:mock_remove_member_from_team.assert_called_once_with(user_id=self.member_id, team_id=self.team_id) mock_reassign_tasks_from_user_to_team.assert_called_once_with(self.member_id, self.team_id, self.admin_id) self.assertEqual(mock_remove_role_by_id.call_count, len(self.member_roles)) args, _ = mock_audit_log_create.call_args self.assertEqual(getattr(args[0], "action"), "member_removed_from_team")
🤖 Prompt for AI Agents
In todo/tests/unit/services/test_team_service.py around lines 317 to 335, the
test_admin_can_remove_member_successfully is missing two patch decorators,
missing mock return setups, and lacks assertions for side effects; prepend
@patch("todo.services.task_assignment_service.TaskAssignmentService.reassign_tasks_from_user_to_team")
and @patch("todo.repositories.audit_log_repository.AuditLogRepository.create")
above the test definition to match its signature, set
mock_remove_member_from_team.return_value = True and
mock_reassign_tasks_from_user_to_team.return_value = 2 inside the test after
existing return setups, then after self.assertTrue(result) add assertions to
verify side effects: assert mock_remove_member_from_team was called once with
user_id=self.member_id and team_id=self.team_id, assert
mock_reassign_tasks_from_user_to_team was called once with (self.member_id,
self.team_id, self.admin_id), assert mock_remove_role_by_id.call_count equals
len(self.member_roles), and inspect mock_audit_log_create.call_args to assert
the created audit log action equals "member_removed_from_team".
todo/views/team.py
Outdated
| if log.task_count: | ||
| entry["task_count"] = log.task_count |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t drop zero counts; check for None instead of truthiness
A value of 0 would be omitted. Use an explicit None check.
- if log.task_count:
- entry["task_count"] = log.task_count
+ if log.task_count is not None:
+ entry["task_count"] = log.task_count📝 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.
| if log.task_count: | |
| entry["task_count"] = log.task_count | |
| if log.task_count is not None: | |
| entry["task_count"] = log.task_count |
🤖 Prompt for AI Agents
In todo/views/team.py around lines 451-452, the current conditional uses
truthiness (if log.task_count) which will omit zero values; change it to an
explicit None check (if log.task_count is not None) so zero counts are preserved
when setting entry["task_count"].
| mock_get_by_team_id.return_value = self.team_model | ||
| mock_has_role.return_value = False | ||
|
|
||
| with self.assertRaises(TeamService.TeamOrUserNotFound): |
There was a problem hiding this comment.
The test logic contains an inconsistency. The mock for TeamService.get_team_by_id() is configured to return self.team_model for any input, but the test expects a TeamOrUserNotFound exception to be raised when using a non-existent team ID.
This creates a contradiction - the mock will always return a valid team model regardless of the team ID passed, preventing the exception from being triggered. To properly test this error case, either:
- Modify the mock to raise the exception when called with "not-teamid-exist", or
- Move the exception expectation to a different part of the flow where the error would actually occur
Without this change, the test will fail with an AttributeError rather than catching the expected exception.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
- add team exception in views - add assigned_from and assigned_to to audit log
…vent sync failures
- fix bug where done tasks were also being reassigned - fix bug where in postgres_user_role where data sync failed if a user was removed and added more than once - updated reassigned tasks status and add sync to postgres db
Achintya-Chatterjee
changed the title
…into remove-from-team-api
…sk reassignment function
…/todo-backend into tests/remove-from-team-api
| if ( | ||
| assignment["task_id"] in tasks_to_clear_deferred_ids | ||
| or assignment["task_id"] in tasks_to_reset_status_ids |
There was a problem hiding this comment.
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]
| 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
Is this helpful? React 👍 or 👎 to let us know.
| ) | ||
| ) | ||
| if not user_task_assignments: | ||
| return 0 |
There was a problem hiding this comment.
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.
| return 0 | |
| return False |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Date: 04 Sept 2025
Developer Name: @Hariom01010
Issue Ticket Number
Closes #254
Description
Added test cases for the function to remove a member from team API
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
Description by Korbit AI
What change is being made?
Add comprehensive tests and supporting changes for removing a member from a team, including new removal-related exceptions, a remove-from-team serializer, updated team and user-role logic, task reassignment on removal, audit logging, and view handling for validation and permission errors.
Why are these changes being made?
Enforce correct removal behavior (owners/POC cannot be removed, only admins can remove others) and ensure associated tasks are reassigned and user roles cleared, while providing thorough tests and clear error handling for the API. This design centralizes removal rules, logs actions, and keeps data consistent across related collections.