mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 07:10:34 +01:00
feat: show and change routine responsible users (#19128)
## Thinking Path > - Paperclip helps people manage AI agents and their work. > - Routines create tasks for repeated work. > - Each routine has a responsible person. > - The Overview did not show this person or permit a change. > - People need to see who will be responsible for automatic tasks. > - This pull request adds a small name picker beside Default agent. > - Authorized people can adopt a routine or transfer it to an active company member. ## Linked Issues or Issue Description **What existing behavior does this improve?** The routine Overview and the routine update API. **Subsystem affected** UI, server, and shared contracts. **Current behavior** The routine stores a responsible user. The Overview does not show that person. The update API cannot change that person. **Proposed behavior** Show Responsible user beside Default agent. Click the name to search active company members. Save only the new person and the current revision ID. **Reason and benefit** People can see who owns future automatic tasks. An authorized person can adopt a routine without making a new routine. **Breaking changes** The detail response adds an optional actor-specific field. The update API adds an optional responsible user ID. Existing requests keep their current behavior. Related: #17368 covers local Board ownership migration. This change adds the routine control and explicit transfer. ## What Changed - Show a quiet, searchable person picker beside Default agent. - Explain that automatic tasks use this person. Manual runs use the person who starts them. - Permit human operators with `tasks:assign`, instance administrators, and local operators to transfer responsibility. - Check active company membership inside the update transaction. - Save a revision and record the old and new person in the activity log. - Keep existing task responsibility and ownership during revision restore. - Show errors for failed saves and stale revisions. - Add tests for permissions, company boundaries, transfers, inheritance, and picker states. - Keep the picker disabled until saved data is refreshed. Refresh the activity list after a transfer. - Capture the actual previous person under the update row lock for the activity record. - Use shared query error handling and retain cached people during a connection failure. ## Verification - Focused checks cover 145 tests in 5 files. The first post-fix run passed 144 tests. One existing Fireflies route test timed out during the first import while the UI typecheck also ran. The same test passed in the pre-fix run. The isolated route suite then passed all 21 tests with no source change. - The new tests reproduce slow detail refreshes, activity refresh, and two concurrent transfers without a base revision. The concurrency test uses PostgreSQL row locks. - UI and server TypeScript checks passed at commit `1df9587eb`. - The UI production build passed for the updated head. - The branch includes master through `26391707f`. - Design token gates, query-error policy, and Git whitespace checks passed. The query-error policy reports zero findings. - Local browser checks passed on desktop and mobile for the initial feature commit. Search, selection, reload persistence, and revision creation passed. No page errors or horizontal overflow were found. The follow-up does not change the normal layout. - Earlier full local checks could not finish: Cargo was absent, and concurrent full test/build checks exceeded the 8 GiB memory limit. These attempts are not passes. - The first CI run failed on two error-rendering policy findings. That failure is fixed locally. The three findings from the first Greptile review are fixed and their threads are resolved. - Greptile reviewed `1df9587eb2d3e034c2769b1f6955ac52323c95bb` and scored it 5/5. It confirms all three prior findings are addressed. There are no open review threads. - [CI attempt 1](https://github.com/paperclipai/paperclip/actions/runs/42046369415/attempts/1) passed 53 checks. The server 8/12 shard failed only because the existing `history-http` runtime fixture did not become ready within 10 seconds (99 refused connections). The fixture and runtime source are unchanged from the merged master. That case passed locally on the same PR head with no source change. Its full 75-test suite also passed locally. The cause of the CI failure remains unknown. - [CI attempt 2](https://github.com/paperclipai/paperclip/actions/runs/42046369415/attempts/2) passed on `1df9587eb`: 54 successful checks, 3 skipped, and 1 neutral. No failed or pending checks remained. The original fixture-readiness failure remains recorded above. - A later review found that viewers with an explicit `tasks:assign` grant could see an editable picker even though writes were denied. Commit `8b95ee707` now mirrors active-membership and viewer restrictions in the GET capability flag. Instance-admin and local-operator behavior is preserved. - Three membership regressions failed on the incorrect editable flag before the fix. After the fix, all 43 routine-route and company-access cases passed (`--testTimeout 60000` accommodated this sandbox's cold route imports; no repository timeout was changed). The first pre-fix run timed out during the cold import in one case; the second pre-fix run reproduced all three assertion failures. - Layered server typechecking and whitespace checks passed after the permission fix. - [Fresh CI](https://github.com/paperclipai/paperclip/actions/runs/42057752313) passed on `8b95ee7075417527687b4a86cbaf63ba87408364` without a retry. Greptile reviewed this exact head, scored it 5/5, and added no new findings. All four review threads are resolved. The security status is also successful. - The separate Paperclip Review check is still queued because its assigned review worker requires runtime recovery. This is the remaining check before completion; the CI workflow itself passed. ## Risks - A transfer changes the person used for future automatic tasks. Existing tasks keep their current person. - Only people can transfer responsibility. The server checks the permission again on every write. - A stale revision rejects the save. The user must reload before retrying. - No database migration is required. ## Model Used OpenAI Codex, based on GPT-6, with reasoning, tool use, and code execution. The exact runtime model ID and context window are not exposed to this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (CI passed; separate Paperclip Review check pending) - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
253bf071f0
commit
21d2a577bb
12 files changed
+473
-25
No files matched your search
+8
-4
@@ -141,10 +141,14 @@ the latest agent that assigned it to them. If no assignment is available, it use
|
||||
the latest other commenter, or leaves the recipient empty. A manual recipient
|
||||
choice stays selected while task data refreshes.
|
||||
|
||||
Task reply and new-task composers submit with Enter by default; Shift+Enter
|
||||
adds a new line. Settings → Experimental → **Command/Ctrl+Enter to Submit**
|
||||
restores the previous behavior, where Enter adds a new line and Command/Ctrl+Enter
|
||||
submits.
|
||||
### Routine responsibility
|
||||
|
||||
The routine Overview shows its responsible user beside Default agent. People with
|
||||
`tasks:assign` can select an active company member from the name's picker.
|
||||
The change creates a routine revision and applies to future automatic tasks.
|
||||
Existing tasks keep their responsibility. Manual runs use the person who starts
|
||||
them. Agents cannot transfer routine responsibility.
|
||||
|
||||
|
||||
### Task collection creation filters
|
||||
|
||||
|
||||
@@ -236,6 +236,8 @@ export interface RoutineTriggerSecretMaterial {
|
||||
}
|
||||
|
||||
export interface RoutineDetail extends Routine {
|
||||
/** Actor-specific affordance. The update route remains authoritative. */
|
||||
canChangeResponsibleUser?: boolean;
|
||||
project: RoutineProjectSummary | null;
|
||||
assignee: RoutineAgentSummary | null;
|
||||
parentIssue: RoutineIssueSummary | null;
|
||||
|
||||
@@ -11,6 +11,12 @@ const triggerId = "33333333-3333-4333-8333-333333333333";
|
||||
const baseRevisionId = "44444444-4444-4444-8444-444444444444";
|
||||
|
||||
describe("routine validators", () => {
|
||||
it("accepts non-UUID person IDs for responsibility, but rejects clearing it", () => {
|
||||
expect(updateRoutineSchema.parse({ responsibleUserId: "person-123" })).toEqual({ responsibleUserId: "person-123" });
|
||||
expect(updateRoutineSchema.safeParse({ responsibleUserId: null }).success).toBe(false);
|
||||
expect(updateRoutineSchema.safeParse({ responsibleUserId: " " }).success).toBe(false);
|
||||
expect(updateRoutineSchema.parse({ title: "Title" })).not.toHaveProperty("responsibleUserId");
|
||||
});
|
||||
it.each(["bearer", "app_webhook", "fireflies_hmac"])("accepts versioned routine revision snapshots with %s trigger metadata", (signingMode) => {
|
||||
const parsed = routineRevisionSnapshotV1Schema.parse({
|
||||
version: 1,
|
||||
|
||||
@@ -84,6 +84,7 @@ export type CreateRoutine = z.infer<typeof createRoutineSchema>;
|
||||
|
||||
export const updateRoutineSchema = objectWithoutDefaults(createRoutineSchema).partial().extend({
|
||||
baseRevisionId: z.string().guid().optional().nullable(),
|
||||
responsibleUserId: z.string().trim().min(1).optional(),
|
||||
});
|
||||
export type UpdateRoutine = z.infer<typeof updateRoutineSchema>;
|
||||
|
||||
|
||||
@@ -529,6 +529,54 @@ describe("routine routes", () => {
|
||||
expect(mockRoutineService.create).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("hides responsibility editing from people without tasks:assign and rejects their writes", async () => {
|
||||
const app = await createApp({ type: "board", userId: "viewer", source: "session", isInstanceAdmin: false, companyIds: [companyId] });
|
||||
mockRoutineService.getDetail.mockResolvedValue(routine);
|
||||
expect((await request(app).get(`/api/routines/${routineId}`)).body.canChangeResponsibleUser).toBe(false);
|
||||
const res = await request(app).patch(`/api/routines/${routineId}`).send({ responsibleUserId: "viewer" });
|
||||
expect(res.status).toBe(403);
|
||||
expect(mockRoutineService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([
|
||||
{ name: "viewer", memberships: [{ companyId, membershipRole: "viewer", status: "active" }] },
|
||||
{ name: "inactive member", memberships: [{ companyId, membershipRole: "operator", status: "suspended" }] },
|
||||
{ name: "missing member", memberships: [] },
|
||||
])("hides responsibility editing from a $name even with tasks:assign", async ({ memberships }) => {
|
||||
const app = await createApp({ type: "board", userId: "read-only-user", source: "session", isInstanceAdmin: false, companyIds: [companyId], memberships });
|
||||
mockAccessService.canUser.mockResolvedValue(true);
|
||||
mockRoutineService.getDetail.mockResolvedValue(routine);
|
||||
|
||||
const detail = await request(app).get(`/api/routines/${routineId}`);
|
||||
expect(detail.status).toBe(200);
|
||||
expect(detail.body.canChangeResponsibleUser).toBe(false);
|
||||
const res = await request(app).patch(`/api/routines/${routineId}`).send({ responsibleUserId: "operator" });
|
||||
expect(res.status).toBe(403);
|
||||
expect(mockRoutineService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each(["granted", "instance-admin", "local"])("allows responsibility changes for a %s operator", async (kind) => {
|
||||
const app = await createApp({ type: "board", userId: "operator", source: kind === "local" ? "local_implicit" : "session", isInstanceAdmin: kind === "instance-admin", companyIds: [companyId], memberships: [{ companyId, membershipRole: kind === "granted" ? "operator" : "viewer", status: "active" }] });
|
||||
mockAccessService.canUser.mockResolvedValue(kind === "granted");
|
||||
mockRoutineService.getDetail.mockResolvedValue(routine);
|
||||
mockRoutineService.update.mockResolvedValue({ ...routine, responsibleUserId: "operator", previousResponsibleUserId: "actual-predecessor" });
|
||||
expect((await request(app).get(`/api/routines/${routineId}`)).body.canChangeResponsibleUser).toBe(true);
|
||||
const res = await request(app).patch(`/api/routines/${routineId}`).send({ responsibleUserId: "operator", baseRevisionId: revisionId });
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockRoutineService.update).toHaveBeenCalledWith(routineId, expect.objectContaining({ responsibleUserId: "operator", baseRevisionId: revisionId }), expect.objectContaining({ userId: "operator" }));
|
||||
expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ action: "routine.updated", details: expect.objectContaining({ responsibleUserId: "operator", previousResponsibleUserId: "actual-predecessor" }) }));
|
||||
expect(res.body).not.toHaveProperty("previousResponsibleUserId");
|
||||
});
|
||||
|
||||
it("prevents even the assigned agent from transferring routine responsibility", async () => {
|
||||
const app = await createApp({ type: "agent", agentId, companyId });
|
||||
mockRoutineService.getDetail.mockResolvedValue(routine);
|
||||
expect((await request(app).get(`/api/routines/${routineId}`)).body.canChangeResponsibleUser).toBe(false);
|
||||
const res = await request(app).patch(`/api/routines/${routineId}`).send({ responsibleUserId: "operator" });
|
||||
expect(res.status).toBe(403);
|
||||
expect(mockRoutineService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("requires tasks:assign permission to retarget a routine assignee", async () => {
|
||||
const app = await createApp({
|
||||
type: "board",
|
||||
|
||||
@@ -8,6 +8,7 @@ import {
|
||||
activityLog,
|
||||
agents,
|
||||
companies,
|
||||
companyMemberships,
|
||||
companySecretBindings,
|
||||
companySecrets,
|
||||
companySecretVersions,
|
||||
@@ -104,6 +105,7 @@ describeEmbeddedPostgres("routine service live-execution coalescing", () => {
|
||||
await db.delete(projectWorkspaces);
|
||||
await db.delete(projects);
|
||||
await db.delete(agents);
|
||||
await db.delete(companyMemberships);
|
||||
await db.delete(companies);
|
||||
await db.delete(instanceSettings);
|
||||
});
|
||||
@@ -1164,6 +1166,87 @@ describeEmbeddedPostgres("routine service live-execution coalescing", () => {
|
||||
expect(serialized).not.toContain("runtimeConfig");
|
||||
});
|
||||
|
||||
it("versions responsibility transfers and uses the new person only for future automatic tasks", async () => {
|
||||
const { companyId, routine, svc } = await seedFixture();
|
||||
const userId = "new-responsible-person";
|
||||
await db.insert(companyMemberships).values({ companyId, principalType: "user", principalId: userId, status: "active" });
|
||||
const before = await svc.runRoutine(routine.id, { source: "api" }, {});
|
||||
await db.update(issues).set({ status: "done" }).where(eq(issues.id, before.linkedIssueId!));
|
||||
const updated = await svc.update(routine.id, {
|
||||
responsibleUserId: userId,
|
||||
baseRevisionId: routine.latestRevisionId,
|
||||
}, { userId: "operator" });
|
||||
expect(updated?.responsibleUserId).toBe(userId);
|
||||
expect(updated?.latestRevisionNumber).toBe(2);
|
||||
const revisions = await svc.listRevisions(routine.id);
|
||||
expect(revisions[0]?.snapshot.routine.responsibleUserId).toBe(userId);
|
||||
expect(revisions[1]?.snapshot.routine.responsibleUserId).toBe(routine.responsibleUserId);
|
||||
const after = await svc.runRoutine(routine.id, { source: "api" }, {});
|
||||
const [oldTask] = await db.select().from(issues).where(eq(issues.id, before.linkedIssueId!));
|
||||
const [newTask] = await db.select().from(issues).where(eq(issues.id, after.linkedIssueId!));
|
||||
expect(oldTask?.responsibleUserId).toBe(routine.responsibleUserId);
|
||||
expect(newTask?.responsibleUserId).toBe(userId);
|
||||
const noOp = await svc.update(routine.id, { responsibleUserId: userId }, {});
|
||||
expect(noOp?.latestRevisionId).toBe(updated?.latestRevisionId);
|
||||
await expect(svc.update(routine.id, {
|
||||
responsibleUserId: userId, baseRevisionId: routine.latestRevisionId,
|
||||
}, {})).rejects.toThrow("Routine was updated by someone else");
|
||||
const restored = await svc.restoreRevision(routine.id, routine.latestRevisionId!, {});
|
||||
expect(restored.routine.responsibleUserId).toBe(userId);
|
||||
expect(restored.revision.snapshot.routine.responsibleUserId).toBe(userId);
|
||||
});
|
||||
|
||||
it("returns the actual predecessor for concurrent transfers without a base revision", async () => {
|
||||
const { companyId, routine, svc } = await seedFixture();
|
||||
const people = ["next-person", "last-person"];
|
||||
await db.insert(companyMemberships).values(people.map((principalId) => ({
|
||||
companyId, principalType: "user" as const, principalId, status: "active" as const,
|
||||
})));
|
||||
const acquired = Promise.withResolvers<void>();
|
||||
const release = Promise.withResolvers<void>();
|
||||
const blocker = db.transaction(async (tx) => {
|
||||
await tx.execute(sql`select id from ${routines} where ${routines.id} = ${routine.id} for update`);
|
||||
acquired.resolve();
|
||||
await release.promise;
|
||||
});
|
||||
await acquired.promise;
|
||||
const transfers = Promise.all(people.map((responsibleUserId) => svc.update(routine.id, { responsibleUserId }, {})));
|
||||
let results;
|
||||
try {
|
||||
await vi.waitFor(async () => {
|
||||
const waiters = await db.execute(sql`select pid from pg_stat_activity
|
||||
where datname = current_database() and wait_event_type = 'Lock'
|
||||
and query like 'select id from "routines"%for update'`);
|
||||
expect(waiters.length).toBe(2);
|
||||
}, { timeout: 5_000, interval: 20 });
|
||||
} finally {
|
||||
release.resolve();
|
||||
await blocker;
|
||||
results = await transfers;
|
||||
}
|
||||
const [first, second] = results.sort((a, b) => a!.latestRevisionNumber! - b!.latestRevisionNumber!);
|
||||
expect(first?.previousResponsibleUserId).toBe(routine.responsibleUserId);
|
||||
expect(second?.previousResponsibleUserId).toBe(first?.responsibleUserId);
|
||||
expect((await svc.get(routine.id))?.responsibleUserId).toBe(second?.responsibleUserId);
|
||||
});
|
||||
|
||||
it.each(["missing", "suspended", "archived", "other-company"])("rejects a %s responsible person", async (state) => {
|
||||
const { companyId, routine, svc } = await seedFixture();
|
||||
const userId = randomUUID();
|
||||
if (state !== "missing") {
|
||||
const memberCompanyId = state === "other-company" ? randomUUID() : companyId;
|
||||
if (state === "other-company") await db.insert(companies).values({ id: memberCompanyId, name: "Other company", issuePrefix: "OTHER" });
|
||||
await db.insert(companyMemberships).values({
|
||||
companyId: memberCompanyId, principalType: "user", principalId: userId,
|
||||
status: state === "other-company" ? "active" : state,
|
||||
});
|
||||
}
|
||||
await expect(svc.update(routine.id, { responsibleUserId: userId }, {}))
|
||||
.rejects.toThrow("Responsible user must be an active company member");
|
||||
expect((await svc.get(routine.id))?.responsibleUserId).toBe(routine.responsibleUserId);
|
||||
expect(await svc.listRevisions(routine.id)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("creates revision 1 on routine create and appends revisions for real updates only", async () => {
|
||||
const { routine, svc } = await seedFixture();
|
||||
|
||||
|
||||
@@ -96,6 +96,17 @@ export function routineRoutes(
|
||||
}
|
||||
}
|
||||
|
||||
async function canChangeResponsibleUser(req: Request, companyId: string) {
|
||||
if (req.actor.type !== "board") return false;
|
||||
if (req.actor.source === "local_implicit" || req.actor.isInstanceAdmin) return true;
|
||||
// Match assertCompanyAccess's write restrictions even on this GET route.
|
||||
if (Array.isArray(req.actor.memberships)) {
|
||||
const membership = req.actor.memberships.find((item) => item.companyId === companyId);
|
||||
if (!membership || membership.status !== "active" || membership.membershipRole === "viewer") return false;
|
||||
}
|
||||
return access.canUser(companyId, req.actor.userId, "tasks:assign");
|
||||
}
|
||||
|
||||
function assertCanManageCompanyRoutine(req: Request, companyId: string, assigneeAgentId?: string | null) {
|
||||
assertCompanyAccess(req, companyId);
|
||||
if (req.actor.type === "board") return;
|
||||
@@ -195,7 +206,7 @@ export function routineRoutes(
|
||||
router.get("/routines/:id", async (req, res) => {
|
||||
const detail = await getAccessibleResource(req, res, svc.getDetail(req.params.id as string), "Routine not found");
|
||||
if (!detail) return;
|
||||
res.json(detail);
|
||||
res.json({ ...detail, canChangeResponsibleUser: await canChangeResponsibleUser(req, detail.companyId) });
|
||||
});
|
||||
|
||||
router.get("/routines/:id/revisions", async (req, res) => {
|
||||
@@ -370,6 +381,12 @@ export function routineRoutes(
|
||||
const assigneeWillChange =
|
||||
req.body.assigneeAgentId !== undefined &&
|
||||
req.body.assigneeAgentId !== routine.assigneeAgentId;
|
||||
if (req.body.responsibleUserId !== undefined) {
|
||||
if (req.actor.type !== "board") {
|
||||
throw forbidden("Only people can change routine responsibility");
|
||||
}
|
||||
await assertBoardCanAssignTasks(req, routine.companyId);
|
||||
}
|
||||
if (assigneeWillChange) {
|
||||
await assertBoardCanAssignTasks(req, routine.companyId);
|
||||
}
|
||||
@@ -403,7 +420,13 @@ export function routineRoutes(
|
||||
action: "routine.updated",
|
||||
entityType: "routine",
|
||||
entityId: routine.id,
|
||||
details: { title: updated?.title ?? routine.title },
|
||||
details: {
|
||||
title: updated?.title ?? routine.title,
|
||||
...(req.body.responsibleUserId !== undefined ? {
|
||||
previousResponsibleUserId: updated?.previousResponsibleUserId,
|
||||
responsibleUserId: updated?.responsibleUserId,
|
||||
} : {}),
|
||||
},
|
||||
});
|
||||
if (updated && updated.latestRevisionId !== routine.latestRevisionId) {
|
||||
await remapRoutineDescriptionAnnotations(req, routine.id);
|
||||
@@ -416,7 +439,12 @@ export function routineRoutes(
|
||||
triggerCount: null,
|
||||
});
|
||||
}
|
||||
res.json(updated);
|
||||
if (updated) {
|
||||
const { previousResponsibleUserId: _previousResponsibleUserId, ...response } = updated;
|
||||
res.json(response);
|
||||
} else {
|
||||
res.json(null);
|
||||
}
|
||||
});
|
||||
|
||||
router.post("/routines/:id/revisions/:revisionId/restore", async (req, res) => {
|
||||
|
||||
@@ -2279,7 +2279,7 @@ export function routineService(
|
||||
return createdRoutine;
|
||||
},
|
||||
|
||||
update: async (id: string, patch: UpdateRoutine, actor: Actor): Promise<Routine | null> => {
|
||||
update: async (id: string, patch: UpdateRoutine, actor: Actor): Promise<(Routine & { previousResponsibleUserId: string | null }) | null> => {
|
||||
const existing = await getRoutineById(id);
|
||||
if (!existing) return null;
|
||||
const nextProjectId = patch.projectId === undefined ? existing.projectId : patch.projectId;
|
||||
@@ -2330,7 +2330,7 @@ export function routineService(
|
||||
if (enabledScheduleTriggers) {
|
||||
assertScheduleCompatibleVariables(nextVariables);
|
||||
}
|
||||
const responsibleUserId = await resolveRoutineResponsibleUserId(
|
||||
const responsibleUserId = patch.responsibleUserId ?? existing.responsibleUserId ?? await resolveRoutineResponsibleUserId(
|
||||
db,
|
||||
existing.companyId,
|
||||
actor.userId,
|
||||
@@ -2339,6 +2339,7 @@ export function routineService(
|
||||
if (!responsibleUserId) {
|
||||
throw unprocessable("Routine requires a responsible user");
|
||||
}
|
||||
let previousResponsibleUserId: string | null = null;
|
||||
const updatedRoutine = await db.transaction(async (tx) => {
|
||||
const txDb = tx as unknown as Db;
|
||||
await notifyDeliveryWork(txDb, DELIVERY_QUEUES.routineSchedules);
|
||||
@@ -2349,6 +2350,7 @@ export function routineService(
|
||||
.where(eq(routines.id, id))
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!locked) return null;
|
||||
previousResponsibleUserId = locked.responsibleUserId;
|
||||
|
||||
if (patch.baseRevisionId && patch.baseRevisionId !== locked.latestRevisionId) {
|
||||
throw conflict("Routine was updated by someone else", {
|
||||
@@ -2356,6 +2358,20 @@ export function routineService(
|
||||
});
|
||||
}
|
||||
|
||||
if (patch.responsibleUserId !== undefined) {
|
||||
const member = await txDb.select({ id: companyMemberships.id })
|
||||
.from(companyMemberships)
|
||||
.where(and(
|
||||
eq(companyMemberships.companyId, locked.companyId),
|
||||
eq(companyMemberships.principalType, "user"),
|
||||
eq(companyMemberships.principalId, patch.responsibleUserId),
|
||||
eq(companyMemberships.status, "active"),
|
||||
))
|
||||
.for("share")
|
||||
.then((rows) => rows[0] ?? null);
|
||||
if (!member) throw unprocessable("Responsible user must be an active company member");
|
||||
}
|
||||
|
||||
const candidate: RoutineRow = {
|
||||
...locked,
|
||||
projectId: nextProjectId,
|
||||
@@ -2373,7 +2389,7 @@ export function routineService(
|
||||
activityGateScope: patch.activityGateScope ?? locked.activityGateScope,
|
||||
variables: nextVariables,
|
||||
env: nextEnv,
|
||||
responsibleUserId: locked.responsibleUserId ?? responsibleUserId,
|
||||
responsibleUserId: patch.responsibleUserId ?? locked.responsibleUserId ?? responsibleUserId,
|
||||
updatedByAgentId: actor.agentId ?? null,
|
||||
updatedByUserId: actor.userId ?? null,
|
||||
};
|
||||
@@ -2459,7 +2475,7 @@ export function routineService(
|
||||
}
|
||||
return routine;
|
||||
});
|
||||
return updatedRoutine;
|
||||
return updatedRoutine ? { ...updatedRoutine, previousResponsibleUserId } : null;
|
||||
},
|
||||
|
||||
createTrigger: async (
|
||||
|
||||
@@ -14,6 +14,10 @@ import {
|
||||
|
||||
const issueRowRender = vi.hoisted(() => vi.fn());
|
||||
|
||||
vi.mock("./RoutineResponsibleUser", () => ({
|
||||
RoutineResponsibleUser: ({ routine }: { routine: RoutineDetail }) => <section data-responsible-routine={routine.id}>Responsible user</section>,
|
||||
}));
|
||||
|
||||
vi.mock("@/components/IssueRow", () => ({
|
||||
IssueRow: (props: { issue: { title: string }; presentation?: string }) => {
|
||||
issueRowRender(props);
|
||||
|
||||
@@ -17,6 +17,7 @@ import {
|
||||
routineDetailHref,
|
||||
} from "./RoutineContextualSidebar";
|
||||
import { useRoutineDetail } from "./routine-sections/context";
|
||||
import { RoutineResponsibleUser } from "./RoutineResponsibleUser";
|
||||
|
||||
export type RoutineScheduleSummary = {
|
||||
label: string;
|
||||
@@ -186,20 +187,23 @@ export function RoutineOverview() {
|
||||
/>
|
||||
</div>
|
||||
|
||||
<section className="flex flex-col gap-2" aria-labelledby="routine-agent-heading">
|
||||
<h2 id="routine-agent-heading" className="text-sm font-semibold">Default agent</h2>
|
||||
{currentAssignee ? (
|
||||
<Link
|
||||
to={`/agents/${currentAssignee.urlKey ?? currentAssignee.id}`}
|
||||
className="flex w-fit items-center gap-2 rounded-md text-sm font-medium hover:underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
|
||||
>
|
||||
<UserRound className="h-4 w-4 text-muted-foreground" aria-hidden="true" />
|
||||
{currentAssignee.name}
|
||||
</Link>
|
||||
) : (
|
||||
<p className="text-sm text-muted-foreground">No default agent. Automatic triggers remain paused.</p>
|
||||
)}
|
||||
</section>
|
||||
<div className="grid gap-6 sm:grid-cols-2">
|
||||
<section className="flex flex-col gap-2" aria-labelledby="routine-agent-heading">
|
||||
<h2 id="routine-agent-heading" className="text-sm font-semibold">Default agent</h2>
|
||||
{currentAssignee ? (
|
||||
<Link
|
||||
to={`/agents/${currentAssignee.urlKey ?? currentAssignee.id}`}
|
||||
className="flex w-fit items-center gap-2 rounded-md text-sm font-medium hover:underline focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring"
|
||||
>
|
||||
<UserRound className="h-4 w-4 text-muted-foreground" aria-hidden="true" />
|
||||
{currentAssignee.name}
|
||||
</Link>
|
||||
) : (
|
||||
<p className="text-sm text-muted-foreground">No default agent. Automatic triggers remain paused.</p>
|
||||
)}
|
||||
</section>
|
||||
<RoutineResponsibleUser key={routine.id} routine={routine} />
|
||||
</div>
|
||||
|
||||
<section className="flex flex-col gap-2" aria-labelledby="routine-description-heading">
|
||||
<h2 id="routine-description-heading" className="text-sm font-semibold">Description</h2>
|
||||
|
||||
@@ -0,0 +1,147 @@
|
||||
// @vitest-environment jsdom
|
||||
import { act } from "react";
|
||||
import { createRoot, type Root } from "react-dom/client";
|
||||
import { QueryClient, QueryClientProvider, useQuery } from "@tanstack/react-query";
|
||||
import type { RoutineDetail } from "@paperclipai/shared";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { accessApi } from "@/api/access";
|
||||
import { ApiError } from "@/api/client";
|
||||
import { routinesApi } from "@/api/routines";
|
||||
import { queryKeys } from "@/lib/queryKeys";
|
||||
import { RoutineResponsibleUser } from "./RoutineResponsibleUser";
|
||||
|
||||
const routine = {
|
||||
id: "routine-1", companyId: "company-1", responsibleUserId: "user-alex",
|
||||
latestRevisionId: "revision-1", canChangeResponsibleUser: true,
|
||||
} as RoutineDetail;
|
||||
const users = ["Alex", "Sam"].map((name) => ({
|
||||
principalId: `user-${name.toLowerCase()}`, status: "active" as const,
|
||||
user: { id: `user-${name.toLowerCase()}`, name, email: `${name.toLowerCase()}@example.test`, image: null },
|
||||
}));
|
||||
|
||||
describe("RoutineResponsibleUser", () => {
|
||||
let root: Root;
|
||||
let container: HTMLDivElement;
|
||||
let client: QueryClient;
|
||||
|
||||
beforeEach(() => {
|
||||
vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true);
|
||||
vi.stubGlobal("ResizeObserver", class { observe() {} unobserve() {} disconnect() {} });
|
||||
HTMLElement.prototype.scrollIntoView = vi.fn();
|
||||
container = document.createElement("div");
|
||||
document.body.appendChild(container);
|
||||
root = createRoot(container);
|
||||
client = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } });
|
||||
vi.spyOn(accessApi, "listUserDirectory").mockResolvedValue({ users });
|
||||
vi.spyOn(routinesApi, "update").mockResolvedValue({ ...routine, responsibleUserId: "user-sam" });
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await act(async () => root.unmount());
|
||||
container.remove();
|
||||
client.clear();
|
||||
vi.restoreAllMocks();
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
async function render(value = routine) {
|
||||
await act(async () => root.render(<QueryClientProvider client={client}><RoutineResponsibleUser routine={value} /></QueryClientProvider>));
|
||||
await act(async () => { await new Promise((resolve) => setTimeout(resolve, 20)); });
|
||||
}
|
||||
|
||||
async function selectSam() {
|
||||
await act(async () => container.querySelector<HTMLButtonElement>("button")!.click());
|
||||
const option = [...document.querySelectorAll<HTMLElement>('[role="option"]')].find((item) => item.textContent?.includes("Sam"));
|
||||
expect(option).toBeDefined();
|
||||
await act(async () => option!.click());
|
||||
await act(async () => { await new Promise((resolve) => setTimeout(resolve, 20)); });
|
||||
}
|
||||
|
||||
it("shows the person and inheritance rules to read-only viewers", async () => {
|
||||
await render({ ...routine, canChangeResponsibleUser: false });
|
||||
expect(container.textContent).toContain("Alex");
|
||||
expect(container.textContent).toContain("Automatic tasks use this person. Manual runs use the person who starts them.");
|
||||
expect(container.querySelector("button")).toBeNull();
|
||||
});
|
||||
|
||||
it("saves only responsibility and its base revision, then refreshes routine data", async () => {
|
||||
const invalidate = vi.spyOn(client, "invalidateQueries");
|
||||
await render();
|
||||
await selectSam();
|
||||
expect(routinesApi.update).toHaveBeenCalledWith("routine-1", { responsibleUserId: "user-sam", baseRevisionId: "revision-1" });
|
||||
expect(invalidate).toHaveBeenCalledWith({ queryKey: queryKeys.routines.detail("routine-1") });
|
||||
expect(invalidate).toHaveBeenCalledWith({ queryKey: queryKeys.routines.revisions("routine-1") });
|
||||
expect(invalidate).toHaveBeenCalledWith({ queryKey: queryKeys.routines.activity("company-1", "routine-1") });
|
||||
expect(document.querySelector('[role="option"]')).toBeNull();
|
||||
});
|
||||
|
||||
it("keeps the picker disabled until the saved detail is refreshed, then uses the new revision", async () => {
|
||||
let resolveRefresh!: (value: RoutineDetail) => void;
|
||||
const refresh = new Promise<RoutineDetail>((resolve) => { resolveRefresh = resolve; });
|
||||
const saved = { ...routine, responsibleUserId: "user-sam", latestRevisionId: "revision-2" };
|
||||
vi.mocked(routinesApi.update).mockResolvedValue(saved);
|
||||
function Detail() {
|
||||
const detail = useQuery({
|
||||
queryKey: queryKeys.routines.detail(routine.id),
|
||||
queryFn: () => refresh,
|
||||
initialData: routine,
|
||||
staleTime: Infinity,
|
||||
});
|
||||
return <RoutineResponsibleUser routine={detail.data} />;
|
||||
}
|
||||
await act(async () => root.render(<QueryClientProvider client={client}><Detail /></QueryClientProvider>));
|
||||
await act(async () => { await new Promise((resolve) => setTimeout(resolve, 20)); });
|
||||
await selectSam();
|
||||
expect(container.querySelector<HTMLButtonElement>("button")?.disabled).toBe(true);
|
||||
expect(container.textContent).toContain("Saving…");
|
||||
await act(async () => resolveRefresh(saved));
|
||||
await act(async () => { await new Promise((resolve) => setTimeout(resolve, 20)); });
|
||||
expect(container.querySelector<HTMLButtonElement>("button")?.disabled).toBe(false);
|
||||
expect(container.textContent).toContain("Sam");
|
||||
await act(async () => container.querySelector<HTMLButtonElement>("button")!.click());
|
||||
const alex = [...document.querySelectorAll<HTMLElement>('[role="option"]')].find((item) => item.textContent?.includes("Alex"));
|
||||
await act(async () => alex!.click());
|
||||
expect(routinesApi.update).toHaveBeenLastCalledWith(routine.id, { responsibleUserId: "user-alex", baseRevisionId: "revision-2" });
|
||||
});
|
||||
|
||||
it("does not save when the current person is selected", async () => {
|
||||
await render();
|
||||
await act(async () => container.querySelector<HTMLButtonElement>("button")!.click());
|
||||
const option = [...document.querySelectorAll<HTMLElement>('[role="option"]')].find((item) => item.textContent?.includes("Alex"));
|
||||
await act(async () => option!.click());
|
||||
expect(routinesApi.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it.each([403, 409])("keeps the saved person and shows the %s failure", async (status) => {
|
||||
vi.mocked(routinesApi.update).mockRejectedValue(new ApiError("Missing permission: tasks:assign", status, null));
|
||||
await render();
|
||||
await selectSam();
|
||||
expect(container.textContent).toContain("Alex");
|
||||
expect(container.querySelector('[role="alert"]')?.textContent).toContain(status === 409 ? "Reload before changing responsibility" : "Missing permission");
|
||||
});
|
||||
|
||||
it("shows an unavailable person without inventing a name and permits repair", async () => {
|
||||
await render({ ...routine, responsibleUserId: "former-user" });
|
||||
expect(container.textContent).toContain("Unavailable user");
|
||||
await selectSam();
|
||||
expect(routinesApi.update).toHaveBeenCalledWith(routine.id, expect.objectContaining({ responsibleUserId: "user-sam" }));
|
||||
});
|
||||
|
||||
it("disables the picker and explains a directory failure", async () => {
|
||||
vi.mocked(accessApi.listUserDirectory).mockRejectedValue(new Error("Unavailable"));
|
||||
await render();
|
||||
expect(container.querySelector<HTMLButtonElement>("button")?.disabled).toBe(true);
|
||||
expect(container.querySelector('[role="alert"]')?.textContent).toContain("Unavailable");
|
||||
expect(container.textContent).toContain("Retry");
|
||||
});
|
||||
|
||||
it("keeps a cached person visible without a local alert during a transient directory failure", async () => {
|
||||
await render();
|
||||
vi.mocked(accessApi.listUserDirectory).mockRejectedValue(new ApiError("tenant_app_unavailable", 503, { error: "tenant_app_unavailable" }));
|
||||
await act(async () => client.invalidateQueries({ queryKey: queryKeys.access.companyUserDirectory(routine.companyId) }));
|
||||
await act(async () => { await new Promise((resolve) => setTimeout(resolve, 20)); });
|
||||
expect(container.textContent).toContain("Alex");
|
||||
expect(container.querySelector<HTMLButtonElement>("button")?.disabled).toBe(false);
|
||||
expect(container.querySelector('[role="alert"]')).toBeNull();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,105 @@
|
||||
import { useState } from "react";
|
||||
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
|
||||
import type { RoutineDetail } from "@paperclipai/shared";
|
||||
import { Check, ChevronDown, UserRound } from "lucide-react";
|
||||
import { accessApi } from "@/api/access";
|
||||
import { ApiError } from "@/api/client";
|
||||
import { describeError } from "@/api/errors";
|
||||
import { routinesApi } from "@/api/routines";
|
||||
import { buildCompanyUserProfileMap } from "@/lib/company-members";
|
||||
import { queryKeys } from "@/lib/queryKeys";
|
||||
import { Command, CommandEmpty, CommandInput, CommandItem, CommandList } from "./ui/command";
|
||||
import { Popover, PopoverContent, PopoverTrigger } from "./ui/popover";
|
||||
import { QueryErrorState, useQueryView } from "./QueryView";
|
||||
|
||||
export function RoutineResponsibleUser({ routine }: { routine: RoutineDetail }) {
|
||||
const [open, setOpen] = useState(false);
|
||||
const queryClient = useQueryClient();
|
||||
const directory = useQuery({
|
||||
queryKey: queryKeys.access.companyUserDirectory(routine.companyId),
|
||||
queryFn: () => accessApi.listUserDirectory(routine.companyId),
|
||||
});
|
||||
const directoryView = useQueryView(directory);
|
||||
const profiles = buildCompanyUserProfileMap(directory.data?.users);
|
||||
const label = routine.responsibleUserId
|
||||
? profiles.get(routine.responsibleUserId)?.label
|
||||
?? (directory.isPending ? "Loading…" : "Unavailable user")
|
||||
: "No responsible user";
|
||||
const change = useMutation({
|
||||
mutationFn: (responsibleUserId: string) => routinesApi.update(routine.id, {
|
||||
responsibleUserId,
|
||||
baseRevisionId: routine.latestRevisionId,
|
||||
}),
|
||||
onSuccess: async (saved) => {
|
||||
queryClient.setQueryData<RoutineDetail>(queryKeys.routines.detail(routine.id), (current) => (
|
||||
current ? { ...current, ...saved } : current
|
||||
));
|
||||
await Promise.all([
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.routines.detail(routine.id) }),
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.routines.list(routine.companyId) }),
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.routines.revisions(routine.id) }),
|
||||
queryClient.invalidateQueries({ queryKey: queryKeys.routines.activity(routine.companyId, routine.id) }),
|
||||
]);
|
||||
},
|
||||
});
|
||||
|
||||
return (
|
||||
<section className="flex min-w-0 flex-col gap-2" aria-labelledby="routine-responsible-heading">
|
||||
<h2 id="routine-responsible-heading" className="text-sm font-semibold">Responsible user</h2>
|
||||
{routine.canChangeResponsibleUser ? (
|
||||
<Popover open={open} onOpenChange={setOpen}>
|
||||
<PopoverTrigger asChild>
|
||||
<button
|
||||
type="button"
|
||||
aria-label={`Change responsible user: ${label}`}
|
||||
disabled={change.isPending || !directory.data}
|
||||
className="group flex w-fit max-w-full items-center gap-2 rounded-md text-left text-sm font-medium hover:text-muted-foreground focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring disabled:opacity-50"
|
||||
>
|
||||
<UserRound className="h-4 w-4 shrink-0 text-muted-foreground" aria-hidden="true" />
|
||||
<span className="truncate">{change.isPending ? "Saving…" : label}</span>
|
||||
<ChevronDown className="h-3.5 w-3.5 shrink-0 text-muted-foreground opacity-60 group-hover:opacity-100 group-focus-visible:opacity-100" aria-hidden="true" />
|
||||
</button>
|
||||
</PopoverTrigger>
|
||||
<PopoverContent align="start" className="w-72 p-0">
|
||||
<Command>
|
||||
<CommandInput placeholder="Find a person…" aria-label="Find a responsible user" />
|
||||
<CommandList aria-label="Responsible user">
|
||||
<CommandEmpty>No people found.</CommandEmpty>
|
||||
{(directory.data?.users ?? []).map((member) => (
|
||||
<CommandItem
|
||||
key={member.principalId}
|
||||
value={`${member.principalId} ${profiles.get(member.principalId)?.label} ${member.user?.email ?? ""}`}
|
||||
onSelect={() => {
|
||||
setOpen(false);
|
||||
if (member.principalId !== routine.responsibleUserId) change.mutate(member.principalId);
|
||||
}}
|
||||
>
|
||||
<UserRound className="h-4 w-4 shrink-0 text-muted-foreground" aria-hidden="true" />
|
||||
<span className="min-w-0 flex-1 truncate">{profiles.get(member.principalId)?.label}</span>
|
||||
{member.principalId === routine.responsibleUserId ? <Check className="h-4 w-4" aria-label="Current responsible user" /> : null}
|
||||
</CommandItem>
|
||||
))}
|
||||
</CommandList>
|
||||
</Command>
|
||||
</PopoverContent>
|
||||
</Popover>
|
||||
) : (
|
||||
<p className="flex items-center gap-2 text-sm font-medium">
|
||||
<UserRound className="h-4 w-4 shrink-0 text-muted-foreground" aria-hidden="true" />
|
||||
<span className="truncate">{label}</span>
|
||||
</p>
|
||||
)}
|
||||
<p className="text-xs text-muted-foreground">Automatic tasks use this person. Manual runs use the person who starts them.</p>
|
||||
{directoryView.kind === "error" ? (
|
||||
<QueryErrorState error={directoryView.error} size="inline" action="load people" onRetry={directoryView.retry} retrying={directoryView.isFetching} />
|
||||
) : null}
|
||||
{change.error ? (
|
||||
<p role="alert" className="text-xs text-destructive">
|
||||
{change.error instanceof ApiError && change.error.status === 409
|
||||
? "This routine changed. Reload before changing responsibility."
|
||||
: describeError(change.error, { action: "change responsibility" }).body}
|
||||
</p>
|
||||
) : null}
|
||||
</section>
|
||||
);
|
||||
}
|
||||
Reference in new issue
Block a user