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

feat(mrf): workflow builder #7072

Merged
merged 10 commits into from
Feb 14, 2024
Merged

feat(mrf): workflow builder #7072

merged 10 commits into from
Feb 14, 2024

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Feb 8, 2024

Problem

We want to build out a workflow builder for MRF with dynamic number of respondents.

Closes FRM-1624

Solution

This implementation reuses many of the components from the logic module but with several significant changes. In particular, a lot of the inputs are removed and replaced with workflow-specific inputs (e.g. step number, respondent emails). Also, the current implementation uses a single "godlike" put endpoint to put the entire workflow array into the backend. This should be granularized so that we have specific creation, update and delete endpoints on a per-workflow-step basis.

Breaking Changes

  • No - this PR is backwards compatible

Before & After Screenshots

BEFORE:
image

AFTER:

Screen.Recording.2024-02-08.at.4.58.16.PM.mov

Tests

Copy link

linear bot commented Feb 8, 2024

@justynoh justynoh marked this pull request as draft February 8, 2024 07:47
@justynoh justynoh requested a review from KenLSM February 8, 2024 16:31
@justynoh justynoh changed the title feat: workflow builder feat(mrf): workflow builder Feb 8, 2024
@justynoh justynoh marked this pull request as ready for review February 9, 2024 02:06
newWorkflowSettings: MultirespondentFormSettings['workflow'],
) => {
return ApiService.patch<FormSettings>(
`${ADMIN_FORM_ENDPOINT}/${formId}/settings`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why is workflow using :formId/settings instead of having it's own PATCH, DEL, PUT :formId/workflow?

Was it to write less boilerplate code?

Copy link
Contributor

Choose a reason for hiding this comment

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

-- saw that you have already noted this on the PR, this will be resolved after we ship? Totally fine by it as we have a good amount of buffer post-release to make refinements like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, in the spirit of making PRs as small as possible to avoid scope creep,, I decided not to do that modification here. This code was already written and I didn't want to touch it too much. Nonetheless, a linear ticket to resolve this has already been opened as FRM-1632!

Copy link
Contributor

Choose a reason for hiding this comment

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

Great division of scope btw! Features are cleanly cut, and isolated.

@KenLSM
Copy link
Contributor

KenLSM commented Feb 13, 2024

Added d2b3327 as we should be swapping these to their own PATCH / PUT / DELETE.

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-9.html#-ts-expect-error-comments:~:text=you%20expect%20a%20fix%20to%20be%20coming%20in%20fairly%20quickly%20and%20you%20just%20need%20a%20quick%20workaround

Copy link
Contributor

@KenLSM KenLSM left a comment

Choose a reason for hiding this comment

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

LGTM! Some minor nits / questions that isn't blocking

@justynoh justynoh merged commit 7497828 into develop Feb 14, 2024
24 checks passed
@justynoh justynoh deleted the feat/mrf-workflow-builder branch February 14, 2024 02:27
@tshuli tshuli mentioned this pull request Feb 14, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants