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: Allow empty params in csv file #200

Merged
merged 5 commits into from
May 12, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion backend/src/core/errors/template.errors.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
export class MissingTemplateKeysError extends Error {
public readonly missingKeys: string[]
constructor(missingKeys: string[]) {
super(`Template contains keys [${missingKeys}] that are not present in uploaded recipient list.`)
super(`The attribute(s) { ${missingKeys} } are not present in uploaded recipient list.`)
this.missingKeys = missingKeys
Object.setPrototypeOf(this, new.target.prototype) // restore prototype chain
Error.captureStackTrace(this)
Expand All @@ -22,4 +22,12 @@ export class TemplateError extends Error {
Object.setPrototypeOf(this, new.target.prototype) // restore prototype chain
Error.captureStackTrace(this)
}
}

export class InvalidRecipientError extends Error {
constructor() {
super('There are invalid recipient(s) in the uploaded recipient list.')
Object.setPrototypeOf(this, new.target.prototype) // restore prototype chain
Error.captureStackTrace(this)
}
}
21 changes: 12 additions & 9 deletions backend/src/core/services/template.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,18 +64,21 @@ const parseTemplate = (templateBody: string, params?: { [key: string]: string })
throw new TemplateError(`Invalid characters in param named {{${key}}}. Only alphanumeric characters allowed.`)
}

// add key regardless, note that this is also returned in lowercase
variables.push(key)

// if no params continue with the loop
if (!params) return

// if params provided == attempt to carry out templating
if (params) {
if (dict[key]) {
const templated = dict[key]
tokens.push(templated)
} else {
throw new TemplateError(`Param ${templateObject.c} not found`)
}
if (dict[key]) {
const templated = dict[key]
tokens.push(templated)
return
}

// add key regardless, note that this is also returned in lowercase
variables.push(key)
// recipient key must have param
if (key === 'recipient') throw new TemplateError(`Param ${templateObject.c} not found`)

} else { // I have not found an edge case that trips this yet
logger.error(`Templating error: templateObject.c of ${templateObject} is undefined.`)
Expand Down
8 changes: 6 additions & 2 deletions backend/src/email/routes/email.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
testHydration,
extractS3Key,
} from '@core/services'
import { populateEmailTemplate, upsertEmailTemplate, getEmailStats } from '@email/services'
import { populateEmailTemplate, upsertEmailTemplate, getEmailStats, hasInvalidEmailRecipient } from '@email/services'
import {
uploadStartHandler,
sendCampaign,
Expand All @@ -25,6 +25,7 @@ import {
HydrationError,
RecipientColumnMissing,
TemplateError,
InvalidRecipientError,
} from '@core/errors'
import { isSuperSet } from '@core/utils'
import logger from '@core/logger'
Expand Down Expand Up @@ -257,6 +258,8 @@ const uploadCompleteHandler = async (req: Request, res: Response, next: NextFunc
templateParams: emailTemplate.params,
})

if (hasInvalidEmailRecipient(records)) throw new InvalidRecipientError()

const recipientCount: number = records.length
// START populate template
// TODO: is actually populate message logs
Expand All @@ -272,7 +275,8 @@ const uploadCompleteHandler = async (req: Request, res: Response, next: NextFunc
throw err
}
} catch (err) {
if (err instanceof RecipientColumnMissing || err instanceof MissingTemplateKeysError) {
if (err instanceof RecipientColumnMissing || err instanceof MissingTemplateKeysError
|| err instanceof InvalidRecipientError) {
return res.status(400).json({ message: err.message })
}
return next(err)
Expand Down
11 changes: 7 additions & 4 deletions backend/src/email/services/email.service.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { chunk } from 'lodash'
import validator from 'validator'
import { Campaign, JobQueue } from '@core/models'
import { EmailMessage, EmailTemplate, EmailOp } from '@email/models'
import logger from '@core/logger'
import { CampaignStats } from '@core/interfaces'
import { getStatsFromTable } from '@core/services'



const upsertEmailTemplate = async ({ subject, body, campaignId }: {subject: string; body: string; campaignId: number}): Promise<EmailTemplate> => {
let transaction
try {
Expand Down Expand Up @@ -97,6 +96,10 @@ const getEmailStats = async (campaignId: number): Promise<CampaignStats> => {

const stats = await getStatsFromTable(EmailMessage, campaignId)
return { error: stats.error, unsent: stats.unsent, sent: stats.sent, status: job.status }
}
}

const hasInvalidEmailRecipient = (records: MessageBulkInsertInterface[]): boolean => {
return records.some((record) => !validator.isEmail(record.recipient))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is the best approach performance wise. I could do this while parsing csv or when we are test hydrating, but it will be messy.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine for now.

}

export { populateEmailTemplate, upsertEmailTemplate, getEmailStats }
export { populateEmailTemplate, upsertEmailTemplate, getEmailStats, hasInvalidEmailRecipient }
8 changes: 6 additions & 2 deletions backend/src/sms/routes/sms.routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
testHydration,
extractS3Key,
} from '@core/services'
import { populateSmsTemplate, upsertSmsTemplate, getSmsStats } from '@sms/services'
import { populateSmsTemplate, upsertSmsTemplate, getSmsStats, hasInvalidSmsRecipient } from '@sms/services'
import {
uploadStartHandler,
sendCampaign,
Expand All @@ -23,6 +23,7 @@ import {
HydrationError,
RecipientColumnMissing,
TemplateError,
InvalidRecipientError,
} from '@core/errors'
import { isSuperSet } from '@core/utils'
import { storeCredentials, getCampaignDetails, previewFirstMessage } from '@sms/middlewares'
Expand Down Expand Up @@ -260,6 +261,8 @@ const uploadCompleteHandler = async (req: Request, res: Response, next: NextFunc
templateParams: smsTemplate.params,
})

if (hasInvalidSmsRecipient(records)) throw new InvalidRecipientError()

const recipientCount: number = records.length
// START populate template
// TODO: is actually populate message logs
Expand All @@ -275,7 +278,8 @@ const uploadCompleteHandler = async (req: Request, res: Response, next: NextFunc
throw err
}
} catch (err) {
if (err instanceof RecipientColumnMissing || err instanceof MissingTemplateKeysError) {
if (err instanceof RecipientColumnMissing || err instanceof MissingTemplateKeysError
|| err instanceof InvalidRecipientError) {
return res.status(400).json({ message: err.message })
}
return next(err)
Expand Down
7 changes: 6 additions & 1 deletion backend/src/sms/services/sms.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,4 +96,9 @@ const getSmsStats = async (campaignId: number): Promise<CampaignStats> => {
return { error: stats.error, unsent: stats.unsent, sent: stats.sent, status: job.status }
}

export { populateSmsTemplate, upsertSmsTemplate, getSmsStats }
const hasInvalidSmsRecipient = (records: MessageBulkInsertInterface[]): boolean => {
const re = /^[0-9+]+$/
Copy link
Contributor Author

@lshaowei18 lshaowei18 May 6, 2020

Choose a reason for hiding this comment

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

How strict should I be with this regex?

Copy link
Contributor

@jeantanzj jeantanzj May 11, 2020

Choose a reason for hiding this comment

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

If a + exists, it should be at the start of the string.

return records.some((record) => !record.recipient.match(re))
Copy link
Contributor

Choose a reason for hiding this comment

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

re.test(record.recipient) should be faster. match returns an array of matched strings

}

export { populateSmsTemplate, upsertSmsTemplate, getSmsStats, hasInvalidSmsRecipient }
21 changes: 12 additions & 9 deletions worker/src/core/services/template.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,21 @@ const parseTemplate = (templateBody: string, params?: { [key: string]: string })
throw new Error(`Invalid characters in param named {{${key}}}. Only alphanumeric characters allowed.`)
}

// add key regardless, note that this is also returned in lowercase
variables.push(key)

// if no params continue with the loop
if (!params) return

// if params provided == attempt to carry out templating
if (params) {
if (dict[key]) {
const templated = dict[key]
tokens.push(templated)
} else {
throw new Error(`Param ${templateObject.c} not found`)
}
if (dict[key]) {
const templated = dict[key]
tokens.push(templated)
return
}

// add key regardless, note that this is also returned in lowercase
variables.push(key)
// recipient key must have param
if (key === 'recipient') throw new Error(`Param ${templateObject.c} not found`)

} else { // I have not found an edge case that trips this yet
logger.error(`Templating error: templateObject.c of ${templateObject} is undefined.`)
Expand Down