-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add support for sending ad hoc batch emails #234
Conversation
We can slice by different groups: All users with an account Users that haven't submitted an application Per Application Branch: Users that have submitted that application branch Per Confirmation Branch: Users that have not submitted their confirmation Users that have submitted their confirmation Closes #193, based on #195 by @mjkaufer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looked over the code. Still need to actually test it out on my local environment
server/routes/api/settings.ts
Outdated
}); | ||
} | ||
}); | ||
|
||
settingsRoutes.route("/email_content/:type/rendered") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is literally the same function code. Could we simplify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, looks like you don't actually use :type for anything so it works with the method removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meant more that we might want to make this a function that we pass to those two event handlers instead of repeating twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind you did fix this
server/common.ts
Outdated
export function sanitize(input: string): string { | ||
if (typeof input !== "string") { | ||
export async function sendBatchMailAsync(mail: IMailObject[]): Promise<void> { | ||
await sendgrid.send(mail); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just return the promise here instead of awaiting it.
Also why does this function exist if it's just a rename of the sendgrid method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I'm just copying the previously existing method (but adding a batch mode). Maybe this was done to allow other email methods in the future?
client/admin.html
Outdated
@@ -412,6 +413,42 @@ <h4><code>config.json</code> options</h4> | |||
</div> | |||
</form> | |||
</section> | |||
<section id="emails"> | |||
<h2>Batch Emails - BE CAREFUL</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tweaked title
function generateFilter(branchFilter: HTMLInputElement, statusFilter: HTMLInputElement) { | ||
let filter: any = {}; | ||
if (branchFilter.value !== "*" && branchFilter.value !== "na") { | ||
let [, type, branchName] = branchFilter.value.match(/^(application|confirmation)-(.*)$/)!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of destructuring 👍
client/js/admin.ts
Outdated
let [, type ] = batchEmailBranchFilterSelect.value.match(/^(application|confirmation)-(.*)$/)!; | ||
// Only confirmation branches have no-submission option since confirmation is manually assigned | ||
if (type === "confirmation") { | ||
let noSubmission = new Option('Have not submitted', 'no-submission'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single quotes aren't consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
client/js/admin.ts
Outdated
let noSubmission = new Option('Have not submitted', 'no-submission'); | ||
batchEmailStatusFilterSelect.add(noSubmission); | ||
} | ||
let submitted = new Option('Submitted', 'submitted'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
client/js/admin.ts
Outdated
let emailBranchFilter = document.getElementById("email-branch-filter") as HTMLInputElement; | ||
let emailStatusFilter = document.getElementById("email-status-filter") as HTMLInputElement; | ||
let sendEmailButton = document.getElementById("sendEmail") as HTMLButtonElement; | ||
let batchEmailSubject = document.getElementById('batch-email-subject') as HTMLInputElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single quotes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
client/js/admin.ts
Outdated
}).then(checkStatus).then(parseJSON).then((result: string) => { | ||
console.log(result); | ||
sendEmailButton.disabled = false; | ||
sweetAlert("Dank!", "Successfully sent out your batch email!", "success"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this say how many emails got sent out so we know if it actually worked? Kinda like the current pre-confirm emails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Hey y'all! A deployment of this PR can be found here: |
@petschekr I did some testing locally but please test it also 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Tested working locally in addition to on the deployed PR instance. Just two minor things:
client/admin.html
Outdated
@@ -36,6 +36,7 @@ <h1 class="center">Admin Panel</h1> | |||
<a href="#users" class="btn">Users</a> | |||
<a href="#applicants" class="btn">Applicants</a> | |||
<a href="#settings" class="btn">Settings</a> | |||
<a href="#emails" class="btn">Emails</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to after Applicants and before Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
client/admin.html
Outdated
<li><strong>\{{confirmation.<code>question-name</code>}}</strong>: Prints the user's response to the confirmation question with the specified name from <code>questions.json</code>.</li> | ||
<li><strong>\{{reimbursementAmount}}</strong>: A string representing how much a user should be reimbursed for.</li> | ||
</ul> | ||
<button id="sendEmail">Send Email</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be centered by wrapping in <div class="row center"></div>
like the structure in the other tabs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Please bump to 2.3.0
* Add support for sending adhoc batch emails We can slice by different groups: All users with an account Users that haven't submitted an application Per Application Branch: Users that have submitted that application branch Per Confirmation Branch: Users that have not submitted their confirmation Users that have submitted their confirmation Closes #193, based on #195 by @mjkaufer. * Updates * Show branch type in dropdown * HTML tweaks * Bump version * Use batch email to speed up acceptance emails
We can slice by different groups:
Per Application Branch:
Per Confirmation Branch:
Closes #190
Closes #193
Based on #195 by @mjkaufer.