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.

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

chore(web): refactor onboarding create workflow from blueprint logic #5233

Conversation

LetItRock
Copy link
Contributor

What change does this PR introduce?

Onboarding Improvements: refactor the logic that creates the workflow from the blueprint.

Why was this change needed?

Other information (Screenshots)

Screen.Recording.2024-02-23.at.23.26.25.mov

@LetItRock LetItRock self-assigned this Feb 23, 2024
apps/web/src/hooks/useCreateWorkflowFromBlueprint.ts Outdated Show resolved Hide resolved
Comment on lines 20 to 23
const workflowIdentifier = slugify(blueprintName, {
lower: true,
strict: true,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

create workflow identifier from the name

strict: true,
});

return await getTemplateById(workflowIdentifier);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

return the workflow if exist, otherwise error will be thrown

Comment on lines 27 to 30
return await createTemplateFromBlueprint({
blueprint: { ...blueprintData, name: blueprintName },
params: { __source: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED },
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

when the workflow doesn't exist, create a new one based on the blueprint

Copy link
Contributor

Choose a reason for hiding this comment

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

❔ question: What should happen if this fails?‏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if this fails, then the mutation will fail and in the result, we will show the error toast

Comment on lines 13 to 25
function buildWorkflowEditorUrl(workflow: INotificationTemplate, step?: OnboardingWorkflowRouteEnum) {
const workflowRoute = parseUrl(ROUTES.WORKFLOWS_EDIT_TEMPLATEID, { templateId: workflow._id! });
if (step === OnboardingWorkflowRouteEnum.TEST_WORKFLOW) {
return `${window.location.origin}${workflowRoute}/${OnboardingWorkflowRouteEnum.TEST_WORKFLOW}`;
}

const stepUuid = workflow.steps.find((el) => el.name?.toLowerCase() === step?.toLowerCase())?.uuid;
if (step && stepUuid) {
return `${window.location.origin}${workflowRoute}/${step}/${stepUuid}`;
}

return `${window.location.origin}${workflowRoute}`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function builds the URL to the Workflow Editor. When:

  • step is TEST_WORKFLOW the URL point to the Test Workflow page with sidebar opened: http://localhost:4200/workflows/edit/65d91b93b0bb8c7b5e68a9ab/test-workflow
  • if the workflow has the step redirect him to the step editor: http://localhost:4200/workflows/edit/65d91b93b0bb8c7b5e68a9ab/in-app/65d91b93b0bb8c7b5e68a9ab
  • otherwise redirect to the Workflow Editor: http://localhost:4200/workflows/edit/65d91b93b0bb8c7b5e68a9ab

@antonjoel82 antonjoel82 self-requested a review February 23, 2024 22:41
Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the fix and refactor! I'll test once you have the doc available.

Side note: while I appreciate the PR comments, I'd much rather see those in code. They add more value there IMO, especially if code needs clarification to its purpose or reasoning behind it. PR comments are great too, but I think should be reserved for reserved for temporary notes related to the review or dev process since they'll be gone in a few days.

apps/web/src/api/notification-templates.ts Show resolved Hide resolved
apps/web/src/hooks/useCreateWorkflowFromBlueprint.ts Outdated Show resolved Hide resolved
Comment on lines 27 to 30
return await createTemplateFromBlueprint({
blueprint: { ...blueprintData, name: blueprintName },
params: { __source: TemplateCreationSourceEnum.ONBOARDING_GET_STARTED },
});
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ question: What should happen if this fails?‏

apps/web/src/hooks/useCreateWorkflowFromBlueprint.ts Outdated Show resolved Hide resolved
apps/web/src/hooks/useCreateWorkflowFromBlueprint.ts Outdated Show resolved Hide resolved
@LetItRock
Copy link
Contributor Author

LetItRock commented Feb 26, 2024

Side note: while I appreciate the PR comments, I'd much rather see those in code. They add more value there IMO, especially if code needs clarification to its purpose or reasoning behind it. PR comments are great too, but I think should be reserved for reserved for temporary notes related to the review or dev process since they'll be gone in a few days.

@antonjoel82 The PR comments have a different purpose and this is something that we established as a team. We don't have a practice of putting the comments in the code, but we can discuss that topic on the tech summit if you would like to bring it.

@LetItRock LetItRock merged commit 9dfc413 into usecase-onboarding-project Feb 26, 2024
17 checks passed
@LetItRock LetItRock deleted the onboarding-create-workflow-from-blueprints branch February 26, 2024 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants