fix(ui-react): delete a problem statement, task or thread bottom-up (#99)

ui-react/src/graphql/__tests__/modeling-delete-order.test.ts

@@ -0,0 +1,101 @@
+import { describe, it, expect } from 'vitest';
+import { print, type DocumentNode } from 'graphql';
+import {
+  DeleteProblemStatementDocument,
+  DeleteTaskDocument,
+  DeleteThreadDocument,
+} from '@/graphql/generated/modeling';
+
+/**
+ * Regression guard for #99.
+ *
+ * The `user` role may delete a problem statement, a task or a thread only while
+ * the row still carries a CREATE provenance event from that user. Every child
+ * delete on the tree is gated the same way, through the thread's provenance or
+ * permission rows.
+ *
+ * These documents used to delete the provenance and permission rows first —
+ * copied verbatim from the Lit app, which has the same bug. That revoked the
+ * permission for everything after it, so all eleven root fields matched 0 rows.
+ * Hasura returns `null` from `delete_*_by_pk` for a filtered-out row, with no
+ * `errors` key, so the app reported a successful delete over an intact tree.
+ *
+ * The other order does not work either: the provenance and permission FKs were
+ * `ON DELETE RESTRICT`, so deleting the row first was refused by Postgres. Both
+ * orders were proven to fail against the dev cluster's database. The fix is the
+ * migration `1771200017000_modeling_provenance_cascade_on_delete`, which makes
+ * those six FKs `ON DELETE CASCADE` so the client never deletes them at all.
+ */
+
+const DOCUMENTS: Array<[string, DocumentNode]> = [
+  ['DeleteProblemStatement', DeleteProblemStatementDocument],
+  ['DeleteTask', DeleteTaskDocument],
+  ['DeleteThread', DeleteThreadDocument],
+];
+
+/** Root field names, in document order. */
+function rootFields(doc: DocumentNode): string[] {
+  const names: string[] = [];
+  for (const def of doc.definitions) {
+    if (def.kind !== 'OperationDefinition') continue;
+    for (const sel of def.selectionSet.selections) {
+      if (sel.kind === 'Field') names.push(sel.name.value);
+    }
+  }
+  return names;
+}
+
+describe.each(DOCUMENTS)('%s', (name, doc) => {
+  const fields = rootFields(doc);
+
+  it('deletes no provenance row — those cascade off the row they authorise', () => {
+    expect(fields.filter((f) => f.endsWith('_provenance'))).toEqual([]);
+  });
+
+  it('deletes no permission row — those cascade too', () => {
+    expect(fields.filter((f) => f.endsWith('_permission'))).toEqual([]);
+  });
+
+  it('ends on the _by_pk delete, so the caller can tell 0 rows from 1', () => {
+    expect(fields.at(-1)).toMatch(/^delete_[a-z_]+_by_pk$/);
+  });
+
+  it('deletes dataslice before thread_data, which its permission filter reads', () => {
+    // `dataslice`'s user-role delete filter walks dataslice -> thread_data ->
+    // thread. Removing thread_data first would leave every dataslice
+    // unmatchable, and thread_data.dataslice_id would then block the delete.
+    const dataslice = fields.indexOf('delete_dataslice');
+    const threadData = fields.indexOf('delete_thread_data');
+    expect(dataslice).toBeGreaterThanOrEqual(0);
+    expect(threadData).toBeGreaterThan(dataslice);
+  });
+
+  it('deletes dataslice_resource before dataslice, which it references', () => {
+    const resource = fields.indexOf('delete_dataslice_resource');
+    expect(resource).toBeGreaterThanOrEqual(0);
+    expect(fields.indexOf('delete_dataslice')).toBeGreaterThan(resource);
+  });
+
+  it('deletes thread_model last of the thread_model_* rows that reference it', () => {
+    const threadModel = fields.indexOf('delete_thread_model');
+    expect(threadModel).toBeGreaterThanOrEqual(0);
+    for (const dependant of fields.filter((f) => f.startsWith('delete_thread_model_'))) {
+      expect(fields.indexOf(dependant)).toBeLessThan(threadModel);
+    }
+  });
+
+  it(`names ${name} so the operation name stays stable for the caller`, () => {
+    expect(print(doc)).toContain(`mutation ${name}(`);
+  });
+});
+
+describe('DeleteProblemStatement', () => {
+  const fields = rootFields(DeleteProblemStatementDocument);
+
+  it('deletes threads, then tasks, then the problem statement', () => {
+    expect(fields.indexOf('delete_thread')).toBeLessThan(fields.indexOf('delete_task'));
+    expect(fields.indexOf('delete_task')).toBeLessThan(
+      fields.indexOf('delete_problem_statement_by_pk'),
+    );
+  });
+});

ui-react/src/graphql/generated/modeling.ts

@@ -567,26 +567,23 @@ export type DeleteProblemStatementMutation = {
   delete_problem_statement_by_pk?: { id: string } | null;
 };
 
+/**
+ * Deletes bottom-up, and never deletes a provenance or permission row.
+ *
+ * Every `user`-role delete on this tree is authorised by the subject's own
+ * CREATE provenance event (or a permission row). Removing those first — which
+ * this document used to do, copied from the Lit app — revoked the permission
+ * for everything that came after, so the whole cascade silently matched 0 rows
+ * (#99). Provenance and permission rows now go by ON DELETE CASCADE, once the
+ * row they authorise is gone.
+ *
+ * Order is load-bearing: `dataslice` is filtered through `thread_data`, so it
+ * must go before `thread_data` does. `thread_data.dataslice_id` is DEFERRABLE
+ * INITIALLY DEFERRED, so that order is legal inside the one transaction Hasura
+ * runs these root fields in.
+ */
 export const DeleteProblemStatementDocument = gql`
   mutation DeleteProblemStatement($id: String!) {
-    delete_thread_permission(
-      where: { thread: { task: { problem_statement_id: { _eq: $id } } } }
-    ) { affected_rows }
-    delete_thread_provenance(
-      where: { thread: { task: { problem_statement_id: { _eq: $id } } } }
-    ) { affected_rows }
-    delete_task_permission(
-      where: { task: { problem_statement_id: { _eq: $id } } }
-    ) { affected_rows }
-    delete_task_provenance(
-      where: { task: { problem_statement_id: { _eq: $id } } }
-    ) { affected_rows }
-    delete_problem_statement_permission(
-      where: { problem_statement_id: { _eq: $id } }
-    ) { affected_rows }
-    delete_problem_statement_provenance(
-      where: { problem_statement_id: { _eq: $id } }
-    ) { affected_rows }
     delete_thread_model_execution_summary(
       where: { thread_model: { thread: { task: { problem_statement_id: { _eq: $id } } } } }
     ) { affected_rows }
@@ -599,6 +596,15 @@ export const DeleteProblemStatementDocument = gql`
     delete_thread_model_parameter(
       where: { thread_model: { thread: { task: { problem_statement_id: { _eq: $id } } } } }
     ) { affected_rows }
+    delete_dataslice_resource(
+      where: { dataslice: { thread_data: { thread: { task: { problem_statement_id: { _eq: $id } } } } } }
+    ) { affected_rows }
+    delete_dataslice(
+      where: { thread_data: { thread: { task: { problem_statement_id: { _eq: $id } } } } }
+    ) { affected_rows }
+    delete_thread_data(
+      where: { thread: { task: { problem_statement_id: { _eq: $id } } } }
+    ) { affected_rows }
     delete_thread_model(
       where: { thread: { task: { problem_statement_id: { _eq: $id } } } }
     ) { affected_rows }
@@ -750,20 +756,9 @@ export type DeleteTaskMutation = {
   delete_task_by_pk?: { id: string } | null;
 };
 
+/** Bottom-up, provenance and permission left to cascade. See DeleteProblemStatementDocument. */
 export const DeleteTaskDocument = gql`
   mutation DeleteTask($id: String!) {
-    delete_thread_permission(
-      where: { thread: { task_id: { _eq: $id } } }
-    ) { affected_rows }
-    delete_thread_provenance(
-      where: { thread: { task_id: { _eq: $id } } }
-    ) { affected_rows }
-    delete_task_permission(where: { task_id: { _eq: $id } }) {
-      affected_rows
-    }
-    delete_task_provenance(where: { task_id: { _eq: $id } }) {
-      affected_rows
-    }
     delete_thread_model_execution_summary(
       where: { thread_model: { thread: { task_id: { _eq: $id } } } }
     ) { affected_rows }
@@ -776,6 +771,15 @@ export const DeleteTaskDocument = gql`
     delete_thread_model_parameter(
       where: { thread_model: { thread: { task_id: { _eq: $id } } } }
     ) { affected_rows }
+    delete_dataslice_resource(
+      where: { dataslice: { thread_data: { thread: { task_id: { _eq: $id } } } } }
+    ) { affected_rows }
+    delete_dataslice(
+      where: { thread_data: { thread: { task_id: { _eq: $id } } } }
+    ) { affected_rows }
+    delete_thread_data(
+      where: { thread: { task_id: { _eq: $id } } }
+    ) { affected_rows }
     delete_thread_model(
       where: { thread: { task_id: { _eq: $id } } }
     ) { affected_rows }
@@ -908,14 +912,9 @@ export type DeleteThreadMutation = {
   delete_thread_by_pk?: { id: string } | null;
 };
 
+/** Bottom-up, provenance and permission left to cascade. See DeleteProblemStatementDocument. */
 export const DeleteThreadDocument = gql`
   mutation DeleteThread($id: String!) {
-    delete_thread_permission(where: { thread_id: { _eq: $id } }) {
-      affected_rows
-    }
-    delete_thread_provenance(where: { thread_id: { _eq: $id } }) {
-      affected_rows
-    }
     delete_thread_model_execution_summary(
       where: { thread_model: { thread_id: { _eq: $id } } }
     ) { affected_rows }
@@ -928,6 +927,15 @@ export const DeleteThreadDocument = gql`
     delete_thread_model_parameter(
       where: { thread_model: { thread_id: { _eq: $id } } }
     ) { affected_rows }
+    delete_dataslice_resource(
+      where: { dataslice: { thread_data: { thread_id: { _eq: $id } } } }
+    ) { affected_rows }
+    delete_dataslice(where: { thread_data: { thread_id: { _eq: $id } } }) {
+      affected_rows
+    }
+    delete_thread_data(where: { thread_id: { _eq: $id } }) {
+      affected_rows
+    }
     delete_thread_model(where: { thread_id: { _eq: $id } }) {
       affected_rows
     }

ui-react/src/lib/modeling/__tests__/assertDeleted.test.ts

@@ -0,0 +1,31 @@
+import { describe, it, expect } from 'vitest';
+import { assertDeleted } from '@/lib/modeling/assertDeleted';
+
+/**
+ * #99: Hasura answers `delete_*_by_pk` with `null` when the row exists but the
+ * role's delete permission filters it out, and sends no `errors` key with it.
+ * That is what let a failed delete render as "Problem statement deleted".
+ */
+describe('assertDeleted', () => {
+  it('throws on null — a filtered-out row, which Hasura reports as success', () => {
+    expect(() => assertDeleted(null, 'Problem statement')).toThrow(/was not deleted/);
+  });
+
+  it('throws on undefined — the field missing from a partial response', () => {
+    expect(() => assertDeleted(undefined, 'Task')).toThrow(/was not deleted/);
+  });
+
+  it('names the subject, so the toast says what survived', () => {
+    expect(() => assertDeleted(null, 'Sub-task')).toThrow(/^Sub-task /);
+  });
+
+  it('returns the row when one was deleted', () => {
+    const row = { id: 'abc' };
+    expect(assertDeleted(row, 'Task')).toBe(row);
+  });
+
+  it('accepts a falsy row that is not null — 0 rows is the only failure', () => {
+    expect(assertDeleted(0, 'Task')).toBe(0);
+    expect(assertDeleted('', 'Task')).toBe('');
+  });
+});

ui-react/src/lib/modeling/assertDeleted.ts

@@ -0,0 +1,18 @@
+/**
+ * Hasura answers `delete_<table>_by_pk` with `null` when the row exists but the
+ * role's delete permission filters it out. That is a 200 with no `errors` key,
+ * so Apollo resolves and the caller reports success while the row is still
+ * there. Issue #99 stayed invisible for exactly this reason.
+ *
+ * Call this on the `_by_pk` field of every delete so a filtered-out row raises
+ * instead of passing.
+ */
+export function assertDeleted<T>(row: T | null | undefined, subject: string): T {
+  if (row === null || row === undefined) {
+    throw new Error(
+      `${subject} was not deleted. The server accepted the request and removed no row, ` +
+        `which usually means your account may not delete it.`,
+    );
+  }
+  return row;
+}

ui-react/src/pages/modeling/MintProblemStatement.tsx

@@ -39,6 +39,7 @@ import { useToast } from '@/components/ui/use-toast';
 import { cn } from '@/lib/utils';
 import { useAuth } from '@/lib/auth/useAuth';
 import { provisionTask } from '@/lib/modeling/provisionTask';
+import { assertDeleted } from '@/lib/modeling/assertDeleted';
 import { EmptyState } from '@/components/common/EmptyState';
 
 import {
@@ -279,7 +280,8 @@ export function MintProblemStatement() {
   async function handleDeleteTask() {
     if (!deleteTaskTarget) return;
     try {
-      await deleteTask({ variables: { id: deleteTaskTarget.id } });
+      const { data: deleted } = await deleteTask({ variables: { id: deleteTaskTarget.id } });
+      assertDeleted(deleted?.delete_task_by_pk, 'Task');
       toast({ title: 'Task deleted' });
       if (selectedTaskId === deleteTaskTarget.id) {
         setSelectedTaskId(null);
@@ -369,7 +371,10 @@ export function MintProblemStatement() {
   async function handleDeleteThread() {
     if (!deleteThreadTarget) return;
     try {
-      await deleteThread({ variables: { id: deleteThreadTarget.id } });
+      const { data: deleted } = await deleteThread({
+        variables: { id: deleteThreadTarget.id },
+      });
+      assertDeleted(deleted?.delete_thread_by_pk, 'Sub-task');
       toast({ title: 'Sub-task deleted' });
       if (selectedThreadId === deleteThreadTarget.id) {
         setSelectedThreadId(null);

ui-react/src/pages/modeling/ProblemStatementsList.tsx

@@ -55,6 +55,7 @@ import {
   type ProblemStatement,
 } from '@/graphql/generated/modeling';
 import { provisionTask } from '@/lib/modeling/provisionTask';
+import { assertDeleted } from '@/lib/modeling/assertDeleted';
 
 // ─── Types ────────────────────────────────────────────────────────────────────
 
@@ -295,7 +296,8 @@ export function ProblemStatementsList({ regionId = 'DEFAULT' }: ProblemStatement
   async function handleDelete() {
     if (!deleteTarget) return;
     try {
-      await deletePS({ variables: { id: deleteTarget.id } });
+      const { data: deleted } = await deletePS({ variables: { id: deleteTarget.id } });
+      assertDeleted(deleted?.delete_problem_statement_by_pk, 'Problem statement');
       toast({ title: 'Problem statement deleted' });
       await refetch();
     } catch (err) {