mirror of
https://github.com/n8n-io/n8n.git
synced 2026-10-12 02:19:43 +00:00
feat(core): Show the suggested owner of each affected workflow (no-changelog) (#40394)
Co-authored-by: Claude Fable 5.1 <[email protected]>
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
af98327a5e
commit
fd04f01e8e
@@ -329,6 +329,8 @@ export type {
|
||||
BreakingChangeRuleImpact,
|
||||
BreakingChangeRecommendation,
|
||||
BreakingChangeAffectedWorkflow,
|
||||
BreakingChangeWorkflowOwner,
|
||||
MigrationOwnerSource,
|
||||
BreakingChangeInstanceIssue,
|
||||
BreakingChangeWorkflowIssue,
|
||||
BreakingChangeInstanceRuleResult,
|
||||
@@ -349,6 +351,7 @@ export {
|
||||
breakingChangeVersionSchema,
|
||||
migrationFindingStatusSchema,
|
||||
migrationFindingTriageStatusSchema,
|
||||
migrationOwnerSourceSchema,
|
||||
} from './schemas/breaking-changes.schema';
|
||||
|
||||
export type {
|
||||
|
||||
@@ -80,6 +80,20 @@ const workflowIssueSchema = instanceIssueSchema.extend({
|
||||
});
|
||||
export type BreakingChangeWorkflowIssue = z.infer<typeof workflowIssueSchema>;
|
||||
|
||||
/** `suggested` by the owner heuristic, or `assigned` by a person. */
|
||||
export const migrationOwnerSourceSchema = z.enum(['suggested', 'assigned']);
|
||||
export type MigrationOwnerSource = z.infer<typeof migrationOwnerSourceSchema>;
|
||||
|
||||
/** The user responsible for fixing a workflow's findings. The name and email columns of a user are nullable. */
|
||||
const workflowOwnerSchema = z.object({
|
||||
id: z.string(),
|
||||
firstName: z.string().nullable(),
|
||||
lastName: z.string().nullable(),
|
||||
email: z.string().nullable(),
|
||||
source: migrationOwnerSourceSchema,
|
||||
});
|
||||
export type BreakingChangeWorkflowOwner = z.infer<typeof workflowOwnerSchema>;
|
||||
|
||||
const affectedWorkflowSchema = z.object({
|
||||
id: z.string(),
|
||||
name: z.string(),
|
||||
@@ -88,6 +102,8 @@ const affectedWorkflowSchema = z.object({
|
||||
lastUpdatedAt: z.date(),
|
||||
lastExecutedAt: z.date().optional(),
|
||||
issues: z.array(workflowIssueSchema),
|
||||
// Absent when no owner is known for the workflow.
|
||||
owner: workflowOwnerSchema.optional(),
|
||||
});
|
||||
export type BreakingChangeAffectedWorkflow = z.infer<typeof affectedWorkflowSchema>;
|
||||
|
||||
|
||||
+35
@@ -384,6 +384,41 @@ describe('MigrationFindingRepository', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('listWorkflowIdsWithOpenFindings', () => {
|
||||
test('returns each given workflow with an open finding once, skipping fixed findings, other versions and other workflows', async () => {
|
||||
const [first, second, third, fourth] = await Promise.all([
|
||||
createWorkflow(),
|
||||
createWorkflow(),
|
||||
createWorkflow(),
|
||||
createWorkflow(),
|
||||
]);
|
||||
await findingRepository.insertMany(
|
||||
[
|
||||
finding(first.id, 'rule-a'),
|
||||
finding(first.id, 'rule-b'),
|
||||
finding(second.id, 'rule-a'),
|
||||
finding(third.id, 'rule-a', 'v2'),
|
||||
finding(fourth.id, 'rule-a'),
|
||||
],
|
||||
ctx,
|
||||
);
|
||||
const [fixed] = await findingRepository.listForWorkflows('v3', [second.id], ctx);
|
||||
await findingRepository.markFixedForIds([fixed.id], ctx);
|
||||
|
||||
const ids = await findingRepository.listWorkflowIdsWithOpenFindings(
|
||||
'v3',
|
||||
[first.id, second.id, third.id],
|
||||
ctx,
|
||||
);
|
||||
|
||||
expect(ids).toEqual([first.id]);
|
||||
});
|
||||
|
||||
test('returns an empty list for an empty id array', async () => {
|
||||
expect(await findingRepository.listWorkflowIdsWithOpenFindings('v3', [], ctx)).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('listTriageableForRule', () => {
|
||||
test('returns each open and wont_fix finding of the rule with its status, workflow name, published state and last update', async () => {
|
||||
const published = await createWorkflowWithHistory({ name: 'Published flow' });
|
||||
|
||||
+1
-3
@@ -1,3 +1,4 @@
|
||||
import type { MigrationOwnerSource } from '@n8n/api-types';
|
||||
import { DateTimeColumn, WithTimestamps } from '@n8n/db';
|
||||
import type { User, WorkflowEntity } from '@n8n/db';
|
||||
import {
|
||||
@@ -10,9 +11,6 @@ import {
|
||||
type Relation,
|
||||
} from '@n8n/typeorm';
|
||||
|
||||
/** `suggested` by the heuristic, or `assigned` by a person. Only a person changes an assignment. */
|
||||
export type MigrationOwnerSource = 'suggested' | 'assigned';
|
||||
|
||||
/**
|
||||
* The user responsible for fixing a workflow's migration findings, one row per
|
||||
* workflow. The row goes with its workflow. A deleted user leaves `userId` null,
|
||||
|
||||
+14
@@ -116,6 +116,20 @@ export class MigrationFindingRepository extends BaseRepository<MigrationFinding>
|
||||
return total;
|
||||
}
|
||||
|
||||
/** The given workflows that have at least one open finding for the version. */
|
||||
async listWorkflowIdsWithOpenFindings(
|
||||
targetVersion: BreakingChangeVersion,
|
||||
workflowIds: string[],
|
||||
ctx: OperationContext,
|
||||
): Promise<string[]> {
|
||||
if (workflowIds.length === 0) return [];
|
||||
const rows = await this.managerFor(ctx).find(MigrationFinding, {
|
||||
select: { workflowId: true },
|
||||
where: { targetVersion, status: 'open', workflowId: In(workflowIds) },
|
||||
});
|
||||
return [...new Set(rows.map((row) => row.workflowId))];
|
||||
}
|
||||
|
||||
/**
|
||||
* Findings of one rule for the version in a status a user can set (open and
|
||||
* won't fix), each with its workflow's report columns. `workflowIds` limits
|
||||
|
||||
+70
@@ -7,6 +7,8 @@ import type {
|
||||
} from '@n8n/api-types';
|
||||
import { mockLogger } from '@n8n/backend-test-utils';
|
||||
import type {
|
||||
User,
|
||||
UserRepository,
|
||||
WorkflowEntity,
|
||||
WorkflowRepository,
|
||||
WorkflowStatistics,
|
||||
@@ -22,11 +24,13 @@ import type { MigrationRegistry } from '../../breaking-changes.migration-registr
|
||||
import type { RuleRegistry } from '../../breaking-changes.rule-registry.service';
|
||||
import type { BreakingChangeService } from '../../breaking-changes.service';
|
||||
import type { MigrationFindingSync } from '../../database/entities/migration-finding-sync.entity';
|
||||
import type { MigrationWorkflowOwner } from '../../database/entities/migration-workflow-owner.entity';
|
||||
import type { MigrationFindingSyncRepository } from '../../database/repositories/migration-finding-sync.repository';
|
||||
import type {
|
||||
MigrationFindingRepository,
|
||||
TriageableMigrationFinding,
|
||||
} from '../../database/repositories/migration-finding.repository';
|
||||
import type { MigrationWorkflowOwnerRepository } from '../../database/repositories/migration-workflow-owner.repository';
|
||||
import type {
|
||||
BreakingChangeRuleMetadata,
|
||||
IBreakingChangeBatchWorkflowRule,
|
||||
@@ -133,6 +137,8 @@ describe('MigrationFindingQueryService', () => {
|
||||
let workflowStatisticsRepository: MockProxy<WorkflowStatisticsRepository>;
|
||||
let findingRepository: MockProxy<MigrationFindingRepository>;
|
||||
let syncRepository: MockProxy<MigrationFindingSyncRepository>;
|
||||
let ownerRepository: MockProxy<MigrationWorkflowOwnerRepository>;
|
||||
let userRepository: MockProxy<UserRepository>;
|
||||
let errorReporter: MockProxy<ErrorReporter>;
|
||||
let service: MigrationFindingQueryService;
|
||||
|
||||
@@ -144,6 +150,8 @@ describe('MigrationFindingQueryService', () => {
|
||||
workflowStatisticsRepository = mock<WorkflowStatisticsRepository>();
|
||||
findingRepository = mock<MigrationFindingRepository>();
|
||||
syncRepository = mock<MigrationFindingSyncRepository>();
|
||||
ownerRepository = mock<MigrationWorkflowOwnerRepository>();
|
||||
userRepository = mock<UserRepository>();
|
||||
errorReporter = mock<ErrorReporter>();
|
||||
|
||||
ruleRegistry.getRules.mockReturnValue(allRules);
|
||||
@@ -158,6 +166,8 @@ describe('MigrationFindingQueryService', () => {
|
||||
findingRepository.countDistinctOpenWorkflows.mockResolvedValue(0);
|
||||
findingRepository.listTriageableForRule.mockResolvedValue([]);
|
||||
syncRepository.getForVersion.mockResolvedValue(null);
|
||||
ownerRepository.findByWorkflowIds.mockResolvedValue([]);
|
||||
userRepository.findManyByIds.mockResolvedValue([]);
|
||||
|
||||
service = new MigrationFindingQueryService(
|
||||
ruleRegistry,
|
||||
@@ -167,6 +177,8 @@ describe('MigrationFindingQueryService', () => {
|
||||
workflowStatisticsRepository,
|
||||
findingRepository,
|
||||
syncRepository,
|
||||
ownerRepository,
|
||||
userRepository,
|
||||
mockLogger(),
|
||||
errorReporter,
|
||||
);
|
||||
@@ -400,6 +412,64 @@ describe('MigrationFindingQueryService', () => {
|
||||
expect(breakingChangeService.detect).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('adds the owner of each workflow that has one, with the user details and the source', async () => {
|
||||
findingRepository.listTriageableForRule.mockResolvedValue([
|
||||
triageableFinding(1, 'rule-a', {
|
||||
id: 'wf-1',
|
||||
name: 'First',
|
||||
activeVersionId: null,
|
||||
updatedAt: UPDATED_AT,
|
||||
}),
|
||||
triageableFinding(2, 'rule-a', {
|
||||
id: 'wf-2',
|
||||
name: 'Second',
|
||||
activeVersionId: null,
|
||||
updatedAt: UPDATED_AT,
|
||||
}),
|
||||
]);
|
||||
ownerRepository.findByWorkflowIds.mockResolvedValue([
|
||||
{ workflowId: 'wf-1', userId: 'user-1', source: 'suggested' } as MigrationWorkflowOwner,
|
||||
]);
|
||||
userRepository.findManyByIds.mockResolvedValue([
|
||||
{ id: 'user-1', firstName: 'Ada', lastName: 'Lovelace', email: '[email protected]' } as User,
|
||||
]);
|
||||
|
||||
const result = await service.getRuleFindings(TARGET_VERSION, 'rule-a', INSTANCE);
|
||||
|
||||
expect(ownerRepository.findByWorkflowIds).toHaveBeenCalledWith(
|
||||
['wf-1', 'wf-2'],
|
||||
expect.anything(),
|
||||
);
|
||||
expect(userRepository.findManyByIds).toHaveBeenCalledWith(['user-1']);
|
||||
expect(result.affectedWorkflows[0].owner).toEqual({
|
||||
id: 'user-1',
|
||||
firstName: 'Ada',
|
||||
lastName: 'Lovelace',
|
||||
email: '[email protected]',
|
||||
source: 'suggested',
|
||||
});
|
||||
expect(result.affectedWorkflows[1].owner).toBeUndefined();
|
||||
});
|
||||
|
||||
it('lists a workflow without an owner when its owner row has no user any more', async () => {
|
||||
findingRepository.listTriageableForRule.mockResolvedValue([
|
||||
triageableFinding(1, 'rule-a', {
|
||||
id: 'wf-1',
|
||||
name: 'First',
|
||||
activeVersionId: null,
|
||||
updatedAt: UPDATED_AT,
|
||||
}),
|
||||
]);
|
||||
ownerRepository.findByWorkflowIds.mockResolvedValue([
|
||||
{ workflowId: 'wf-1', userId: null, source: 'assigned' } as MigrationWorkflowOwner,
|
||||
]);
|
||||
|
||||
const result = await service.getRuleFindings(TARGET_VERSION, 'rule-a', INSTANCE);
|
||||
|
||||
expect(userRepository.findManyByIds).not.toHaveBeenCalled();
|
||||
expect(result.affectedWorkflows[0].owner).toBeUndefined();
|
||||
});
|
||||
|
||||
it('lists only the findings of the workflows in scope', async () => {
|
||||
const scope: ReportScope = { kind: 'workflows', workflowIds: ['wf-2'] };
|
||||
findingRepository.listTriageableForRule.mockResolvedValue([
|
||||
|
||||
@@ -5,10 +5,12 @@ import type {
|
||||
BreakingChangeRuleDetailWorkflow,
|
||||
BreakingChangeVersion,
|
||||
BreakingChangeWorkflowIssue,
|
||||
BreakingChangeWorkflowOwner,
|
||||
BreakingChangeWorkflowRuleResult,
|
||||
} from '@n8n/api-types';
|
||||
import { Logger } from '@n8n/backend-common';
|
||||
import {
|
||||
UserRepository,
|
||||
WorkflowRepository,
|
||||
WorkflowStatisticsRepository,
|
||||
type WorkflowEntity,
|
||||
@@ -23,6 +25,7 @@ import { RuleRegistry } from '../breaking-changes.rule-registry.service';
|
||||
import { BreakingChangeService } from '../breaking-changes.service';
|
||||
import { MigrationFindingSyncRepository } from '../database/repositories/migration-finding-sync.repository';
|
||||
import { MigrationFindingRepository } from '../database/repositories/migration-finding.repository';
|
||||
import { MigrationWorkflowOwnerRepository } from '../database/repositories/migration-workflow-owner.repository';
|
||||
import { groupNodesByType } from '../group-nodes-by-type';
|
||||
import { summarizeExecutionStatistics } from '../summarize-execution-statistics';
|
||||
import {
|
||||
@@ -67,6 +70,8 @@ export class MigrationFindingQueryService {
|
||||
private readonly workflowStatisticsRepository: WorkflowStatisticsRepository,
|
||||
private readonly findingRepository: MigrationFindingRepository,
|
||||
private readonly syncRepository: MigrationFindingSyncRepository,
|
||||
private readonly ownerRepository: MigrationWorkflowOwnerRepository,
|
||||
private readonly userRepository: UserRepository,
|
||||
private readonly logger: Logger,
|
||||
private readonly errorReporter: ErrorReporter,
|
||||
) {
|
||||
@@ -161,9 +166,10 @@ export class MigrationFindingQueryService {
|
||||
{},
|
||||
);
|
||||
const workflowIds = findings.map((finding) => finding.workflowId);
|
||||
const [workflows, statistics] = await Promise.all([
|
||||
const [workflows, statistics, ownersByWorkflow] = await Promise.all([
|
||||
this.workflowRepository.findByIds(workflowIds, { fields: WORKFLOW_FIELDS }),
|
||||
this.workflowStatisticsRepository.findByWorkflowIds(workflowIds),
|
||||
this.loadOwners(workflowIds),
|
||||
]);
|
||||
const statisticsByWorkflow = groupByWorkflowId(statistics);
|
||||
// A batch rule decides from all workflows at once, so its issues come from a scan of that rule.
|
||||
@@ -183,6 +189,7 @@ export class MigrationFindingQueryService {
|
||||
// A workflow the rule no longer flags stays listed, without issues, until the next sync.
|
||||
issues: issuesByWorkflow.get(finding.workflowId) ?? [],
|
||||
status: finding.status,
|
||||
owner: ownersByWorkflow.get(finding.workflowId),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -205,6 +212,30 @@ export class MigrationFindingQueryService {
|
||||
};
|
||||
}
|
||||
|
||||
/** The owner per workflow id. A deleted user leaves the owner row without a user, so no owner. */
|
||||
private async loadOwners(
|
||||
workflowIds: string[],
|
||||
): Promise<Map<string, BreakingChangeWorkflowOwner>> {
|
||||
const rows = await this.ownerRepository.findByWorkflowIds(workflowIds, {});
|
||||
const userIds = [...new Set(rows.flatMap((row) => (row.userId ? [row.userId] : [])))];
|
||||
const users = userIds.length > 0 ? await this.userRepository.findManyByIds(userIds) : [];
|
||||
const usersById = new Map(users.map((user) => [user.id, user]));
|
||||
|
||||
const owners = new Map<string, BreakingChangeWorkflowOwner>();
|
||||
for (const row of rows) {
|
||||
const user = row.userId ? usersById.get(row.userId) : undefined;
|
||||
if (!user) continue;
|
||||
owners.set(row.workflowId, {
|
||||
id: user.id,
|
||||
firstName: user.firstName,
|
||||
lastName: user.lastName,
|
||||
email: user.email,
|
||||
source: row.source,
|
||||
});
|
||||
}
|
||||
return owners;
|
||||
}
|
||||
|
||||
/** Issues per workflow id for one batch rule, from a scan of that rule alone. */
|
||||
private async issuesFromScan(
|
||||
targetVersion: BreakingChangeVersion,
|
||||
|
||||
+100
@@ -18,6 +18,8 @@ import type { MigrationFinding } from '../../database/entities/migration-finding
|
||||
import type { MigrationFindingSync } from '../../database/entities/migration-finding-sync.entity';
|
||||
import type { MigrationFindingSyncRepository } from '../../database/repositories/migration-finding-sync.repository';
|
||||
import type { MigrationFindingRepository } from '../../database/repositories/migration-finding.repository';
|
||||
import type { MigrationWorkflowOwnerRepository } from '../../database/repositories/migration-workflow-owner.repository';
|
||||
import type { MigrationOwnerSuggestionService } from '../../owners/migration-owner-suggestion.service';
|
||||
import type { IBreakingChangeRule } from '../../types';
|
||||
import type { MigrationFindingHit } from '../migration-finding-diff';
|
||||
import {
|
||||
@@ -86,6 +88,8 @@ describe('MigrationFindingSyncService', () => {
|
||||
let workflowRepository: MockProxy<WorkflowRepository>;
|
||||
let findingRepository: MockProxy<MigrationFindingRepository>;
|
||||
let syncRepository: MockProxy<MigrationFindingSyncRepository>;
|
||||
let ownerSuggestionService: MockProxy<MigrationOwnerSuggestionService>;
|
||||
let ownerRepository: MockProxy<MigrationWorkflowOwnerRepository>;
|
||||
let txRunner: MockProxy<TransactionRunner>;
|
||||
let errorReporter: MockProxy<ErrorReporter>;
|
||||
let service: MigrationFindingSyncService;
|
||||
@@ -108,6 +112,8 @@ describe('MigrationFindingSyncService', () => {
|
||||
workflowRepository = mock<WorkflowRepository>();
|
||||
findingRepository = mock<MigrationFindingRepository>();
|
||||
syncRepository = mock<MigrationFindingSyncRepository>();
|
||||
ownerSuggestionService = mock<MigrationOwnerSuggestionService>();
|
||||
ownerRepository = mock<MigrationWorkflowOwnerRepository>();
|
||||
txRunner = mock<TransactionRunner>();
|
||||
errorReporter = mock<ErrorReporter>();
|
||||
|
||||
@@ -116,6 +122,8 @@ describe('MigrationFindingSyncService', () => {
|
||||
workflowRepository.findExistingIds.mockImplementation(async (ids) => ids);
|
||||
ruleRegistry.getRules.mockReturnValue(rules('rule-a', 'rule-b'));
|
||||
findingRepository.listForWorkflows.mockResolvedValue([]);
|
||||
findingRepository.listWorkflowIdsWithOpenFindings.mockResolvedValue([]);
|
||||
ownerSuggestionService.suggestOwners.mockResolvedValue([]);
|
||||
breakingChangeService.detect.mockResolvedValue(detectionResult([]));
|
||||
|
||||
service = new MigrationFindingSyncService(
|
||||
@@ -124,6 +132,8 @@ describe('MigrationFindingSyncService', () => {
|
||||
workflowRepository,
|
||||
findingRepository,
|
||||
syncRepository,
|
||||
ownerSuggestionService,
|
||||
ownerRepository,
|
||||
txRunner,
|
||||
mockLogger(),
|
||||
errorReporter,
|
||||
@@ -459,6 +469,76 @@ describe('MigrationFindingSyncService', () => {
|
||||
expect(breakingChangeService.detect).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
describe('suggested owners', () => {
|
||||
it('stores a suggestion for each workflow in the batch with an open finding and clears the rest', async () => {
|
||||
givenWorkflows(3);
|
||||
findingRepository.listWorkflowIdsWithOpenFindings.mockResolvedValue(['wf-0000', 'wf-0002']);
|
||||
ownerSuggestionService.suggestOwners.mockResolvedValue([
|
||||
{ workflowId: 'wf-0000', userId: 'user-1' },
|
||||
]);
|
||||
|
||||
await service.sync(TARGET_VERSION);
|
||||
|
||||
expect(findingRepository.listWorkflowIdsWithOpenFindings).toHaveBeenCalledWith(
|
||||
TARGET_VERSION,
|
||||
['wf-0000', 'wf-0001', 'wf-0002'],
|
||||
expect.anything(),
|
||||
);
|
||||
expect(ownerSuggestionService.suggestOwners).toHaveBeenCalledWith(['wf-0000', 'wf-0002']);
|
||||
expect(ownerRepository.replaceSuggestions).toHaveBeenCalledWith(
|
||||
['wf-0000', 'wf-0001', 'wf-0002'],
|
||||
[{ workflowId: 'wf-0000', userId: 'user-1' }],
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it('skips the heuristic when no workflow in the batch has an open finding, and still clears old suggestions', async () => {
|
||||
givenWorkflows(2);
|
||||
|
||||
await service.sync(TARGET_VERSION);
|
||||
|
||||
expect(ownerSuggestionService.suggestOwners).not.toHaveBeenCalled();
|
||||
expect(ownerRepository.replaceSuggestions).toHaveBeenCalledWith(
|
||||
['wf-0000', 'wf-0001'],
|
||||
[],
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it('refreshes owners once per batch, after the findings of that batch were written', async () => {
|
||||
givenWorkflows(150);
|
||||
|
||||
await service.sync(TARGET_VERSION);
|
||||
|
||||
expect(ownerRepository.replaceSuggestions).toHaveBeenCalledTimes(2);
|
||||
expect(ownerRepository.replaceSuggestions.mock.calls[0][0]).toHaveLength(100);
|
||||
expect(ownerRepository.replaceSuggestions.mock.calls[1][0]).toHaveLength(50);
|
||||
expect(txRunner.run.mock.invocationCallOrder[0]).toBeLessThan(
|
||||
ownerRepository.replaceSuggestions.mock.invocationCallOrder[0],
|
||||
);
|
||||
});
|
||||
|
||||
it('reports a failed owner refresh and still records the sync', async () => {
|
||||
givenWorkflows(1);
|
||||
const failure = new Error('owner write failed');
|
||||
ownerRepository.replaceSuggestions.mockRejectedValue(failure);
|
||||
|
||||
await service.sync(TARGET_VERSION);
|
||||
|
||||
expect(errorReporter.error).toHaveBeenCalledWith(failure, expect.anything());
|
||||
expect(syncRepository.upsertForVersion).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('does not refresh owners for a batch whose finding write failed', async () => {
|
||||
givenWorkflows(1);
|
||||
txRunner.run.mockRejectedValueOnce(new Error('batch failed'));
|
||||
|
||||
await service.sync(TARGET_VERSION);
|
||||
|
||||
expect(ownerRepository.replaceSuggestions).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('syncWorkflow()', () => {
|
||||
const WORKFLOW_ID = 'wf-0001';
|
||||
const REPORT_VERSION: BreakingChangeVersion = 'v3';
|
||||
@@ -609,6 +689,26 @@ describe('MigrationFindingSyncService', () => {
|
||||
expect(txRunner.run).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
it('refreshes the suggested owner of that workflow', async () => {
|
||||
findingRepository.listWorkflowIdsWithOpenFindings.mockResolvedValue([WORKFLOW_ID]);
|
||||
ownerSuggestionService.suggestOwners.mockResolvedValue([
|
||||
{ workflowId: WORKFLOW_ID, userId: 'user-1' },
|
||||
]);
|
||||
|
||||
await service.syncWorkflow(WORKFLOW_ID);
|
||||
|
||||
expect(findingRepository.listWorkflowIdsWithOpenFindings).toHaveBeenCalledWith(
|
||||
REPORT_VERSION,
|
||||
[WORKFLOW_ID],
|
||||
expect.anything(),
|
||||
);
|
||||
expect(ownerRepository.replaceSuggestions).toHaveBeenCalledWith(
|
||||
[WORKFLOW_ID],
|
||||
[{ workflowId: WORKFLOW_ID, userId: 'user-1' }],
|
||||
expect.anything(),
|
||||
);
|
||||
});
|
||||
|
||||
it('does nothing when there is no report target version', async () => {
|
||||
reportTarget.version = null;
|
||||
|
||||
|
||||
@@ -13,6 +13,8 @@ import { RuleRegistry } from '../breaking-changes.rule-registry.service';
|
||||
import { BreakingChangeService } from '../breaking-changes.service';
|
||||
import { MigrationFindingSyncRepository } from '../database/repositories/migration-finding-sync.repository';
|
||||
import { MigrationFindingRepository } from '../database/repositories/migration-finding.repository';
|
||||
import { MigrationWorkflowOwnerRepository } from '../database/repositories/migration-workflow-owner.repository';
|
||||
import { MigrationOwnerSuggestionService } from '../owners/migration-owner-suggestion.service';
|
||||
import { diffMigrationFindings, type MigrationFindingHit } from './migration-finding-diff';
|
||||
|
||||
/** Stable hash of a rule set. The order of `ruleIds` does not change the result. */
|
||||
@@ -43,6 +45,8 @@ export class MigrationFindingSyncService {
|
||||
private readonly workflowRepository: WorkflowRepository,
|
||||
private readonly findingRepository: MigrationFindingRepository,
|
||||
private readonly syncRepository: MigrationFindingSyncRepository,
|
||||
private readonly ownerSuggestionService: MigrationOwnerSuggestionService,
|
||||
private readonly ownerRepository: MigrationWorkflowOwnerRepository,
|
||||
private readonly txRunner: TransactionRunner,
|
||||
private readonly logger: Logger,
|
||||
private readonly errorReporter: ErrorReporter,
|
||||
@@ -122,6 +126,7 @@ export class MigrationFindingSyncService {
|
||||
workflowIds = await this.workflowRepository.getIdsAfter(afterId, take);
|
||||
try {
|
||||
await this.syncBatch(targetVersion, workflowIds, hitsByWorkflow, unknownByWorkflow);
|
||||
await this.refreshSuggestedOwners(targetVersion, workflowIds);
|
||||
} catch (error) {
|
||||
// One bad batch must not lose the rest. The sync record stays cleared
|
||||
// below, so the next read syncs and visits this batch again.
|
||||
@@ -201,6 +206,7 @@ export class MigrationFindingSyncService {
|
||||
groupByWorkflow(hits),
|
||||
groupByWorkflow([...failedChecks, ...batchRulePairs]),
|
||||
);
|
||||
await this.refreshSuggestedOwners(targetVersion, [workflowId]);
|
||||
} catch (error) {
|
||||
this.logger.warn('Migration finding sync for one workflow failed', {
|
||||
targetVersion,
|
||||
@@ -210,6 +216,33 @@ export class MigrationFindingSyncService {
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Stores the heuristic's owner for each workflow in the batch that has an open
|
||||
* finding, and drops the suggestion of the others. An owner a person assigned
|
||||
* is kept. Owners are a hint, so a failure here does not fail the finding sync.
|
||||
*/
|
||||
private async refreshSuggestedOwners(
|
||||
targetVersion: BreakingChangeVersion,
|
||||
workflowIds: string[],
|
||||
): Promise<void> {
|
||||
try {
|
||||
const affectedIds = await this.findingRepository.listWorkflowIdsWithOpenFindings(
|
||||
targetVersion,
|
||||
workflowIds,
|
||||
{},
|
||||
);
|
||||
const suggestions =
|
||||
affectedIds.length > 0 ? await this.ownerSuggestionService.suggestOwners(affectedIds) : [];
|
||||
await this.ownerRepository.replaceSuggestions(workflowIds, suggestions, {});
|
||||
} catch (error) {
|
||||
this.logger.warn('Refreshing the suggested owners failed, the findings are unaffected', {
|
||||
targetVersion,
|
||||
batchStart: workflowIds[0],
|
||||
});
|
||||
this.errorReporter.error(error, { extra: { targetVersion, batchStart: workflowIds[0] } });
|
||||
}
|
||||
}
|
||||
|
||||
/** Reads, diffs, and writes one batch inside a single transaction. */
|
||||
private async syncBatch(
|
||||
targetVersion: BreakingChangeVersion,
|
||||
|
||||
@@ -4626,6 +4626,8 @@
|
||||
"settings.migrationReport.findingCount.wontFix": "{count} won't fix",
|
||||
"settings.migrationReport.detail.affectedTag": "{count} affected",
|
||||
"settings.migrationReport.detail.table.name": "Name",
|
||||
"settings.migrationReport.detail.table.owner": "Owner",
|
||||
"settings.migrationReport.detail.table.unassigned": "Unassigned",
|
||||
"settings.migrationReport.detail.table.status": "Status",
|
||||
"settings.migrationReport.detail.table.active": "Active",
|
||||
"settings.migrationReport.detail.table.deactivated": "Deactivated",
|
||||
|
||||
+77
@@ -48,6 +48,13 @@ const mockWorkflowWithIssue = {
|
||||
lastUpdatedAt: new Date('2024-01-15'),
|
||||
lastExecutedAt: new Date('2024-01-14'),
|
||||
status: 'open' as const,
|
||||
owner: {
|
||||
id: 'user-1',
|
||||
firstName: 'Ada',
|
||||
lastName: 'Lovelace',
|
||||
email: '[email protected]',
|
||||
source: 'suggested' as const,
|
||||
},
|
||||
issues: [
|
||||
{
|
||||
nodeId: 'node-1',
|
||||
@@ -186,6 +193,7 @@ describe('MigrationRuleDetail', () => {
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('Name')).toBeInTheDocument();
|
||||
expect(screen.getByText('Owner')).toBeInTheDocument();
|
||||
expect(screen.getByText('Status', { selector: 'th' })).toBeInTheDocument();
|
||||
expect(screen.getByText('Nodes affected')).toBeInTheDocument();
|
||||
expect(screen.getByText(/Number of executions/)).toBeInTheDocument();
|
||||
@@ -448,6 +456,42 @@ describe('MigrationRuleDetail', () => {
|
||||
});
|
||||
});
|
||||
|
||||
it('should show the owner name, or Unassigned when the workflow has none', async () => {
|
||||
renderComponent({
|
||||
props: {
|
||||
migrationRuleId: 'rule-1',
|
||||
},
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('Ada Lovelace')).toBeInTheDocument();
|
||||
expect(screen.getByText('Unassigned')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
it('should fall back to the email when the owner has no name', async () => {
|
||||
vi.mocked(breakingChangesApi.getReportForRule).mockResolvedValue(
|
||||
createMockRuleResult({
|
||||
affectedWorkflows: [
|
||||
{
|
||||
...mockWorkflowWithIssue,
|
||||
owner: { ...mockWorkflowWithIssue.owner, firstName: null, lastName: null },
|
||||
},
|
||||
],
|
||||
}),
|
||||
);
|
||||
|
||||
renderComponent({
|
||||
props: {
|
||||
migrationRuleId: 'rule-1',
|
||||
},
|
||||
});
|
||||
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('[email protected]')).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
it('should display workflow execution counts', async () => {
|
||||
renderComponent({
|
||||
props: {
|
||||
@@ -560,6 +604,39 @@ describe('MigrationRuleDetail', () => {
|
||||
expect(firstDataRow.textContent).toContain('100');
|
||||
});
|
||||
});
|
||||
|
||||
it('should sort by the shown owner label in both directions', async () => {
|
||||
vi.mocked(breakingChangesApi.getReportForRule).mockResolvedValue(
|
||||
createMockRuleResult({
|
||||
affectedWorkflows: [
|
||||
{
|
||||
...mockWorkflowWithIssue,
|
||||
owner: { ...mockWorkflowWithIssue.owner, firstName: 'Zed', lastName: 'Zulu' },
|
||||
},
|
||||
{ ...mockWorkflowWithMultipleNodes, owner: mockWorkflowWithIssue.owner },
|
||||
],
|
||||
}),
|
||||
);
|
||||
renderComponent({
|
||||
props: {
|
||||
migrationRuleId: 'rule-1',
|
||||
},
|
||||
});
|
||||
// Default order is by executions, so the Zed Zulu workflow comes first.
|
||||
await waitFor(() => {
|
||||
expect(screen.getAllByRole('row')[1].textContent).toContain('Test Workflow 1');
|
||||
});
|
||||
|
||||
await userEvent.click(screen.getByRole('columnheader', { name: /Owner/ }));
|
||||
await waitFor(() => {
|
||||
expect(screen.getAllByRole('row')[1].textContent).toContain('Ada Lovelace');
|
||||
});
|
||||
|
||||
await userEvent.click(screen.getByRole('columnheader', { name: /Owner/ }));
|
||||
await waitFor(() => {
|
||||
expect(screen.getAllByRole('row')[1].textContent).toContain('Zed Zulu');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('error handling', () => {
|
||||
|
||||
+18
-2
@@ -76,6 +76,14 @@ const { state, isLoading } = useAsyncState<BreakingChangeRuleDetailResult>(
|
||||
|
||||
type AffectedWorkflow = BreakingChangeRuleDetailWorkflow;
|
||||
|
||||
function ownerLabel(workflow: AffectedWorkflow): string {
|
||||
const owner = workflow.owner;
|
||||
const fullName = [owner?.firstName, owner?.lastName].filter(Boolean).join(' ');
|
||||
return (
|
||||
fullName || owner?.email || i18n.baseText('settings.migrationReport.detail.table.unassigned')
|
||||
);
|
||||
}
|
||||
|
||||
const tableHeaders = computed<Array<TableHeader<AffectedWorkflow>>>(() => {
|
||||
const headers: Array<TableHeader<AffectedWorkflow>> = [
|
||||
{
|
||||
@@ -83,6 +91,12 @@ const tableHeaders = computed<Array<TableHeader<AffectedWorkflow>>>(() => {
|
||||
key: 'name',
|
||||
width: 240,
|
||||
},
|
||||
{
|
||||
title: i18n.baseText('settings.migrationReport.detail.table.owner'),
|
||||
key: 'owner',
|
||||
value: ownerLabel,
|
||||
width: 160,
|
||||
},
|
||||
{
|
||||
title: i18n.baseText('settings.migrationReport.detail.table.status'),
|
||||
key: 'active',
|
||||
@@ -291,10 +305,12 @@ const filteredWorkflows = computed(() => {
|
||||
const sortedWorkflows = computed(() => {
|
||||
if (!sortBy.value.length) return filteredWorkflows.value;
|
||||
|
||||
const { id, desc } = sortBy.value[0];
|
||||
// The owner cell shows a label, so it sorts by that label and not by the owner object.
|
||||
return orderBy(
|
||||
filteredWorkflows.value,
|
||||
[sortBy.value[0].id],
|
||||
[sortBy.value[0].desc ? 'desc' : 'asc'],
|
||||
[id === 'owner' ? ownerLabel : id],
|
||||
[desc ? 'desc' : 'asc'],
|
||||
);
|
||||
});
|
||||
</script>
|
||||
|
||||
Reference in New Issue
Block a user