Make the software-request notification deliverable, async, and observable

[kennedydane]

Why

The request form saved its row and then called send_mail synchronously with fail_silently=False — while nothing anywhere configured an SMTP host. In production that meant Django's fallbacks (unauthenticated localhost:25 as webmaster@localhost) inside a container with no MTA: every submit saved the row and then showed the requester a 500. The synchronous send was also a recorded TODO at the site, waiting on a decision about where failure should surface once it moved off the request cycle.

What

• SMTP settings contract — typed fields on Settings (EMAIL_HOST, port, TLS/SSL mode, credentials, DEFAULT_FROM_EMAIL, EMAIL_TIMEOUT — Django's backend has no socket timeout of its own). Defaults suit smtp-relay.gmail.com in IP-allowlist mode (587 + STARTTLS, no auth). ProductionSettings requires EMAIL_HOST and DEFAULT_FROM_EMAIL: prod hardcodes the SMTP backend, so omitting them can only produce a deployment whose notifications die silently — the same refuse-to-boot shape as ILIFU_ADMIN_EMAILS. TLS+SSL together is refused at boot rather than at Django's send-time check.
• SoftwareRequest.support_notified_at — nullable timestamp; NULL means "support has not been told", pending or given up. Stamped by queryset .update() so the task can't clobber a concurrent triage.
• Async send — requests_app/tasks.py::notify_support_of_request, three retries at 60s. The submit succeeds the moment the row exists; send failure, retry exhaustion and a dead broker (the enqueue is guarded — this caller is a human, not a retrying collector) all log ERROR and leave the stamp NULL. Email content unchanged, including the Keycloak-lookup line.
• Admin queue marker — support not notified (--warn) on queue rows whose stamp is NULL: the failure now surfaces where support already looks. It shows transiently on fresh rows until the worker runs — deliberate, noted in the template.

Notes for review

• The retry-path tests run the task via .apply() under CELERY_TASK_EAGER_PROPAGATES=False (override_settings — celery's config chain reads django.conf:settings live and shadows app.conf writes). With propagation on, trace re-raises Retry before Task.apply's eager re-execution branch runs, so a transient failure is indistinguishable from a permanent one through the view.
• The give-up branch counts attempts itself rather than catching MaxRetriesExceededError: Task.retry(exc=...) re-raises the original exception on exhaustion, so that catch is a branch that never runs.
• Deploy: the worker container must be recreated so it sees the new task module and env vars; prod refuses to boot until EMAIL_HOST and DEFAULT_FROM_EMAIL are set (values expected at deploy time; see .env.example and ANSIBLE_DEPLOY.md).

Verification

Full suite in the containerised Postgres/Redis setup: 1241 passed, 5 skipped, 100% coverage. pre-commit run --all-files clean (including the djlint lint pass on the template change).

🤖 Generated with Claude Code

https://claude.ai/code/session_015fwHvQmMcVNe86kqX9jS2V

[Copilot]

Pull request overview

This PR makes software-request email notifications non-blocking and production-safe by moving SMTP delivery off the request cycle into a Celery task, adding an explicit SMTP settings contract, and exposing delivery failures in the admin queue.

Changes:

• Add typed SMTP configuration to Settings (with production-required EMAIL_HOST + DEFAULT_FROM_EMAIL) and export these into Django settings.
• Introduce SoftwareRequest.support_notified_at plus a Celery task (notify_support_of_request) with retries and error logging to send/stamp asynchronously.
• Surface “support not notified” in the admin request queue and update tests/docs accordingly.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
README.md	Documents new production-required mail relay settings.
ANSIBLE_DEPLOY.md	Adds deploy-time guidance for required SMTP variables.
.env.example	Documents SMTP env vars and their defaults/intent.
catalog/src/ilifu_catalog/settings/base.py	Adds validated SMTP settings fields + exports into Django settings.
catalog/src/ilifu_catalog/settings/prod.py	Requires email_host / default_from_email in prod and hardcodes SMTP backend.
catalog/src/ilifu_catalog/requests_app/models.py	Adds support_notified_at timestamp to track notification delivery.
catalog/src/ilifu_catalog/requests_app/migrations/0002_softwarerequest_support_notified_at.py	Migration adding support_notified_at.
catalog/src/ilifu_catalog/requests_app/views.py	Enqueues notification task after saving the request; guards/logs broker failures.
catalog/src/ilifu_catalog/requests_app/tasks.py	New Celery task to send mail with retries, logging, and stamp update.
catalog/src/ilifu_catalog/software/views_admin.py	Adds support_notified flag to queue row view-model.
catalog/src/ilifu_catalog/templates/software/partials/admin/_queue_card.html	Shows “support not notified” marker when the stamp is still NULL.
catalog/tests/test_settings.py	Tests SMTP settings export, default timeout, TLS/SSL mutual exclusion, and prod requirements.
catalog/tests/test_request_form.py	Updates request-form behavior tests for async notification + retry paths.
catalog/tests/test_admin_screen.py	Tests the admin queue marker behavior for un-notified requests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kennedydane]

Code review

Verdict: ready to merge with fixes. No critical issues. The core mechanics — retry accounting, .update() stamping, settings validation, worker wiring, the no-email-storage invariant — were verified against Celery's own source (app/task.py, app/trace.py) rather than taken on faith, and all hold. ruff and mypy clean.

Verified strengths

• The give-up condition self.request.retries >= self.max_retries fires exactly on the boundary where Task.retry() re-raises the original exception (celery/app/task.py:745-775) — the "catch MaxRetriesExceededError" trap described in the PR notes is real and correctly avoided; call_count == 4 pins it.
• Queryset .update() stamping cannot clobber a concurrent triage's status/admin_note.
• The eager-retry test methodology (.apply() under CELERY_TASK_EAGER_PROPAGATES=False) is correct, not a workaround — trace.py re-raises Retry before Task.apply's eager re-execution branch when propagation is on.
• Settings validators fire in the claimed shapes; prod's required-field reasoning is sound (prod.py hardcodes the SMTP backend).
• Worker wiring: requests_app is in INSTALLED_APPS, autodiscover_tasks() picks up tasks.py, compose services share env_file.

Important

1. No backfill — every pre-existing request shows "support not notified" forever. Migration 0002 leaves the column NULL on rows that were notified by the old synchronous code. A permanently false alarm on the majority of queue rows trains the marker out of admins' attention from day one. Fix: RunPython stamping support_notified_at = created_at for existing rows.
2. The marker squeezes the package name out of the queue row. In the 300px sidebar, badge and username are flex: none, so .admin-queue__name is the only shrinkable item; "support not notified" (~115px in JetBrains Mono at --fs-9-5) ellipsizes the package name on exactly the rows needing attention.
3. The enqueue is still an unbounded blocking call in the request path. No CELERY_BROKER_TRANSPORT_OPTIONS are set, so kombu's redis transport gets no socket_connect_timeout, and the publish retry policy defaults to 3 retries — against a hung (not refused) broker the submit blocks for OS-connect-timeout × retries, the same shape this PR removed from SMTP. _REDIS_SOCKET_OPTIONS (settings/base.py:293) is applied to the cache aliases but not the broker.
4. (Latent) .delay() is not transaction.on_commit-guarded. Correct today under autocommit, but if ATOMIC_REQUESTS ever appears, the task's missing-row branch returns without retrying — silent permanent loss. Deserves at least a # NOTE: pinning the autocommit dependency.

Minor

• Task is not idempotent: a redelivered message double-emails support and re-stamps. Cheap fix: bail if already stamped, and stamp via .filter(pk=..., support_notified_at__isnull=True).
• test_should_leave_support_notified_at_null_when_the_send_fails passes for the wrong reason — under eager propagation the only ERROR record is the view's; anchor the assertion on the message fragment.
• Module docstring's reason for "must never raise" is wrong: the Retry does propagate eagerly; the view's catch is what preserves the contract.
• Stale comment in compose.prod.yaml still citing the deleted synchronous send_mail; CLAUDE.md's Celery section now undercounts the tasks; settings/test.py's "everything explicit" docstring is violated by the eight new email fields (ambient EMAIL_USE_TLS=1 EMAIL_USE_SSL=1 would fail the whole suite at import).
• Fourth copy of the str(settings.SUPPORT_EMAIL) # type: ignore[misc] accessor; the TLS/SSL two-variable trap (implicit-SSL relay needs EMAIL_USE_SSL=1 and EMAIL_USE_TLS=0) is absent from ANSIBLE_DEPLOY.md's table.

Recommendations

• The only recovery from a permanently failed notification is a shell; a "resend" control on the triage form would close the loop the marker opens — or record it in the README's "Open" list.
• requests_app/tasks.py has the silent-failure profile of the 100%-gate modules and appears to be at 100% already — adding it to domain_module_names is free today.

Fixes for all of the above are being pushed to this branch.

🤖 Generated with Claude Code

[kennedydane]

All review findings addressed, one per commit:

• Important 1 (no backfill) → e10bd61 — migration 0003 stamps support_notified_at = created_at on rows the synchronous code notified; rows with a real send record are untouched; reverse is a deliberate no-op.
• Important 2 (badge crushes the name) → 2641f0f — the marker is now the design's ! glyph (--warn, title/aria-label carry the wording), so the package name keeps the row's shrinkable space.
• Important 3 (unbounded publish) → b009f7e — CELERY_BROKER_TRANSPORT_OPTIONS gets the same 2s socket bounds as the cache aliases, publish retries once instead of kombu's three. Verified safe for the worker: its idle BRPOP wait lives in kombu's epoll hub, not a socket read.
• Important 4 (on_commit) → a8f695c — kept the bare .delay() and pinned the autocommit dependency with a NOTE: at the site. on_commit would move the enqueue past the try/except and dismantle the guarded-enqueue contract, so the trade-off is recorded rather than silently rebalanced.
• Minor: idempotency → a8f695c — the task bails on an already-stamped row and the stamp filters on isnull, so a redelivered message neither re-emails support nor overwrites the first send's timestamp. New test drives the task directly against a stamped row.
• Minor: wrong-reason test, docstring, test style → a8f695c — the send-fails test now names the guard's log message it actually observes; the module docstring states the real never-raise reason; eager_retries is Iterator[None] and the function-local imports are hoisted.
• Minor: SUPPORT_EMAIL duplication → 7ed4aa2 — the accessor and its type: ignore live once in ilifu_catalog.support (a leaf module; the per-screen helpers couldn't be shared without cycles).
• Minor: docs/config → 8363acc — settings/test.py passes the eight email fields explicitly (the ambient EMAIL_USE_TLS=1 EMAIL_USE_SSL=1 import failure is closed), the stale --threads comment is gone, CLAUDE.md counts all three Celery tasks, ANSIBLE_DEPLOY.md documents the SSL/TLS two-variable trap.
• Recommendation: coverage gate → 8363acc — tasks.py joins domain_module_names (matched by basename, so the ingest apply is gated with it; both measure 100% today).
• Recommendation: resend control → 8363acc — recorded as README "Open" item 10 rather than built; it belongs with a broader decision about what triage can trigger.

Verification: full suite in the containerised Postgres/Redis setup (with mandoc): 1244 passed, 5 skipped, 100% coverage, including the new two-file tasks.py gate. pre-commit run clean on every commit.

🤖 Generated with Claude Code