feat: add range validation to number field

[LeonardYam]

Problem

Closes #378

Solution

Breaking Changes

• Yes - this PR contains breaking changes
  • Schema change in shared/types/field/numberField.ts

Changes by file

• Updated the existing NumberValidationOptions schema in shared/types/field/numberField.ts to include range validation
• Added components, validation and updated schema for range validation in the form builder drawer located in frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/EditFieldDrawer/edit-fieldtype/EditNumber/EditNumber.tsx
• Updated field creation in frontend/src/features/admin-form/create/builder-and-design/utils/fieldCreation.ts to reflect the updated schema
• Updated frontend validation for the NumberField in frontend/src/utils/fieldValidation.ts
• Updated mongoose schema in src/app/models/field/numberField.ts
• Updated backend validators in src/app/utils/field-validation/validators/numberValidator.ts

Before & After Screenshots

BEFORE:

AFTER:

Tests

Manual tests for range validation:

• E2E: Range validation works as expected.
  • Select Range of values allowed under Field restriction.
    • Submit the form without entering a minimum or maximum value. This should display an error.
    • Submit the form with input 0 for minimum or maximum value. This should display an error.
    • Submit the form with an invalid range where minimum >= maximum value. This should display an error.
    • Submit the form with only one of minimum and maximum value specified. This should work and create a number field as expected.
    • Submit the form with a valid range where minimum < maximum. This should work and create a number field as expected.

Regression tests (for EditNumber.tsx)

• E2E: Try to create a number field with no selected validation. This should work and create a number field as expected.
• E2E: Existing number validation still works as expected.
  • Select Number of characters allowed under Field restriction.
    • Submit the form without choosing length validation. This should display an error.
  • Select Min/Max/Exact for length validation.
    • Submit the form without an input for Number of characters. This should display an error.
    • Submit the form with 0 as the input. This should display an error.
    • Submit the form with a number > 0 as the input. This should work and create a number field as expected.

Concerns

• Check if the naming and structuring of the new variables and schema is acceptable
• Check if the EditNumber component requires refactoring into smaller components
• Check for errors in the typing
• Check if there needs to be design changes to the front-end components (in particular, we currently allow number fields with Character Length or Number Range selected but no custom values to be created)
• Check if the error messages are acceptable
• Updating existing tests to use new schema

Deploy notes

New scripts:

• migrate-number-field-schema.js: This script migrates ValidationOptions to ValidationOptions.LengthValidationOptions for all existing number form fields.

[justynoh]

Hi @LeonardYam! Thanks for the contribution. I'll be working with you to get this PR merged in as soon as possible 😄

While I review this PR, could I trouble you to open another PR to clean up the old virtuals in the numberField.ts file? If they are retained, they will expose incorrect properties after this PR is merged in (as they are set based on the assumption of character length validation, not number range validation). To keep our codebase clean, can I ask you to take up this (small) issue as well? I've added the concrete items to be done in the issue description. Once that is ready, feel free to tag me and I'll merge it in, and then you can rebase this PR onto develop. Meanwhile, I'll take a look at this PR.

Thanks again!

[LeonardYam]

Hi @justynoh, I've just rebased the pull request. Let me know if there are any issues! 😄

[justynoh]

Thanks for this! I've briefly reviewed the backend, while our designer is on leave. I'll get to the frontend when our designer is back later this week, and let you know what changes need to be made.

[justynoh]

Additionally, we would need your help to write a mongo script to perform a migration of existing form documents from the old to the new schema. You can look in the /scripts folder for examples of how our migrations are structured. In essence, each migration exists in its own folder prefixed by the date in yyyymmdd format. What the script for this migration might look like is:

• Retrieve all forms with number field validation. Retrieve the count.
• Update all retrieved forms to convert the number field validation from the old schema to the new schema.
• Ensure that the updated count is the same as the initial count.

[LeonardYam]

Hi @justynoh, apologies for the delay! I have gone through the previous code review and applied the suggestions accordingly.

Could I double-check on what tests I should perform to verify that the script works as expected?

[justynoh]

Hi @LeonardYam, just a quick update from my end. I'll take a look as soon as I can, but this period has been quite busy for me! I'd seek your understanding to give me some time to review it when I next can, which will likely come next week. Don't worry, we haven't forgotten about your contribution! And apologies for the delay 😅

[LeonardYam]

Hi @justynoh, no worries!

I've just added the mongodb script, which ended up being very verbose. However to my understanding, there aren't many alternatives since arrayFilters currently do not work with aggregation pipelines. I've updated the original post with the details about the script!

[justynoh]

This PR is starting to shape up nicely! A number of comments from my end, as well as some updated copy. The migration script also looks good, thanks for writing it up!

The only thing I haven't reviewed is the updated EditNumber.tsx frontend component. The reason for this is that we have updated screens for your reference. If you could help implement them, then on my next review I'll look at 1) the new number field creation UI and EditNumber.tsx and 2) the updates based on my comments.

The screens and some other considerations are coming up in my next comment, for your reference.

[justynoh]

New UI screens

Initial state

Dropdown options

Length validation (note that the dropdown for "Minimum/Maximum/Exact" is no longer clearable)

Range validation

Validating the range itself

I realized that we currently allow some strange combinations of values for the range. Some considerations for what range values we should allow during field creation (I've also given the appropriate error messages to be displayed for each error case, which should be used exactly as written):

1. We should allow the following possibilities in terms of existence: a) Both min and max specified, b) Only min specified, c) Only max specified. We should disallow field creation if both min and max are not specified (Error message: "Please enter range values") - in your current implementation, the field is saveable.
2. We should have the following restrictions in terms of value: a) If both min and max are specified, it must be that min is strictly less than max (Error message: "Minimum must be less than maximum"), b) since number fields can only contain non-negative numbers, in all cases, both the min and max values should not be 0 (Error messages: "Minimum cannot be 0", "Maximum cannot be 0").

Thanks again for contributing on this!

[LeonardYam]

Hi @justynoh, I've just went through the code review and applied the initial round of fixes.

Here's a quick look at the current UI screens: (updated copy of error messages, validation of empty and 0 values etc.)

range.mp4

Other than the comments above, I also have some questions about the new UI screens.

I think the inclusion of a None option for Field Restriction might be slightly confusing, as clearing the selection would leave an empty input, and we would allow both an empty input and None to represent that no validation is required. It seems that we should either:

1. Remove None and let an empty input represent no validation
2. Keep None as an option but make the selection no longer clearable

I'm leaning towards the first option since this would be more consistent with the current UI, as this pattern is already used in date validation for example.

On a related note, should we also keep the dropdown for length validation clearable? This way, we would be consistent with the character length validation in short answer and long answer validation.

Let me know what would be best!

[justynoh]

Hi Leonard! Thanks for looking at the UI and doing up the new frontend.

For your points, I agree with doing (1). We can remove the "None" option and use an empty input to represent no validation instead.

As for the dropdown for length validation, it should not be clearable. Since we already only display it when the user selects "Length validation", the user has made a conscious choice to select a validation type. Once they pick one type of length validation (Min/Max/Exact), it doesn't make sense to allow them to remove it at that level. If they want to clear the validation, they should press "X" on the validation type dropdown, not the length validation dropdown.

Finally, as a UI nit: we should remove the form labels for "Minimum and/or maximum value" and "Number of characters allowed", above the conditionally rendered inputs. Instead, the spacing between the upper dropdown and the lower inputs should be 0.5rem.

Thanks!

[LeonardYam]

Hi @justynoh, I've just went through the review again and there is only one last part I'm unsure about. Other than that, I've just realized that we actually use the validatorjs library in some files. Would it be better to replace the current range checks with validator.isInt?

Thanks for looking through the lengthy comments 😅 !

[justynoh]

Just a couple more nits, and we should be good to go!

As for using the validatorjs library, I think it's up to you. What you've implemented is fine, and I suspect there will be some overhead for handling null cases when using the library anyway, so it might not really simplify the code that much. I would prefer to just keep it as you've implemented it.

[LeonardYam]

Hi @justynoh, I've just made the relevant changes. Let me know if there are any final changes needed!

On a side note, while I was checking the validity of the back-end validators, I actually had an issue with getting them to run as my IncomingSubmission.responses was always empty on form submit. Most likely, there is a misunderstanding or setup issue on my end but I wanted to check if you could reproduce this issue.

[justynoh]

Hey Leonard,

Overall, the code on this PR looks pretty much good to go! Feel free to address the one comment I gave on your question above.

To answer your question regarding incomingSubmission.responses being empty - since you have encountered this, I would hazard a guess that you're testing on a storage mode form. Since storage mode responses are end-to-end encrypted, we cannot in theory (and therefore, do not in practice), perform backend validation on the form responses or encrypted response payload. This is an issue we are actively addressing but requires some work on our end on the product side before this can be done. In storage mode, the incomingSubmission.responses are used only to handle certain specific types of responses (in particular, email addresses entered in email fields with the "Send email confirmation" toggled on, so that our backend can send the email confirmations to respondents). Therefore, in order to test your backend validators, try using an email mode form instead.

Now, the last piece of this PR before I can approve it would be writing appropriate manual tests for the feature as well as updating the existing test suite to account for the changes.

Concretely, you will definitely have to update the tests in src/app/utils/field-validation/validators/__tests__/number-validation.spec.ts. I don't see any other files off the top of my head, but you can run npm run test:backend to see if there are any relevant backend tests that need to be updated.

Additionally, I would request your help to write manual tests. Generally, we try and write two types of tests - one set to cover new functionality, and another set for regression. Here is a sample of what we might expect to see in the "Tests" section of the PR:

## Tests

New functionality
- [ ] E2E: Range validation works correctly when both minimum and maximum is set.
    - [ ] Create a new form
    - [ ] Create a number field, and for "Field restriction" select "Range validation"
    - [ ] Click "Create" with no value entered for minimum and maximum. An error should be triggered saying a range must be entered.
    - [ ] Enter a value for minimum and maximum, where the minimum is >= maximum. An error should be triggered saying a valid range must be entered.
    - [ ] Enter a value for minimum and maximum, where the minimum is < maximum. Click "Create". The field should be successfully created.
    - [ ] Go to the public form. Enter a value outside the range, and click "Submit form". This should trigger an error on the form field.
    - [ ] Enter a value within the range instead, and click "Submit form". This should be be successfully submitted.
- [ ] BE: Range validator correctly blocks submissions where the submitted value does not pass range validation.
    - [ ] Create an email mode form, with number field with range validation.
    - [ ] On the command line, run the following cURL command: `<curl command with incorrect submission>`
    - [ ] The submission should be rejected with 400, and the error message "<error message here>".
... (add more if desired) ...

Regression tests
- [ ] E2E: Number length validation still works as expected. Repeat for Min/Max/Exact
    - [ ] Create a number field with length validation. 
    - [ ] Go to the form. Enter a number not satisfying the validation selected.
    - [ ] Click "Submit form". An error should be triggered on the number field.
... (add more if desired) ...

Feel free to let me know once you've updated the tests, and I'll be happy to add on if required, and then approve and merge the PR. Thanks!

[LeonardYam]

Hi @justynoh, you were right - I was indeed using storage mode responses 😅, after changing the mode the backend validators managed to fire.

I just did a quick update on the backend tests, mainly renaming the test names to include length and updating the schemas.

During the update, I came across the test it('should allow number with maximum length left undefined'), and noticed that with the current implementation of our validators, we actually allow selected length validations with no customVal, even though this behaviour is not allowed in the frontend. Is this intentional?

Unfortunately, I've been very busy recently and I would need some time to finish up the PR 😅. Just to confirm, the remaining work would be to:

1. Add unit tests for range validation
2. Write the manual tests
3. Update the existing tests (searching for BasicField.Number shows me some frontend and e2e tests that needs updating).
4. Confirm and implement the expected behaviour for clearing frontend inputs

Am I missing out on any additional tasks?

[justynoh]

For your question regarding validator implementation - I don't think this is intentional. We should implement our validators for this feature in a way that ensures that this is not possible (i.e. if Length, it must have both LengthSelectedValidation and customVal, and if Range, it must have at least one of customMin and customMax). If there are other existing validators that are incorrect, please let us know and we can fix them separately from this PR!

As for the remaining tasks, yes, those seem like the remaining items to me. Thanks for the continuous work on this @LeonardYam! We're excited to get this merged.

[LeonardYam]

Hi @justynoh, I took a look at the validators again and I believe that for our backend validators number-validation.spec.ts and numberValidator.ts, they are only responsible for validating responses and operate on an implicit assumption that the validation options are valid.

Hence, there should not be any changes needed for the current validators. I've instead deleted the tests that are based on response validation on invalid options and re-written them as mentioned below.

For robustness, I've written automated tests for:

1. E2E tests for creation and submission, with valid options and values
2. BE tests for valid/invalid field creation and response submission (separately)
3. FE tests for valid and invalid response submissions

I believe the tests which cannot be automated currently involve testing of error messages on EditNumber.tsx, which I've instead included as manual tests above.

Additionally, I've also added placeholder text to selectedLengthValidation.

Let me know if there's anything more to add!

[justynoh]

lgtm! this is great work, thanks so much for the contribution @LeonardYam! We'll merge it in as soon as we can.

[justynoh]

Closing and re-opening to get tests to run.

[justynoh]

merged develop into this branch to include CI updates!