Nv 1458 web abstract variable manager to not #2529

ainouzgali · 2023-01-19T05:37:58Z

What change does this PR introduce?

Show var manager in layout editor, allow to set defaultValue and required for layout vars. Apply layout vars defaults in send email.

Why was this change needed?

Other information (Screenshots)

…bstract-variable-manager-to-not # Conflicts: # apps/web/src/pages/brand/tabs/LayoutEditor.tsx

…l-content-in-Trigger' into nv-1458-web-abstract-variable-manager-to-not # Conflicts: # apps/web/src/pages/brand/tabs/LayoutEditor.tsx

linear · 2023-01-19T05:38:02Z

NV-1458 Web - abstract variable manager to not apply only to template steps.

Currently, our variable manager is built to watch only the variables inside the template editor form. As it is now needed in layouts - different context and separate form - we need to abstract it so it we would be able to reuse our var manager.

ainouzgali · 2023-01-19T08:45:23Z

.../src/app/content-templates/usecases/compile-template/email/compile-email-template.usecase.ts

@@ -49,12 +38,25 @@ export class CompileEmailTemplateUsecase {
        useNovuDefault = true;
      }
    }
+    const layoutVariables = layout?.variables || [];
+    const defaultPayload = verifyPayloadService.verifyPayload(layoutVariables, command.payload);


So defaultValue defined for the layout variables will also apply when sending email/preview/test

what-the-diff · 2023-01-19T09:15:09Z

Added VerifyPayloadService to verify payload for events and content templates
Updated layouts editor with new variables handling logic
Added path and control props to VariableManager component
Changed the way of getting variableName, variableType in VariableComponent from useFormContext() hook to useWatch() hook
Updated formPrefix for all Controller components inside VariableComponent based on new path prop value (if it's not empty) or template prop value otherwise
Added a new modal for managing variables
Changed the layout editor to use react-hooks-form instead of formik
Removed some unused code and added tests for this component

apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts

p-fernandez

Great work. Some considerations. Do you plan to do Cypress tests for this piece?

p-fernandez · 2023-01-19T10:43:45Z

.../src/app/content-templates/usecases/compile-template/email/compile-email-template.usecase.ts

+  private async loadTemplateContent(name: string) {
+    return new Promise<string>((resolve, reject) => {
+      let path = '/../../compile-template';
+      if (!process.env.E2E_RUNNER) {
+        path = '/src/app/content-templates/usecases/compile-template';
+      }
+      fs.readFile(`${__dirname}${path}/templates/${name}`, (err, content) => {
+        if (err) {
+          return reject(err);
+        }
+
+        return resolve(content.toString());
+      });
+    });
+  }


Well done making it reusable.

p-fernandez · 2023-01-19T10:57:00Z

libs/shared/src/consts/handlebar-helpers/index.ts

+export const novuReservedVariableNames = ['body'];
+
+export function isReservedVariableName(variableName: string) {
+  return novuReservedVariableNames.includes(variableName);
+}


p-fernandez · 2023-01-19T10:59:53Z

apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts


 @Injectable()
 export class CreateLayoutUseCase {
  constructor(private layoutRepository: LayoutRepository) {}

  async execute(command: CreateLayoutCommand) {
-    const entity = this.mapToEntity(command);
+    const variables = this.getExtractedVariables(command.variables as ITemplateVariable[], command.content);


Instead of applying it here you could directly apply it, inside of the mapToEntity. It can be a matter of preference: either we want it clearly independently like you did, or we want to hide it a bit inside of the method that maps all properties.

I did that at first :) But it felt that the map function look more complicated and less clear. Also, he really didn't like the types😅

p-fernandez · 2023-01-19T11:01:39Z

apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts

@@ -43,4 +47,27 @@ export class CreateLayoutUseCase {
      isDeleted: layout.deleted,
    };
  }
+
+  private getExtractedVariables(variables: ITemplateVariable[], content: string): ITemplateVariable[] {
+    const contentService = new ContentService();


What's our internal take on the use of a service? Just asking to consider suggesting to add it to the constructor of the use case rather that instantiate it every time the method is being called.

Yes, I was also wondering what will be better

p-fernandez · 2023-01-19T11:09:13Z

apps/api/src/app/shared/helpers/verify-payload.service.ts

+      const variableTypeHumanize = {
+        String: 'Value',
+        Array: 'Array',
+        Boolean: 'Boolean',
+      }[variable.type];


I understand that you are trying to humanize the variable type. So just swap the order in the name would reads more natural I think: humanizedVariableType. Though you could also to refer to it as variableTypeLabel that might go more in line with being the text will be rendered like in an UI.

Sure, its an old code, I just moved it.

p-fernandez · 2023-01-19T11:18:45Z

apps/web/src/components/templates/VariableManager.tsx

-export const VariableComponent = ({ index, template }: VariableComponentProps) => {
-  const { control, watch } = useFormContext();
+export const VariableComponent = ({ index, template, control, path }: VariableComponentProps) => {
+  const formPrefix = path ? `${path}variables.${index}` : `${template}.variables.${index}`;


Just to double check that the intention is to create the formPrefix for the case where path exists as: ${path}variables.${index} and not ${path}.variables.${index}.

Yes. Because I want to allow thw option of path being ''.
I want to refactor all the hooks and components related to var manager, we can do it much better. But will probably do it in cool down

apps/web/src/components/templates/email-editor/variables-management/VariablesManagement.tsx

p-fernandez · 2023-01-19T11:21:01Z

apps/web/src/pages/brand/tabs/LayoutEditor.tsx

+interface ILayoutForm {
+  content: string;
+  name: string;
+  description: string;
+  isDefault: boolean;
+  variables: ITemplateVariable[];
+}


You can generate the interface from Pick<LayoutDto, 'content' | 'name' | 'description' | 'isDefault' | 'variables'>.

p-fernandez · 2023-01-19T11:25:21Z

apps/web/src/pages/brand/tabs/LayoutEditor.tsx

+    variables.forEach((vari) => {
+      if (!arrayFields.find((field) => field.name === vari.name)) {
+        arrayFields.push(vari);
+      }
+    });
+    arrayFields.forEach((vari, ind) => {
+      if (!variables.find((field) => field.name === vari.name)) {
+        delete arrayFields[ind];
+      }
+    });


I am struggling to understand this bit. I have a feeling that this could be done with a single loop. Maybe with arrayFields being a Set and using delete.

Yes, it is a replicated code. Currently the hook we have for it is very dependent on a specific form context (the steps template.
I thought so to a few months ago haha. Worked on it for hours, eventually realized why it is needed- I'll tell you the problem later when we have more time and I remind myself a bit.

p-fernandez · 2023-01-19T11:28:07Z

apps/web/src/pages/brand/tabs/LayoutEditor.tsx

+        styles={{
+          modal: {
+            backgroundColor: theme.colorScheme === 'dark' ? colors.B15 : colors.white,
+            width: '90%',
+          },
+          body: {
+            paddingTop: '5px',
+            paddingInline: '8px',
+          },
+        }}


Should we use useStyles?

I'm not sure I like doing that for a very specific Modal. We already have a Modal in our design system. I would prefer to make that one more flexible to style changes. Will add it to backlog ❤️ Maybe a community member would like to help with thah.

ainouzgali · 2023-01-19T12:16:28Z

Great work. Some considerations. Do you plan to do Cypress tests for this piece?

Definitely! So many tests! But because of the huge PR I thought it's better if I do it separately.

Thank you @p-fernandez for the thorough code review! I really appreciate it

…ement/VariablesManagement.tsx Co-authored-by: Pablo Fernández <pablo@novu.co>

…l-content-in-Trigger' into nv-1458-web-abstract-variable-manager-to-not # Conflicts: # apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts

ainouzgali added 6 commits January 11, 2023 10:23

feat(wip): start variable extraction from layout content

44bc053

Merge remote-tracking branch 'origin/feat-layouts' into nv-1458-web-a…

c4e176f

…bstract-variable-manager-to-not # Conflicts: # apps/web/src/pages/brand/tabs/LayoutEditor.tsx

Merge remote-tracking branch 'origin/feat-layouts' into nv-1458-web-a…

06d46fd

…bstract-variable-manager-to-not # Conflicts: # apps/web/src/pages/brand/tabs/LayoutEditor.tsx

feat(wip): variables initial

0ef6f30

Merge remote-tracking branch 'origin/NV-1475-Compile-layout-with-emai…

4a1e01b

…l-content-in-Trigger' into nv-1458-web-abstract-variable-manager-to-not # Conflicts: # apps/web/src/pages/brand/tabs/LayoutEditor.tsx

feat: variables for layout

7615c1d

github-actions bot added @novu/api @novu/web labels Jan 19, 2023

ainouzgali marked this pull request as draft January 19, 2023 05:38

revert: variable management, extract section component

6e3e018

ainouzgali commented Jan 19, 2023

View reviewed changes

apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts Outdated Show resolved Hide resolved

ainouzgali requested a review from p-fernandez January 19, 2023 09:43

ainouzgali marked this pull request as ready for review January 19, 2023 09:47

feat: add body as a reserved var name, should not show as a variable

90efaf9

github-actions bot added the @novu/shared label Jan 19, 2023

p-fernandez reviewed Jan 19, 2023

View reviewed changes

ainouzgali and others added 3 commits January 19, 2023 14:52

Update apps/web/src/components/templates/email-editor/variables-manag…

f70d828

…ement/VariablesManagement.tsx Co-authored-by: Pablo Fernández <pablo@novu.co>

fix: mismatched typing for variabels

0113d1c

Merge remote-tracking branch 'origin/NV-1475-Compile-layout-with-emai…

ace20f9

…l-content-in-Trigger' into nv-1458-web-abstract-variable-manager-to-not # Conflicts: # apps/api/src/app/layouts/use-cases/create-layout/create-layout.use-case.ts

ainouzgali merged commit aaca4c5 into NV-1475-Compile-layout-with-email-content-in-Trigger Jan 19, 2023

ainouzgali deleted the nv-1458-web-abstract-variable-manager-to-not branch January 19, 2023 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nv 1458 web abstract variable manager to not #2529

Nv 1458 web abstract variable manager to not #2529

ainouzgali commented Jan 19, 2023 •

edited

linear bot commented Jan 19, 2023

ainouzgali Jan 19, 2023

what-the-diff bot commented Jan 19, 2023

p-fernandez left a comment

p-fernandez Jan 19, 2023

p-fernandez Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

p-fernandez Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

p-fernandez Jan 19, 2023

ainouzgali Jan 19, 2023

ainouzgali commented Jan 19, 2023 •

edited

Nv 1458 web abstract variable manager to not #2529

Nv 1458 web abstract variable manager to not #2529

Conversation

ainouzgali commented Jan 19, 2023 • edited

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Jan 19, 2023

Choose a reason for hiding this comment

what-the-diff bot commented Jan 19, 2023

p-fernandez 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

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

ainouzgali commented Jan 19, 2023 • edited

ainouzgali commented Jan 19, 2023 •

edited

ainouzgali commented Jan 19, 2023 •

edited