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): frontend field locking #7091

Merged
merged 7 commits into from
Feb 21, 2024
Merged

feat(mrf): frontend field locking #7091

merged 7 commits into from
Feb 21, 2024

Conversation

justynoh
Copy link
Contributor

@justynoh justynoh commented Feb 19, 2024

Problem

We want to implement field locking for MRF. This PR does this for frontend locking. Backend locking validation will be implemented in a separate PR.

Closes FRM-1639

Solution

Admin-side

  • Update workflow builder frontend
    • For active state (EditStepBlock), add a new component, QuestionsBlock which contains the multiselect dropdown for admins to choose questions to edit.
    • Also, update inactive state (InactiveStepBlock) with new section to display field labels for selected questions to be edited by each step.
  • Add edit key to
    • workflow step schemas in shared/*,
    • backend types, and
    • API middlewares for workflow settings, which is where the workflow object still gets PATCHed 😢.

Public-side

  • Frontend disabling of fields. There are two things that need to be done.
    • Visually, the fields are in a disabled state.
      • A new augmentation augmentWithWorkflowDisabling is added, which disables the field only if the field is in the workflow step, and it's an input field (i.e. not header, paragraph or image). This uses the disabled key in the FormFieldDto type.
      • Some field UI updates had to be done to support this, as not all fields fully supported the disabled key. In particular, phone numbers and the table field were updated.
    • Required validation should be turned off for these fields.
      • Achieved through similar means as before, using the disableRequiredValidation key in the FieldFactory component. However this time instead of checking if the form is MRF and disabling the validation universally, we implement a field-level check using the same util as augmentWithWorkflowDisabling so that the logic is always consistent.

Breaking Changes

  • No - this PR is backwards compatible

Before & After Screenshots

image

image

image

image

image

Tests

Deploy Notes

New scripts:

  • 20240214_mrf-workflow-field-locking : This DB migration script adds the edit key for all workflow steps, which allows users to choose the fields each respondent is allowed to edit. To preserve existing behavior, all respondents must be allowed to edit all fields. Therefore, a copy of the form fields (excl non-input fields section, statement and image) are copied into every edit key.

Copy link

linear bot commented Feb 19, 2024

@justynoh justynoh requested a review from KenLSM February 19, 2024 09:12
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 nits / questions

() =>
formFields
.map(augmentWithMyInfo)
.map(augmentWithWorkflowDisabling.bind(this, workflowStep)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Noob question: why there's a need to bind here? Didn't see any usage of this in the inner function calls of augmentWithWorkflowDisabling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 17083cf

Actually here i was a bit annoyed at how i have to give a variable name haha. Basically the idea is I want a partially-applied function, with only the first argument provided. There are three ways of doing it

  1. Write the function with two parameters, and create an anonymous function inline (this is the way i did it in the updated commit, above)
// Define
const augmentWithWorkflowDisabling = (workflowStep, formFields) => ...

// Call
formFields.map((fields) => augmentWithWorkflowDisabling(workflowStep, fields))

The advantage is this is all very nice and maintainable, we're used to functions with multiple parameters, but is annoying because i shouldn't really need to define a new variable and think about it's name and what it means just to pass it in to a function and never do anything else interesting with it.

  1. Write the function with two parameters, and bind it to create a partially applied function (this is the way I did it previously)
// Define
const augmentWithWorkflowDisabling = (workflowStep, formFields) => ...

// Call
formFields.map(augmentWithWorkflowDisabling.bind(this, workflowStep))

In this case, this is nice because I've removed the need to define a new variable, but now i include this random reference to ".bind" which no one really understands, and references a context "this" which doesn't exist (it's the global module context, which is a bit weird). Also not ideal

  1. Curry the function, and partially apply it as usual (my preferred way, but not typical coding pattern in FormSG)
// Define
const augmentWithWorkflowDisabling = (workflowStep) => (formFields) => ...

// Call
formFields.map(augmentWithWorkflowDisabling(workflowStep))

This is very nice when used in this scenario, solves all the problems and is easy to read. But, using curried functions aren't very typical in the codebase, and will look weird if used in other places where partial application is not necessary (e.g. if someone else down the line needs to use augmentWithWorkflowDisabling but has both workflowStep and formFields available in the surrounding context, the call signature becomes augmentWithWorkflowDisabling(workflowStep)(formFields), which can look confusing in a sea of typically-uncurried functions).

In the end I think just going with 1 makes the most sense and is most consistent with the rest of the codebase, even though i like 3 the most hahaha

Copy link
Contributor

Choose a reason for hiding this comment

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

https://ramdajs.com/docs/#curry helps for 3. But ramda isn't a library that FormSG uses.

I notice that lodash also supports placeholders in curried functions. But this can be an exercise for the next time.

I've seen 1/3, but never 2 😆.

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 i have to write .curry then that feels a bit like, it defeats the purpose of currying anyway though. HAHAHA

Copy link
Contributor Author

@justynoh justynoh Feb 21, 2024

Choose a reason for hiding this comment

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

re: ramda, What?? what is this javascript magic that allows you to write f(1)(2,3) or f(1,2)(3) tf 🤣

@justynoh justynoh merged commit 1f567a8 into develop Feb 21, 2024
24 checks passed
@justynoh justynoh deleted the feat/field-locking branch February 21, 2024 01:45
@justynoh justynoh mentioned this pull request Feb 21, 2024
30 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.

None yet

2 participants