fix(api): Use the MongoId for stepId when fetching step controls #6679

rifont · 2024-10-11T11:29:54Z

What changed? Why was the change needed?

Use the MongoId for stepId when fetching step controls
- Previously the user-provided stepId was used when fetching step controls. A recent PR removed persistence of stepId, this caused null to always be returned when fetching step controls from the Dashboard, meaning that the default controls were always shown
Remove redundant controlSchema property from UpsertControlValues use-case
- Correspondingly, remove the json-schema-defaults package from API which is redundant - @novu/framework already generates the defaults
Remove redundant workflowId and stepId properties from ControlValuesEntity

Screenshots

persisted-controls-fetching.mov

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

linear · 2024-10-11T11:29:57Z

NV-4484 Persisted control values don't load in Dashboard

netlify · 2024-10-11T11:30:10Z

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name	Link
🔨 Latest commit	`db86d8c`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/67090c355ba5150008303cbb
😎 Deploy Preview	https://deploy-preview-6679--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rifont · 2024-10-11T11:32:02Z

libs/dal/src/repositories/control-variables/controlValuesEntity.ts

@@ -12,6 +12,4 @@ export class ControlValuesEntity {
  controls: Record<string, unknown>;
  _workflowId: string;
  _stepId: string;
-  workflowId: string;
-  stepId: string;


These are no longer used.

rifont · 2024-10-11T11:32:11Z

libs/application-generic/src/usecases/upsert-control-values/upsert-control-values.command.ts

@@ -14,7 +13,4 @@ export class UpsertControlValuesCommand extends OrganizationLevelCommand {
  @IsObject()
  @IsOptional()
  newControlValues?: Record<string, unknown>;
-
-  @IsObject()
-  controlSchemas: { schema: JsonSchema };


This was never used. Cleaning up.

rifont · 2024-10-11T11:32:40Z

libs/application-generic/src/usecases/upsert-control-values/upsert-control-values.command.ts


-export class UpsertControlValuesCommand extends OrganizationLevelCommand {
+export class UpsertControlValuesCommand extends EnvironmentCommand {


The incorrect base command was used here, which resulted in environmentId being an optional type when it should have been required.

rifont · 2024-10-11T11:32:49Z

libs/application-generic/src/usecases/upsert-control-values/index.ts

-export * from './upsert-control-values-command';
-export * from './upsert-control-values-use-case';
+export * from './upsert-control-values.command';
+export * from './upsert-control-values.usecase';


Fixing to follow naming conventions.

rifont · 2024-10-11T11:33:10Z

apps/api/src/app/bridge/usecases/store-control-variables/store-control-variables.usecase.ts

-      (step.template as any)?.controls?.schema || (step.template as any)?.inputs?.schema,
-      {}
-    ) as JsonSchema;
-


Defaults are generated on @novu/framework, there is no need to generate defaults on the API.

question just to clarify, for the dashboard, we need to make sure it gets the default values for the code-first solution, so it can display them. If we have a code-first preview, we first get the default values from the bridge app and then override them with control values if they exist (persisted in novu), correct?

The JSON schema from @novu/framework is passed to the frontend to render the default values, so we don't need to create defaults on the API.

rifont · 2024-10-11T11:33:23Z

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


    const result = await this.controlValuesRepository.findOne({
      _environmentId: user.environmentId,
      _organizationId: user.organizationId,
      _workflowId: workflowExist._id,
-      stepId,
+      _stepId: step._id,


This change fixes the fetching of persisted step controls.

rifont · 2024-10-11T11:33:28Z

apps/api/package.json

@@ -71,7 +71,6 @@
    "helmet": "^6.0.1",
    "i18next": "^23.7.6",
    "ioredis": "5.3.2",
-    "json-schema-defaults": "^0.4.0",


Not needed.

djabarovgeorge

awesome work, left couple of comments for your consideration

djabarovgeorge · 2024-10-11T11:53:57Z

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

+    if (!step || !step._id) {
+      throw new NotFoundException('Step not found');
+    }


Question: can we do something like this?
I started to use this pattern in this PR this way we have the clean message to search by with the additional metadata (step id)

Suggested change

if (!step || !step._id) {

throw new NotFoundException('Step not found');

}

if (!step || !step._id) {

throw new NotFoundException({

message: 'Step not found',

stepId: stepId,

});

}

As discussed, we will address this holistically across the API using shared classes.

djabarovgeorge · 2024-10-11T11:54:38Z

apps/api/src/app/bridge/usecases/store-control-variables/store-control-variables.usecase.ts

+      throw new NotFoundException('Workflow not found');
    }

    const step = workflowExist?.steps.find((item) => item.stepId === command.stepId);

    if (!step || !step._id) {
-      throw new ApiException('Step not found');
+      throw new NotFoundException('Step not found');


djabarovgeorge · 2024-10-11T11:59:31Z

apps/api/src/app/bridge/usecases/store-control-variables/store-control-variables.usecase.ts

-      (step.template as any)?.controls?.schema || (step.template as any)?.inputs?.schema,
-      {}
-    ) as JsonSchema;
-


question just to clarify, for the dashboard, we need to make sure it gets the default values for the code-first solution, so it can display them. If we have a code-first preview, we first get the default values from the bridge app and then override them with control values if they exist (persisted in novu), correct?

djabarovgeorge · 2024-10-11T11:59:51Z

apps/api/src/app/bridge/usecases/store-control-variables/store-control-variables.usecase.ts

    return await this.upsertControlValuesUseCase.execute(
      UpsertControlValuesCommand.create({
        organizationId: command.organizationId,
        environmentId: command.environmentId,
        notificationStepEntity: step,
        workflowId: workflowExist._id,
        newControlValues: command.variables,
-        controlSchemas: { schema: stepDefaultControls },


djabarovgeorge · 2024-10-11T12:01:20Z

libs/application-generic/src/usecases/upsert-control-values/upsert-control-values.usecase.ts

@@ -2,15 +2,15 @@ import { Injectable } from '@nestjs/common';

 import { ControlValuesEntity, ControlValuesRepository } from '@novu/dal';
 import { ControlVariablesLevelEnum } from '@novu/shared';
-import { UpsertControlValuesCommand } from './upsert-control-values-command';


i am a bit confused should not it be a dot here?

The new code shows a dot.

refactor(api): remove unused json-schema-defaults dependency

db86d8c

rifont temporarily deployed to Linting October 11, 2024 11:30 — with GitHub Actions Inactive

github-actions bot added @novu/api @novu/dal labels Oct 11, 2024

rifont requested a review from djabarovgeorge October 11, 2024 11:31

rifont commented Oct 11, 2024

View reviewed changes

rifont requested a review from tatarco October 11, 2024 11:33

djabarovgeorge approved these changes Oct 11, 2024

View reviewed changes

rifont merged commit 4a3da93 into next Oct 11, 2024
52 checks passed

rifont deleted the nv-4484-persisted-control-values-dont-load-in-dashboard branch October 11, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(api): Use the MongoId for stepId when fetching step controls #6679

fix(api): Use the MongoId for stepId when fetching step controls #6679

rifont commented Oct 11, 2024 •

edited

Loading

linear bot commented Oct 11, 2024

netlify bot commented Oct 11, 2024 •

edited

Loading

rifont Oct 11, 2024

rifont Oct 11, 2024

rifont Oct 11, 2024

rifont Oct 11, 2024

rifont Oct 11, 2024

djabarovgeorge Oct 11, 2024

rifont Oct 11, 2024

rifont Oct 11, 2024

rifont Oct 11, 2024

djabarovgeorge left a comment

djabarovgeorge Oct 11, 2024

rifont Oct 11, 2024

djabarovgeorge Oct 11, 2024

djabarovgeorge Oct 11, 2024

djabarovgeorge Oct 11, 2024

djabarovgeorge Oct 11, 2024

rifont Oct 11, 2024


		export class UpsertControlValuesCommand extends OrganizationLevelCommand {
		export class UpsertControlValuesCommand extends EnvironmentCommand {

fix(api): Use the MongoId for stepId when fetching step controls #6679

fix(api): Use the MongoId for stepId when fetching step controls #6679

Conversation

rifont commented Oct 11, 2024 • edited Loading

What changed? Why was the change needed?

Screenshots

Related enterprise PR

Special notes for your reviewer

linear bot commented Oct 11, 2024

netlify bot commented Oct 11, 2024 • edited Loading

✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!

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

djabarovgeorge left a 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

rifont commented Oct 11, 2024 •

edited

Loading

netlify bot commented Oct 11, 2024 •

edited

Loading