Fix QA review issues: transaction safety, validation, accessibility
Critical fixes: - [C1] _reactivate_recurring_todos now uses flush + with_for_update instead of mid-request commit; get_todos commits the full transaction - [C2] recurrence_rule validated via Literal["daily","weekly","monthly"] in Pydantic schemas (rejects invalid values with 422) Warnings fixed: - [W3] Clear due_time when due_date is set to null in update endpoint Suggestions applied: - [S2] Wrap filteredTodos in useMemo for consistent memoization - [S6] Add aria-labels to edit/delete icon buttons Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
cd868bd6ea
commit
79b3097410
@ -73,7 +73,11 @@ def _calculate_recurrence(
|
|||||||
|
|
||||||
|
|
||||||
async def _reactivate_recurring_todos(db: AsyncSession) -> None:
|
async def _reactivate_recurring_todos(db: AsyncSession) -> None:
|
||||||
"""Auto-reactivate recurring todos whose reset_at has passed."""
|
"""Auto-reactivate recurring todos whose reset_at has passed.
|
||||||
|
|
||||||
|
Uses flush (not commit) so changes are visible to the subsequent query
|
||||||
|
within the same transaction. The caller's commit handles persistence.
|
||||||
|
"""
|
||||||
now = datetime.now()
|
now = datetime.now()
|
||||||
query = select(Todo).where(
|
query = select(Todo).where(
|
||||||
and_(
|
and_(
|
||||||
@ -82,7 +86,7 @@ async def _reactivate_recurring_todos(db: AsyncSession) -> None:
|
|||||||
Todo.reset_at.isnot(None),
|
Todo.reset_at.isnot(None),
|
||||||
Todo.reset_at <= now,
|
Todo.reset_at <= now,
|
||||||
)
|
)
|
||||||
)
|
).with_for_update()
|
||||||
result = await db.execute(query)
|
result = await db.execute(query)
|
||||||
todos = result.scalars().all()
|
todos = result.scalars().all()
|
||||||
|
|
||||||
@ -95,7 +99,7 @@ async def _reactivate_recurring_todos(db: AsyncSession) -> None:
|
|||||||
todo.next_due_date = None
|
todo.next_due_date = None
|
||||||
|
|
||||||
if todos:
|
if todos:
|
||||||
await db.commit()
|
await db.flush()
|
||||||
|
|
||||||
|
|
||||||
@router.get("/", response_model=List[TodoResponse])
|
@router.get("/", response_model=List[TodoResponse])
|
||||||
@ -130,6 +134,8 @@ async def get_todos(
|
|||||||
result = await db.execute(query)
|
result = await db.execute(query)
|
||||||
todos = result.scalars().all()
|
todos = result.scalars().all()
|
||||||
|
|
||||||
|
await db.commit()
|
||||||
|
|
||||||
return todos
|
return todos
|
||||||
|
|
||||||
|
|
||||||
@ -189,6 +195,10 @@ async def update_todo(
|
|||||||
update_data["reset_at"] = None
|
update_data["reset_at"] = None
|
||||||
update_data["next_due_date"] = None
|
update_data["next_due_date"] = None
|
||||||
|
|
||||||
|
# Clear due_time if due_date is being removed
|
||||||
|
if "due_date" in update_data and update_data["due_date"] is None:
|
||||||
|
update_data["due_time"] = None
|
||||||
|
|
||||||
for key, value in update_data.items():
|
for key, value in update_data.items():
|
||||||
setattr(todo, key, value)
|
setattr(todo, key, value)
|
||||||
|
|
||||||
|
|||||||
@ -3,6 +3,7 @@ from datetime import datetime, date, time
|
|||||||
from typing import Optional, Literal
|
from typing import Optional, Literal
|
||||||
|
|
||||||
TodoPriority = Literal["none", "low", "medium", "high"]
|
TodoPriority = Literal["none", "low", "medium", "high"]
|
||||||
|
RecurrenceRule = Literal["daily", "weekly", "monthly"]
|
||||||
|
|
||||||
|
|
||||||
class TodoCreate(BaseModel):
|
class TodoCreate(BaseModel):
|
||||||
@ -12,7 +13,7 @@ class TodoCreate(BaseModel):
|
|||||||
due_date: Optional[date] = None
|
due_date: Optional[date] = None
|
||||||
due_time: Optional[time] = None
|
due_time: Optional[time] = None
|
||||||
category: Optional[str] = None
|
category: Optional[str] = None
|
||||||
recurrence_rule: Optional[str] = None
|
recurrence_rule: Optional[RecurrenceRule] = None
|
||||||
project_id: Optional[int] = None
|
project_id: Optional[int] = None
|
||||||
|
|
||||||
|
|
||||||
@ -24,7 +25,7 @@ class TodoUpdate(BaseModel):
|
|||||||
due_time: Optional[time] = None
|
due_time: Optional[time] = None
|
||||||
completed: Optional[bool] = None
|
completed: Optional[bool] = None
|
||||||
category: Optional[str] = None
|
category: Optional[str] = None
|
||||||
recurrence_rule: Optional[str] = None
|
recurrence_rule: Optional[RecurrenceRule] = None
|
||||||
project_id: Optional[int] = None
|
project_id: Optional[int] = None
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -149,12 +149,13 @@ export default function TodoItem({ todo, onEdit }: TodoItemProps) {
|
|||||||
)}
|
)}
|
||||||
|
|
||||||
{/* Actions */}
|
{/* Actions */}
|
||||||
<Button variant="ghost" size="icon" onClick={() => onEdit(todo)} className="h-7 w-7 shrink-0">
|
<Button variant="ghost" size="icon" onClick={() => onEdit(todo)} className="h-7 w-7 shrink-0" aria-label="Edit todo">
|
||||||
<Pencil className="h-3 w-3" />
|
<Pencil className="h-3 w-3" />
|
||||||
</Button>
|
</Button>
|
||||||
<Button
|
<Button
|
||||||
variant="ghost"
|
variant="ghost"
|
||||||
size="icon"
|
size="icon"
|
||||||
|
aria-label="Delete todo"
|
||||||
onClick={() => deleteMutation.mutate()}
|
onClick={() => deleteMutation.mutate()}
|
||||||
disabled={deleteMutation.isPending}
|
disabled={deleteMutation.isPending}
|
||||||
className="h-7 w-7 shrink-0 hover:bg-destructive/10 hover:text-destructive"
|
className="h-7 w-7 shrink-0 hover:bg-destructive/10 hover:text-destructive"
|
||||||
|
|||||||
@ -45,7 +45,9 @@ export default function TodosPage() {
|
|||||||
return Array.from(cats).sort();
|
return Array.from(cats).sort();
|
||||||
}, [todos]);
|
}, [todos]);
|
||||||
|
|
||||||
const filteredTodos = todos.filter((todo) => {
|
const filteredTodos = useMemo(
|
||||||
|
() =>
|
||||||
|
todos.filter((todo) => {
|
||||||
if (filters.priority && todo.priority !== filters.priority) return false;
|
if (filters.priority && todo.priority !== filters.priority) return false;
|
||||||
if (filters.category && todo.category?.toLowerCase() !== filters.category.toLowerCase())
|
if (filters.category && todo.category?.toLowerCase() !== filters.category.toLowerCase())
|
||||||
return false;
|
return false;
|
||||||
@ -53,7 +55,9 @@ export default function TodosPage() {
|
|||||||
if (filters.search && !todo.title.toLowerCase().includes(filters.search.toLowerCase()))
|
if (filters.search && !todo.title.toLowerCase().includes(filters.search.toLowerCase()))
|
||||||
return false;
|
return false;
|
||||||
return true;
|
return true;
|
||||||
});
|
}),
|
||||||
|
[todos, filters]
|
||||||
|
);
|
||||||
|
|
||||||
const now = new Date();
|
const now = new Date();
|
||||||
const todayStr = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;
|
const todayStr = `${now.getFullYear()}-${String(now.getMonth() + 1).padStart(2, '0')}-${String(now.getDate()).padStart(2, '0')}`;
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user