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

feature: add-smtp-mail #204

Closed
wants to merge 2 commits into from

Conversation

sulthonzh
Copy link
Contributor

hello i just created new PR for smtp mail action

Copy link
Collaborator

@rennerocha rennerocha left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.
Including SMTP as an option to send e-mails is a good feature.
I included some first comments in your code. Also, please introduce tests to your code.



class SendSmtpEmail(SendEmail):
smtp_host = "smtp.gmail.com"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be provided by the user as a mandatory setting. Do not set a fixed default.

return kwargs

def send_message(self, message):
if not self.sender:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Required arguments needs to be validated when instantiating the Action (__init__), not when sending the e-mail.

if not self.to:
raise NotConfigured("You must provide the receiver.")

if not self.sender or not self.to:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of your previous validation, this will never be executed.

if not self.sender or not self.to:
return

server = smtplib.SMTP(self.smtp_host, self.smtp_port)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes using smtplib.SMTP is not appropriated and you need to use smtplib.SMTP_SSL. It will be interesting to have an option so the user can define which one to use depending on the needs of the SMTP server being connected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes sense. Using the built-in SMTP method from Scrapy would be a good solution.

@rennerocha
Copy link
Collaborator

This feature is being continued in #345

@rennerocha rennerocha closed this Jun 24, 2022
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.

None yet

3 participants