-
Notifications
You must be signed in to change notification settings - Fork 351
Email the requester when a UI-launched run finishes #6869
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
Open
ayoubdiourin7
wants to merge
13
commits into
mozilla:master
Choose a base branch
from
ayoubdiourin7:notify-run-completed
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
9da27bd
Email the requester when a UI-launched run finishes
ayoubdiourin7 752b32e
Simplify run status wording in emails
ayoubdiourin7 571ba62
Remove input details from run completion emails
ayoubdiourin7 8ac8352
Move notification email configuration into app settings
ayoubdiourin7 a3341bb
Simplify notification recipient selection
ayoubdiourin7 f861f03
add a timeout
ayoubdiourin7 2de8ca9
Merge remote-tracking branch 'upstream/master' into notify-run-completed
ayoubdiourin7 87a177b
Move requester emails to the run completed event handler
ayoubdiourin7 914b09b
Remove unnecessary future annotations import
ayoubdiourin7 839d796
Include run and bug IDs in completion emails
ayoubdiourin7 3b314e2
Use an HTML template for run completion emails
ayoubdiourin7 4b03f3a
Move requester check into the event handler
ayoubdiourin7 2a05123
Rename UI URL setting to hackbot_ui_url for consistency
ayoubdiourin7 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| """Email the person who requested a run when it reaches a terminal state. | ||
|
|
||
| A run launched from the UI carries its requester's email. Once the run has | ||
| succeeded, failed or timed out, that address gets a short note with the | ||
| outcome and a link to the run page, so nobody has to keep a tab open. Runs | ||
| with no requester (triggered by webhooks) are skipped. | ||
|
|
||
| This module only composes and delivers the message. It keeps no state, so | ||
| calling it once per run is the caller's job. Delivery is best-effort: a | ||
| failed send is logged, never raised. Only the requester is addressed; this | ||
| is a personal ping, not a report. | ||
| """ | ||
|
|
||
| import asyncio | ||
| import logging | ||
| from pathlib import Path | ||
| from string import Template | ||
|
|
||
| import sendgrid | ||
| from sendgrid.helpers.mail import From, HtmlContent, Mail, Subject, To | ||
|
|
||
| from app.config import settings | ||
| from app.database.models import Run | ||
|
|
||
| log = logging.getLogger(__name__) | ||
|
|
||
| # A stalled SendGrid call must not hold finalization open. | ||
| _SEND_TIMEOUT_SECONDS = 10 | ||
|
|
||
| _TEMPLATES = Path(__file__).parent / "templates" | ||
| _HTML_TEMPLATE = Template((_TEMPLATES / "run_completed.html").read_text()) | ||
|
|
||
|
|
||
| def run_url(run_id: str) -> str: | ||
| return f"{settings.hackbot_ui_url.rstrip('/')}/runs/{run_id}" | ||
|
|
||
|
|
||
| def build_message(run: Run) -> tuple[str, str]: | ||
| """Compose the subject and HTML body of the notice for ``run``.""" | ||
| outcome = run.status.replace("_", " ") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would mention the first part of the run ID, similar to the UI. If there is a bug id, I would mention it here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated in839d796 |
||
| label = f"{run.agent} run {str(run.run_id)[:8]}" | ||
| bug_id = run.inputs.get("bug_id") | ||
| if bug_id is not None: | ||
| label += f" for bug {bug_id}" | ||
| subject = f"[Hackbot] {label} {outcome}" | ||
|
|
||
| values = {"label": label, "outcome": outcome, "url": run_url(str(run.run_id))} | ||
| html_body = _HTML_TEMPLATE.substitute(values) | ||
| return subject, html_body | ||
|
|
||
|
|
||
| def _recipient(run: Run) -> str: | ||
| return settings.notification_override_email.strip() or run.requested_by | ||
|
|
||
|
|
||
| def _send_sync(recipient: str, subject: str, html_body: str) -> int: | ||
| message = Mail( | ||
| From(settings.notification_sender), | ||
| To(recipient), | ||
| Subject(subject), | ||
| html_content=HtmlContent(html_body), | ||
| ) | ||
| client = sendgrid.SendGridAPIClient(api_key=settings.sendgrid_api_key) | ||
| # The SendGrid wrapper has no timeout option; its HTTP client does. | ||
| client.client.timeout = _SEND_TIMEOUT_SECONDS | ||
| response = client.send(message=message) | ||
| return response.status_code | ||
|
|
||
|
|
||
| async def notify_requester(run: Run) -> bool: | ||
| """Mail the run's requester about its terminal state. Returns whether it sent.""" | ||
| recipient = _recipient(run) | ||
| subject, html_body = build_message(run) | ||
| try: | ||
| status_code = await asyncio.to_thread(_send_sync, recipient, subject, html_body) | ||
| except Exception: | ||
| log.exception("Failed to notify %s about run %s", recipient, run.run_id) | ||
| return False | ||
| log.info( | ||
| "Notified %s about run %s (%s): SendGrid %s", | ||
| recipient, | ||
| run.run_id, | ||
| run.status, | ||
| status_code, | ||
| ) | ||
| return True | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| <!doctype html> | ||
| <html> | ||
| <body> | ||
| <p>Your <strong>$label</strong> has <strong>$outcome</strong>.</p> | ||
| <p><a href="$url">Open the run</a></p> | ||
| </body> | ||
| </html> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| """Tests for the run-completion email to the requester (app/notifications.py).""" | ||
|
|
||
| import uuid | ||
| from dataclasses import dataclass, field | ||
|
|
||
| import pytest | ||
| from app import notifications | ||
| from app.config import settings | ||
| from app.notifications import build_message, notify_requester | ||
| from app.schemas import RunStatus | ||
|
|
||
|
|
||
| @dataclass | ||
| class _FakeRun: | ||
| run_id: uuid.UUID = field(default_factory=uuid.uuid4) | ||
| agent: str = "bug-fix" | ||
| status: str = RunStatus.succeeded.value | ||
| requested_by: str | None = "someone@mozilla.com" | ||
| inputs: dict = field(default_factory=dict) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def sent(monkeypatch): | ||
| """Capture outgoing mail instead of hitting SendGrid.""" | ||
| calls = [] | ||
|
|
||
| def fake_send(recipient, subject, html_body): | ||
| calls.append((recipient, subject, html_body)) | ||
| return 202 | ||
|
|
||
| monkeypatch.setattr(notifications, "_send_sync", fake_send) | ||
| monkeypatch.setattr(settings, "sendgrid_api_key", "sg-test") | ||
| monkeypatch.setattr(settings, "notification_sender", "hackbot@mozilla.com") | ||
| monkeypatch.setattr(settings, "notification_override_email", "") | ||
| return calls | ||
|
|
||
|
|
||
| def test_build_message_links_to_run_page(monkeypatch): | ||
| monkeypatch.setattr(settings, "hackbot_ui_url", "https://ui.example/") | ||
| run = _FakeRun(status=RunStatus.timed_out.value) | ||
| subject, html_body = build_message(run) | ||
| url = f"https://ui.example/runs/{run.run_id}" | ||
| assert subject == f"[Hackbot] bug-fix run {str(run.run_id)[:8]} timed out" | ||
| assert f"bug-fix run {str(run.run_id)[:8]}" in html_body | ||
| assert f'<a href="{url}">' in html_body | ||
| assert "<strong>timed out</strong>" in html_body | ||
|
|
||
|
|
||
| async def test_sends_to_requester(sent): | ||
| run = _FakeRun() | ||
| assert await notify_requester(run) is True | ||
| assert len(sent) == 1 | ||
| recipient, subject, _ = sent[0] | ||
| assert recipient == "someone@mozilla.com" | ||
| assert subject == f"[Hackbot] bug-fix run {str(run.run_id)[:8]} succeeded" | ||
|
|
||
|
|
||
| def test_build_message_includes_bug_id(): | ||
| run = _FakeRun( | ||
| run_id=uuid.UUID("ab603010-c278-4d55-bc29-f89463f78906"), | ||
| inputs={"bug_id": 123456}, | ||
| ) | ||
| subject, html_body = build_message(run) | ||
| assert subject == "[Hackbot] bug-fix run ab603010 for bug 123456 succeeded" | ||
| assert "bug-fix run ab603010 for bug 123456" in html_body | ||
|
|
||
|
|
||
| async def test_override_email_replaces_recipient(sent, monkeypatch): | ||
| monkeypatch.setattr(settings, "notification_override_email", "dev@example.com") | ||
| assert await notify_requester(_FakeRun()) is True | ||
| assert sent[0][0] == "dev@example.com" | ||
|
|
||
|
|
||
| async def test_send_failure_is_logged_not_raised(monkeypatch, caplog): | ||
| monkeypatch.setattr(settings, "sendgrid_api_key", "sg-test") | ||
| monkeypatch.setattr(settings, "notification_sender", "hackbot@mozilla.com") | ||
|
|
||
| def boom(*_a): | ||
| raise RuntimeError("sendgrid down") | ||
|
|
||
| monkeypatch.setattr(notifications, "_send_sync", boom) | ||
| assert await notify_requester(_FakeRun()) is False | ||
| assert "Failed to notify" in caplog.text |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why we need
notification_override_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.
It’s for testing. It redirects emails to a test inbox so we can verify that notifications work correctly.