Nv 3510 - Open onboarding workflow node #5207

djabarovgeorge · 2024-02-17T14:06:32Z

What change does this PR introduce?

This ticket is responsible for:

navigate to a specific workflow node.
reuse existing workflow.
allow to fetch workflow by identifier.

Why was this change needed?

Other information (Screenshots)

linear · 2024-02-17T14:06:35Z

NV-3510 Open onboarding workflow node

djabarovgeorge · 2024-02-19T10:47:49Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    segment.track('[Get Started] Click Create Notification Template', {
+      templateIdentifier: blueprintIdentifier,
+      location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,
+    });


This way we will know if the user wanted to Create new use case workflow, or Configure use case node.

Suggested change

segment.track('[Get Started] Click Create Notification Template', {

templateIdentifier: blueprintIdentifier,

location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,

});

if(node){

segment.track('Click Configure Use-Case Node [Get Started] ', {

templateIdentifier: blueprintIdentifier,

node: node

});

} else {

segment.track('Click Create Workflow [Get Started] ', {

templateIdentifier: blueprintIdentifier,

});

Not sure I understand why we'd switch the [ ] to the end.

Also, this seems like something that could be handled with a ternary:

Suggested change

segment.track('[Get Started] Click Create Notification Template', {

templateIdentifier: blueprintIdentifier,

location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,

});

const action = node ? 'Click Create Notification Template' : 'Click Create Workflow';

segment.track(`[Get Started] ${action}`, {

templateIdentifier: blueprintIdentifier,

location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,

});

Let's centralize the topic regarding the [area] in one place, i already addressed this question here :)
#5212 (comment)

Regarding the ternary, it has two cons in my opinion

one event we have a node and another does not.

as I love less code with reusable variables, in those scenarios where we log data it will be harder to debug code and search when the log is dynamic.

let me know what you think on the matter

When we don't have a node, it should be undefined, correct? In that event, it wouldn't appear anyway on the tracking event

antonjoel82

Thanks for the updates!

apps/api/src/app/workflows/notification-template.controller.ts

.../app/workflows/usecases/delete-notification-template/delete-notification-template.command.ts

antonjoel82 · 2024-02-20T21:50:25Z

apps/web/src/pages/get-started/consts/types.ts

@@ -13,3 +13,10 @@ export interface OnboardingUseCase {
  steps: IOnboardingStep[];
  Demo: React.ComponentType<any>;
 }
+
+export enum OnboardingNodeEnum {


🎯 suggestion: ‏‏Would prefer to reuse an existing enum that making another one

Could you point me to the right enum, I did not find a proper one.

Now when I am looking at the name I find it misleading, as we do not pass a node but a route. I will update it. i think OnboardingWorkflowRouteEnum would be more appropriate but naming in not the easy part 😅

apps/web/src/pages/get-started/consts/DigestUseCase.const.tsx

antonjoel82 · 2024-02-20T22:00:40Z

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

-  async findBlueprint(idOrIdentifier: string) {
-    if (!this.blueprintOrganizationId) throw new DalException('Blueprint environment id was not found');
-
+  buildIdOrIdentifierQuery(idOrIdentifier: string) {


💭 thought: This seems like something that may be handled better at the service layer rather than making than opening up this query to multiple input types. While this may be a bit of an exaggeration, if we continue with this pattern, we could end up with a single query for 7 different identifier types.

I'd love to hear other opinions on it, but I think this query should be "closed" for modification and instead create a separate query function for the other type of identifier. This allows another function to delegate which query should be used. I also imagine a worst-case scenario that could involve | across an unindexed field resulting in terrible performance for every execution.

Definitely not a blocker, but perhaps a discussion item!‏

Firstly, let me say that I completely agree. I really dislike having to add it here instead of at the application layer. Now, let me explain why it's included here, and if you have any other suggestions, I'd be more than happy to hear them, given the reasons mentioned above.

Here's why it's included here:

We want to provide users of those endpoints, starting with the blueprint and now the template itself, the ability to fetch by the identifier in cases where our user might not be aware of the _id.

The main reason is the validation we perform on the query payload build. To construct a proper payload, we need to determine whether it is a MongoDB ID or not.

But you got me thinking 👀, let me try something out.

Done! :)
let me know what you think, and btw thanks for questioning the implementation ♥️

antonjoel82 · 2024-02-20T22:04:22Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    segment.track('[Get Started] Click Create Notification Template', {
+      templateIdentifier: blueprintIdentifier,
+      location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,
+    });


Not sure I understand why we'd switch the [ ] to the end.

Also, this seems like something that could be handled with a ternary:

Suggested change

segment.track('[Get Started] Click Create Notification Template', {

templateIdentifier: blueprintIdentifier,

location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,

});

const action = node ? 'Click Create Notification Template' : 'Click Create Workflow';

segment.track(`[Get Started] ${action}`, {

templateIdentifier: blueprintIdentifier,

location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,

});

antonjoel82 · 2024-02-22T16:52:13Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    segment.track('[Get Started] Click Create Notification Template', {
+      templateIdentifier: blueprintIdentifier,
+      location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,
+    });


When we don't have a node, it should be undefined, correct? In that event, it wouldn't appear anyway on the tracking event

antonjoel82 · 2024-02-22T16:54:50Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    },
+  });
+
+  const openNewTab = (url) => window.open(url, '_blank', 'noreferrer noopener');


Suggested change

const openNewTab = (url) => window.open(url, '_blank', 'noreferrer noopener');

const openNewTab = (url: string) => window.open(url, '_blank', 'noreferrer noopener');

And this function should be extracted outside of the component for reusability and to avoid re-computing it unnecessarily every time

antonjoel82 · 2024-02-22T16:58:42Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+  return <Link onClick={() => handleOpenWorkflowClick()}>{children}</Link>;
+}
+
+function buildUrl(workflowData, node?: OnboardingWorkflowRouteEnum) {


💣 chore: ‏Please add types to every param

antonjoel82 · 2024-02-22T17:04:02Z

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

+    return this.mapEntity(item);
+  }
+
+  async findBlueprintByTriggerIdentifier(identifier: string) {


👏 praise: Nice, well done!‏

antonjoel82 · 2024-02-22T17:08:31Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+import { useSegment } from '../../../components/providers/SegmentProvider';
+import { OnboardingWorkflowRouteEnum } from '../consts/types';
+
+export function OpenWorkflowButton({


🎯 suggestion: This component and file are quite overloaded with functionality. I'd suggest we refactor it to improve readability and testability‏

…g-workflow-node

LetItRock · 2024-02-18T22:13:20Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    segment.track('[Get Started] Click Create Notification Template', {
+      templateIdentifier: blueprintIdentifier,
+      location: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED,
+    });


duplicated call to the above

LetItRock · 2024-02-22T16:53:12Z

apps/web/src/pages/get-started/components/OpenWorkflowButton.tsx

+    const blueprintData = await getBlueprint();
+
+    if (!blueprintData) {
+      return;
+    }
+
+    try {
+      const workflowIdentifier = blueprintData.name.toLowerCase();
+      const workflowData = await getTemplateById(workflowIdentifier);
+      if (workflowData) {
+        const url = buildUrl(workflowData, node);
+        openNewTab(url);
+      }
+    } catch (error) {
+      createTemplateFromBlueprint({
+        blueprint: blueprintData,
+        params: { __source: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED },
+      });
+    }


I think that we should extract this logic to a reusable hook and do all of this under the react query mutation, like:

const useCreateTemplateFromBlueprint = (options) => { const { mutate: createTemplateFromBlueprint, ...rest } = useMutation(() => { // all logic should be placed here }, { ...options, onSuccess: ..., onError: ..., }); return { createTemplateFromBlueprint, } }

antonjoel82 · 2024-02-22T17:48:17Z

Per discussion with @LetItRock, we will merge this for now, and handle the discussion items above in a separate PR

djabarovgeorge added 3 commits February 17, 2024 15:28

feat(api,dal): create fetch by id or identifier

34cfc2b

feat(web): add route to a node

a60ccdb

refactor(web): polish after merge

3f5bdf2

github-actions bot added @novu/api @novu/web @novu/dal labels Feb 17, 2024

djabarovgeorge mentioned this pull request Feb 17, 2024

[Use Case Onboarding Project] - Open onboarding workflow node. #5206

Closed

djabarovgeorge commented Feb 19, 2024

View reviewed changes

djabarovgeorge mentioned this pull request Feb 19, 2024

Nv 3352 - Onboarding Analytics #5212

Merged

djabarovgeorge requested a review from antonjoel82 February 20, 2024 16:41

antonjoel82 reviewed Feb 20, 2024

View reviewed changes

refactor: after pr comments

669aace

antonjoel82 approved these changes Feb 22, 2024

View reviewed changes

Merge branch 'usecase-onboarding-project' into nv-3510-open-onboardin…

266afe7

…g-workflow-node

LetItRock reviewed Feb 22, 2024

View reviewed changes

antonjoel82 merged commit ac591ab into usecase-onboarding-project Feb 22, 2024
16 of 24 checks passed

antonjoel82 deleted the nv-3510-open-onboarding-workflow-node branch February 22, 2024 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nv 3510 - Open onboarding workflow node #5207

Nv 3510 - Open onboarding workflow node #5207

djabarovgeorge commented Feb 17, 2024

linear bot commented Feb 17, 2024

djabarovgeorge Feb 19, 2024 •

edited

Loading

antonjoel82 Feb 20, 2024

djabarovgeorge Feb 21, 2024

antonjoel82 Feb 22, 2024

antonjoel82 left a comment

antonjoel82 Feb 20, 2024

djabarovgeorge Feb 21, 2024

antonjoel82 Feb 20, 2024

djabarovgeorge Feb 21, 2024

djabarovgeorge Feb 21, 2024

djabarovgeorge Feb 21, 2024

antonjoel82 Feb 20, 2024

antonjoel82 Feb 22, 2024

antonjoel82 Feb 22, 2024

antonjoel82 Feb 22, 2024

antonjoel82 Feb 22, 2024

antonjoel82 Feb 22, 2024

LetItRock Feb 18, 2024

LetItRock Feb 22, 2024

antonjoel82 commented Feb 22, 2024

	const openNewTab = (url) => window.open(url, '_blank', 'noreferrer noopener');
	const openNewTab = (url: string) => window.open(url, '_blank', 'noreferrer noopener');

Nv 3510 - Open onboarding workflow node #5207

Nv 3510 - Open onboarding workflow node #5207

Conversation

djabarovgeorge commented Feb 17, 2024

What change does this PR introduce?

Why was this change needed?

Other information (Screenshots)

linear bot commented Feb 17, 2024

djabarovgeorge Feb 19, 2024 • edited Loading

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

antonjoel82 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

antonjoel82 commented Feb 22, 2024

djabarovgeorge Feb 19, 2024 •

edited

Loading