From 7dae1b64de4299fd2c78e5e63f3bb5f23d72af8b Mon Sep 17 00:00:00 2001 From: Alex Grozav Date: Mon, 12 Oct 2026 12:51:08 +0000 Subject: [PATCH] feat(editor): Add bulk state actions to the migration rule detail (no-changelog) (#40780) Co-authored-by: Michael Siega Co-authored-by: Claude Fable 5.1 --- ...ation-finding-statuses-request.dto.test.ts | 23 +++ ...-migration-finding-statuses-request.dto.ts | 9 + packages/@n8n/api-types/src/dto/index.ts | 1 + .../breaking-changes.controller.test.ts | 30 ++++ ...ion-finding.repository.integration.test.ts | 95 ++++++++++ .../breaking-changes.controller.ts | 13 ++ .../migration-finding.repository.ts | 25 +++ .../migration-finding-triage.service.test.ts | 36 ++++ .../migration-finding-triage.service.ts | 41 +++-- .../frontend/@n8n/i18n/src/locales/en.json | 2 + .../api/__tests__/breaking-changes.test.ts | 27 +++ .../src/api/breaking-changes.ts | 12 ++ .../MigrationRuleDetail.test.ts | 162 +++++++++++++++++- .../migrationReport/MigrationRuleDetail.vue | 124 ++++++++++++-- 14 files changed, 577 insertions(+), 23 deletions(-) create mode 100644 packages/@n8n/api-types/src/dto/breaking-changes/__tests__/update-migration-finding-statuses-request.dto.test.ts create mode 100644 packages/@n8n/api-types/src/dto/breaking-changes/update-migration-finding-statuses-request.dto.ts create mode 100644 packages/frontend/@n8n/rest-api-client/src/api/__tests__/breaking-changes.test.ts diff --git a/packages/@n8n/api-types/src/dto/breaking-changes/__tests__/update-migration-finding-statuses-request.dto.test.ts b/packages/@n8n/api-types/src/dto/breaking-changes/__tests__/update-migration-finding-statuses-request.dto.test.ts new file mode 100644 index 00000000000..ca163be437f --- /dev/null +++ b/packages/@n8n/api-types/src/dto/breaking-changes/__tests__/update-migration-finding-statuses-request.dto.test.ts @@ -0,0 +1,23 @@ +import { UpdateMigrationFindingStatusesRequestDto } from '../update-migration-finding-statuses-request.dto'; + +describe('UpdateMigrationFindingStatusesRequestDto', () => { + test.each(['open', 'wont_fix'])('should accept the status %s', (status) => { + expect( + UpdateMigrationFindingStatusesRequestDto.safeParse({ workflowIds: ['wf-1', 'wf-2'], status }) + .success, + ).toBe(true); + }); + + test.each([ + { + name: 'a status that only the scan sets', + request: { workflowIds: ['wf-1'], status: 'fixed' }, + }, + { name: 'an empty workflow list', request: { workflowIds: [], status: 'wont_fix' } }, + { name: 'an empty workflow id', request: { workflowIds: [''], status: 'wont_fix' } }, + { name: 'a missing workflow list', request: { status: 'wont_fix' } }, + { name: 'a missing status', request: { workflowIds: ['wf-1'] } }, + ])('should reject $name', ({ request }) => { + expect(UpdateMigrationFindingStatusesRequestDto.safeParse(request).success).toBe(false); + }); +}); diff --git a/packages/@n8n/api-types/src/dto/breaking-changes/update-migration-finding-statuses-request.dto.ts b/packages/@n8n/api-types/src/dto/breaking-changes/update-migration-finding-statuses-request.dto.ts new file mode 100644 index 00000000000..d598ef00447 --- /dev/null +++ b/packages/@n8n/api-types/src/dto/breaking-changes/update-migration-finding-statuses-request.dto.ts @@ -0,0 +1,9 @@ +import { z } from 'zod'; + +import { migrationFindingTriageStatusSchema } from '../../schemas/breaking-changes.schema'; +import { Z } from '../../zod-class'; + +export class UpdateMigrationFindingStatusesRequestDto extends Z.class({ + workflowIds: z.array(z.string().min(1)).min(1), + status: migrationFindingTriageStatusSchema, +}) {} diff --git a/packages/@n8n/api-types/src/dto/index.ts b/packages/@n8n/api-types/src/dto/index.ts index 98abcfd7527..1b887091073 100644 --- a/packages/@n8n/api-types/src/dto/index.ts +++ b/packages/@n8n/api-types/src/dto/index.ts @@ -54,6 +54,7 @@ export type { DiscoverDataPublic } from './discover/discover-public.dto'; export { BreakingChangeReportQueryDto } from './breaking-changes/breaking-change-report-query.dto'; export { UpdateMigrationFindingStatusRequestDto } from './breaking-changes/update-migration-finding-status-request.dto'; +export { UpdateMigrationFindingStatusesRequestDto } from './breaking-changes/update-migration-finding-statuses-request.dto'; export { AssignMigrationOwnerDto } from './breaking-changes/assign-migration-owner.dto'; export { diff --git a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts index 522bcf88564..a4a15712944 100644 --- a/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts +++ b/packages/cli/src/modules/breaking-changes/__tests__/breaking-changes.controller.test.ts @@ -333,6 +333,36 @@ describe('BreakingChangesController', () => { }); }); + describe('PATCH /report/:ruleId/workflows', () => { + it('passes the rule, workflows and status to the triage service and returns nothing', async () => { + triageService.setStatuses.mockResolvedValue(undefined); + + const result = await controller.updateFindingStatuses(req, res, 'removed-nodes-v3', { + workflowIds: ['wf-1', 'wf-2'], + status: 'wont_fix', + }); + + expect(result).toBeUndefined(); + expect(triageService.setStatuses).toHaveBeenCalledWith( + 'removed-nodes-v3', + ['wf-1', 'wf-2'], + 'wont_fix', + ); + expect(syncService.syncIfStale).not.toHaveBeenCalled(); + }); + + it('requires the global breakingChanges:migrate scope', () => { + const metadata = Container.get(ControllerRegistryMetadata).getControllerMetadata( + BreakingChangesController as Controller, + ); + + expect(metadata.routes.get('updateFindingStatuses')?.accessScope).toEqual({ + scope: 'breakingChanges:migrate', + globalOnly: true, + }); + }); + }); + describe('PATCH /report/:ruleId/workflows/:workflowId', () => { it('passes the rule, workflow and status to the triage service and returns nothing', async () => { triageService.setStatus.mockResolvedValue(undefined); diff --git a/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts b/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts index 60c8e136cda..e3467ec14ae 100644 --- a/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts +++ b/packages/cli/src/modules/breaking-changes/__tests__/migration-finding.repository.integration.test.ts @@ -666,6 +666,101 @@ describe('MigrationFindingRepository', () => { }); }); + describe('setTriageStatusForWorkflows', () => { + const RULE_ID = 'removed-nodes-v3'; + + test('sets the status on the findings of the given workflows and moves statusChangedAt', async () => { + const [first, second] = await Promise.all([createWorkflow(), createWorkflow()]); + await insertWithStatusChangedAt(first.id, PAST); + await insertWithStatusChangedAt(second.id, PAST); + + await findingRepository.setTriageStatusForWorkflows( + 'v3', + RULE_ID, + [first.id, second.id], + 'wont_fix', + ctx, + ); + + const rows = await findingRepository.listForWorkflows('v3', [first.id, second.id], ctx); + expect(rows.map((row) => row.status)).toEqual(['wont_fix', 'wont_fix']); + for (const row of rows) { + expect(row.statusChangedAt.getTime()).toBeGreaterThan(PAST.getTime()); + } + }); + + test('keeps statusChangedAt on a finding that already has the status', async () => { + const workflow = await createWorkflow(); + const id = await insertWithStatusChangedAt(workflow.id, PAST); + await findingRepository.update({ id }, { status: 'wont_fix', statusChangedAt: PAST }); + + await findingRepository.setTriageStatusForWorkflows( + 'v3', + RULE_ID, + [workflow.id], + 'wont_fix', + ctx, + ); + + const [after] = await findingRepository.listForWorkflows('v3', [workflow.id], ctx); + expect(after.status).toBe('wont_fix'); + expect(after.statusChangedAt.getTime()).toBe(PAST.getTime()); + }); + + test('keeps a fixed finding fixed and still updates the others', async () => { + const [fixed, open] = await Promise.all([createWorkflow(), createWorkflow()]); + const fixedId = await insertWithStatusChangedAt(fixed.id, PAST); + await insertWithStatusChangedAt(open.id, PAST); + await findingRepository.markFixedForIds([fixedId], ctx); + + await findingRepository.setTriageStatusForWorkflows( + 'v3', + RULE_ID, + [fixed.id, open.id], + 'wont_fix', + ctx, + ); + + const rows = await findingRepository.find(); + expect(rows.find((row) => row.workflowId === fixed.id)?.status).toBe('fixed'); + expect(rows.find((row) => row.workflowId === open.id)?.status).toBe('wont_fix'); + }); + + test('changes only the findings of the given version, rule and workflows', async () => { + const [target, other] = await Promise.all([createWorkflow(), createWorkflow()]); + await findingRepository.insertMany( + [ + finding(target.id, RULE_ID), + finding(target.id, RULE_ID, 'v2'), + finding(target.id, 'rule-b'), + finding(other.id, RULE_ID), + ], + ctx, + ); + + await findingRepository.setTriageStatusForWorkflows( + 'v3', + RULE_ID, + [target.id], + 'wont_fix', + ctx, + ); + + const rows = await findingRepository.find(); + const statusOf = (workflowId: string, ruleId: string, targetVersion: string) => + rows.find( + (row) => + row.workflowId === workflowId && + row.ruleId === ruleId && + row.targetVersion === targetVersion, + )?.status; + expect(statusOf(target.id, RULE_ID, 'v3')).toBe('wont_fix'); + expect(statusOf(target.id, RULE_ID, 'v2')).toBe('open'); + expect(statusOf(target.id, 'rule-b', 'v3')).toBe('open'); + expect(statusOf(other.id, RULE_ID, 'v3')).toBe('open'); + }); + }); + describe('cascade delete', () => { test('deletes the findings of a deleted workflow and keeps the others', async () => { const [deleted, kept] = await Promise.all([createWorkflow(), createWorkflow()]); diff --git a/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts b/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts index 0bc0ce9c6da..d52274219a6 100644 --- a/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts +++ b/packages/cli/src/modules/breaking-changes/breaking-changes.controller.ts @@ -6,6 +6,7 @@ import { BreakingChangeVersion, MigrationWorkflowOwnerResponse, UpdateMigrationFindingStatusRequestDto, + UpdateMigrationFindingStatusesRequestDto, WorkflowMigrationResult, } from '@n8n/api-types'; import { EventService, WorkflowSharingService } from '@n8n/backend-services'; @@ -140,6 +141,18 @@ export class BreakingChangesController { return await this.queryService.getRuleFindings(version, ruleId, scope); } + /** Sets the status a user picks for the findings of one rule on many workflows. */ + @Patch('/report/:ruleId/workflows') + @GlobalScope('breakingChanges:migrate') + async updateFindingStatuses( + _req: AuthenticatedRequest, + _res: Response, + @Param('ruleId') ruleId: string, + @Body body: UpdateMigrationFindingStatusesRequestDto, + ): Promise { + await this.triageService.setStatuses(ruleId, body.workflowIds, body.status); + } + /** Sets the status a user picks for the finding of one rule on one workflow. */ @Patch('/report/:ruleId/workflows/:workflowId') @GlobalScope('breakingChanges:migrate') diff --git a/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts index 650fa118786..7079a72c508 100644 --- a/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts +++ b/packages/cli/src/modules/breaking-changes/database/repositories/migration-finding.repository.ts @@ -196,6 +196,31 @@ export class MigrationFindingRepository extends BaseRepository return (result.affected ?? 0) > 0; } + /** + * Sets the status a user picked on the findings of one rule on many workflows. + * Findings in a status only the scan sets, for example `fixed`, stay as they are. + * `statusChangedAt` moves only on the findings whose status changes. + */ + async setTriageStatusForWorkflows( + targetVersion: BreakingChangeVersion, + ruleId: string, + workflowIds: string[], + status: MigrationFindingTriageStatus, + ctx: OperationContext, + ): Promise { + const otherTriageStatuses = TRIAGE_STATUSES.filter((triageStatus) => triageStatus !== status); + // One transaction, so a failed chunk does not leave the earlier chunks changed. + await this.runInTransaction(ctx, async (manager) => { + for (const chunk of chunkIds(workflowIds)) { + await manager.update( + MigrationFinding, + { targetVersion, ruleId, workflowId: In(chunk), status: In(otherTriageStatuses) }, + { status, statusChangedAt: new Date() }, + ); + } + }); + } + /** * Inserts the findings as `open`. A finding that exists for the same workflow, * rule and target version is left as it is, so two syncs that run at the same diff --git a/packages/cli/src/modules/breaking-changes/triage/__tests__/migration-finding-triage.service.test.ts b/packages/cli/src/modules/breaking-changes/triage/__tests__/migration-finding-triage.service.test.ts index cf41e7e60ad..0fec99ad8fd 100644 --- a/packages/cli/src/modules/breaking-changes/triage/__tests__/migration-finding-triage.service.test.ts +++ b/packages/cli/src/modules/breaking-changes/triage/__tests__/migration-finding-triage.service.test.ts @@ -77,4 +77,40 @@ describe('MigrationFindingTriageService', () => { expect.anything(), ); }); + + describe('setStatuses', () => { + it("sets the status on the given workflows for the rule's version", async () => { + registerRule('removed-nodes-v3', 'v3', 'workflow'); + + await expect( + service.setStatuses('removed-nodes-v3', ['wf-1', 'wf-2'], 'wont_fix'), + ).resolves.toBeUndefined(); + + expect(findingRepository.setTriageStatusForWorkflows).toHaveBeenCalledWith( + 'v3', + 'removed-nodes-v3', + ['wf-1', 'wf-2'], + 'wont_fix', + expect.anything(), + ); + }); + + it('rejects an unknown rule with not-found before writing', async () => { + ruleRegistry.getRule.mockReturnValue(undefined); + + await expect(service.setStatuses('unknown', ['wf-1'], 'open')).rejects.toBeInstanceOf( + NotFoundError, + ); + expect(findingRepository.setTriageStatusForWorkflows).not.toHaveBeenCalled(); + }); + + it('rejects an instance rule with not-found before writing', async () => { + registerRule('docker-only-deployment-v3', 'v3', 'instance'); + + await expect( + service.setStatuses('docker-only-deployment-v3', ['wf-1'], 'open'), + ).rejects.toBeInstanceOf(NotFoundError); + expect(findingRepository.setTriageStatusForWorkflows).not.toHaveBeenCalled(); + }); + }); }); diff --git a/packages/cli/src/modules/breaking-changes/triage/migration-finding-triage.service.ts b/packages/cli/src/modules/breaking-changes/triage/migration-finding-triage.service.ts index 4114eb5ef42..6dc5b8dfae3 100644 --- a/packages/cli/src/modules/breaking-changes/triage/migration-finding-triage.service.ts +++ b/packages/cli/src/modules/breaking-changes/triage/migration-finding-triage.service.ts @@ -14,21 +14,13 @@ export class MigrationFindingTriageService { private readonly findingRepository: MigrationFindingRepository, ) {} - /** - * Sets the status of the finding of one workflow rule on one workflow. The - * rule decides the target version, as on the rule detail route. - */ + /** Sets the status of the finding of one workflow rule on one workflow. */ async setStatus( ruleId: string, workflowId: string, status: MigrationFindingTriageStatus, ): Promise { - const rule = this.ruleRegistry.getRule(ruleId); - if (!rule || !isWorkflowLevelRule(rule)) { - throw new NotFoundError(`Breaking change rule with ID '${ruleId}' not found.`); - } - - const version = rule.getMetadata().version; + const version = this.getWorkflowRuleVersion(ruleId); const updated = await this.findingRepository.setTriageStatus( version, ruleId, @@ -43,4 +35,33 @@ export class MigrationFindingTriageService { ); } } + + /** + * Sets the status of the findings of one workflow rule on many workflows. A + * workflow without a finding in a status a user can set is skipped, so one + * fixed finding does not fail the others. + */ + async setStatuses( + ruleId: string, + workflowIds: string[], + status: MigrationFindingTriageStatus, + ): Promise { + const version = this.getWorkflowRuleVersion(ruleId); + await this.findingRepository.setTriageStatusForWorkflows( + version, + ruleId, + workflowIds, + status, + {}, + ); + } + + /** The rule decides the target version, as on the rule detail route. */ + private getWorkflowRuleVersion(ruleId: string) { + const rule = this.ruleRegistry.getRule(ruleId); + if (!rule || !isWorkflowLevelRule(rule)) { + throw new NotFoundError(`Breaking change rule with ID '${ruleId}' not found.`); + } + return rule.getMetadata().version; + } } diff --git a/packages/frontend/@n8n/i18n/src/locales/en.json b/packages/frontend/@n8n/i18n/src/locales/en.json index 2df9db08502..aded1f13081 100644 --- a/packages/frontend/@n8n/i18n/src/locales/en.json +++ b/packages/frontend/@n8n/i18n/src/locales/en.json @@ -4657,6 +4657,8 @@ "settings.migrationReport.detail.state.wontFix": "Won't fix", "settings.migrationReport.detail.state.wontFix.description": "Counts as resolved without a fix", "settings.migrationReport.detail.state.error.title": "Could not change the state", + "settings.migrationReport.detail.bulk.wontFix": "Mark won't fix", + "settings.migrationReport.detail.bulk.reopen": "Reopen", "settings.migrationReport.detail.search.placeholder": "Search workflows...", "settings.migrationReport.detail.quickFilter.assignedToMe": "Assigned to me {count}", "settings.migrationReport.detail.quickFilter.open": "Open {count}", diff --git a/packages/frontend/@n8n/rest-api-client/src/api/__tests__/breaking-changes.test.ts b/packages/frontend/@n8n/rest-api-client/src/api/__tests__/breaking-changes.test.ts new file mode 100644 index 00000000000..ab6be8aa423 --- /dev/null +++ b/packages/frontend/@n8n/rest-api-client/src/api/__tests__/breaking-changes.test.ts @@ -0,0 +1,27 @@ +import type { IRestApiContext } from '../../types'; +import * as utils from '../../utils'; +import { updateFindingStatuses } from '../breaking-changes'; + +vi.mock('../../utils'); + +const context = {} as IRestApiContext; + +describe('updateFindingStatuses', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('sends a PATCH to the rule workflows endpoint with the workflow IDs and status', async () => { + vi.mocked(utils.makeRestApiRequest).mockResolvedValueOnce(undefined); + + await updateFindingStatuses(context, 'removed-nodes-v3', ['wf-1', 'wf-2'], 'wont_fix'); + + expect(utils.makeRestApiRequest).toHaveBeenCalledTimes(1); + expect(utils.makeRestApiRequest).toHaveBeenCalledWith( + context, + 'PATCH', + '/breaking-changes/report/removed-nodes-v3/workflows', + { workflowIds: ['wf-1', 'wf-2'], status: 'wont_fix' }, + ); + }); +}); diff --git a/packages/frontend/@n8n/rest-api-client/src/api/breaking-changes.ts b/packages/frontend/@n8n/rest-api-client/src/api/breaking-changes.ts index dd1982c00d1..40a9fe2c633 100644 --- a/packages/frontend/@n8n/rest-api-client/src/api/breaking-changes.ts +++ b/packages/frontend/@n8n/rest-api-client/src/api/breaking-changes.ts @@ -88,3 +88,15 @@ export async function updateFindingStatus( { status }, ); } + +export async function updateFindingStatuses( + context: IRestApiContext, + ruleId: string, + workflowIds: string[], + status: MigrationFindingTriageStatus, +): Promise { + await makeRestApiRequest(context, 'PATCH', `/breaking-changes/report/${ruleId}/workflows`, { + workflowIds, + status, + }); +} diff --git a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts index cf86dde71d0..c875f7775e9 100644 --- a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts +++ b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.test.ts @@ -19,6 +19,7 @@ vi.mock('@n8n/rest-api-client/api/breaking-changes', () => ({ getReportForRule: vi.fn(), migrateWorkflowForRule: vi.fn(), updateFindingStatus: vi.fn(), + updateFindingStatuses: vi.fn(), assignWorkflowOwner: vi.fn(), unassignWorkflowOwner: vi.fn(), })); @@ -211,7 +212,8 @@ describe('MigrationRuleDetail', () => { const titles = screen .getAllByRole('columnheader') .map((th) => th.textContent?.replace(/[↑↓]/g, '').trim()); - expect(titles).toEqual(['Name', 'Nodes affected', 'Usage', 'Owner', 'State', '']); + // The first column holds the select-all checkbox. + expect(titles).toEqual(['', 'Name', 'Nodes affected', 'Usage', 'Owner', 'State', '']); }); }); }); @@ -353,6 +355,155 @@ describe('MigrationRuleDetail', () => { }); }); + describe('bulk selection', () => { + const getRowCheckbox = (workflowName: string) => { + const row = screen.getByText(workflowName).closest('tr'); + if (!row) throw new Error('Row not found'); + return within(row).getByRole('checkbox'); + }; + + const getStateSelect = (workflowName: string) => { + const row = screen.getByText(workflowName).closest('tr'); + if (!row) throw new Error('Row not found'); + return within(row).getByTestId('migration-finding-state-select'); + }; + + it('should show the selection bar only while rows are selected', async () => { + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + expect(screen.queryByTestId('selected-items-info')).not.toBeInTheDocument(); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + + expect(await screen.findByTestId('selected-items-info')).toHaveTextContent('1 row selected'); + + await userEvent.click(screen.getByTestId('clear-selection-button')); + + await waitFor(() => + expect(screen.queryByTestId('selected-items-info')).not.toBeInTheDocument(), + ); + }); + + it("should mark the selected rows as won't fix in one request and clear the selection", async () => { + vi.mocked(breakingChangesApi.updateFindingStatuses).mockResolvedValue(); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + await userEvent.click(getRowCheckbox('Test Workflow 2')); + await userEvent.click(screen.getByTestId('migration-bulk-wont-fix-button')); + + await waitFor(() => { + expect(getStateSelect('Test Workflow 1')).toHaveTextContent("Won't fix"); + expect(getStateSelect('Test Workflow 2')).toHaveTextContent("Won't fix"); + expect(screen.getByText('0 affected')).toBeInTheDocument(); + expect(screen.queryByTestId('selected-items-info')).not.toBeInTheDocument(); + }); + expect(breakingChangesApi.updateFindingStatuses).toHaveBeenCalledTimes(1); + const [, ruleId, workflowIds, status] = vi.mocked(breakingChangesApi.updateFindingStatuses) + .mock.calls[0]; + expect(ruleId).toBe('rule-1'); + expect([...workflowIds].sort()).toEqual(['workflow-1', 'workflow-2']); + expect(status).toBe('wont_fix'); + }); + + it('should keep a row that the user selects while the request is in flight', async () => { + let finishSave = () => {}; + vi.mocked(breakingChangesApi.updateFindingStatuses).mockImplementation( + async () => await new Promise((resolve) => (finishSave = resolve)), + ); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + await userEvent.click(screen.getByTestId('migration-bulk-wont-fix-button')); + await userEvent.click(getRowCheckbox('Test Workflow 2')); + finishSave(); + + await waitFor(() => + expect(screen.getByTestId('selected-items-info')).toHaveTextContent('1 row selected'), + ); + expect(getRowCheckbox('Test Workflow 2')).toBeChecked(); + expect(getRowCheckbox('Test Workflow 1')).not.toBeChecked(); + }); + + it('should reopen only the selected rows that are not open', async () => { + vi.mocked(breakingChangesApi.getReportForRule).mockResolvedValue( + createMockRuleResult({ + affectedWorkflows: [ + mockWorkflowWithIssue, + { ...mockWorkflowWithMultipleNodes, status: 'wont_fix' }, + ], + }), + ); + vi.mocked(breakingChangesApi.updateFindingStatuses).mockResolvedValue(); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('1 affected'); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + await userEvent.click(getRowCheckbox('Test Workflow 2')); + await userEvent.click(screen.getByTestId('migration-bulk-reopen-button')); + + await waitFor(() => expect(screen.getByText('2 affected')).toBeInTheDocument()); + expect(breakingChangesApi.updateFindingStatuses).toHaveBeenCalledWith( + rootStore.restApiContext, + 'rule-1', + ['workflow-2'], + 'open', + ); + }); + + it('should disable an action that would change none of the selected rows', async () => { + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + + expect(await screen.findByTestId('migration-bulk-reopen-button')).toBeDisabled(); + expect(screen.getByTestId('migration-bulk-wont-fix-button')).toBeEnabled(); + }); + + it('should revert the rows, keep the selection and show an error when the save fails', async () => { + const error = new Error('Request failed'); + vi.mocked(breakingChangesApi.updateFindingStatuses).mockRejectedValue(error); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + await userEvent.click(getRowCheckbox('Test Workflow 1')); + await userEvent.click(screen.getByTestId('migration-bulk-wont-fix-button')); + + await waitFor(() => { + expect(showError).toHaveBeenCalledWith(error, 'Could not change the state'); + }); + expect(getStateSelect('Test Workflow 1')).toHaveTextContent('Open'); + expect(screen.getByText('2 affected')).toBeInTheDocument(); + expect(screen.getByTestId('selected-items-info')).toHaveTextContent('1 row selected'); + }); + + it('should drop rows that a search hides from the selection', async () => { + const user = userEvent.setup({ delay: null }); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + await user.click(getRowCheckbox('Test Workflow 1')); + await user.click(getRowCheckbox('Test Workflow 2')); + await user.type(screen.getByPlaceholderText('Search workflows...'), 'workflow 1'); + + await vi.waitFor( + () => expect(screen.getByTestId('selected-items-info')).toHaveTextContent('1 row selected'), + { timeout: 1000 }, + ); + }); + + it('should not offer selection to a user without the migrate scope', async () => { + rbacStore.hasScope.mockReturnValue(false); + renderComponent({ props: { migrationRuleId: 'rule-1' } }); + await screen.findByText('2 affected'); + + expect(screen.queryAllByRole('checkbox')).toHaveLength(0); + }); + }); + describe('migration', () => { it('should not render a Migrate button when the rule is not migratable', async () => { vi.mocked(breakingChangesApi.getReportForRule).mockResolvedValue(mockRuleResult); @@ -526,7 +677,14 @@ describe('MigrationRuleDetail', () => { // The first row's search hangs; the second row's answers at once. await userEvent.click(pickers[0]); await userEvent.click(pickers[1]); - await waitFor(() => expect(usersApi.getUsers).toHaveBeenCalledTimes(2)); + // An open picker also runs its debounced remote search, so a slow run can see + // more calls. Wait for the second row's search instead of a call count. + await waitFor(() => + expect(usersApi.getUsers).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ filter: { projectId: 'project-2' } }), + ), + ); await waitFor(() => { expect(baseElement.querySelector('#user-select-option-id-user-2')).not.toBeNull(); }); diff --git a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.vue b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.vue index fe8c8ec97e9..d838a10e44d 100644 --- a/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.vue +++ b/packages/frontend/editor-ui/src/features/settings/migrationReport/MigrationRuleDetail.vue @@ -21,6 +21,7 @@ import { N8nInput, N8nLink, N8nLoading, + N8nSelectedItemsInfo, N8nSettingsLayout, N8nText, N8nUserSelect, @@ -35,7 +36,7 @@ import { useRootStore } from '@n8n/stores/useRootStore'; import { createEventBus } from '@n8n/utils/event-bus'; import { useAsyncState, useDebounceFn } from '@vueuse/core'; import orderBy from 'lodash/orderBy'; -import { computed, nextTick, ref } from 'vue'; +import { computed, nextTick, ref, watch } from 'vue'; import { useRouter } from 'vue-router'; import { I18nT } from 'vue-i18n'; import FindingStateSelect from './components/FindingStateSelect.vue'; @@ -296,15 +297,25 @@ const openCount = computed( const savingWorkflowIds = ref>(new Set()); // The state is a shallow ref, so replace the list to make the table update. -function setFindingStatus(workflowId: string, status: MigrationFindingTriageStatus) { +function setFindingStatuses(statuses: Map) { state.value = { ...state.value, - affectedWorkflows: state.value.affectedWorkflows.map((workflow) => - workflow.id === workflowId ? { ...workflow, status } : workflow, - ), + affectedWorkflows: state.value.affectedWorkflows.map((workflow) => { + const status = statuses.get(workflow.id); + return status ? { ...workflow, status } : workflow; + }), }; } +function setSaving(workflowIds: string[], saving: boolean) { + const next = new Set(savingWorkflowIds.value); + for (const workflowId of workflowIds) { + if (saving) next.add(workflowId); + else next.delete(workflowId); + } + savingWorkflowIds.value = next; +} + async function onFindingStatusChange( workflow: AffectedWorkflow, status: MigrationFindingTriageStatus, @@ -312,8 +323,8 @@ async function onFindingStatusChange( const previousStatus = workflow.status; if (status === previousStatus || savingWorkflowIds.value.has(workflow.id)) return; - setFindingStatus(workflow.id, status); - savingWorkflowIds.value = new Set(savingWorkflowIds.value).add(workflow.id); + setFindingStatuses(new Map([[workflow.id, status]])); + setSaving([workflow.id], true); try { await breakingChangesApi.updateFindingStatus( rootStore.restApiContext, @@ -322,12 +333,59 @@ async function onFindingStatusChange( status, ); } catch (error) { - setFindingStatus(workflow.id, previousStatus); + setFindingStatuses(new Map([[workflow.id, previousStatus]])); toast.showError(error, i18n.baseText('settings.migrationReport.detail.state.error.title')); } finally { - const saving = new Set(savingWorkflowIds.value); - saving.delete(workflow.id); - savingWorkflowIds.value = saving; + setSaving([workflow.id], false); + } +} + +const selectedWorkflowIds = ref([]); + +const selectedWorkflows = computed(() => { + const selected = new Set(selectedWorkflowIds.value); + return state.value.affectedWorkflows.filter((workflow) => selected.has(workflow.id)); +}); + +// A migrated row has no state to change, so it cannot be selected. +function isSelectable(workflow: AffectedWorkflow): boolean { + return !migratedWorkflowIds.value.has(workflow.id); +} + +/** The selected rows that a bulk change to `status` would change. */ +function bulkTargets(status: MigrationFindingTriageStatus): AffectedWorkflow[] { + return selectedWorkflows.value.filter( + (workflow) => + workflow.status !== status && + isSelectable(workflow) && + !savingWorkflowIds.value.has(workflow.id), + ); +} + +async function onBulkStatusChange(status: MigrationFindingTriageStatus) { + const workflows = bulkTargets(status); + if (workflows.length === 0) return; + + const workflowIds = workflows.map((workflow) => workflow.id); + setFindingStatuses(new Map(workflowIds.map((workflowId) => [workflowId, status]))); + setSaving(workflowIds, true); + try { + await breakingChangesApi.updateFindingStatuses( + rootStore.restApiContext, + props.migrationRuleId, + workflowIds, + status, + ); + // Keep the rows that the user selected while the request was in flight. + const changed = new Set(workflowIds); + selectedWorkflowIds.value = selectedWorkflowIds.value.filter( + (workflowId) => !changed.has(workflowId), + ); + } catch (error) { + setFindingStatuses(new Map(workflows.map((workflow) => [workflow.id, workflow.status]))); + toast.showError(error, i18n.baseText('settings.migrationReport.detail.state.error.title')); + } finally { + setSaving(workflowIds, false); } } @@ -458,6 +516,14 @@ const filteredWorkflows = computed(() => { ); }); +// A bulk action must not change rows the user cannot see, so a search or a +// filter drops the hidden rows from the selection. +watch(filteredWorkflows, (workflows) => { + const visible = new Set(workflows.map((workflow) => workflow.id)); + const kept = selectedWorkflowIds.value.filter((workflowId) => visible.has(workflowId)); + if (kept.length !== selectedWorkflowIds.value.length) selectedWorkflowIds.value = kept; +}); + // The owner cell shows a label, so it sorts by that label and not by the owner object. // Dates come from JSON as strings, and a workflow that never ran has no date. // A timestamp sorts them right, with a missing date as the oldest. @@ -617,6 +683,9 @@ const sortedWorkflows = computed(() => { { + +
+ + + +
@@ -786,6 +879,15 @@ const sortedWorkflows = computed(() => { gap: var(--spacing--sm); } +/* Sticks to the bottom of the viewport, so the floating selection bar stays in + view while a long table scrolls. The height holds the bar and its bottom + offset, so the bar does not cover the rows of a short table. */ +.selectionBar { + position: sticky; + bottom: 0; + height: calc(var(--spacing--3xl) + var(--spacing--2xl)); +} + .toolbar { display: flex; flex-wrap: wrap;