-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement Skeleton Email Service #6
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,8 @@ | |||
from app.services.interfaces.email_service_provider import IEmailServiceProvider |
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.
i think we should move the interfaces folder directly under the app directory. Let me know if yall agree.
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.
yeah, I don't mind, the nesting can be confusing.
from app.services.email.email_service import EmailService | ||
from app.services.email.email_service_provider import AmazonSESEmailProvider | ||
|
||
router = APIRouter() |
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.
router = APIRouter() | |
router = APIRouter( | |
prefix='/email', | |
tags=["email"] # for autogenerated OpenAPI docs | |
) |
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.
Let's do this instead of doing it in server.py
load_dotenv() | ||
app = FastAPI() | ||
|
||
app.include_router(email.router, tags=["email"], prefix="/email") |
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.
As mentioned, let's try to keep the prefix inside the same router file.
def __init__(self, aws_access_key: str, aws_secret_key: str): | ||
pass | ||
|
||
def send_email(self, recipient: str, subject: str, body_html: str, body_text: str) -> dict: |
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.
We should make a separate object for this, the number of parameters is getting large. Could you also give an example of body_text
being inserted into body_html
? Is this about templating?
def send_email(self, to: str, subject: str, body: str) -> dict: | ||
pass | ||
|
||
def send_welcome_email(self, recipient: str, user_name: str) -> dict: |
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.
Add a TODO to convert recipient
and user_name
to a User object.
def send_password_reset_email(self, recipient: str, reset_link: str) -> dict: | ||
pass | ||
|
||
def send_notification_email(self, recipient: str, notification_text: str) -> dict: |
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.
here too
def send_notification_email(self, recipient: str, notification_text: str) -> dict: | ||
pass | ||
|
||
def _helper_method_example(self): |
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.
what's this for? If it's not needed, then we should remove this.
@@ -0,0 +1,8 @@ | |||
from app.services.interfaces.email_service_provider import IEmailServiceProvider |
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.
yeah, I don't mind, the nesting can be confusing.
return EmailService(email_provider) | ||
|
||
@router.post("/send-test-email/") | ||
async def send_welcome_email(recipient: str, user_name: str, email_service: IEmailService = Depends(get_email_service)): |
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.
async def send_welcome_email(recipient: str, user_name: str, email_service: IEmailService = Depends(get_email_service)): | |
async def send_welcome_email(recipient: str, user_name: str, email_service: Annotated[IEmailService, Depends(get_email_service)]): |
I think it's recommended to use Annotated, at least for code deduplication it makes sense. Don't need to spend too much time if it doesn't work though.
@@ -0,0 +1,25 @@ | |||
from abc import ABC, abstractmethod | |||
|
|||
class IEmailServiceProvider(ABC): |
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.
Thanks for adding this, I think this is a good idea if we decide to swap out Email providers or decide to mock the provider.
Notion ticket link
Setup Basic Email System
Implementation description
Setup a quick skeleton for email service with interfaces and classes defined. Will be implementing actual functionality in next pr. Created a test endpoint to ensure that implementation of skeleton classes is done properly just need to add actual logic.
Steps to test
What should reviewers focus on?
Checklist