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

feat(reports): allowing the email mutator to update recipients #27851

Merged
merged 2 commits into from
May 29, 2024
Merged

feat(reports): allowing the email mutator to update recipients #27851

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14e2307
allowing the email mutator to update recipients
SkinnyPigeon Apr 2, 2024
9fd99a8
updated recipients check
SkinnyPigeon Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions superset/utils/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,8 @@ def send_email_smtp( # pylint: disable=invalid-name,too-many-arguments,too-many
msg_mutator = config["EMAIL_HEADER_MUTATOR"]
# the base notification returns the message without any editing.
new_msg = msg_mutator(msg, **(header_data or {}))
if new_msg["To"].split(", ") != recipients:
Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain whether this should be handled here for the specific bespoke use case or we should change the EMAIL_HEADER_MUTATOR (or similar) to return recipients as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @john-bodley, thanks for taking a look. Ideally we'd love to have control over the actual email object including the dry_run flag but I thought this would be a simple step in that direction that would suit our immediate needs.

Changing the return values for this would probably cause a bit of a headache for many existing users. How would you suggest we continue?

recipients = new_msg["To"].split(", ")
send_mime_email(smtp_mail_from, recipients, new_msg, config, dryrun=dryrun)


Expand Down
30 changes: 30 additions & 0 deletions tests/integration_tests/email_tests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,36 @@ def mutator(msg, **kwargs):
assert msg.get_payload()[-1].get_payload() == mimeapp.get_payload()
app.config["EMAIL_HEADER_MUTATOR"] = base_email_mutator

@mock.patch("superset.utils.core.send_mime_email")
def test_send_smtp_with_email_mutator_changing_recipients(self, mock_send_mime):
attachment = tempfile.NamedTemporaryFile()
attachment.write(b"attachment")
attachment.seek(0)

# putting this into a variable so that we can reset after the test
base_email_mutator = app.config["EMAIL_HEADER_MUTATOR"]

def mutator(msg, **kwargs):
msg.replace_header("To", "mutated")
return msg

app.config["EMAIL_HEADER_MUTATOR"] = mutator
utils.send_email_smtp(
"to", "subject", "content", app.config, files=[attachment.name]
)
assert mock_send_mime.called
call_args = mock_send_mime.call_args[0]
logger.debug(call_args)
assert call_args[0] == app.config["SMTP_MAIL_FROM"]
assert call_args[1] == ["mutated"]
msg = call_args[2]
assert msg["Subject"] == "subject"
assert msg["From"] == app.config["SMTP_MAIL_FROM"]
assert len(msg.get_payload()) == 2
mimeapp = MIMEApplication("attachment")
assert msg.get_payload()[-1].get_payload() == mimeapp.get_payload()
app.config["EMAIL_HEADER_MUTATOR"] = base_email_mutator

@mock.patch("superset.utils.core.send_mime_email")
def test_send_smtp_data(self, mock_send_mime):
utils.send_email_smtp(
Expand Down
Loading