From 7773bd2327785eef2f1127144c3c1fd755fe3f34 Mon Sep 17 00:00:00 2001 From: anujchhikara Date: Sat, 18 Oct 2025 02:07:13 +0530 Subject: [PATCH 1/3] feat(team): enhance team update functionality with POC validation and error handling --- todo/constants/messages.py | 2 + todo/repositories/task_repository.py | 7 +- todo/serializers/update_team_serializer.py | 27 +-- todo/services/team_service.py | 69 +++++--- todo/tests/integration/test_team_update.py | 165 ++++++++++++++++++ todo/tests/unit/services/test_team_service.py | 116 ++++++++++++ todo/views/team.py | 36 ++-- 7 files changed, 345 insertions(+), 77 deletions(-) create mode 100644 todo/tests/integration/test_team_update.py diff --git a/todo/constants/messages.py b/todo/constants/messages.py index 4f80b52e..777b56e8 100644 --- a/todo/constants/messages.py +++ b/todo/constants/messages.py @@ -55,6 +55,7 @@ class ApiErrors: SEARCH_QUERY_EMPTY = "Search query cannot be empty" TASK_ALREADY_IN_WATCHLIST = "Task is already in the watchlist" CANNOT_REMOVE_OWNER = "Owner cannot be removed from the team" + MEMBER_NOT_FOUND = "Member with ID {0} not found." CANNOT_REMOVE_POC = "POC cannot be removed from a team. Reassign the POC first." @@ -84,6 +85,7 @@ class ValidationErrors: SEARCH_QUERY_EMPTY = "Search query cannot be empty" TASK_ID_STRING_REQUIRED = "Task ID must be a string." INVALID_IS_ACTIVE_VALUE = "Invalid value for is_active" + POC_NOT_PROVIDED = "POC is required for team update" # Auth messages diff --git a/todo/repositories/task_repository.py b/todo/repositories/task_repository.py index 038d6540..50d5cc3c 100644 --- a/todo/repositories/task_repository.py +++ b/todo/repositories/task_repository.py @@ -130,8 +130,13 @@ def _get_assigned_task_ids_for_user(cls, user_id: str) -> List[ObjectId]: if team_ids: # Get teams where user is POC poc_teams = TeamRepository.get_collection().find( - {"_id": {"$in": [ObjectId(team_id) for team_id in team_ids]}, "is_deleted": False, "poc_id": user_id} + { + "_id": {"$in": [ObjectId(team_id) for team_id in team_ids]}, + "is_deleted": False, + "poc_id": {"$in": [ObjectId(user_id), user_id]}, + } ) + poc_team_ids = [str(team["_id"]) for team in poc_teams] # Get team assignments for POC teams diff --git a/todo/serializers/update_team_serializer.py b/todo/serializers/update_team_serializer.py index 0c6a0deb..fd0f5928 100644 --- a/todo/serializers/update_team_serializer.py +++ b/todo/serializers/update_team_serializer.py @@ -7,23 +7,10 @@ class UpdateTeamSerializer(serializers.Serializer): """ Serializer for updating team details. - All fields are optional for PATCH operations. + """ - name = serializers.CharField(max_length=100, required=False, allow_blank=False) - description = serializers.CharField(max_length=500, required=False, allow_blank=True, allow_null=True) poc_id = serializers.CharField(required=False, allow_null=True, allow_blank=False) - member_ids = serializers.ListField(child=serializers.CharField(), required=False, allow_empty=True, default=None) - - def validate_name(self, value): - if value is not None and not value.strip(): - raise serializers.ValidationError("Team name cannot be blank") - return value.strip() if value else None - - def validate_description(self, value): - if value is not None: - return value.strip() - return value def validate_poc_id(self, value): if not value or not value.strip(): @@ -32,10 +19,8 @@ def validate_poc_id(self, value): raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) return value - def validate_member_ids(self, value): - if value is None: - return value - for member_id in value: - if not ObjectId.is_valid(member_id): - raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(member_id)) - return value + def validate(self, data): + """Validate that the POC ID is provided if updating the POC.""" + if data.get("poc_id") is None: + raise serializers.ValidationError(ValidationErrors.POC_NOT_PROVIDED) + return data diff --git a/todo/services/team_service.py b/todo/services/team_service.py index a8a3c736..8e2fa4cd 100644 --- a/todo/services/team_service.py +++ b/todo/services/team_service.py @@ -1,12 +1,11 @@ from todo.dto.team_dto import CreateTeamDTO, TeamDTO -from todo.dto.update_team_dto import UpdateTeamDTO from todo.dto.responses.create_team_response import CreateTeamResponse from todo.dto.responses.get_user_teams_response import GetUserTeamsResponse from todo.models.team import TeamModel, UserTeamDetailsModel from todo.models.common.pyobjectid import PyObjectId from todo.repositories.team_creation_invite_code_repository import TeamCreationInviteCodeRepository from todo.repositories.team_repository import TeamRepository, UserTeamDetailsRepository -from todo.constants.messages import AppMessages +from todo.constants.messages import AppMessages, ApiErrors from todo.constants.role import RoleName from todo.utils.invite_code_utils import generate_invite_code from typing import List @@ -23,6 +22,8 @@ CannotRemoveTeamPOCException, ) +from todo.exceptions.user_exceptions import UserNotFoundException + DEFAULT_ROLE_ID = "1" @@ -324,14 +325,14 @@ def join_team_by_invite_code(cls, invite_code: str, user_id: str) -> TeamDTO: ) @classmethod - def update_team(cls, team_id: str, dto: UpdateTeamDTO, updated_by_user_id: str) -> TeamDTO: + def update_team(cls, team_id: str, poc_id: str, user_id: str) -> TeamDTO: """ Update a team by its ID. Args: team_id: ID of the team to update - dto: Team update data including name, description, and POC - updated_by_user_id: ID of the user updating the team + poc_id: ID of the new POC + user_id: ID of the user updating the team Returns: TeamDTO with the updated team details @@ -339,40 +340,29 @@ def update_team(cls, team_id: str, dto: UpdateTeamDTO, updated_by_user_id: str) Raises: ValueError: If team update fails or team not found """ - try: - # Check if team exists - existing_team = TeamRepository.get_by_id(team_id) - if not existing_team: - raise ValueError(f"Team with id {team_id} not found") + existing_team = TeamRepository.get_by_id(team_id) + if not existing_team: + raise ValueError(f"Team with id {team_id} not found") + + cls._validate_is_user_team_admin(team_id, user_id, existing_team) - # Prepare update data - update_data = {} - if dto.name is not None: - update_data["name"] = dto.name - if dto.description is not None: - update_data["description"] = dto.description - if dto.poc_id is not None: - update_data["poc_id"] = PyObjectId(dto.poc_id) if dto.poc_id and dto.poc_id.strip() else None + update_data = {} + if poc_id is not None: + cls._validate_is_user_team_member(team_id, poc_id) + update_data["poc_id"] = PyObjectId(poc_id) + try: # Update the team - updated_team = TeamRepository.update(team_id, update_data, updated_by_user_id) + updated_team = TeamRepository.update(team_id, update_data, user_id) if not updated_team: raise ValueError(f"Failed to update team with id {team_id}") - # Handle member updates if provided - if dto.member_ids is not None: - from todo.repositories.team_repository import UserTeamDetailsRepository - - success = UserTeamDetailsRepository.update_team_members(team_id, dto.member_ids, updated_by_user_id) - if not success: - raise ValueError(f"Failed to update team members for team with id {team_id}") - # Audit log for team update AuditLogRepository.create( AuditLogModel( team_id=PyObjectId(team_id), action="team_updated", - performed_by=PyObjectId(updated_by_user_id), + performed_by=PyObjectId(user_id), ) ) @@ -388,7 +378,6 @@ def update_team(cls, team_id: str, dto: UpdateTeamDTO, updated_by_user_id: str) created_at=updated_team.created_at, updated_at=updated_team.updated_at, ) - except Exception as e: raise ValueError(f"Failed to update team: {str(e)}") @@ -530,3 +519,25 @@ def remove_member_from_team(cls, user_id: str, team_id: str, removed_by_user_id: TaskAssignmentService.reassign_tasks_from_user_to_team(user_id, team_id, removed_by_user_id) return True + + @classmethod + def _validate_is_user_team_admin(cls, team_id: str, user_id: str, team): + """ + Validate that the user has admin role in the team. + """ + if str(team.created_by) == user_id: + return + + if UserRoleService.has_role(user_id, RoleName.ADMIN.value, RoleScope.TEAM.value, team_id): + return + + raise PermissionError(ApiErrors.UNAUTHORIZED_TITLE) + + @classmethod + def _validate_is_user_team_member(cls, team_id: str, poc_id: str): + """ + Validate that the user is a member of the team. + """ + + if not UserRoleService.has_role(poc_id, RoleName.MEMBER.value, RoleScope.TEAM.value, team_id): + raise UserNotFoundException(ApiErrors.MEMBER_NOT_FOUND.format(poc_id)) diff --git a/todo/tests/integration/test_team_update.py b/todo/tests/integration/test_team_update.py new file mode 100644 index 00000000..7608528b --- /dev/null +++ b/todo/tests/integration/test_team_update.py @@ -0,0 +1,165 @@ +from http import HTTPStatus +from django.urls import reverse +from bson import ObjectId +from datetime import datetime, timezone +import json + +from todo.tests.integration.base_mongo_test import AuthenticatedMongoTestCase +from todo.constants.messages import ApiErrors, ValidationErrors + + +class TeamUpdateIntegrationTests(AuthenticatedMongoTestCase): + def setUp(self): + super().setUp() + self.db.teams.delete_many({}) + self.db.user_roles.delete_many({}) + + self.team_id = str(ObjectId()) + self.owner_id = str(self.user_id) + self.admin_id = str(ObjectId()) + self.member_id = str(ObjectId()) + self.non_member_id = str(ObjectId()) + + team_doc = { + "_id": ObjectId(self.team_id), + "name": "Test Team", + "description": "Test Description", + "poc_id": ObjectId(self.member_id), + "invite_code": "TEST123", + "created_by": ObjectId(self.owner_id), + "updated_by": ObjectId(self.owner_id), + "created_at": datetime.now(timezone.utc), + "updated_at": datetime.now(timezone.utc), + "is_deleted": False, + } + self.db.teams.insert_one(team_doc) + + owner_role_doc = { + "_id": ObjectId(), + "name": "owner", + "scope": "TEAM", + "description": "Team Owner", + "created_at": datetime.now(timezone.utc), + "updated_at": datetime.now(timezone.utc), + } + self.db.roles.insert_one(owner_role_doc) + + member_role_doc = { + "_id": ObjectId(), + "name": "member", + "scope": "TEAM", + "description": "Team Member", + "created_at": datetime.now(timezone.utc), + "updated_at": datetime.now(timezone.utc), + } + self.db.roles.insert_one(member_role_doc) + + owner_role = { + "_id": ObjectId(), + "user_id": ObjectId(self.owner_id), + "role_id": owner_role_doc["_id"], + "role_name": "owner", + "scope": "TEAM", + "team_id": ObjectId(self.team_id), + "is_active": True, + "created_by": ObjectId(self.owner_id), + "created_at": datetime.now(timezone.utc), + } + self.db.user_roles.insert_one(owner_role) + + authenticated_user_role = { + "_id": ObjectId(), + "user_id": str(self.user_id), + "role_id": owner_role_doc["_id"], + "role_name": "owner", + "scope": "TEAM", + "team_id": str(self.team_id), + "is_active": True, + "created_by": str(self.owner_id), + "created_at": datetime.now(timezone.utc), + } + self.db.user_roles.insert_one(authenticated_user_role) + + member_role = { + "_id": ObjectId(), + "user_id": self.member_id, + "role_id": member_role_doc["_id"], + "role_name": "member", + "scope": "TEAM", + "team_id": self.team_id, + "is_active": True, + "created_by": self.owner_id, + "created_at": datetime.now(timezone.utc), + } + self.db.user_roles.insert_one(member_role) + + self.db.users.insert_one( + { + "_id": ObjectId(self.member_id), + "google_id": "member_google_id", + "email_id": "member@example.com", + "name": "Member User", + "picture": "member_picture", + "createdAt": datetime.now(timezone.utc), + "updatedAt": datetime.now(timezone.utc), + } + ) + + self.db.users.insert_one( + { + "_id": ObjectId(self.non_member_id), + "google_id": "non_member_google_id", + "email_id": "nonmember@example.com", + "name": "Non Member User", + "picture": "non_member_picture", + "createdAt": datetime.now(timezone.utc), + "updatedAt": datetime.now(timezone.utc), + } + ) + + self.existing_team_id = self.team_id + self.non_existent_id = str(ObjectId()) + self.invalid_team_id = "invalid-team-id" + + def test_update_team_success_by_owner(self): + url = reverse("team_detail", args=[self.existing_team_id]) + response = self.client.patch( + url, + data=json.dumps({"poc_id": self.member_id}), + content_type="application/json", + ) + + self.assertEqual(response.status_code, HTTPStatus.OK) + data = response.json() + self.assertEqual(data["poc_id"], self.member_id) + self.assertNotIn("invite_code", data) + + def test_update_team_unauthorized_user(self): + other_user_id = ObjectId() + self._create_test_user(other_user_id) + self._set_auth_cookies() + + url = reverse("team_detail", args=[self.existing_team_id]) + response = self.client.patch(url, data=json.dumps({"name": "Updated Team"}), content_type="application/json") + + self.assertEqual(response.status_code, HTTPStatus.FORBIDDEN) + data = response.json() + self.assertEqual(data["detail"], ApiErrors.UNAUTHORIZED_TITLE) + + def test_update_team_empty_payload(self): + url = reverse("team_detail", args=[self.existing_team_id]) + response = self.client.patch(url, data=json.dumps({}), content_type="application/json") + + self.assertEqual(response.status_code, HTTPStatus.BAD_REQUEST) + data = response.json() + self.assertIn("non_field_errors", data["errors"]) + self.assertIn(ValidationErrors.POC_NOT_PROVIDED, str(data["errors"]["non_field_errors"])) + + def test_update_team_invalid_poc_id_format(self): + url = reverse("team_detail", args=[self.existing_team_id]) + response = self.client.patch(url, data=json.dumps({"poc_id": "invalid-id"}), content_type="application/json") + + self.assertEqual(response.status_code, HTTPStatus.BAD_REQUEST) + data = response.json() + self.assertIn("poc_id", data["errors"]) + self.assertIn(ValidationErrors.INVALID_OBJECT_ID.format("invalid-id"), str(data["errors"]["poc_id"])) diff --git a/todo/tests/unit/services/test_team_service.py b/todo/tests/unit/services/test_team_service.py index 6e3b8f30..21d3a1e0 100644 --- a/todo/tests/unit/services/test_team_service.py +++ b/todo/tests/unit/services/test_team_service.py @@ -2,22 +2,27 @@ from unittest.mock import patch from datetime import datetime, timezone +from todo.constants.messages import ApiErrors from todo.exceptions.team_exceptions import ( CannotRemoveOwnerException, CannotRemoveTeamPOCException, NotTeamAdminException, ) +from todo.exceptions.user_exceptions import UserNotFoundException from todo.services.team_service import TeamService from todo.dto.responses.get_user_teams_response import GetUserTeamsResponse from todo.models.team import TeamModel, UserTeamDetailsModel from todo.models.common.pyobjectid import PyObjectId from todo.dto.user_dto import UserDTO from todo.dto.team_dto import TeamDTO +from todo.constants.role import RoleName, RoleScope class TeamServiceTests(TestCase): def setUp(self): self.user_id = "507f1f77bcf86cd799439011" + self.owner_id = "507f1f77bcf86cd799439011" + self.admin_user_id = "507f1f77bcf86cd799439020" self.team_id = "507f1f77bcf86cd799439012" self.poc_id = "507f1f77bcf86cd799439014" self.admin_id = "507f1f77bcf86cd799439015" @@ -54,6 +59,9 @@ def setUp(self): updated_at=datetime.now(timezone.utc), ) + self.user_model = UserDTO( + id=self.member_id, name="Test User", addedOn=datetime.now(timezone.utc), tasksAssignedCount=0 + ) # Mock user team details model self.user_team_details = UserTeamDetailsModel( id=PyObjectId("507f1f77bcf86cd799439013"), @@ -300,3 +308,111 @@ def test_user_can_remove_themselves( self.assertEqual(log_entry.action, "member_left_team") self.assertEqual(str(log_entry.team_id), self.team_id) self.assertEqual(str(log_entry.performed_by), self.member_id) + + @patch("todo.services.team_service.UserRoleService.has_role") + def test_validate_is_team_admin_success(self, mock_has_role): + mock_has_role.return_value = True + + result = TeamService._validate_is_user_team_admin(self.team_id, self.admin_id, self.team_model) + self.assertIsNone(result) + mock_has_role.assert_called_once_with(self.admin_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) + + def test_validate_is_team_admin_success_for_owner(self): + result = TeamService._validate_is_user_team_admin(self.team_id, self.owner_id, self.team_model) + self.assertIsNone(result) + + @patch("todo.services.team_service.UserRoleService.has_role") + def test_validate_is_team_admin_fails_for_regular_member(self, mock_has_role): + mock_has_role.return_value = False + + with self.assertRaises(PermissionError) as context: + TeamService._validate_is_user_team_admin(self.team_id, self.member_id, self.team_model) + + self.assertIn(ApiErrors.UNAUTHORIZED_TITLE, str(context.exception)) + mock_has_role.assert_called_once_with(self.member_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) + + @patch("todo.dto.update_team_dto.UserRepository.get_by_id") + @patch("todo.services.team_service.TeamRepository.get_by_id") + @patch("todo.services.team_service.TeamRepository.update") + @patch("todo.services.team_service.AuditLogRepository.create") + @patch("todo.services.team_service.UserRoleService.has_role") + def test_update_team_success_by_admin( + self, mock_has_role, mock_audit_log_create, mock_team_update, mock_team_get, mock_user_get + ): + mock_team_get.return_value = self.team_model + mock_team_update.return_value = self.team_model + mock_has_role.return_value = True + mock_user_get.return_value = UserDTO( + id=self.member_id, name="Test User", addedOn=datetime.now(timezone.utc), tasksAssignedCount=1 + ) + + result = TeamService.update_team( + team_id=self.team_id, + poc_id=self.member_id, + user_id=self.admin_id, + ) + + self.assertIsInstance(result, TeamDTO) + mock_team_get.assert_called_once_with(self.team_id) + mock_team_update.assert_called_once() + mock_audit_log_create.assert_called_once() + self.assertEqual(mock_has_role.call_count, 2) + mock_has_role.assert_any_call(self.admin_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) + mock_has_role.assert_any_call(self.member_id, RoleName.MEMBER.value, RoleScope.TEAM.value, self.team_id) + + @patch("todo.services.team_service.TeamRepository.get_by_id") + @patch("todo.services.team_service.TeamRepository.update") + @patch("todo.services.team_service.AuditLogRepository.create") + @patch("todo.repositories.user_repository.UserRepository.get_by_id") + @patch("todo.services.team_service.UserRoleService.has_role") + def test_update_team_success_by_owner( + self, mock_has_role, mock_user_get, mock_audit_log_create, mock_team_update, mock_team_get + ): + mock_team_get.return_value = self.team_model + mock_team_update.return_value = self.team_model + mock_user_get.return_value = self.user_model + mock_has_role.return_value = True + + result = TeamService.update_team( + team_id=self.team_id, + poc_id=self.member_id, + user_id=self.owner_id, + ) + + self.assertIsInstance(result, TeamDTO) + mock_team_get.assert_called_once_with(self.team_id) + mock_team_update.assert_called_once() + mock_audit_log_create.assert_called_once() + + @patch("todo.services.team_service.TeamRepository.get_by_id") + @patch("todo.services.team_service.UserRoleService.has_role") + def test_update_team_fails_for_non_admin(self, mock_has_role, mock_team_get): + mock_team_get.return_value = self.team_model + mock_has_role.return_value = False + + with self.assertRaises(PermissionError) as context: + TeamService.update_team(team_id=self.team_id, poc_id=self.member_id, user_id=self.member_id) + + self.assertIn(ApiErrors.UNAUTHORIZED_TITLE, str(context.exception)) + mock_has_role.assert_called_once_with(self.member_id, RoleName.ADMIN.value, RoleScope.TEAM.value, self.team_id) + + @patch("todo.dto.update_team_dto.UserRepository.get_by_id") + @patch("todo.services.team_service.TeamRepository.update") + @patch("todo.services.team_service.TeamRepository.get_by_id") + @patch("todo.services.team_service.UserRoleService.has_role") + def test_update_team_poc_not_team_member_raises_error( + self, mock_has_role, mock_team_get, mock_team_update, mock_user_get + ): + mock_team_get.return_value = self.team_model + mock_team_update.return_value = self.team_model + mock_user_get.return_value = UserDTO( + id=self.member_id, name="Test User", addedOn=datetime.now(timezone.utc), tasksAssignedCount=1 + ) + mock_has_role.side_effect = [True, False] + + with self.assertRaises(UserNotFoundException) as context: + TeamService.update_team(team_id=self.team_id, poc_id=self.member_id, user_id=self.admin_user_id) + + self.assertIn("Member with ID", str(context.exception)) + self.assertEqual(mock_has_role.call_count, 2) + mock_has_role.assert_any_call(self.member_id, RoleName.MEMBER.value, RoleScope.TEAM.value, self.team_id) diff --git a/todo/views/team.py b/todo/views/team.py index 27c56e37..39eee957 100644 --- a/todo/views/team.py +++ b/todo/views/team.py @@ -9,7 +9,6 @@ from todo.serializers.add_team_member_serializer import AddTeamMemberSerializer from todo.services.team_service import TeamService from todo.dto.team_dto import CreateTeamDTO -from todo.dto.update_team_dto import UpdateTeamDTO from todo.dto.responses.create_team_response import CreateTeamResponse from todo.dto.responses.get_user_teams_response import GetUserTeamsResponse from todo.dto.responses.error_response import ApiErrorResponse, ApiErrorDetail, ApiErrorSource @@ -125,6 +124,10 @@ def _handle_validation_errors(self, errors): class TeamDetailView(APIView): + def _handle_validation_errors(self, errors): + """Handle validation errors.""" + return Response(data={"errors": errors}, status=status.HTTP_400_BAD_REQUEST) + @extend_schema( operation_id="get_team_by_id", summary="Get team by ID", @@ -211,32 +214,13 @@ def patch(self, request: Request, team_id: str): if not serializer.is_valid(): return self._handle_validation_errors(serializer.errors) - try: - dto = UpdateTeamDTO(**serializer.validated_data) - updated_by_user_id = request.user_id - response: TeamDTO = TeamService.update_team(team_id, dto, updated_by_user_id) - data = response.model_dump(mode="json") - data.pop("invite_code", None) - return Response(data=data, status=status.HTTP_200_OK) + poc_id = serializer.validated_data.get("poc_id") - except ValueError as e: - if isinstance(e.args[0], ApiErrorResponse): - error_response = e.args[0] - return Response(data=error_response.model_dump(mode="json"), status=error_response.statusCode) - - fallback_response = ApiErrorResponse( - statusCode=404, - message=str(e), - errors=[{"detail": str(e)}], - ) - return Response(data=fallback_response.model_dump(mode="json"), status=404) - except Exception as e: - fallback_response = ApiErrorResponse( - statusCode=500, - message=ApiErrors.UNEXPECTED_ERROR_OCCURRED, - errors=[{"detail": str(e) if settings.DEBUG else ApiErrors.INTERNAL_SERVER_ERROR}], - ) - return Response(data=fallback_response.model_dump(mode="json"), status=500) + user_id = request.user_id + response: TeamDTO = TeamService.update_team(team_id, poc_id, user_id) + data = response.model_dump(mode="json") + data.pop("invite_code", None) + return Response(data=data, status=status.HTTP_200_OK) class JoinTeamByInviteCodeView(APIView): From 9b72c56d5808f79c1a3763dde1d6820ac4182d61 Mon Sep 17 00:00:00 2001 From: anujchhikara Date: Sat, 18 Oct 2025 02:32:17 +0530 Subject: [PATCH 2/3] refactor(team): improve user membership validation and error handling in team updates --- todo/constants/messages.py | 2 +- todo/exceptions/exception_handler.py | 3 ++- todo/services/team_service.py | 15 +++++++++++---- todo/tests/unit/services/test_team_service.py | 7 +++---- todo/views/team.py | 5 ++++- 5 files changed, 21 insertions(+), 11 deletions(-) diff --git a/todo/constants/messages.py b/todo/constants/messages.py index 777b56e8..226a3159 100644 --- a/todo/constants/messages.py +++ b/todo/constants/messages.py @@ -55,7 +55,6 @@ class ApiErrors: SEARCH_QUERY_EMPTY = "Search query cannot be empty" TASK_ALREADY_IN_WATCHLIST = "Task is already in the watchlist" CANNOT_REMOVE_OWNER = "Owner cannot be removed from the team" - MEMBER_NOT_FOUND = "Member with ID {0} not found." CANNOT_REMOVE_POC = "POC cannot be removed from a team. Reassign the POC first." @@ -85,6 +84,7 @@ class ValidationErrors: SEARCH_QUERY_EMPTY = "Search query cannot be empty" TASK_ID_STRING_REQUIRED = "Task ID must be a string." INVALID_IS_ACTIVE_VALUE = "Invalid value for is_active" + USER_NOT_TEAM_MEMBER = "User is not a member of the team" POC_NOT_PROVIDED = "POC is required for team update" diff --git a/todo/exceptions/exception_handler.py b/todo/exceptions/exception_handler.py index b52f06cb..b7c415f7 100644 --- a/todo/exceptions/exception_handler.py +++ b/todo/exceptions/exception_handler.py @@ -20,10 +20,11 @@ TokenInvalidError, RefreshTokenExpiredError, APIException, - UserNotFoundException, TokenMissingError, ) +from todo.exceptions.user_exceptions import UserNotFoundException + def format_validation_errors(errors) -> List[ApiErrorDetail]: formatted_errors = [] diff --git a/todo/services/team_service.py b/todo/services/team_service.py index 8e2fa4cd..54367a08 100644 --- a/todo/services/team_service.py +++ b/todo/services/team_service.py @@ -5,7 +5,7 @@ from todo.models.common.pyobjectid import PyObjectId from todo.repositories.team_creation_invite_code_repository import TeamCreationInviteCodeRepository from todo.repositories.team_repository import TeamRepository, UserTeamDetailsRepository -from todo.constants.messages import AppMessages, ApiErrors +from todo.constants.messages import AppMessages, ApiErrors, ValidationErrors from todo.constants.role import RoleName from todo.utils.invite_code_utils import generate_invite_code from typing import List @@ -22,7 +22,6 @@ CannotRemoveTeamPOCException, ) -from todo.exceptions.user_exceptions import UserNotFoundException DEFAULT_ROLE_ID = "1" @@ -348,7 +347,9 @@ def update_team(cls, team_id: str, poc_id: str, user_id: str) -> TeamDTO: update_data = {} if poc_id is not None: - cls._validate_is_user_team_member(team_id, poc_id) + validation_error = cls._validate_is_user_team_member(team_id, poc_id) + if validation_error: + return validation_error update_data["poc_id"] = PyObjectId(poc_id) try: @@ -537,7 +538,13 @@ def _validate_is_user_team_admin(cls, team_id: str, user_id: str, team): def _validate_is_user_team_member(cls, team_id: str, poc_id: str): """ Validate that the user is a member of the team. + Returns ApiErrorResponse if validation fails, None if validation passes. """ if not UserRoleService.has_role(poc_id, RoleName.MEMBER.value, RoleScope.TEAM.value, team_id): - raise UserNotFoundException(ApiErrors.MEMBER_NOT_FOUND.format(poc_id)) + return ApiErrorResponse( + statusCode=400, + message=ValidationErrors.USER_NOT_TEAM_MEMBER, + errors=[ApiErrorDetail(detail=ValidationErrors.USER_NOT_TEAM_MEMBER)], + ) + return diff --git a/todo/tests/unit/services/test_team_service.py b/todo/tests/unit/services/test_team_service.py index 21d3a1e0..2c388243 100644 --- a/todo/tests/unit/services/test_team_service.py +++ b/todo/tests/unit/services/test_team_service.py @@ -8,7 +8,6 @@ CannotRemoveTeamPOCException, NotTeamAdminException, ) -from todo.exceptions.user_exceptions import UserNotFoundException from todo.services.team_service import TeamService from todo.dto.responses.get_user_teams_response import GetUserTeamsResponse from todo.models.team import TeamModel, UserTeamDetailsModel @@ -410,9 +409,9 @@ def test_update_team_poc_not_team_member_raises_error( ) mock_has_role.side_effect = [True, False] - with self.assertRaises(UserNotFoundException) as context: - TeamService.update_team(team_id=self.team_id, poc_id=self.member_id, user_id=self.admin_user_id) + result = TeamService.update_team(team_id=self.team_id, poc_id=self.member_id, user_id=self.admin_user_id) - self.assertIn("Member with ID", str(context.exception)) + self.assertIsNotNone(result) + self.assertIn("User is not a member of the team", str(result)) self.assertEqual(mock_has_role.call_count, 2) mock_has_role.assert_any_call(self.member_id, RoleName.MEMBER.value, RoleScope.TEAM.value, self.team_id) diff --git a/todo/views/team.py b/todo/views/team.py index 39eee957..f9451f0f 100644 --- a/todo/views/team.py +++ b/todo/views/team.py @@ -217,7 +217,10 @@ def patch(self, request: Request, team_id: str): poc_id = serializer.validated_data.get("poc_id") user_id = request.user_id - response: TeamDTO = TeamService.update_team(team_id, poc_id, user_id) + response = TeamService.update_team(team_id, poc_id, user_id) + + if isinstance(response, ApiErrorResponse): + return Response(data=response.model_dump(mode="json"), status=response.statusCode) data = response.model_dump(mode="json") data.pop("invite_code", None) return Response(data=data, status=status.HTTP_200_OK) From 1454f4e0218e4eb16f8e3468817ae5a12a670865 Mon Sep 17 00:00:00 2001 From: anujchhikara Date: Sat, 18 Oct 2025 02:43:02 +0530 Subject: [PATCH 3/3] refactor(team): enforce required POC ID in team updates and streamline validation logic --- todo/serializers/update_team_serializer.py | 10 +--------- todo/services/team_service.py | 10 +++++----- todo/tests/integration/test_team_update.py | 9 --------- 3 files changed, 6 insertions(+), 23 deletions(-) diff --git a/todo/serializers/update_team_serializer.py b/todo/serializers/update_team_serializer.py index fd0f5928..3d0f2eea 100644 --- a/todo/serializers/update_team_serializer.py +++ b/todo/serializers/update_team_serializer.py @@ -10,17 +10,9 @@ class UpdateTeamSerializer(serializers.Serializer): """ - poc_id = serializers.CharField(required=False, allow_null=True, allow_blank=False) + poc_id = serializers.CharField(required=True, allow_null=False, allow_blank=False) def validate_poc_id(self, value): - if not value or not value.strip(): - return None if not ObjectId.is_valid(value): raise serializers.ValidationError(ValidationErrors.INVALID_OBJECT_ID.format(value)) return value - - def validate(self, data): - """Validate that the POC ID is provided if updating the POC.""" - if data.get("poc_id") is None: - raise serializers.ValidationError(ValidationErrors.POC_NOT_PROVIDED) - return data diff --git a/todo/services/team_service.py b/todo/services/team_service.py index 54367a08..93c735ee 100644 --- a/todo/services/team_service.py +++ b/todo/services/team_service.py @@ -346,11 +346,11 @@ def update_team(cls, team_id: str, poc_id: str, user_id: str) -> TeamDTO: cls._validate_is_user_team_admin(team_id, user_id, existing_team) update_data = {} - if poc_id is not None: - validation_error = cls._validate_is_user_team_member(team_id, poc_id) - if validation_error: - return validation_error - update_data["poc_id"] = PyObjectId(poc_id) + + validation_error = cls._validate_is_user_team_member(team_id, poc_id) + if validation_error: + return validation_error + update_data["poc_id"] = PyObjectId(poc_id) try: # Update the team diff --git a/todo/tests/integration/test_team_update.py b/todo/tests/integration/test_team_update.py index 7608528b..9f0eed27 100644 --- a/todo/tests/integration/test_team_update.py +++ b/todo/tests/integration/test_team_update.py @@ -146,15 +146,6 @@ def test_update_team_unauthorized_user(self): data = response.json() self.assertEqual(data["detail"], ApiErrors.UNAUTHORIZED_TITLE) - def test_update_team_empty_payload(self): - url = reverse("team_detail", args=[self.existing_team_id]) - response = self.client.patch(url, data=json.dumps({}), content_type="application/json") - - self.assertEqual(response.status_code, HTTPStatus.BAD_REQUEST) - data = response.json() - self.assertIn("non_field_errors", data["errors"]) - self.assertIn(ValidationErrors.POC_NOT_PROVIDED, str(data["errors"]["non_field_errors"])) - def test_update_team_invalid_poc_id_format(self): url = reverse("team_detail", args=[self.existing_team_id]) response = self.client.patch(url, data=json.dumps({"poc_id": "invalid-id"}), content_type="application/json")