Nv 2403 tenant id trigger engine integration #3994

ainouzgali · 2023-08-17T13:31:20Z

What change does this PR introduce?

Allow to override tenant identifier in the context of a trigger.

I still have a lot of refactoring to do - opened this PR to allow discussion about this direction.

When using tenant in an editor(will also have autocomplete for tenant):

Trigger snippet will recognize that a tenant object needs to be included in the trigger event:

The tenant object in the trigger will add the tenant to the context of this run. Also allows to create/update the tenant, similar to what we do for subscribers.

Execution detail example:

Why was this change needed?

Other information (Screenshots)

linear · 2023-08-17T13:31:27Z

NV-2403 Tenant Id Trigger Engine Integration

What?

Allow our API users to pass a tenantIdentifier during the trigger (the custom "Identifier/ID" our users have provided during tenant creation.)

Once a trigger is generated with the tenantIdentifier property, the tenant entity should be added to the template generation context of all channels, meaning it should be possible to use it as the variable for ex. in the content editor.

Why? (Context)

The user should be able to utilize common tenant information provided in the creation phase of the tenant (things like tenant name, address, or any other custom data) as part of trigger process.

Definition of Done

User is able to pass the tenantId on the trigger level
Tenant entity information is accessible and could be rendered inside an e-mail/in-app/etc… templates
E2E tests for template compilation

…to nv-2403-tenant-id-trigger-engine-integration

ainouzgali · 2023-08-17T13:48:17Z

libs/shared/src/entities/notification-template/notification-template.interface.ts

+export enum TriggerContextTypeEnum {
+  TENANT = 'tenant',
+  ACTOR = 'actor',
+}


I will remove the actor behviour from this PR, but just wanted to show the direction I intend to take here. It will make adding new sections of trigger objects easier and more immediate

ainouzgali · 2023-08-17T13:49:48Z

libs/shared/src/entities/message-template/message-template.interface.ts

+export const ReservedVariablesMap = {
+  [TriggerContextTypeEnum.TENANT]: [{ name: 'identifier', type: TemplateVariableTypeEnum.STRING }],
+  [TriggerContextTypeEnum.ACTOR]: [{ name: 'subscriberId', type: TemplateVariableTypeEnum.STRING }],


The variables we need to be included for our 'reserved' variables to work in the context of a trigger.

ainouzgali · 2023-08-17T13:52:18Z

libs/dal/src/repositories/job/job.schema.ts

+    _tenantId: {
+      type: Schema.Types.ObjectId,
+      ref: 'Tenant',
+    },
+    tenantIdentifier: {
+      type: Schema.Types.String,
+    },


I will remove the _tenantId and leave only the tenantIdentifier.
I feel it does make sense to have the tenantIdentifier as part of the job entity, also for future use when we incorporate more of the tenants logic in the future.

ainouzgali · 2023-08-17T13:53:14Z

libs/shared/src/dto/events/event.interface.ts

@@ -7,3 +7,5 @@ export type TriggerRecipient = TriggerRecipientSubscriber | ITopic;
 export type TriggerRecipients = TriggerRecipient[];

 export type TriggerRecipientsPayload = TriggerRecipientSubscriber | TriggerRecipients;
+
+export type TriggerTenantContext = string | ITenantDefine;


Name help? ;)

ainouzgali · 2023-08-17T14:04:31Z

apps/worker/src/app/workflow/usecases/send-message/send-message-sms.usecase.ts

+    const tenantIdentifier = command.job.tenantIdentifier;
+    let tenant: TenantEntity | null = null;
+
+    if (tenantIdentifier) {
+      tenant = await this.tenantRepository.findOne({
+        _environmentId: command.environmentId,
+        identifier: tenantIdentifier,
+      });
+      if (!tenant) {
+        await this.createExecutionDetails.execute(
+          CreateExecutionDetailsCommand.create({
+            ...CreateExecutionDetailsCommand.getDetailsFromJob(command.job),
+            detail: DetailEnum.TENANT_NOT_FOUND,
+            source: ExecutionDetailsSourceEnum.INTERNAL,
+            status: ExecutionDetailsStatusEnum.FAILED,
+            isTest: false,
+            isRetry: false,
+            raw: JSON.stringify({
+              tenantIdentifier: tenantIdentifier,
+            }),
+          })
+        );
+
+        return;
+      }
+      await this.sendSelectedTenantExecution(command.job, tenant);
+    }
+


I will extract this duplication and add to all the other channels

…to nv-2403-tenant-id-trigger-engine-integration

ainouzgali · 2023-08-21T10:12:37Z

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.usecase.ts

+    for (const reservedVariableType of reservedVariablesTypes) {
+      const payload = command[reservedVariableType];
+      if (!payload) {
+        invalidKeys.push(`${reservedVariableType} object`);
+      } else {
+        const requiredVariablesContextNames = ReservedVariablesMap[reservedVariableType].map(
+          (variable) => variable.name
+        );
+        for (const variableName of requiredVariablesContextNames) {
+          const variableNameExists = typeof payload === 'string' ? payload : payload[variableName];
+          if (!variableNameExists) {
+            invalidKeys.push(`${variableName} property of ${reservedVariableType}`);
+          }


On trigger, if missing the tenant object in trigger or missing the mandatory property 'identifier' will throw error

ainouzgali · 2023-08-21T10:23:39Z

apps/worker/src/app/workflow/usecases/send-message/send-message-email.usecase.ts

+    const [tenant, overrideLayoutId] = await Promise.all([
+      this.handleTenantExecution(command.job),
+      this.getOverrideLayoutId(command),
+      this.sendSelectedIntegrationExecution(command.job, integration),
+    ]);


I felt the order it will show in activity feed is not crucial

ainouzgali · 2023-08-21T10:25:57Z

libs/dal/src/repositories/job/job.entity.ts

@@ -16,6 +16,7 @@ export class JobEntity {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  overrides: Record<string, Record<string, unknown>>;
  step: NotificationStepEntity;
+  tenant?: ITenantDefine;


Added so we can show it in the snippet in activity feed - so we can show the user a correct and accurate presentation of the snippet he used + from here we take the identifier for the messages compilation

ainouzgali · 2023-08-21T10:29:48Z

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts

+      if (!tenantProcessed) {
+        Logger.warn(
+          `Tenant with identifier ${JSON.stringify(
+            tenant.identifier
+          )} of organization ${command.organizationId} in transaction ${
+            command.transactionId
+          } could not be processed.`,
+          LOG_CONTEXT
+        );
+      }


If there was a problem creating/updating the tenant, we can't add to execution details at this stage. It will continue execution and in send message, if he can't find it -> will stop execution.

…igger-engine-integration

davidsoderberg · 2023-08-21T13:17:07Z

packages/application-generic/src/usecases/trigger-event/trigger-event.usecase.ts

@@ -98,6 +107,28 @@ export class TriggerEvent {
      template
    );

+    if (tenant) {


Could this be a function?

davidsoderberg · 2023-08-21T13:19:10Z

packages/application-generic/src/usecases/process-tenant/process-tenant.usecase.ts

+        tenant
+      );
+    } catch (e) {
+      tenantEntity = null;


Why do we do null here? will getTenant in some cases return null otherwise we could reduce this code with just do the catch with no code and skip the null check on line 38?

davidsoderberg · 2023-08-21T13:20:27Z

apps/worker/src/app/workflow/usecases/send-message/send-message.base.ts

+        _environmentId: job._environmentId,
+        identifier: tenantIdentifier,
+      });
+      if (!tenant) {


Could this be a part of sendSelectedTenantExecution?

davidsoderberg · 2023-08-21T13:22:20Z

apps/web/src/pages/templates/components/TriggerSnippetTabs.tsx

@@ -19,14 +20,20 @@ export function TriggerSnippetTabs({ trigger }: { trigger: INotificationTrigger
  const toValue = getSubscriberValue(subscriberVariables, (variable) => variable.value || '<REPLACE_WITH_DATA>');
  const payloadValue = getPayloadValue(trigger.variables);

+  const reservedValue = triggerSnippetVariables.reduce((acc, variable) => {


This will not create a lot of rerenders?

The Tabs component is not memoized, so it will re-render it and all the children when TriggerSnippetTabs will render, like it is right now. 😉

The thing that we can improve is memoize the calculations for this variable, so React won't need to do it every time on a new re-render. Actually same for tabs.

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.usecase.ts

…igger-engine-integration

apps/api/src/app/events/dtos/trigger-event-request.dto.ts

LetItRock · 2023-08-17T15:51:19Z

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.command.ts

@@ -23,4 +23,7 @@ export class ParseEventRequestCommand extends EnvironmentWithUserCommand {

  @IsOptional()
  actor?: TriggerRecipientSubscriber | null;
+
+  @IsOptional()
+  tenant?: TriggerTenantContext | null;


I think here we should have a class that extends this interface and do the nested validation over the fields

apps/api/src/app/events/events.controller.ts

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.usecase.ts

LetItRock · 2023-08-22T07:28:32Z

apps/web/src/pages/templates/components/TriggerSnippetTabs.tsx

@@ -19,14 +20,20 @@ export function TriggerSnippetTabs({ trigger }: { trigger: INotificationTrigger
  const toValue = getSubscriberValue(subscriberVariables, (variable) => variable.value || '<REPLACE_WITH_DATA>');
  const payloadValue = getPayloadValue(trigger.variables);

+  const reservedValue = triggerSnippetVariables.reduce((acc, variable) => {


The Tabs component is not memoized, so it will re-render it and all the children when TriggerSnippetTabs will render, like it is right now. 😉

The thing that we can improve is memoize the calculations for this variable, so React won't need to do it every time on a new re-render. Actually same for tabs.

libs/dal/src/repositories/notification-template/notification-template.entity.ts

LetItRock

🚀

p-fernandez · 2023-08-23T07:34:45Z

apps/worker/src/app/workflow/workflow.module.ts

@@ -59,6 +62,7 @@ import {
  WebhookFilterBackoffStrategy,
 } from './usecases';
 import { MetricQueueService } from './services/metric-queue.service';
+import { ProcessTenant } from '@novu/application-generic/build/main/usecases/process-tenant';


@ainouzgali this import slipped through the cracks. Could you please fix it in next? 🙏🏻

ainouzgali added 2 commits August 17, 2023 16:27

feat(wip): add tenant identifier override

44251a9

feat(wip): missing usecases in module

cd9f63f

ainouzgali marked this pull request as draft August 17, 2023 13:31

github-actions bot added @novu/api @novu/web @novu/dal @novu/shared @novu/worker labels Aug 17, 2023

Merge remote-tracking branch 'origin/feat-add-tenants-to-node-sdk' in…

1adb4a8

…to nv-2403-tenant-id-trigger-engine-integration

ainouzgali changed the base branch from next to feat-add-tenants-to-node-sdk August 17, 2023 13:34

ainouzgali commented Aug 17, 2023

View reviewed changes

feat(wip): add tenant to content compilation for email and sms

adc4ffd

ainouzgali commented Aug 17, 2023

View reviewed changes

ainouzgali added 4 commits August 20, 2023 15:36

Merge remote-tracking branch 'origin/feat-add-tenants-to-node-sdk' in…

7bbb8fe

…to nv-2403-tenant-id-trigger-engine-integration

feat(wip): change reserved name

d64cd11

feat: process tenant and validation

ae390d8

feat: remove actor for now

12522c1

ainouzgali commented Aug 21, 2023

View reviewed changes

ainouzgali marked this pull request as ready for review August 21, 2023 10:30

ainouzgali added 2 commits August 21, 2023 13:41

fix: failing test

934b309

Merge branch 'feat-add-tenants-to-node-sdk' into nv-2403-tenant-id-tr…

3aed906

…igger-engine-integration

davidsoderberg reviewed Aug 21, 2023

View reviewed changes

apps/api/src/app/events/usecases/parse-event-request/parse-event-request.usecase.ts Outdated Show resolved Hide resolved

Merge branch 'feat-add-tenants-to-node-sdk' into nv-2403-tenant-id-tr…

a8cf5d8

…igger-engine-integration

LetItRock reviewed Aug 22, 2023

View reviewed changes

ainouzgali added 3 commits August 22, 2023 13:36

feat: after pr comments

d2efa59

feat: typing variables

98064aa

feat: validate nested

d0e0121

ainouzgali requested a review from LetItRock August 22, 2023 12:37

Base automatically changed from feat-add-tenants-to-node-sdk to next August 22, 2023 12:39

Merge branch 'next' into nv-2403-tenant-id-trigger-engine-integration

7189ac0

ainouzgali requested a review from davidsoderberg August 22, 2023 13:40

LetItRock approved these changes Aug 22, 2023

View reviewed changes

ainouzgali added this pull request to the merge queue Aug 23, 2023

Merged via the queue into next with commit 16603e3 Aug 23, 2023

ainouzgali deleted the nv-2403-tenant-id-trigger-engine-integration branch August 23, 2023 07:00

p-fernandez reviewed Aug 23, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nv 2403 tenant id trigger engine integration #3994

Nv 2403 tenant id trigger engine integration #3994

ainouzgali commented Aug 17, 2023 •

edited

Loading

linear bot commented Aug 17, 2023

What?

Why? (Context)

Definition of Done

ainouzgali Aug 17, 2023

ainouzgali Aug 17, 2023

ainouzgali Aug 17, 2023

ainouzgali Aug 17, 2023

ainouzgali Aug 17, 2023

ainouzgali Aug 21, 2023

ainouzgali Aug 21, 2023

ainouzgali Aug 21, 2023

ainouzgali Aug 21, 2023

davidsoderberg Aug 21, 2023

davidsoderberg Aug 21, 2023

davidsoderberg Aug 21, 2023

davidsoderberg Aug 21, 2023

LetItRock Aug 22, 2023

LetItRock Aug 17, 2023

LetItRock Aug 22, 2023

LetItRock left a comment

p-fernandez Aug 23, 2023

Nv 2403 tenant id trigger engine integration #3994

Nv 2403 tenant id trigger engine integration #3994

Conversation

ainouzgali commented Aug 17, 2023 • edited Loading

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Aug 17, 2023

What?

Why? (Context)

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LetItRock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ainouzgali commented Aug 17, 2023 •

edited

Loading