Skip to content

Commit 65e6667

Browse files
Copilothotlong
andcommitted
Address code review feedback: enable re-delegation and reduce code duplication
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
1 parent 38825d9 commit 65e6667

4 files changed

Lines changed: 19 additions & 51 deletions

File tree

packages/plugins/workflow/src/api.ts

Lines changed: 13 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,20 @@ import type {
1212
WorkflowQueryOptions,
1313
} from './types';
1414
import { WorkflowEngine } from './engine';
15+
import { ApprovalService } from './approval';
1516

1617
/**
1718
* Workflow API class
1819
*/
1920
export class WorkflowAPI {
2021
private engine: WorkflowEngine;
2122
private storage: WorkflowStorage;
23+
private approvalService: ApprovalService;
2224

2325
constructor(storage: WorkflowStorage, engine?: WorkflowEngine) {
2426
this.storage = storage;
2527
this.engine = engine || new WorkflowEngine();
28+
this.approvalService = new ApprovalService(storage);
2629
}
2730

2831
/**
@@ -261,29 +264,12 @@ export class WorkflowAPI {
261264
delegatedBy: string,
262265
reason?: string
263266
): Promise<WorkflowTask> {
264-
const task = await this.storage.getTask(taskId);
265-
if (!task) {
266-
throw new Error(`Task not found: ${taskId}`);
267-
}
268-
269-
if (task.status !== 'pending') {
270-
throw new Error(`Cannot delegate task in status: ${task.status}`);
271-
}
272-
273-
// Store original assignee if not already delegated
274-
const originalAssignee = task.originalAssignee || task.assignedTo;
275-
276-
await this.storage.updateTask(taskId, {
277-
status: 'delegated',
278-
originalAssignee,
279-
delegatedTo: delegateTo,
280-
delegatedAt: new Date(),
281-
delegationReason: reason,
282-
assignedTo: delegateTo,
267+
return this.approvalService.delegateTask({
268+
taskId,
269+
delegateTo,
270+
delegatedBy,
271+
reason,
283272
});
284-
285-
const updatedTask = await this.storage.getTask(taskId);
286-
return updatedTask!;
287273
}
288274

289275
/**
@@ -295,25 +281,12 @@ export class WorkflowAPI {
295281
reason?: string,
296282
escalatedBy?: string
297283
): Promise<WorkflowTask> {
298-
const task = await this.storage.getTask(taskId);
299-
if (!task) {
300-
throw new Error(`Task not found: ${taskId}`);
301-
}
302-
303-
if (task.status === 'completed' || task.status === 'rejected') {
304-
throw new Error(`Cannot escalate task in status: ${task.status}`);
305-
}
306-
307-
await this.storage.updateTask(taskId, {
308-
status: 'escalated',
309-
escalatedTo: escalateTo,
310-
escalatedAt: new Date(),
311-
escalationReason: reason,
312-
assignedTo: escalateTo,
284+
return this.approvalService.escalateTask({
285+
taskId,
286+
escalateTo,
287+
reason,
288+
escalatedBy,
313289
});
314-
315-
const updatedTask = await this.storage.getTask(taskId);
316-
return updatedTask!;
317290
}
318291

319292
/**

packages/plugins/workflow/src/approval.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ export class ApprovalService {
2929
throw new Error(`Task not found: ${taskId}`);
3030
}
3131

32-
if (task.status !== 'pending') {
32+
// Allow delegation of pending or already delegated tasks (for re-delegation)
33+
if (task.status !== 'pending' && task.status !== 'delegated') {
3334
throw new Error(`Cannot delegate task in status: ${task.status}`);
3435
}
3536

packages/plugins/workflow/test/api.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -466,10 +466,7 @@ describe('WorkflowAPI', () => {
466466
'First delegation'
467467
);
468468

469-
// Update to pending to allow second delegation
470-
await storage.updateTask(task.id, { status: 'pending' });
471-
472-
// Second delegation
469+
// Second delegation (should work on delegated task)
473470
const delegated = await api.delegateTask(
474471
task.id,
475472
'assistant2@example.com',
@@ -487,7 +484,7 @@ describe('WorkflowAPI', () => {
487484
).rejects.toThrow('Task not found');
488485
});
489486

490-
it('should throw error when delegating non-pending task', async () => {
487+
it('should throw error when delegating completed task', async () => {
491488
const task = await api.createTask({
492489
instanceId,
493490
name: 'Approval Task',

packages/plugins/workflow/test/approval.test.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,7 @@ describe('ApprovalService', () => {
6565
reason: 'First delegation',
6666
});
6767

68-
// Update status back to pending for second delegation
69-
await storage.updateTask('task_1', { status: 'pending' });
70-
71-
// Second delegation
68+
// Second delegation (should work on delegated task)
7269
const delegated = await approvalService.delegateTask({
7370
taskId: 'task_1',
7471
delegateTo: 'user3@example.com',
@@ -91,7 +88,7 @@ describe('ApprovalService', () => {
9188
).rejects.toThrow('Task not found');
9289
});
9390

94-
it('should throw error when delegating non-pending task', async () => {
91+
it('should throw error when delegating completed task', async () => {
9592
const task: WorkflowTask = {
9693
id: 'task_1',
9794
instanceId: 'wf_1',

0 commit comments

Comments
 (0)