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

refactor: migrate checkbox validator to typescript #1225

Merged
merged 17 commits into from
Mar 8, 2021

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Feb 23, 2021

Problem

Part of #7

Solution

Improvements

  • Migrates checkbox validation to Typescript

Tests

  • Create two required checkbox fields. Hide one of them behind logic. Check that normal submission using frontend succeeds for both visible and hidden field.
  • Check that following validation works when submitting programatically
  • Answer array cannot be empty for required field
  • Answer array cannot have less than the specified minimum number of options
  • Answer array cannot have more than the specified maximum number of options
  • Answer array cannot have duplicate responses from fieldoption
  • Answer array cannot have answers which are not in the fieldoption
  • If 'others' is enabled, the others response cannot be empty (i.e. answer in answerArray cannot be 'Others: ')
  • Answer array can have two identical 'Others' responses as long as one of them is a valid fieldoption, apart from enabling others

Unit test coverage

Checkbox validation
    Required or optional
      ✓ should disallow empty submission if checkbox is required
      ✓ should allow empty submission if checkbox is optional
    Validation of field options
      ✓ should disallow responses submitted for hidden fields 
      ✓ should allow a valid option to be selected
      ✓ should allow multiple valid options to be selected
      ✓ should disallow answers not in fieldOptions
      ✓ should disallow duplicate answers 
      ✓ should allow self-configured others options in field options
      ✓ should allow Others option to be submitted if field is configured for Others 
      ✓ should disallow Others option to be submitted if field is not configured for Others
      ✓ should disallow multiple Others option to be submitted if field is configured for Others
      ✓ should disallow Others option to be submitted with blank answer if field is configured for Others
      ✓ should allow submission without Others option even if field is configured for Others 
    Selection limits
      ✓ should disallow more answers than customMax if selection limits are configured 
      ✓ should disallow fewer answers than customMin if selection limits are configured 
      ✓ should allow more answers than customMax if selection limits are not configured
      ✓ should allow fewer answers than customMin if selection limits are not configured
      ✓ should disallow more answers than customMax, and fewer answers than customMin, if selection limits are configured 

@tshuli tshuli changed the base branch from develop to refactor/attachment-validator February 23, 2021 06:04
@tshuli tshuli marked this pull request as ready for review February 24, 2021 07:39
Copy link
Contributor

@liangyuanruo liangyuanruo left a comment

Choose a reason for hiding this comment

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

Missing tests

@liangyuanruo liangyuanruo dismissed their stale review February 24, 2021 08:53

Existing test coverage appears sufficient.

@karrui karrui requested a review from liangyuanruo March 2, 2021 03:32
@tshuli tshuli force-pushed the refactor/attachment-validator branch from afd7d79 to 77c3959 Compare March 3, 2021 02:57
@tshuli tshuli force-pushed the refactor/checkbox-validator branch from 3a79417 to 45664c6 Compare March 3, 2021 03:08
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

could you double check that the implementation of othersRadioButton validation is correct?

also, we disallow saving of forms with duplicate checkbox field options on the frontend, and we invalidate responses with duplicates at the point of submission. should we add the corresponding model validation for checkbox fields as well?

Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

if I'm right about the implementation of othersRadioButton, we definitely need test coverage on that edge case. it's scary that none of our many layers of tests caught it.

@tshuli tshuli force-pushed the refactor/checkbox-validator branch from 45664c6 to 0aa3529 Compare March 3, 2021 08:24
@tshuli
Copy link
Contributor Author

tshuli commented Mar 3, 2021

@mantariksh comments addressed, for re-review

@tshuli tshuli requested a review from mantariksh March 3, 2021 08:58
@tshuli tshuli force-pushed the refactor/attachment-validator branch 2 times, most recently from b32cb7a to 9c44655 Compare March 4, 2021 02:28
@tshuli tshuli force-pushed the refactor/checkbox-validator branch from c949d50 to 7b5b629 Compare March 4, 2021 02:51
@tshuli
Copy link
Contributor Author

tshuli commented Mar 4, 2021

@mantariksh comments addressed, for re-review

@tshuli tshuli requested a review from mantariksh March 4, 2021 02:58
@tshuli tshuli force-pushed the refactor/checkbox-validator branch from 7b5b629 to 4d1f566 Compare March 4, 2021 03:52
@tshuli tshuli changed the base branch from refactor/attachment-validator to develop March 4, 2021 03:53
@tshuli tshuli force-pushed the refactor/checkbox-validator branch from 4d1f566 to 738a9a2 Compare March 4, 2021 03:54
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

approving first as the code looks good. great job on thinking about all the edge cases and covering them in the tests!

@tshuli tshuli force-pushed the refactor/checkbox-validator branch from 92727b1 to 2a1e4a6 Compare March 8, 2021 03:28
@tshuli tshuli merged commit 7f68e9c into develop Mar 8, 2021
@tshuli tshuli mentioned this pull request Mar 10, 2021
@karrui karrui deleted the refactor/checkbox-validator branch April 21, 2021 02:16
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

3 participants