Dev to Main Sync #277
Dev to Main Sync #277
Conversation
* feat(team): restrict member removal to admins and reassign tasks - 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 * feat(remove-member): resolve comments by bot * feat(remove-member): instantiate the exception raised - fix cannot remove error being shown for correct exception * feat(remove-from-team): sync changes for task assignment to postgres - update postgres model for audit log to include task_count * feat(remove-member): update cannot remove POC exception message - fix formatting * feat(remove-member-from-team): handle mixed ObjectId and string types 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. * feat(remove-member-from-team): move UserTeamDetailsRepository to the function * feat(remove-from-team): set updated_by to None in the ops for task assignment creation in reassign_tasks_from_user_to_team - fix formatting issues * feat(remove-member): move task_reassignment function in transaction - add team exception in views - add assigned_from and assigned_to to audit log * feat(remove-member): fix formatting issues * feat(remove-from-team): include is_active in unique constraint to prevent sync failures * feat(remove-from-team): remove unecessary comments - 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 * feat(remove-from-member): add serializer for path params validation * feat(remove-from-team): removed aggregation pipeline and use normal db queries * feat(remove-from-todo): remove unecessary migration files * feat(remove-from-team): remove task_count field from audit log model * feat(remove-from-team): fix bug where reassigned todos not showing on poc dashboard * feat(remove-from-team): move remove role and validation to individual function * feat(remove-from-team): fix formatting issues * feat(remove-from-team): use bool return value instead of count for task reassignment function
* feat(team): restrict member removal to admins and reassign tasks - 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 * feat(remove-member): resolve comments by bot * feat(remove-member): instantiate the exception raised - fix cannot remove error being shown for correct exception * feat(remove-from-team): sync changes for task assignment to postgres - update postgres model for audit log to include task_count * feat(remove-member): update cannot remove POC exception message - fix formatting * feat(remove-member-from-team): handle mixed ObjectId and string types 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. * feat(remove-member-from-team): move UserTeamDetailsRepository to the function * feat(remove-from-team): set updated_by to None in the ops for task assignment creation in reassign_tasks_from_user_to_team - fix formatting issues * test(remove-member): add tests for remove_member_from_team * test(remove-member): move import to function * feat(remove-member): move task_reassignment function in transaction - add team exception in views - add assigned_from and assigned_to to audit log * feat(remove-member): fix formatting issues * feat(remove-from-team): include is_active in unique constraint to prevent sync failures * feat(remove-from-team): remove unecessary comments - 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 * feat(remove-from-member): add serializer for path params validation * feat(remove-from-team): removed aggregation pipeline and use normal db queries * feat(remove-from-todo): remove unecessary migration files * feat(remove-from-team): remove task_count field from audit log model * feat(remove-from-team): fix bug where reassigned todos not showing on poc dashboard * feat(remove-from-team): move remove role and validation to individual function * feat(remove-from-team): fix formatting issues * feat(remove-from-team): use bool return value instead of count for task reassignment function * feat(remove-from-team): update test cases for changed implementation * test(remove-from-team): remove unnecssary setup values
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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}
| if not user_task_assignments: | ||
| return 0 |
There was a problem hiding this comment.
Bug: Function returns 0 when no assignments found, but the function signature indicates it should return bool. This type inconsistency will cause issues for callers expecting boolean. Should return False instead of 0.
| if not user_task_assignments: | |
| return 0 | |
| if not user_task_assignments: | |
| return False |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![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 |
|---|---|---|
 |
Missing Migration Rationale ▹ view | |
 |
Suboptimal Exception Hierarchy ▹ view | |
|
Duplicate ObjectId validation logic ▹ view | |
 |
Missing audit logging for task reassignment ▹ view | |
 |
Ignoring individual role removal failures ▹ view | |
|
Unsafe exception attribute access ▹ view | |
 |
Inconsistent Error Handling Implementation ▹ view | |
|
Generic exception handling without context ▹ view | |
|
Misleading Iterator Variable Name ▹ view | |
 |
Breaking change in method signature ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| todo/exceptions/team_exceptions.py | ✅ |
| todo/serializers/remove_from_team_serializer.py | ✅ |
| todo/migrations/0003_alter_postgresuserrole_unique_together_and_more.py | ✅ |
| todo/models/postgres/user_role.py | ✅ |
| todo/constants/messages.py | ✅ |
| todo/services/user_role_service.py | ✅ |
| todo/services/task_assignment_service.py | ✅ |
| todo/services/team_service.py | ✅ |
| todo/views/team.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.
| migrations.AddConstraint( | ||
| model_name="postgresuserrole", | ||
| constraint=models.UniqueConstraint( | ||
| condition=models.Q(("is_active", True)), | ||
| fields=("user_id", "role_name", "scope", "team_id"), | ||
| name="unique_active_user_team_role", | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Missing Migration Rationale 
Tell me more
What is the issue?
The migration lacks a comment explaining the business reason for replacing unique_together with a conditional unique constraint.
Why this matters
Future developers may not understand why this specific database constraint change was necessary, making it harder to maintain or modify the constraint logic.
Suggested change ∙ Feature Preview
Replace unique_together with conditional unique constraint to allow multiple inactive roles
while ensuring only one active role per user-team-role-scope combination
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| class NotTeamAdminException(BaseTeamException): | ||
| def __init__(self, message=ApiErrors.UNAUTHORIZED_TITLE): | ||
| super().__init__(message) |
There was a problem hiding this comment.
Suboptimal Exception Hierarchy 
Tell me more
What is the issue?
The exception hierarchy doesn't reflect the semantic relationship between exceptions. NotTeamAdminException is more of a permission/authorization exception.
Why this matters
Current design makes it harder to catch and handle related exceptions together. A better hierarchy would group exceptions by their semantic meaning (e.g., all permission-related exceptions together).
Suggested change ∙ Feature Preview
class TeamPermissionException(BaseTeamException):
"""Base class for all permission-related team exceptions"""
pass
class NotTeamAdminException(TeamPermissionException):
def __init__(self, message=ApiErrors.UNAUTHORIZED_TITLE):
super().__init__(message)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| def validate_team_id(self, value): | ||
| if not ObjectId.is_valid(value): | ||
| raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) | ||
| return value | ||
|
|
||
| def validate_user_id(self, value): | ||
| if not ObjectId.is_valid(value): | ||
| raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) | ||
| return value |
There was a problem hiding this comment.
Duplicate ObjectId validation logic
Tell me more
What is the issue?
The validation logic for ObjectId is duplicated between validate_team_id and validate_user_id methods.
Why this matters
Code duplication increases maintenance burden and risk of inconsistencies when changes are needed. If the ObjectId validation logic needs to change, it would need to be updated in multiple places.
Suggested change ∙ Feature Preview
Extract the common validation logic into a reusable method:
def validate_object_id(self, value):
if not ObjectId.is_valid(value):
raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value))
return value
def validate_team_id(self, value):
return self.validate_object_id(value)
def validate_user_id(self, value):
return self.validate_object_id(value)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| @classmethod | ||
| def reassign_tasks_from_user_to_team(cls, user_id: str, team_id: str, performed_by_user_id: str): | ||
| """ | ||
| Reassign all tasks of user to team | ||
| """ | ||
| return TaskAssignmentRepository.reassign_tasks_from_user_to_team(user_id, team_id, performed_by_user_id) |
There was a problem hiding this comment.
Missing audit logging for task reassignment 
Tell me more
What is the issue?
The method does not create audit logs for the reassignment operations, unlike other assignment operations in the service.
Why this matters
This breaks the audit trail consistency established by other methods in the service, making it impossible to track when and by whom task reassignments occurred during team member removal.
Suggested change ∙ Feature Preview
Add audit logging after successful reassignment:
@classmethod
def reassign_tasks_from_user_to_team(cls, user_id: str, team_id: str, performed_by_user_id: str):
"""
Reassign all tasks of user to team
"""
# ... validation code ...
result = TaskAssignmentRepository.reassign_tasks_from_user_to_team(user_id, team_id, performed_by_user_id)
# Log the reassignment action
if result: # Assuming result indicates success
AuditLogRepository.create(
AuditLogModel(
task_id=None, # Multiple tasks, so no specific task_id
team_id=PyObjectId(team_id),
action="tasks_reassigned_from_user_to_team",
performed_by=PyObjectId(performed_by_user_id),
)
)
return resultProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| for role_id in user_role_ids: | ||
| cls.remove_role_by_id(user_id, role_id, RoleScope.TEAM.value, team_id) |
There was a problem hiding this comment.
Ignoring individual role removal failures 
Tell me more
What is the issue?
The method does not check the return value of remove_role_by_id, which could fail silently for individual role removals while still returning True overall.
Why this matters
If any individual role removal fails, the method will still return True, giving a false indication of success. This could lead to inconsistent state where some roles are removed but others remain, without the caller being aware of the partial failure.
Suggested change ∙ Feature Preview
Check the return value of each remove_role_by_id call and track failures:
@classmethod
def remove_all_user_roles_for_team(cls, user_id: str, team_id: str) -> bool:
"""Remove all roles for a user within a specific team."""
try:
user_roles = cls.get_user_roles(user_id, RoleScope.TEAM.value, team_id)
user_role_ids = [roles["role_id"] for roles in user_roles]
all_removed = True
for role_id in user_role_ids:
if not cls.remove_role_by_id(user_id, role_id, RoleScope.TEAM.value, team_id):
all_removed = False
logger.error(f"Failed to remove role {role_id} for user {user_id} in team {team_id}")
return all_removed
except Exception as e:
logger.error(f"Failed to remove roles of user: {str(e)}")
return FalseProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| except NotTeamAdminException as e: | ||
| return Response({"detail": e.message}, status=status.HTTP_403_FORBIDDEN) | ||
| except CannotRemoveTeamPOCException as e: | ||
| return Response({"detail": e.message}, status=status.HTTP_403_FORBIDDEN) | ||
| except CannotRemoveOwnerException as e: | ||
| return Response({"detail": e.message}, status=status.HTTP_403_FORBIDDEN) |
There was a problem hiding this comment.
Unsafe exception attribute access
Tell me more
What is the issue?
The exception handlers assume that the custom exceptions have a message attribute, but this is not guaranteed and could cause AttributeError if the exceptions don't have this attribute.
Why this matters
If these custom exceptions don't have a message attribute, accessing e.message will raise an AttributeError, causing the application to crash instead of returning the intended 403 Forbidden response.
Suggested change ∙ Feature Preview
Use str(e) instead of e.message to safely get the exception message, or use getattr(e, 'message', str(e)) as a fallback:
except NotTeamAdminException as e:
return Response({"detail": str(e)}, status=status.HTTP_403_FORBIDDEN)
except CannotRemoveTeamPOCException as e:
return Response({"detail": str(e)}, status=status.HTTP_403_FORBIDDEN)
except CannotRemoveOwnerException as e:
return Response({"detail": str(e)}, status=status.HTTP_403_FORBIDDEN)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| def _handle_validation_errors(self, errors): | ||
| """Handle validation errors and return appropriate response.""" | ||
| formatted = [] | ||
| for field, msgs in errors.items(): | ||
| for msg in msgs: | ||
| formatted.append( | ||
| { | ||
| "source": field, | ||
| "title": "Invalid value", | ||
| "detail": str(msg), | ||
| } | ||
| ) |
There was a problem hiding this comment.
Inconsistent Error Handling Implementation 
Tell me more
What is the issue?
The _handle_validation_errors method is duplicated in both TeamListView and RemoveTeamMemberView with slightly different implementations, causing confusion about which version should be used.
Why this matters
Having multiple versions of the same error handling logic makes the code harder to maintain and increases the risk of inconsistent error responses across different endpoints.
Suggested change ∙ Feature Preview
Extract the error handling logic into a shared mixin or base class:
class ValidationErrorHandlerMixin:
def _handle_validation_errors(self, errors):
formatted = []
for field, msgs in errors.items():
for msg in msgs:
formatted.append({
"source": field,
"title": "Invalid value",
"detail": str(msg),
})
return Response({
"statusCode": 400,
"message": "Validation Error",
"errors": formatted,
"authenticated": getattr(self.request, "user", None) is not None,
}, status=400)
class TeamListView(ValidationErrorHandlerMixin, APIView):
...
class RemoveTeamMemberView(ValidationErrorHandlerMixin, APIView):
...Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| except Exception: | ||
| return False |
There was a problem hiding this comment.
Generic exception handling without context
Tell me more
What is the issue?
Generic catch-all exception handling without logging or contextual information about what went wrong.
Why this matters
When exceptions occur, no information is captured about the root cause, making debugging and monitoring extremely difficult. The method silently fails without any trace of what actually happened.
Suggested change ∙ Feature Preview
Add logging with contextual information about the error:
except Exception as e:
import logging
logger = logging.getLogger(__name__)
logger.error(f"Failed to reassign tasks from user {user_id} to team {team_id}: {str(e)}", exc_info=True)
return FalseProvide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| tasks_to_reset_status_ids = [] | ||
| tasks_to_clear_deferred_ids = [] | ||
| for tasks in active_tasks: | ||
| if tasks["status"] == TaskStatus.IN_PROGRESS.value: | ||
| tasks_to_reset_status_ids.append(tasks["_id"]) | ||
| elif tasks.get("deferredDetails") is not None: | ||
| tasks_to_clear_deferred_ids.append(tasks["_id"]) |
There was a problem hiding this comment.
Misleading Iterator Variable Name
Tell me more
What is the issue?
Variable name 'tasks' is used in singular form when iterating over 'active_tasks', suggesting it represents multiple tasks when it actually represents a single task.
Why this matters
Misleading variable names can cause confusion about data structures and their contents.
Suggested change ∙ Feature Preview
tasks_to_reset_status_ids = []
tasks_to_clear_deferred_ids = []
for task in active_tasks:
if task["status"] == TaskStatus.IN_PROGRESS.value:
tasks_to_reset_status_ids.append(task["_id"])
elif task.get("deferredDetails") is not None:
tasks_to_clear_deferred_ids.append(task["_id"])Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| raise NotTeamAdminException() | ||
|
|
||
| @classmethod | ||
| def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str): |
There was a problem hiding this comment.
Breaking change in method signature 
Tell me more
What is the issue?
The method signature changed from optional removed_by_user_id to required, but the method doesn't handle the case where a user removes themselves.
Why this matters
This breaks backward compatibility and may cause issues where self-removal scenarios expect the parameter to be optional or the same as user_id.
Suggested change ∙ Feature Preview
Make removed_by_user_id optional and default to user_id when not provided:
def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: str = None):
if removed_by_user_id is None:
removed_by_user_id = user_id
cls._validate_remove_member_permissions(user_id, team_id, removed_by_user_id)Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
3 participants
Date:
September 20, 2025Developer Name:
Issue Ticket Number
PR's Going in this Sync
#255
#271
Description
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?
Implement team ownership and POC removal protections, add task reassignment on member removal with audit logging, introduce validation and exception handling for team operations, and update models, serializers, services, views, tests, and migrations to support these behaviors.
Why are these changes being made?
Introduce stricter governance around removing team owners/POCs and ensure tasks are consistently reassigned and tracked when a team member is removed, improving data integrity and auditability. The changes add comprehensive error handling and tests to cover new edge cases.