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 be field validation #7718

Merged
merged 48 commits into from
Oct 15, 2024
Merged

feat: mrf be field validation #7718

merged 48 commits into from
Oct 15, 2024

Conversation

kevin9foong
Copy link
Contributor

@kevin9foong kevin9foong commented Sep 25, 2024

Problem

BE validation is currently not performed for MRF submissions. This allows for invalid responses to be saved for MRF submissions.

For example:

  • duplicate options selected for checkboxes
image

Closes FRM-1688

Solution

Add validator for V3 responses and use this V3 validator in the MRF middleware which uses V3 responses.

Breaking Changes

  • No - this PR is backwards compatible

Tests

Regression test:

  • Create an MRF form with all types of basic fields, try a combination of different field settings.
  • Create a table with dropdown field and text field col.
  • Fill in with valid values and ensure that we can still submit all fields without validation error.

Setup for next few tests:

  • Set all fields in earlier MRF form to optional to prepare for the next few test cases.
  • Go to web console (inspect -> find the XHR request -> copy as cURL), save this in a text editor to create invalidate submissions

Checkbox field validation - no duplicates with others:

  • Create a option called Others: A
  • Enabled others
  • Submit the form with checkbox response selecting the option Others: A and a custom A value in others
  • Should return a validation failure, which matches Storage/email mode validation.

Table field validation:

  • From the cURL command in setup, add row to table field type value despite addMoreRows disabled, it should reject.
  • From the cURL command in setup, add row to table field type value, enable addMoreRows, it should accept.
  • From the cURL command in setup, add more rows than maxRowsAllowed to table field type value with addMoreRows enabled, it should reject.
  • Make text field required, set the textField value in row to '' empty string, it should reject.
  • Change dropdown to invalid value, it should reject.
  • Set text field and dropdown to not required, send empty string values for both, it should accept.

Short text length enforced:

  • From the cURL command in setup, change the value of 'textfield' to less than the minimum text length set and submit, it should reject.

Children field not allowed in MRF:

  • From the cURL command in setup, change the fieldType of any to children and submit, it should reject.

Decimal field range is enforced:

  • From the cURL command in setup, change the value of decimal fieldType to beyond the valid range and submit, it should reject.

Short text length is enforced:

  • From the cURL command in setup, change the value of short fieldType to beyond the valid range and submit, it should reject.

Valid country/region only:

  • From the cURL command in setup, change the value of country_region fieldType to invalid value and submit, it should reject.

NRIC must be valid:

  • From the cURL command in setup, change the value of nric fieldType to invalid nric value and submit, it should reject.

Invalid field types not allowed:

  • From the cURL command in setup, change the value of fieldType to invalid field type eg, abc and submit, it should reject.

@kevin9foong kevin9foong changed the title Feat/mrf be validation feat: mrf be field validation Sep 25, 2024
@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Sep 25, 2024

Datadog Report

Branch report: feat/mrf-be-validation
Commit report: 2190435
Test service: formsg

✅ 0 Failed, 1085 Passed, 0 Skipped, 1m 46.57s Total duration (5m 51.94s time saved)

@kevin9foong kevin9foong force-pushed the feat/mrf-be-validation branch 2 times, most recently from c11efad to 651ac16 Compare October 7, 2024 04:45
@kevin9foong kevin9foong force-pushed the feat/mrf-be-validation branch 3 times, most recently from 9c0453a to 5f76bcc Compare October 7, 2024 11:38
@kevin9foong
Copy link
Contributor Author

Pending UT.

The core code is ready for review.

@kevin9foong kevin9foong marked this pull request as ready for review October 7, 2024 11:53
@@ -513,8 +577,12 @@ export const validateMultirespondentSubmission = async (
).map(() => undefined)
})
.andThen(() =>
// TODO: Step 4: Validate each field content with each field's validator rules individually.
ok(undefined),
validateMrfFieldResponses({
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're potentially blocking submissions here and we don't have full confidence that we're making the right blocks. Would recommend us to implement this through a "shadow"[1] instead.

Lmk if you have similar concerns on the confidence of the validation implementation.

[1] We log that a validation has failed, instead of block. Once we have sufficient information and confidence, we can officially swap that to a block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, when a validation error for v3 occurs, ValidateFieldErrorV3 will be thrown which will be logged on DD and we can use that for monitoring for the next 1-2 weeks.

@kevin9foong
Copy link
Contributor Author

Written all UT for test coverage:

Field types:
| HeaderResponseV3 > dont need to test since header is not submitted.
| RadioResponseV3
| CheckboxResponseV3
| AttachmentResponseV3
| TableResponseV3
| ChildBirthRecordsResponseV3 > dont need to test since not currently used.
| YesNoResponseV3
| EmailResponseV3
| MobileResponseV3
| NumberResponseV3
| DecimalResponseV3
| ShortTextResponseV3
| LongTextResponseV3
| HomeNoResponseV3
| DropdownResponseV3
| RatingResponseV3
| NricResponseV3
| UenResponseV3
| DateResponseV3
| CountryRegionResponseV3

TC for middleware to invoke validator:

  • assert children field not supported for MRF
  • assert isVisible is correctly detected

@kevin9foong kevin9foong self-assigned this Oct 14, 2024
@@ -103,8 +109,20 @@ export const isPossibleEmailFieldSchema = (
return get(field, 'fieldType') === BasicField.Email
}

export const isVerifiableMobileField = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Good cleanup!

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!

@kevin9foong kevin9foong merged commit 9aa7d6b into develop Oct 15, 2024
18 checks passed
@kevin9foong kevin9foong deleted the feat/mrf-be-validation branch October 15, 2024 10:44
@KenLSM KenLSM mentioned this pull request Oct 20, 2024
43 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