Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Consolidate email templates #2071

Open
ojeytonwilliams opened this issue Dec 8, 2022 · 4 comments
Open

Consolidate email templates #2071

ojeytonwilliams opened this issue Dec 8, 2022 · 4 comments
Labels
API Email Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running. Help Wanted Extra attention is needed

Comments

@ojeytonwilliams
Copy link
Contributor

Currently the text of the emails that Chapter sends is embedded directly in the controllers. If we move them into their own file this both removes clutter from the controllers and makes it easier to audit the text and ensure that it has a consistent voice.

server/src/email-templates.ts seems like a good enough place for these helper functions for now.

@ojeytonwilliams ojeytonwilliams added Help Wanted Extra attention is needed API Email Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running. labels Dec 8, 2022
@UrielOfir
Copy link

Hi :)
I would like to assign myself for this issue.
I didn't understand what did you mean that "emails sending is embedded directly in the controllers".
Did you mean to the code in this file-
https://github.com/freeCodeCamp/chapter/blob/08cb5b009187e7ce7530350365b881c37daa8c3e/server/src/controllers/Users/resolver.ts
line 60?

@ojeytonwilliams
Copy link
Contributor Author

Hi @UrielOfir. What I was trying to say was that we should not put the email content directly in the resolvers that handle the user's request. For example

const emailSubject = `Instance role changed`;
const emailContent = `Hello, ${user.name}.<br />
Your instance role has been changed to ${newRole}.`;
await mailerService.sendEmail({
emailList: [user.email],
subject: emailSubject,
htmlEmail: emailContent,
});

bloats the resolver logic. If it read like this

    emailTemplates.sendInstanceRoleChanged({ name: user.name, newRole })

it would be a lot cleaner.

@Chirag2193
Copy link
Contributor

Chirag2193 commented Apr 30, 2023

This issue seems to have been merged under above PR #2319
We ok to close this?

@allella
Copy link
Contributor

allella commented Apr 30, 2023

It looks like the constants were moved out of the revolvers in PR #2319, but @ojeytonwilliams 's example above seemed to go further by moving the mailerService.sendEmail out of the resolver.

Oliver, is there more to go done here?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API Email Good First Issue This issue is beginner friendly! It shouldn't take much experience to get up and running. Help Wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants