diff --git a/backend/druks/contrib/ship/routes.py b/backend/druks/contrib/ship/routes.py index 11c0be4b..f6c88403 100644 --- a/backend/druks/contrib/ship/routes.py +++ b/backend/druks/contrib/ship/routes.py @@ -1,7 +1,7 @@ import logging from fastapi import APIRouter, Body, Depends, HTTPException, Path, Query, Response, status -from sqlalchemy import func, select, update +from sqlalchemy import delete, func, select, update from druks.accounts.dependencies import current_account from druks.accounts.models import Account @@ -131,21 +131,15 @@ async def update_project( @projects_router.delete("/{project_id}", status_code=status.HTTP_204_NO_CONTENT) async def delete_project(project_id: int) -> None: - """Delete a project. Refuses when any WorkItem still points at it — - ``work_items.project_id`` is NOT NULL, so the operator must move - or delete the children first.""" + """Delete a project and everything it owns. A project's work items are its own + children, so they go with it — ``work_items.project_id`` is a plain FK, so the + cascade is an explicit child delete in the same session before the project (and + its repo ``delete-orphan`` cascade) is deleted.""" session = db_session() project = Project.get(project_id) if not project: raise HTTPException(status.HTTP_404_NOT_FOUND, "project not found") - referencing = session.scalar( - select(func.count()).select_from(WorkItem).where(WorkItem.project_id == project_id) - ) - if referencing: - raise HTTPException( - status.HTTP_409_CONFLICT, - f"{referencing} work item(s) still reference this project; move or delete them first.", - ) + session.execute(delete(WorkItem).where(WorkItem.project_id == project_id)) session.delete(project) session.flush() diff --git a/backend/tests/ship/test_project_repo_routes.py b/backend/tests/ship/test_project_repo_routes.py index 76cea72a..c8f0d96a 100644 --- a/backend/tests/ship/test_project_repo_routes.py +++ b/backend/tests/ship/test_project_repo_routes.py @@ -107,6 +107,50 @@ def test_nested_repo_routes_are_scoped_to_their_project(client: TestClient, monk assert ProjectRepo.get(repo_id) is None +def _make_work_item(project_id: int, ticket_key: str, *, resolved: bool = False): + from datetime import datetime + + from druks.contrib.ship.models import WorkItem + + item = WorkItem.create( + project_id=project_id, + title=ticket_key, + ticket_key=ticket_key, + repo="acme/widget", + ) + if resolved: + item.resolution = "closed" + item.resolved_at = datetime(2026, 1, 1) + return item + + +def test_deleting_a_project_cascades_its_work_items_and_spares_others(client: TestClient, druks_db): + """DELETE cascades: the project and every work item it owns go, with no 409 + reference guard — while another project's graph is left fully intact.""" + from druks.contrib.ship.models import Project, WorkItem + + target = Project.create(name="Target") + control = Project.create(name="Control") + doomed = _make_work_item(target.id, "ENG-1") + doomed_resolved = _make_work_item(target.id, "ENG-2", resolved=True) + survivor = _make_work_item(control.id, "ENG-3") + target_id, control_id = target.id, control.id + doomed_id, doomed_resolved_id, survivor_id = doomed.id, doomed_resolved.id, survivor.id + + response = client.delete(f"/api/ship/projects/{target_id}") + + assert response.status_code == 204 + # The route committed on its own session; drop this session's identity map so + # the reads below reflect the committed graph rather than cached instances. + druks_db.expire_all() + assert Project.get(target_id) is None + assert WorkItem.get(doomed_id) is None + assert WorkItem.get(doomed_resolved_id) is None + # The control project and its work item are untouched. + assert Project.get(control_id) is not None + assert WorkItem.get(survivor_id) is not None + + def test_the_repo_subject_read_side_mounts(client: TestClient, druks_db): """Profile is about a repo, so the repo gets the board and the page it never had — its own runs' status and timeline, keyed by the repo's id.""" diff --git a/frontend/src/extensions/ship/projects/ProjectsPage.test.tsx b/frontend/src/extensions/ship/projects/ProjectsPage.test.tsx new file mode 100644 index 00000000..3da702a6 --- /dev/null +++ b/frontend/src/extensions/ship/projects/ProjectsPage.test.tsx @@ -0,0 +1,90 @@ +import { QueryClient, QueryClientProvider } from '@tanstack/react-query' +import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { afterEach, describe, expect, it, vi } from 'vitest' + +import { projectsApi } from './api' +import { ProjectsPage } from './ProjectsPage' +import type { Project, ProjectsResponse } from './types' + +// The page reads the projects list and the repo board through projectsApi and +// deletes through it — stub the module so the test drives those directly. +vi.mock('./api', () => ({ + projectsApi: { + list: vi.fn(), + repoBoard: vi.fn(), + delete: vi.fn(), + }, +})) + +const listMock = vi.mocked(projectsApi.list) +const repoBoardMock = vi.mocked(projectsApi.repoBoard) +const deleteMock = vi.mocked(projectsApi.delete) + +function project(overrides: Partial = {}): Project { + return { + id: 7, + name: 'Target', + createdAt: '2026-01-01T00:00:00Z', + updatedAt: '2026-01-01T00:00:00Z', + repos: [], + ...overrides, + } +} + +function renderPage(projects: Project[]) { + listMock.mockResolvedValue({ projects } satisfies ProjectsResponse) + repoBoardMock.mockResolvedValue({ rows: [] }) + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }) + return render( + + + , + ) +} + +afterEach(() => { + cleanup() + vi.clearAllMocks() + vi.restoreAllMocks() +}) + +describe('ProjectsPage delete', () => { + it('confirms with the project name and destructive scope, and sends no DELETE on cancel', async () => { + const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(false) + renderPage([project({ name: 'Target' })]) + + fireEvent.click(await screen.findByText('delete')) + + expect(confirmSpy).toHaveBeenCalledTimes(1) + const prompt = confirmSpy.mock.calls[0]![0] as string + expect(prompt).toContain('Target') + expect(prompt).toContain('every work item it owns') + expect(deleteMock).not.toHaveBeenCalled() + }) + + it('sends the DELETE for the project once confirmed', async () => { + vi.spyOn(window, 'confirm').mockReturnValue(true) + deleteMock.mockResolvedValue(undefined) + renderPage([project({ id: 7, name: 'Target' })]) + + fireEvent.click(await screen.findByText('delete')) + + await waitFor(() => expect(deleteMock).toHaveBeenCalledWith(7)) + expect(deleteMock).toHaveBeenCalledTimes(1) + }) + + it('surfaces a failed delete in an error toast and leaves the card in place', async () => { + vi.spyOn(window, 'confirm').mockReturnValue(true) + deleteMock.mockRejectedValue(new Error('project is locked')) + renderPage([project({ name: 'Target' })]) + + fireEvent.click(await screen.findByText('delete')) + + const toast = await screen.findByRole('alert') + expect(toast.textContent).toContain('project is locked') + // The card is still rendered — the failure was surfaced, not swallowed. + expect(screen.getByText('Target')).toBeTruthy() + }) +}) diff --git a/frontend/src/extensions/ship/projects/ProjectsPage.tsx b/frontend/src/extensions/ship/projects/ProjectsPage.tsx index 8692cc6d..4c53b921 100644 --- a/frontend/src/extensions/ship/projects/ProjectsPage.tsx +++ b/frontend/src/extensions/ship/projects/ProjectsPage.tsx @@ -3,6 +3,7 @@ import { useState } from 'react' import { EmptyState } from '../../../components/EmptyState' import { Page } from '../../../components/Page' +import { useFlashNote } from '../../../lib/useFlashNote' import { projectsApi } from './api' import { repoProfiling, useRepoRuns, type RepoProfiling } from './profiling' import type { Project, ProjectRepo } from './types' @@ -26,6 +27,9 @@ export function ProjectsPage() { }) const [draft, setDraft] = useState('') + // A project delete can still fail (a race, a server error); surface it here at + // page level as a transient error toast so it's never a silent no-op. + const [deleteError, setDeleteError] = useFlashNote() const createMutation = useMutation({ mutationFn: projectsApi.create, onSuccess: () => { @@ -56,6 +60,11 @@ export function ProjectsPage() { return (
+ {deleteError && ( +
+ {deleteError} +
+ )}
Projects ({data.projects.length}) @@ -92,7 +101,7 @@ export function ProjectsPage() { {String(createMutation.error)} )} {data.projects.map((p) => ( - + ))}
)} @@ -138,7 +147,13 @@ function CreateRow({ ) } -function ProjectCard({ project }: { project: Project }) { +function ProjectCard({ + project, + onDeleteError, +}: { + project: Project + onDeleteError: (message: string) => void +}) { const queryClient = useQueryClient() const invalidate = () => void queryClient.invalidateQueries({ queryKey: ['projects'] }) @@ -157,6 +172,9 @@ function ProjectCard({ project }: { project: Project }) { const remove = useMutation({ mutationFn: () => projectsApi.delete(project.id), onSuccess: invalidate, + // Leave the card in place and surface the reason at page level — never a + // silent no-op, which is the failure ENG-846 set out to fix. + onError: (error) => onDeleteError(String(error)), }) const repoCount = `${project.repos.length} ${project.repos.length === 1 ? 'repo' : 'repos'}` @@ -213,7 +231,7 @@ function ProjectCard({ project }: { project: Project }) { onClick={() => { if ( confirm( - `Delete project "${project.name}"? Move or delete its work items first — a project still referenced by work items can't be deleted.`, + `Delete project "${project.name}"? This permanently deletes the project and every work item it owns.`, ) ) { remove.mutate() diff --git a/frontend/src/styles.css b/frontend/src/styles.css index 2d853d38..4bb3e642 100644 --- a/frontend/src/styles.css +++ b/frontend/src/styles.css @@ -2066,6 +2066,16 @@ textarea.set-textarea { background-image: none; height: auto; min-height: 96px; .pj-err { display: block; margin-top: 8px; font-size: 11px; color: var(--bucket-dead); } +/* page-level transient toast — e.g. a project delete that failed */ +.pj-toast { + margin-bottom: 16px; padding: 10px 14px; border-radius: 4px; font-size: 12px; +} +.pj-toast-error { + color: var(--bucket-dead); + border: 1px solid color-mix(in oklch, var(--bucket-dead) 45%, transparent); + background: color-mix(in oklch, var(--bucket-dead) 12%, transparent); +} + /* create-project row */ .pj-create { display: flex; gap: 10px; margin-bottom: 26px; } .pj-create-input {