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

Email SeS implementation #19

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Email SeS implementation #19

wants to merge 2 commits into from

Conversation

Mayank808
Copy link
Collaborator

@Mayank808 Mayank808 commented Dec 30, 2024 •
edited
Loading

Notion ticket link

Setup Email System

NOTE THIS IS A PR for feedback not to merge.
Will be removing email.py route as its simply there right now for testing
will be removing todos and adding comments
will be removing any prints or updating them to logs

Implementation description

  • Implemented Email sending using amazon ses.
  • Create email templates at startup
  • use email templates to send single and bulk emails to users with predefined templates
  • Email templates can use inline CSS need to define a text file with only text for each respective email

Process to create a new Email Template to use:

  1. Create the HTML and Text files for the new template. Use inline CSS and direct hosted links to any images required.
  2. Update backend/app/utilities/ses/ses_templates.json to include template name subject and absolute paths to the above file
  3. Create a new data class Type for the Email template
@dataclass
class NewEmailTemplatData():
    name: str
    date: str
    random: int
  1. Update Email template enum to include the file name of the template defined in amazon ses and the ses_templates.json file
class EmailTemplate(Enum):
   NEWEMAILTEMPLATE = "newemailtemplate"
  1. Use in code to send emails using email service
email_service.send_email(
    template=EmailTemplate.NEWEMAILTEMPLATE,
    content=EmailContent[NewEmailTemplatData](
        recipient=recipient, 
        data=NewEmailTemplatData(name=user_name, date="2021-12-01", random: 20)
    ),
)

Todo

  • Add bulk send email after general flow is good
  • Look into schedule send
  • Clean up logging and comments
  • Remove email.py route
  • Unit Testing
  • File clean up

Steps to test

  1. Simply hit the /email/send-test email route when running the be on local

What should reviewers focus on?

  • Focus on overall design of things and how things are laid out as this pr still needs to be cleaned up.
  • How templates are defined, what folders code blocks should be stored in
  • Are we okay with running the email template check on startup
  • Will be removing email.py route as its simply there right now for testing

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@Mayank808 Mayank808 requested a review from mslwang December 30, 2024 06:25
mslwang
mslwang reviewed Dec 31, 2024
View reviewed changes
Copy link
Collaborator

@mslwang mslwang left a comment

Choose a reason for hiding this comment

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

Great work here! Left some general comments

backend/app/interfaces/email_service.py
formatted_string = json.dumps(class_dict) # Try to convert to a JSON string
except (TypeError, ValueError) as e:
# Handle errors and return a message instead
return f"Error in converting data to JSON: {e}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're planning on using this as the basis of other template data, I think we should propagate the error instead of gracefully failing, especially if this is being sent by email.

backend/app/interfaces/email_service.py


@dataclass
class EmailContent(Generic[T]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: let's call T EmailData or EmailPayload to be more concrete

Cool use of generics in this situation though!

backend/app/interfaces/email_service.py
T = TypeVar("T")


class TemplateData(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you be able to move this to a schema file? Given that routes is using this too, maybe we should treat this the same as a schema.

backend/app/interfaces/email_service.py
date: str


class EmailTemplate(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we name this EmailTemplateType to signal that it's an enum?

backend/app/interfaces/email_service_provider.py
def send_email(
self, recipient: str, subject: str, body_html: str, body_text: str
) -> dict:
def send_email(self, template: EmailTemplate, content: EmailContent) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto template vs templateType, as template could be a full formed template class.

backend/app/services/email/email_service_provider.py
aws_secret_key: str,
region: str,
source_email: str,
is_sandbox: bool = True,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does is_sandbox do? For quick glancers, would you be able to leave a comment describing this option.

backend/app/services/email/email_service_provider.py
try:
self._verify_email(content.recipient)

self.ses_client.get_template(TemplateName=template.value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does this line do right now? Should it be getting a response value?

backend/app/services/email/email_service_provider.py

self.ses_client.get_template(TemplateName=template.value)

template_data = content.data.get_formatted_string()
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Imo you should group this with self._verify_email(content.recipient), I got confused for a second reading the code given that get_template doesn't return anything right now.

backend/app/utilities/ses/ses_init.py
return ""


templates_metadata = load_templates_metadata(TEMPLATES_FILE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this into ensure_ses_templates

backend/app/utilities/ses/ses_init.py
template = {
"TemplateName": template_metadata["TemplateName"],
"SubjectPart": template_metadata["SubjectPart"],
"TextPart": text_part,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: needed when user can't see HTML content, but HTML is default. https://docs.aws.amazon.com/ses/latest/APIReference/API_Template.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mslwang mslwang mslwang left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Mayank808 @mslwang