feat(API): Add user id information on push tracking when available (#14519)

This commit is contained in:
Guillaume Jacquart
2025-04-10 17:04:48 +02:00
committed by GitHub
parent faecb47f15
commit 61957899e1
10 changed files with 171 additions and 21 deletions

View File

@@ -217,8 +217,10 @@ describe('Source Control', () => {
});
it('should get tracking information from pre-push results', () => {
const trackingResult = getTrackingInformationFromPrePushResult(pushResult);
const userId = 'userId';
const trackingResult = getTrackingInformationFromPrePushResult(userId, pushResult);
expect(trackingResult).toEqual({
userId,
workflowsEligible: 3,
workflowsEligibleWithConflicts: 1,
credsEligible: 1,
@@ -228,8 +230,10 @@ describe('Source Control', () => {
});
it('should get tracking information from post-push results', () => {
const trackingResult = getTrackingInformationFromPostPushResult(pushResult);
const userId = 'userId';
const trackingResult = getTrackingInformationFromPostPushResult(userId, pushResult);
expect(trackingResult).toEqual({
userId,
workflowsPushed: 2,
workflowsEligible: 3,
credsPushed: 1,
@@ -238,8 +242,10 @@ describe('Source Control', () => {
});
it('should get tracking information from pull results', () => {
const trackingResult = getTrackingInformationFromPullResult(pullResult);
const userId = 'userId';
const trackingResult = getTrackingInformationFromPullResult(userId, pullResult);
expect(trackingResult).toEqual({
userId,
credConflicts: 1,
workflowConflicts: 1,
workflowUpdates: 3,

View File

@@ -0,0 +1,103 @@
import type { PullWorkFolderRequestDto, PushWorkFolderRequestDto } from '@n8n/api-types';
import type { Response } from 'express';
import { mock } from 'jest-mock-extended';
import type { EventService } from '@/events/event.service';
import type { AuthenticatedRequest } from '@/requests';
import type { SourceControlPreferencesService } from '../source-control-preferences.service.ee';
import { SourceControlController } from '../source-control.controller.ee';
import type { SourceControlService } from '../source-control.service.ee';
import type { SourceControlRequest } from '../types/requests';
import type { SourceControlGetStatus } from '../types/source-control-get-status';
describe('SourceControlController', () => {
let controller: SourceControlController;
let sourceControlService: SourceControlService;
let sourceControlPreferencesService: SourceControlPreferencesService;
let eventService: EventService;
beforeEach(() => {
sourceControlService = {
pushWorkfolder: jest.fn().mockResolvedValue({ statusCode: 200 }),
pullWorkfolder: jest.fn().mockResolvedValue({ statusCode: 200 }),
getStatus: jest.fn().mockResolvedValue([]),
setGitUserDetails: jest.fn(),
} as unknown as SourceControlService;
sourceControlPreferencesService = mock<SourceControlPreferencesService>();
eventService = mock<EventService>();
controller = new SourceControlController(
sourceControlService,
sourceControlPreferencesService,
eventService,
);
});
describe('pushWorkfolder', () => {
it('should push workfolder with expected parameters', async () => {
const req = mock<AuthenticatedRequest>({
user: { firstName: 'John', lastName: 'Doe', email: 'john.doe@example.com' },
});
const res = mock<Response>();
const payload = { force: true } as PushWorkFolderRequestDto;
await controller.pushWorkfolder(req, res, payload);
expect(sourceControlService.setGitUserDetails).toHaveBeenCalledWith(
'John Doe',
'john.doe@example.com',
);
expect(sourceControlService.pushWorkfolder).toHaveBeenCalledWith(req.user, payload);
});
});
describe('pullWorkfolder', () => {
it('should pull workfolder with expected parameters', async () => {
const req = mock<AuthenticatedRequest>({
user: { firstName: 'John', lastName: 'Doe', email: 'john.doe@example.com' },
});
const res = mock<Response>();
const payload = { force: true } as PullWorkFolderRequestDto;
await controller.pullWorkfolder(req, res, payload);
expect(sourceControlService.pullWorkfolder).toHaveBeenCalledWith(req.user, payload);
});
});
describe('getStatus', () => {
it('should call getStatus with expected parameters', async () => {
const user = { firstName: 'John', lastName: 'Doe', email: 'john.doe@example.com' };
const query = {
direction: 'pull',
preferLocalVersion: true,
verbose: false,
} as SourceControlGetStatus;
const req = mock<SourceControlRequest.GetStatus>({
query,
user,
});
await controller.getStatus(req);
expect(sourceControlService.getStatus).toHaveBeenCalledWith(user, query);
});
});
describe('status', () => {
it('should call getStatus with expected parameters', async () => {
const user = { firstName: 'John', lastName: 'Doe', email: 'john.doe@example.com' };
const query = {
direction: 'pull',
preferLocalVersion: true,
verbose: false,
} as SourceControlGetStatus;
const req = mock<SourceControlRequest.GetStatus>({
query,
user,
});
await controller.status(req);
expect(sourceControlService.getStatus).toHaveBeenCalledWith(user, query);
});
});
});

View File

@@ -45,8 +45,9 @@ describe('SourceControlService', () => {
describe('pushWorkfolder', () => {
it('should throw an error if a file is given that is not in the workfolder', async () => {
const user = mock<User>();
await expect(
sourceControlService.pushWorkfolder({
sourceControlService.pushWorkfolder(user, {
fileNames: [
{
file: '/etc/passwd',
@@ -115,8 +116,9 @@ describe('SourceControlService', () => {
});
it('should throw an error if a file is given that is not in the workfolder', async () => {
const user = mock<User>();
await expect(
sourceControlService.pushWorkfolder({
sourceControlService.pushWorkfolder(user, {
fileNames: [
{
file: '/etc/passwd',
@@ -138,6 +140,7 @@ describe('SourceControlService', () => {
describe('getStatus', () => {
it('conflict depends on the value of `direction`', async () => {
// ARRANGE
const user = mock<User>();
// Define a credential that does only exist locally.
// Pulling this would delete it so it should be marked as a conflict.
@@ -200,13 +203,13 @@ describe('SourceControlService', () => {
});
// ACT
const pullResult = await sourceControlService.getStatus({
const pullResult = await sourceControlService.getStatus(user, {
direction: 'pull',
verbose: false,
preferLocalVersion: false,
});
const pushResult = await sourceControlService.getStatus({
const pushResult = await sourceControlService.getStatus(user, {
direction: 'push',
verbose: false,
preferLocalVersion: false,

View File

@@ -131,9 +131,13 @@ function filterSourceControlledFilesUniqueIds(files: SourceControlledFile[]) {
);
}
export function getTrackingInformationFromPullResult(result: SourceControlledFile[]) {
export function getTrackingInformationFromPullResult(
userId: string,
result: SourceControlledFile[],
) {
const uniques = filterSourceControlledFilesUniqueIds(result);
return {
userId,
credConflicts: uniques.filter(
(file) =>
file.type === 'credential' && file.status === 'modified' && file.location === 'local',
@@ -145,9 +149,13 @@ export function getTrackingInformationFromPullResult(result: SourceControlledFil
};
}
export function getTrackingInformationFromPrePushResult(result: SourceControlledFile[]) {
export function getTrackingInformationFromPrePushResult(
userId: string,
result: SourceControlledFile[],
) {
const uniques = filterSourceControlledFilesUniqueIds(result);
return {
userId,
workflowsEligible: uniques.filter((file) => file.type === 'workflow').length,
workflowsEligibleWithConflicts: uniques.filter(
(file) => file.type === 'workflow' && file.conflict,
@@ -160,9 +168,13 @@ export function getTrackingInformationFromPrePushResult(result: SourceControlled
};
}
export function getTrackingInformationFromPostPushResult(result: SourceControlledFile[]) {
export function getTrackingInformationFromPostPushResult(
userId: string,
result: SourceControlledFile[],
) {
const uniques = filterSourceControlledFilesUniqueIds(result);
return {
userId,
workflowsPushed: uniques.filter((file) => file.pushed && file.type === 'workflow').length ?? 0,
workflowsEligible: uniques.filter((file) => file.type === 'workflow').length ?? 0,
credsPushed:

View File

@@ -175,7 +175,8 @@ export class SourceControlController {
`${req.user.firstName} ${req.user.lastName}`,
req.user.email,
);
const result = await this.sourceControlService.pushWorkfolder(payload);
const result = await this.sourceControlService.pushWorkfolder(req.user, payload);
res.statusCode = result.statusCode;
return result.statusResult;
} catch (error) {
@@ -213,6 +214,7 @@ export class SourceControlController {
async getStatus(req: SourceControlRequest.GetStatus) {
try {
const result = (await this.sourceControlService.getStatus(
req.user,
new SourceControlGetStatus(req.query),
)) as SourceControlledFile[];
return result;
@@ -224,7 +226,10 @@ export class SourceControlController {
@Get('/status', { middlewares: [sourceControlLicensedMiddleware] })
async status(req: SourceControlRequest.GetStatus) {
try {
return await this.sourceControlService.getStatus(new SourceControlGetStatus(req.query));
return await this.sourceControlService.getStatus(
req.user,
new SourceControlGetStatus(req.query),
);
} catch (error) {
throw new BadRequestError((error as { message: string }).message);
}

View File

@@ -213,7 +213,10 @@ export class SourceControlService {
return;
}
async pushWorkfolder(options: PushWorkFolderRequestDto): Promise<{
async pushWorkfolder(
user: User,
options: PushWorkFolderRequestDto,
): Promise<{
statusCode: number;
pushResult: PushResult | undefined;
statusResult: SourceControlledFile[];
@@ -239,7 +242,7 @@ export class SourceControlService {
// only determine file status if not provided by the frontend
let statusResult: SourceControlledFile[] = filesToPush;
if (statusResult.length === 0) {
statusResult = (await this.getStatus({
statusResult = (await this.getStatus(user, {
direction: 'push',
verbose: false,
preferLocalVersion: true,
@@ -332,7 +335,7 @@ export class SourceControlService {
// #region Tracking Information
this.eventService.emit(
'source-control-user-finished-push-ui',
getTrackingInformationFromPostPushResult(statusResult),
getTrackingInformationFromPostPushResult(user.id, statusResult),
);
// #endregion
@@ -393,7 +396,7 @@ export class SourceControlService {
): Promise<{ statusCode: number; statusResult: SourceControlledFile[] }> {
await this.sanityCheck();
const statusResult = (await this.getStatus({
const statusResult = (await this.getStatus(user, {
direction: 'pull',
verbose: false,
preferLocalVersion: false,
@@ -457,7 +460,7 @@ export class SourceControlService {
// #region Tracking Information
this.eventService.emit(
'source-control-user-finished-pull-ui',
getTrackingInformationFromPullResult(statusResult),
getTrackingInformationFromPullResult(user.id, statusResult),
);
// #endregion
@@ -477,7 +480,7 @@ export class SourceControlService {
* @returns either SourceControlledFile[] if verbose is false,
* or multiple SourceControlledFile[] with all determined differences for debugging purposes
*/
async getStatus(options: SourceControlGetStatus) {
async getStatus(user: User, options: SourceControlGetStatus) {
await this.sanityCheck();
const sourceControlledFiles: SourceControlledFile[] = [];
@@ -514,12 +517,12 @@ export class SourceControlService {
if (options.direction === 'push') {
this.eventService.emit(
'source-control-user-started-push-ui',
getTrackingInformationFromPrePushResult(sourceControlledFiles),
getTrackingInformationFromPrePushResult(user.id, sourceControlledFiles),
);
} else if (options.direction === 'pull') {
this.eventService.emit(
'source-control-user-started-pull-ui',
getTrackingInformationFromPullResult(sourceControlledFiles),
getTrackingInformationFromPullResult(user.id, sourceControlledFiles),
);
}
// #endregion

View File

@@ -225,12 +225,14 @@ describe('TelemetryEventRelay', () => {
it('should track on `source-control-user-finished-pull-ui` event', () => {
const event: RelayEventMap['source-control-user-finished-pull-ui'] = {
userId: 'userId',
workflowUpdates: 3,
};
eventService.emit('source-control-user-finished-pull-ui', event);
expect(telemetry.track).toHaveBeenCalledWith('User finished pull via UI', {
user_id: 'userId',
workflow_updates: 3,
});
});
@@ -251,6 +253,7 @@ describe('TelemetryEventRelay', () => {
it('should track on `source-control-user-started-push-ui` event', () => {
const event: RelayEventMap['source-control-user-started-push-ui'] = {
userId: 'userId',
workflowsEligible: 10,
workflowsEligibleWithConflicts: 2,
credsEligible: 5,
@@ -261,6 +264,7 @@ describe('TelemetryEventRelay', () => {
eventService.emit('source-control-user-started-push-ui', event);
expect(telemetry.track).toHaveBeenCalledWith('User started push via UI', {
user_id: 'userId',
workflows_eligible: 10,
workflows_eligible_with_conflicts: 2,
creds_eligible: 5,
@@ -271,6 +275,7 @@ describe('TelemetryEventRelay', () => {
it('should track on `source-control-user-finished-push-ui` event', () => {
const event: RelayEventMap['source-control-user-finished-push-ui'] = {
userId: 'userId',
workflowsEligible: 10,
workflowsPushed: 8,
credsPushed: 5,
@@ -280,6 +285,7 @@ describe('TelemetryEventRelay', () => {
eventService.emit('source-control-user-finished-push-ui', event);
expect(telemetry.track).toHaveBeenCalledWith('User finished push via UI', {
user_id: 'userId',
workflows_eligible: 10,
workflows_pushed: 8,
creds_pushed: 5,

View File

@@ -381,12 +381,14 @@ export type RelayEventMap = {
};
'source-control-user-started-pull-ui': {
userId?: string;
workflowUpdates: number;
workflowConflicts: number;
credConflicts: number;
};
'source-control-user-finished-pull-ui': {
userId?: string;
workflowUpdates: number;
};
@@ -396,6 +398,7 @@ export type RelayEventMap = {
};
'source-control-user-started-push-ui': {
userId?: string;
workflowsEligible: number;
workflowsEligibleWithConflicts: number;
credsEligible: number;
@@ -404,6 +407,7 @@ export type RelayEventMap = {
};
'source-control-user-finished-push-ui': {
userId: string;
workflowsEligible: number;
workflowsPushed: number;
credsPushed: number;

View File

@@ -170,11 +170,13 @@ export class TelemetryEventRelay extends EventRelay {
}
private sourceControlUserStartedPullUi({
userId,
workflowUpdates,
workflowConflicts,
credConflicts,
}: RelayEventMap['source-control-user-started-pull-ui']) {
this.telemetry.track('User started pull via UI', {
user_id: userId,
workflow_updates: workflowUpdates,
workflow_conflicts: workflowConflicts,
cred_conflicts: credConflicts,
@@ -182,9 +184,11 @@ export class TelemetryEventRelay extends EventRelay {
}
private sourceControlUserFinishedPullUi({
userId,
workflowUpdates,
}: RelayEventMap['source-control-user-finished-pull-ui']) {
this.telemetry.track('User finished pull via UI', {
user_id: userId,
workflow_updates: workflowUpdates,
});
}
@@ -200,6 +204,7 @@ export class TelemetryEventRelay extends EventRelay {
}
private sourceControlUserStartedPushUi({
userId,
workflowsEligible,
workflowsEligibleWithConflicts,
credsEligible,
@@ -207,6 +212,7 @@ export class TelemetryEventRelay extends EventRelay {
variablesEligible,
}: RelayEventMap['source-control-user-started-push-ui']) {
this.telemetry.track('User started push via UI', {
user_id: userId,
workflows_eligible: workflowsEligible,
workflows_eligible_with_conflicts: workflowsEligibleWithConflicts,
creds_eligible: credsEligible,
@@ -216,12 +222,14 @@ export class TelemetryEventRelay extends EventRelay {
}
private sourceControlUserFinishedPushUi({
userId,
workflowsEligible,
workflowsPushed,
credsPushed,
variablesPushed,
}: RelayEventMap['source-control-user-finished-push-ui']) {
this.telemetry.track('User finished push via UI', {
user_id: userId,
workflows_eligible: workflowsEligible,
workflows_pushed: workflowsPushed,
creds_pushed: credsPushed,

View File

@@ -40,7 +40,7 @@ export = {
if (result.statusCode === 200) {
Container.get(EventService).emit('source-control-user-pulled-api', {
...getTrackingInformationFromPullResult(result.statusResult),
...getTrackingInformationFromPullResult(req.user.id, result.statusResult),
forced: payload.force ?? false,
});
return res.status(200).send(result.statusResult);