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

Email support #228

Merged
merged 2 commits into from
Jun 19, 2020
Merged

Email support #228

merged 2 commits into from
Jun 19, 2020

Conversation

wasade
Copy link
Member

@wasade wasade commented Jun 19, 2020

An object to support sending jinja2 templated emails. This is provides partial support for biocore/microsetta-admin#17

@wasade wasade requested a review from dhakim87 June 19, 2020 00:57
if cls.connection is None or cls.connection.noop()[0] == 421:
return True
else:
return False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either this method, or connection.noop() has a really unfortunate name. It's not clear to me how this if statement can be connecting or reconnecting via side effects from a function called no op :).

Maybe this could be renamed to requires_reconnect() or is_active() with appropriate changes to booleans returned

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh, wait, I'm misunderstanding what the comment is attached to :D

Copy link
Member Author

Choose a reason for hiding this comment

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

.noop() is part of smtplib.SMTP and issues a NOOP command server side. This will report a timeout if the connection has died.

return connection

@classmethod
def connect(cls):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this synchronous?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, per slack though, I think we defer async for now

message.attach(second)

cls.connect()
cls.connection.send_message(message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Coming from a mobile development background, where connections unexpectedly drop all the freaking time, I think this pattern is slightly wrong, though I doubt we'll ever notice while the server is colocated. If the connect call succeeds, but then the connection immediately drops out, what happens when you call send_message?

Copy link
Member Author

Choose a reason for hiding this comment

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

fair. happy to wrap and catch?

@wasade wasade merged commit de4bde2 into biocore:master Jun 19, 2020
@wasade wasade deleted the emailing branch December 13, 2021 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants