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.

Sign up for GitHub

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

Jump to bottom

Migrate field validators and associated tests to Typescript #7

Closed
r00dgirl opened this issue Aug 3, 2020 · 5 comments · Fixed by #1475
Closed

Migrate field validators and associated tests to Typescript #7

r00dgirl opened this issue Aug 3, 2020 · 5 comments · Fixed by #1475
Assignees
@tshuli
Labels
contribute free for contributors to pick up P2 planned for next 1-2 months tech debt
Projects
TypeScript server migration Scrum Board
Milestone
typescriptify bac...

Comments

@r00dgirl
Copy link
Contributor

r00dgirl commented Aug 3, 2020 •
edited
Loading

Migrate the field validators in src/app/utils/field-validation to TypeScript.

As these validators affect email submissions directly, take care to keep existing logic and unit tests intact - and expand on them if necessary.
@r00dgirl r00dgirl added the P2 planned for next 1-2 months label Aug 3, 2020
@r00dgirl r00dgirl added this to the typescriptify backend milestone Aug 3, 2020
@karrui
Copy link
Contributor

karrui commented Aug 24, 2020

@jia1's PR on email domain restriction #143 may have some conflicts with the refactor; take note

@r00dgirl r00dgirl mentioned this issue Aug 25, 2020
Closed
13 tasks
@liangyuanruo
Copy link
Contributor

Potential idea - do we want to refactor this away from class-based implementation to a functional implementation instead? the rationale being that validators are best implemented without having to hold state. also, we have good test coverage so it ought to be fairly safe to do so.

@liangyuanruo liangyuanruo added help wanted Extra attention is needed tech debt contribute free for contributors to pick up and removed help wanted Extra attention is needed labels Sep 8, 2020
@liangyuanruo
Copy link
Contributor

liangyuanruo commented Sep 30, 2020 •
edited
Loading

Assigning to myself first to spearhead a sample FP implementation that should simplify what we have today.

@liangyuanruo liangyuanruo mentioned this issue Oct 5, 2020
Merged
liangyuanruo added a commit that referenced this issue Oct 9, 2020
@liangyuanruo
refactor(field-validators): introduce scaffolding & reference impleme…
aea6f86 
…ntations for section, short text & long text fields (#409)

Email submission validators are crucial for preventing spoof submissions, and need to be migrated to TypeScript as part of push towards type safety. The existing implementation is problematic to port over however, due to

* Type consistency
* Misuse of the Inheritance pattern
* Lack of Codebase clarity

Solution

* Composition over Inheritance - Encourage code reuse in order to compose larger validation functions from smaller ones. For example, Table validation can be composed from Text and Number validator functions.

* Scaffolding for further migration - Perform full submission validation for all form field responses, instead of throwing an exception on a short-circuit basis upon encountering the first error that occurs. The hope is to use functional programming utilities so that all responses can be evaluated in order to obtain as much information as possible before rejecting a submission.

Changes:

* refactor: convert field-validation/index.ts and associated tests to typescript
* refactor(field-validators): textFieldValidatorConstructor sectionValidatorConstructor
* fix: only non-empty answers to be validated
* refactor: delete AnswerNotAllowedValidator, TextValidator, unused PlaceholderValidator
* refactor(ValidationOptions): Extract common validation options enum from short/long text fields, and use it in textValidator

Addresses #7
@liangyuanruo
Copy link
Contributor

liangyuanruo commented Nov 16, 2020 •
edited
Loading

Reference implementation has been added in #409 . As usual, please add your name to the validator if you are working on it:

Validator

Tests

For safety of email submissions, these are to be ported from Jasmine to Jest after its corresponding validator has been migrated and running in production for at least a week.

  • Attachment
  • Checkbox
  • Date
  • Decimal
  • Dropdown
  • Email
  • HomeNo
  • Mobile
  • Nric
  • Number
  • Radiobutton
  • Rating
  • Section
  • Table
  • Text

@liangyuanruo liangyuanruo mentioned this issue Nov 16, 2020
Merged
@tshuli
Copy link
Contributor

tshuli commented Nov 16, 2020

self-assigning a couple first to get more chances to practice :)

This was referenced Nov 23, 2020
Merged
Merged
Merged
Merged
@liangyuanruo liangyuanruo mentioned this issue Nov 25, 2020
Open
This was referenced Nov 25, 2020
Closed
Merged
Merged
@tshuli tshuli mentioned this issue Dec 11, 2020
Merged
3 tasks
@liangyuanruo liangyuanruo moved this from To do to In progress in TypeScript server migration Feb 1, 2021
This was referenced Feb 11, 2021
Merged
Merged
This was referenced Feb 18, 2021
Merged
Merged
@tshuli tshuli mentioned this issue Mar 8, 2021
Merged
2 tasks
@tshuli tshuli mentioned this issue Mar 15, 2021
Merged
2 tasks
@liangyuanruo liangyuanruo moved this from In progress to Review in TypeScript server migration Mar 18, 2021
@liangyuanruo liangyuanruo added this to Review in progress in Scrum Board Mar 22, 2021
@liangyuanruo liangyuanruo moved this from Review in progress to In progress in Scrum Board Mar 22, 2021
@liangyuanruo liangyuanruo changed the title Migrate field validators to Typescript Migrate field validators and associated tests to Typescript Mar 22, 2021
@tshuli tshuli mentioned this issue Mar 24, 2021
Merged
@liangyuanruo liangyuanruo mentioned this issue Mar 29, 2021
Merged
TypeScript server migration automation moved this from Review to Done Mar 29, 2021
Scrum Board automation moved this from In progress to Done Mar 29, 2021
KenLSM added a commit that referenced this issue Feb 14, 2024
@KenLSM
Revert "chore(dev): add virus scanner npm install to root install com…
372951c 
…mand (#7…"

This reverts commit d77a7c3.
KenLSM added a commit that referenced this issue Feb 14, 2024
@KenLSM
revert: "chore(dev): add virus scanner npm install to root install co…
c84b979 
…mmand" (#7082)

Revert "chore(dev): add virus scanner npm install to root install command (#7…"

This reverts commit d77a7c3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tshuli tshuli

Labels
contribute free for contributors to pick up P2 planned for next 1-2 months tech debt
Projects
Scrum Board
Done
TypeScript server migration
  
Done
Milestone
typescriptify backend
Development

Successfully merging a pull request may close this issue.

refactor: remove unnecessary field validator JS tests in favour of TS tests opengovsg/FormSG
5 participants
@liangyuanruo @karrui @mantariksh @r00dgirl @tshuli