Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core): Decouple user events from internal hooks (no-changelog) #10292

Merged
merged 3 commits into from
Aug 5, 2024
Merged

refactor(core): Decouple user events from internal hooks (no-changelog) #10292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ec7903c
refactor(core): Decouple user events from internal hooks (no-changelog)
ivov Aug 5, 2024
6092861
Fix typecheck
ivov Aug 5, 2024
506654d
Merge master, fix conflicts
ivov Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions packages/cli/src/Interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,14 +267,6 @@ export interface IWebhookManager {
executeWebhook(req: WebhookRequest, res: Response): Promise<IResponseCallbackData>;
}

export interface ITelemetryUserDeletionData {
user_id: string;
target_user_old_status: 'active' | 'invited';
migration_strategy?: 'transfer_data' | 'delete_data';
target_user_id?: string;
migration_user_id?: string;
}

export interface IVersionNotificationSettings {
enabled: boolean;
endpoint: string;
Expand Down
96 changes: 0 additions & 96 deletions packages/cli/src/InternalHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { Service } from 'typedi';
import { snakeCase } from 'change-case';
import type { ITelemetryTrackProperties } from 'n8n-workflow';
import type { AuthProviderType } from '@db/entities/AuthIdentity';
import type { User } from '@db/entities/User';
import type { ITelemetryUserDeletionData } from '@/Interfaces';
import { WorkflowStatisticsService } from '@/services/workflow-statistics.service';
import { Telemetry } from '@/telemetry';
import { MessageEventBus } from './eventbus/MessageEventBus/MessageEventBus';
Expand Down Expand Up @@ -39,16 +36,6 @@ export class InternalHooks {
this.telemetry.track('Session started', { session_id: pushRef });
}

onPersonalizationSurveySubmitted(userId: string, answers: Record<string, string>): void {
const camelCaseKeys = Object.keys(answers);
const personalizationSurveyData = { user_id: userId } as Record<string, string | string[]>;
camelCaseKeys.forEach((camelCaseKey) => {
personalizationSurveyData[snakeCase(camelCaseKey)] = answers[camelCaseKey];
});

this.telemetry.track('User responded to personalization questions', personalizationSurveyData);
}

onWorkflowSharingUpdate(workflowId: string, userId: string, userList: string[]) {
const properties: ITelemetryTrackProperties = {
workflow_id: workflowId,
Expand All @@ -67,76 +54,6 @@ export class InternalHooks {
return await Promise.race([timeoutPromise, this.telemetry.trackN8nStop()]);
}

onUserDeletion(userDeletionData: {
user: User;
telemetryData: ITelemetryUserDeletionData;
publicApi: boolean;
}) {
this.telemetry.track('User deleted user', {
...userDeletionData.telemetryData,
user_id: userDeletionData.user.id,
public_api: userDeletionData.publicApi,
});
}

onUserInvite(userInviteData: {
user: User;
target_user_id: string[];
public_api: boolean;
email_sent: boolean;
invitee_role: string;
}) {
this.telemetry.track('User invited new user', {
user_id: userInviteData.user.id,
target_user_id: userInviteData.target_user_id,
public_api: userInviteData.public_api,
email_sent: userInviteData.email_sent,
invitee_role: userInviteData.invitee_role,
});
}

onUserRoleChange(userRoleChangeData: {
user: User;
target_user_id: string;
public_api: boolean;
target_user_new_role: string;
}) {
const { user, ...rest } = userRoleChangeData;

this.telemetry.track('User changed role', { user_id: user.id, ...rest });
}

onUserRetrievedUser(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved user', userRetrievedData);
}

onUserRetrievedAllUsers(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved all users', userRetrievedData);
}

onUserRetrievedExecution(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved execution', userRetrievedData);
}

onUserRetrievedAllExecutions(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved all executions', userRetrievedData);
}

onUserRetrievedWorkflow(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved workflow', userRetrievedData);
}

onUserRetrievedAllWorkflows(userRetrievedData: { user_id: string; public_api: boolean }) {
this.telemetry.track('User retrieved all workflows', userRetrievedData);
}

onUserUpdate(userUpdateData: { user: User; fields_changed: string[] }) {
this.telemetry.track('User changed personal settings', {
user_id: userUpdateData.user.id,
fields_changed: userUpdateData.fields_changed,
});
}

onUserInviteEmailClick(userInviteClickData: { inviter: User; invitee: User }) {
this.telemetry.track('User clicked invite link from email', {
user_id: userInviteClickData.invitee.id,
Expand Down Expand Up @@ -172,19 +89,6 @@ export class InternalHooks {
this.telemetry.track('Owner finished instance setup', instanceOwnerSetupData);
}

onUserSignup(
user: User,
userSignupData: {
user_type: AuthProviderType;
was_disabled_ldap_user: boolean;
},
) {
this.telemetry.track('User signed up', {
user_id: user.id,
...userSignupData,
});
}

onEmailFailed(failedEmailData: {
user: User;
message_type:
Expand Down
14 changes: 7 additions & 7 deletions packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { validCursor } from '../../shared/middlewares/global.middleware';
import type { ExecutionRequest } from '../../../types';
import { getSharedWorkflowIds } from '../workflows/workflows.service';
import { encodeNextCursor } from '../../shared/services/pagination.service';
import { InternalHooks } from '@/InternalHooks';
import { EventService } from '@/events/event.service';
import { ExecutionRepository } from '@db/repositories/execution.repository';
import { ConcurrencyControlService } from '@/concurrency/concurrency-control.service';

Expand Down Expand Up @@ -78,9 +78,9 @@ export = {
return res.status(404).json({ message: 'Not Found' });
}

Container.get(InternalHooks).onUserRetrievedExecution({
user_id: req.user.id,
public_api: true,
Container.get(EventService).emit('user-retrieved-execution', {
userId: req.user.id,
publicApi: true,
});

return res.json(replaceCircularReferences(execution));
Expand Down Expand Up @@ -130,9 +130,9 @@ export = {
const count =
await Container.get(ExecutionRepository).getExecutionsCountForPublicApi(filters);

Container.get(InternalHooks).onUserRetrievedAllExecutions({
user_id: req.user.id,
public_api: true,
Container.get(EventService).emit('user-retrieved-all-executions', {
userId: req.user.id,
publicApi: true,
});

return res.json({
Expand Down
22 changes: 9 additions & 13 deletions packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
validLicenseWithUserQuota,
} from '../../shared/middlewares/global.middleware';
import type { UserRequest } from '@/requests';
import { InternalHooks } from '@/InternalHooks';
import { EventService } from '@/events/event.service';
import { ProjectRelationRepository } from '@/databases/repositories/projectRelation.repository';
import type { Response } from 'express';
import { InvitationController } from '@/controllers/invitation.controller';
Expand All @@ -37,12 +37,10 @@ export = {
});
}

const telemetryData = {
user_id: req.user.id,
public_api: true,
};

Container.get(InternalHooks).onUserRetrievedUser(telemetryData);
Container.get(EventService).emit('user-retrieved-user', {
userId: req.user.id,
publicApi: true,
});

return res.json(clean(user, { includeRole }));
},
Expand All @@ -65,12 +63,10 @@ export = {
in: _in,
});

const telemetryData = {
user_id: req.user.id,
public_api: true,
};

Container.get(InternalHooks).onUserRetrievedAllUsers(telemetryData);
Container.get(EventService).emit('user-retrieved-all-users', {
userId: req.user.id,
publicApi: true,
});

return res.json({
data: clean(users, { includeRole }),
Expand Down
13 changes: 6 additions & 7 deletions packages/cli/src/PublicApi/v1/handlers/workflows/workflows.handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
updateTags,
} from './workflows.service';
import { WorkflowService } from '@/workflows/workflow.service';
import { InternalHooks } from '@/InternalHooks';
import { WorkflowHistoryService } from '@/workflows/workflowHistory/workflowHistory.service.ee';
import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository';
import { TagRepository } from '@/databases/repositories/tag.repository';
Expand Down Expand Up @@ -119,9 +118,9 @@ export = {
return res.status(404).json({ message: 'Not Found' });
}

Container.get(InternalHooks).onUserRetrievedWorkflow({
user_id: req.user.id,
public_api: true,
Container.get(EventService).emit('user-retrieved-workflow', {
userId: req.user.id,
publicApi: true,
});

return res.json(workflow);
Expand Down Expand Up @@ -185,9 +184,9 @@ export = {
...(!config.getEnv('workflowTagsDisabled') && { relations: ['tags'] }),
});

Container.get(InternalHooks).onUserRetrievedAllWorkflows({
user_id: req.user.id,
public_api: true,
Container.get(EventService).emit('user-retrieved-all-workflows', {
userId: req.user.id,
publicApi: true,
});

return res.json({
Expand Down
9 changes: 4 additions & 5 deletions packages/cli/src/auth/methods/ldap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Container } from 'typedi';

import { InternalHooks } from '@/InternalHooks';
import { LdapService } from '@/Ldap/ldap.service.ee';
import {
createLdapUserOnLocalDb,
Expand Down Expand Up @@ -51,11 +50,11 @@ export const handleLdapLogin = async (
await updateLdapUserOnLocalDb(identity, ldapAttributesValues);
} else {
const user = await createLdapUserOnLocalDb(ldapAttributesValues, ldapId);
Container.get(InternalHooks).onUserSignup(user, {
user_type: 'ldap',
was_disabled_ldap_user: false,
Container.get(EventService).emit('user-signed-up', {
user,
userType: 'ldap',
wasDisabledLdapUser: false,
});
Container.get(EventService).emit('user-signed-up', { user });
return user;
}
} else {
Expand Down
8 changes: 5 additions & 3 deletions packages/cli/src/controllers/__tests__/me.controller.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ import { InternalHooks } from '@/InternalHooks';
import { License } from '@/License';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { UserRepository } from '@/databases/repositories/user.repository';
import { EventService } from '@/events/event.service';
import { badPasswords } from '@test/testData';
import { mockInstance } from '@test/mocking';

const browserId = 'test-browser-id';

describe('MeController', () => {
const externalHooks = mockInstance(ExternalHooks);
const internalHooks = mockInstance(InternalHooks);
mockInstance(InternalHooks);
const eventService = mockInstance(EventService);
const userService = mockInstance(UserService);
const userRepository = mockInstance(UserRepository);
mockInstance(License).isWithinUsersLimit.mockReturnValue(true);
Expand Down Expand Up @@ -202,9 +204,9 @@ describe('MeController', () => {
req.user.password,
]);

expect(internalHooks.onUserUpdate).toHaveBeenCalledWith({
expect(eventService.emit).toHaveBeenCalledWith('user-updated', {
user: req.user,
fields_changed: ['password'],
fieldsChanged: ['password'],
});
});
});
Expand Down
10 changes: 4 additions & 6 deletions packages/cli/src/controllers/invitation.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@ import type { User } from '@/databases/entities/User';
import { UserRepository } from '@db/repositories/user.repository';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { ForbiddenError } from '@/errors/response-errors/forbidden.error';
import { InternalHooks } from '@/InternalHooks';
import { ExternalHooks } from '@/ExternalHooks';
import { EventService } from '@/events/event.service';

@RestController('/invitations')
export class InvitationController {
constructor(
private readonly logger: Logger,
private readonly internalHooks: InternalHooks,
private readonly externalHooks: ExternalHooks,
private readonly authService: AuthService,
private readonly userService: UserService,
Expand Down Expand Up @@ -168,11 +166,11 @@ export class InvitationController {

this.authService.issueCookie(res, updatedUser, req.browserId);

this.internalHooks.onUserSignup(updatedUser, {
user_type: 'email',
was_disabled_ldap_user: false,
this.eventService.emit('user-signed-up', {
user: updatedUser,
userType: 'email',
wasDisabledLdapUser: false,
});
this.eventService.emit('user-signed-up', { user: updatedUser });

const publicInvitee = await this.userService.toPublic(invitee);

Expand Down
9 changes: 4 additions & 5 deletions packages/cli/src/controllers/me.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import { isSamlLicensedAndEnabled } from '@/sso/saml/samlHelpers';
import { UserService } from '@/services/user.service';
import { Logger } from '@/Logger';
import { ExternalHooks } from '@/ExternalHooks';
import { InternalHooks } from '@/InternalHooks';
import { BadRequestError } from '@/errors/response-errors/bad-request.error';
import { UserRepository } from '@/databases/repositories/user.repository';
import { isApiEnabled } from '@/PublicApi';
Expand All @@ -40,7 +39,6 @@ export class MeController {
constructor(
private readonly logger: Logger,
private readonly externalHooks: ExternalHooks,
private readonly internalHooks: InternalHooks,
private readonly authService: AuthService,
private readonly userService: UserService,
private readonly passwordUtility: PasswordUtility,
Expand Down Expand Up @@ -101,7 +99,6 @@ export class MeController {
this.authService.issueCookie(res, user, req.browserId);

const fieldsChanged = Object.keys(payload);
this.internalHooks.onUserUpdate({ user, fields_changed: fieldsChanged });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing bug: Even when updating a single field, all fields exist in the payload, so all fields are being reported as having changed. To be fixed separately.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a follow-up for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll follow up on this, won't forget

this.eventService.emit('user-updated', { user, fieldsChanged });

const publicUser = await this.userService.toPublic(user);
Expand Down Expand Up @@ -151,7 +148,6 @@ export class MeController {

this.authService.issueCookie(res, updatedUser, req.browserId);

this.internalHooks.onUserUpdate({ user: updatedUser, fields_changed: ['password'] });
this.eventService.emit('user-updated', { user: updatedUser, fieldsChanged: ['password'] });

await this.externalHooks.run('user.password.update', [updatedUser.email, updatedUser.password]);
Expand Down Expand Up @@ -186,7 +182,10 @@ export class MeController {

this.logger.info('User survey updated successfully', { userId: req.user.id });

this.internalHooks.onPersonalizationSurveySubmitted(req.user.id, personalizationAnswers);
this.eventService.emit('user-submitted-personalization-survey', {
userId: req.user.id,
answers: personalizationAnswers,
});

return { success: true };
}
Expand Down
Loading
Loading