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: use validator's isEmail for validating email domains #386

Merged
merged 7 commits into from
Oct 1, 2020

Conversation

jia1
Copy link
Member

@jia1 jia1 commented Sep 29, 2020

Problem

Currently, email domain validation is done via regex. We would like to use validator.isEmail to validate email domains. In order to use isEmail, we will prepend email domains with a string i.e. "bob".

Closes #54

Interesting things to note

I put in a lot of thought before finally deciding on "bob". I wanted something short and not nonsensical like abc.

I received these linting errors:

/Users/jiayee/FormSG/src/app/models/field/emailField.ts
  7:32  warning  Missing return type on function  @typescript-eslint/explicit-module-boundary-types

/Users/jiayee/FormSG/src/public/modules/forms/admin/directives/validate-email-domain-from-text.directive.js
  3:1  error  Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

As such, I had to do the following extra stuff:

  1. const EmailFieldSchema: Schema<IEmailFieldSchema> = ... instead of leaving it alone (originally const EmailFieldSchema = ...
  2. const { validateEmailDomains } = require('shared/util/email-domain-validation') instead of import { ... } from '...'.

I also found out that src/app/models/agency.server.model.ts validates email domains by match: [/.+\..+/, 'Please fill a valid email domain.'],.

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.

Lgtm!

@jia1
Copy link
Member Author

jia1 commented Sep 29, 2020

Wait ah, I broke the build because of wrong import path. Will fix it on my end first...

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm! The second error is because you used import ... as ... syntax in a Javascript file. Our app only supports import syntax in Typescript files

src/app/models/field/emailField.ts Outdated Show resolved Hide resolved
@jia1
Copy link
Member Author

jia1 commented Sep 29, 2020

lgtm! The second error is because you used import ... as ... syntax in a Javascript file. Our app only supports import syntax in Typescript files

Ah! I see

@jia1 jia1 marked this pull request as ready for review September 29, 2020 14:11
@jia1 jia1 self-assigned this Sep 29, 2020
@jia1 jia1 added tech debt contribute free for contributors to pick up P3 labels Sep 29, 2020
@karrui karrui removed the P3 label Sep 29, 2020
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

Update import to be relative before merging; tests are currently passing probably because they do not reach the invalid directive

Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

lgtm

@karrui karrui merged commit e59ea2d into develop Oct 1, 2020
@karrui karrui deleted the refactor/use-validator-is-email branch October 1, 2020 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribute free for contributors to pick up tech debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email domain restriction
3 participants