-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: Send mail action failed #21
Conversation
Reviewer's Guide by SourceryThis pull request fixes a bug in the Sequence diagram for the fixed send_mail action flowsequenceDiagram
participant Form
participant SendMailAction
participant DjangoMail
Form->>SendMailAction: execute(form, request)
SendMailAction->>SendMailAction: get_parameter(sendemail_recipients)
SendMailAction->>SendMailAction: split recipients string
SendMailAction->>SendMailAction: render templates
alt no recipients
SendMailAction->>DjangoMail: mail_admins(subject, message, html_message)
else has recipients
SendMailAction->>DjangoMail: send_mail(subject, message, from_mail, recipients, html_message)
end
Class diagram for the updated SendMailAction classclassDiagram
class SendMailAction {
+execute(form, request)
-get_parameter(form, param_name)
-subject: str
-from_mail: str
}
note for SendMailAction "Fixed recipients handling and template rendering"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @fsbraun - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding email address validation before attempting to send. This would provide better error messages and fail faster than waiting for send_mail to raise an exception.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
b36e989
to
816f3c1
Compare
@sourcery-ai review |
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.
Hey @fsbraun - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
recipients = (self.get_parameter(form, "sendemail_recipients") or []) | ||
from django.core.mail import mail_admins, send_mail | ||
|
||
recipients = (self.get_parameter(form, "sendemail_recipients") or "").split() |
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.
issue (bug_risk): Using whitespace splitting for email addresses is risky. Consider using a more robust delimiter like commas.
Email addresses might contain spaces (e.g., in quoted strings). This could lead to invalid recipient lists.
plugin.instance = plugin_instance | ||
|
||
# Simulate form submission | ||
with patch("django.core.mail.send_mail") as mock_send_mail: |
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.
issue (code-quality): Extract duplicate code into method (extract-duplicate-method
)
This PR fixes the send_mail_action and adds a test for it
Summary by Sourcery
Fix the send mail action to handle missing request headers and add tests for the action.
Bug Fixes:
Tests: