fix: address code review issues and improve code quality
- Add owner_id check in repository delete operation - Fix time range validation for partial updates - Wrap pre-query in try/except for consistent error handling - Use default_factory instead of mutable defaults - Add max_length constraint for timezone field - Remove unused dependencies and empty validators - Extract magic numbers to constants - Simplify update logic with model_dump
This commit is contained in:
@@ -93,33 +93,39 @@ class ScheduleItemService(BaseService):
|
||||
) -> ScheduleItemResponse:
|
||||
user_id = self.require_user_id()
|
||||
|
||||
existing = await self._repository.get_by_item_id(item_id, user_id)
|
||||
if existing is None:
|
||||
raise HTTPException(status_code=404, detail="Schedule item not found")
|
||||
|
||||
update_data: dict = {}
|
||||
if request.title is not None:
|
||||
update_data["title"] = request.title
|
||||
if request.description is not None:
|
||||
update_data["description"] = request.description
|
||||
if request.start_at is not None:
|
||||
update_data["start_at"] = request.start_at
|
||||
if request.end_at is not None:
|
||||
update_data["end_at"] = request.end_at
|
||||
if request.timezone is not None:
|
||||
update_data["timezone"] = request.timezone
|
||||
if request.status is not None:
|
||||
update_data["status"] = request.status
|
||||
if request.metadata is not None:
|
||||
update_data["metadata"] = request.metadata.model_dump()
|
||||
|
||||
if request.end_at and request.start_at and request.end_at <= request.start_at:
|
||||
raise HTTPException(status_code=400, detail="end_at must be after start_at")
|
||||
|
||||
if not update_data:
|
||||
return self._to_response(existing)
|
||||
|
||||
try:
|
||||
existing = await self._repository.get_by_item_id(item_id, user_id)
|
||||
if existing is None:
|
||||
raise HTTPException(status_code=404, detail="Schedule item not found")
|
||||
|
||||
update_data: dict = {}
|
||||
if request.title is not None:
|
||||
update_data["title"] = request.title
|
||||
if request.description is not None:
|
||||
update_data["description"] = request.description
|
||||
if request.start_at is not None:
|
||||
update_data["start_at"] = request.start_at
|
||||
if request.end_at is not None:
|
||||
update_data["end_at"] = request.end_at
|
||||
if request.timezone is not None:
|
||||
update_data["timezone"] = request.timezone
|
||||
if request.status is not None:
|
||||
update_data["status"] = request.status
|
||||
if request.metadata is not None:
|
||||
update_data["metadata"] = request.metadata.model_dump()
|
||||
|
||||
next_start = (
|
||||
request.start_at if request.start_at is not None else existing.start_at
|
||||
)
|
||||
next_end = request.end_at if request.end_at is not None else existing.end_at
|
||||
if next_end is not None and next_end <= next_start:
|
||||
raise HTTPException(
|
||||
status_code=400, detail="end_at must be after start_at"
|
||||
)
|
||||
|
||||
if not update_data:
|
||||
return self._to_response(existing)
|
||||
|
||||
item = await self._repository.update_by_item_id(
|
||||
item_id, user_id, update_data
|
||||
)
|
||||
@@ -139,11 +145,11 @@ class ScheduleItemService(BaseService):
|
||||
async def delete(self, item_id: UUID) -> None:
|
||||
user_id = self.require_user_id()
|
||||
|
||||
existing = await self._repository.get_by_item_id(item_id, user_id)
|
||||
if existing is None:
|
||||
raise HTTPException(status_code=404, detail="Schedule item not found")
|
||||
|
||||
try:
|
||||
existing = await self._repository.get_by_item_id(item_id, user_id)
|
||||
if existing is None:
|
||||
raise HTTPException(status_code=404, detail="Schedule item not found")
|
||||
|
||||
await self._repository.delete_by_item_id(item_id, user_id)
|
||||
await self._session.commit()
|
||||
except SQLAlchemyError:
|
||||
|
||||
Reference in New Issue
Block a user