WPB-25932: Send meeting emails on create/update/link-refresh/delete by blackheaven · Pull Request #5606 · wireapp/wire-server

blackheaven · 2026-10-19T12:04:23Z

https://wearezeta.atlassian.net/browse/WPB-25932

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

🟡 Changes recommended

Active critical and moderate issues remain, including synchronous email-related failures that can alter meeting operation outcomes, incorrect branding configuration indentation, duplicate recipient emails, an unused reply-to setting, and a test compilation error.

8 open issues

_{Review effort: Lite}

blackheaven · 2026-11-09T10:56:54Z

+        mHost <- meetingHostProfile sm
+        for_ mHost $ \(hostName, hostEmail, loc) -> do
+          apiMeeting <- storedMeetingToMeeting domain sm
+          for_ recipients $ \addr -> sendKind addr hostName hostEmail apiMeeting (Just loc)


Already addressed by 6bbe94e: prepareMeetingEmails is fully best-effort returning Maybe — brig lookup failures are isolated in an Async child and collapse to Nothing (warn-logged), so email preparation can never change the outcome of the meeting operation.

blackheaven · 2026-11-09T10:56:52Z

 import Test.Hspec.QuickCheck (prop)
-import Test.QuickCheck (NonNegative, counterexample, getNonNegative, ioProperty, (.&&.), (===), (==>))
-import Text.Email.Parser (unsafeEmailAddress)
+import Test.QuickCheck (NonNegative, arbitrary, counterexample, generate, getNonNegative, ioProperty, (.&&.), (===), (==>))


Addressed by 2ddc4d1 (and previously documented in the 2026-11-06 comment by @blackheaven): unsafeEmailAddress resolves transitively Wire.API.User → Wire.API.User.Identity → Wire.API.User.EmailAddress → Text.Email.Parser; an explicit import is rejected by -Werror -Wunused-imports as redundant (GHC-66111). False positive.

blackheaven · 2026-11-09T10:56:56Z

+    templates <- loadMeetingTemplates (O._templateDir cfg) def (O._from cfg)
+    let meetingBranding = fromMaybe Map.empty (O._branding cfg)


Already addressed by 6bbe94e: meReplyTo flows through Galley/App.hs into EmailSubsystem/Interpreter.hs and is set as the Reply-To header (mailHeaders).

blackheaven · 2026-11-06T10:02:36Z

Addressed all 8 inline findings (commit to follow shortly; currently staged in gdifolco/WPB-25932-meetings-send-emails):

r4490318702 / r4490318805 (email prep can fail the operation): sendMeetingEmails is split into best-effort prepareMeetingEmails (returns Maybe; link-lookup failure is warn-logged and swallowed, Error MeetingError constraint dropped) and dispatchMeetingEmails. Preparation can no longer change the outcome of create/update/refresh/delete.
r4490318748 (no guard before Brig RPC / code lookup): new MeetingSystemConfig.emailsEnabled (isJust (settings.meetings.email)) short-circuits before the brig RPC, license read, and code-store lookup. Cleanup worker passes False.
r4490318926 (cancel email before deletion): cancellations are rendered in the preflight (join_url still live) but dispatched only after deleteLocalConversation succeeds.
r4490318955 (duplicates): dedup moved into dispatchMeetingEmails (now order-preserving via nubOrd), enforced on every lifecycle path; covered by a new test.
r4490318996 (replyTo unused): settings.meetings.email.replyTo now flows through MeetingEmailEnv.meReplyTo -> meetingEmailSubsystemInterpreter -> renderMeetingEmail as a Reply-To header; brig passes Nothing. TemplateSpec asserts the header absent/present.
r4490318895 (configmap branding): nindent 10 -> nindent 12; brand/legal now nest under branding:.
r4490318845 (spec does not compile): false positive as stated — the name resolves through the legitimate re-export chain Wire.API.User -> module Wire.API.User.Identity -> module Wire.API.User.EmailAddress -> module Text.Email.Parser, not via Wire.CodeStore (an exportless header does not re-export its imports). Restoring the explicit import is rejected by -Werror -Wunused-imports (GHC marks it redundant because the imported Name is identical), so it stays out; the chain is documented in the commit message.

Verified: make c gates (wire-subsystems, galley, background-worker) clean under -Werror + hlint + treefmt; wire-subsystems-tests full suite passes (620 examples, 0 failures) including 3 new tests (duplicate recipients, emails unconfigured, best-effort delete).

Copilot

🟡 Changes recommended

Critical and moderate issues remain, including plaintext SMTP password exposure, unlicensed migration tests, unhandled Brig and SMTP failures, and a removed test import that breaks existing references.

7 open issues

5 resolved since last review

_{Review effort: Lite}

blackheaven · 2026-11-09T10:56:58Z

+withLicensedBackend :: (HasCallStack) => (String -> App a) -> App a
+withLicensedBackend k = do
+  resourcePool <- asks (.resourcePool)
+  runCodensity (acquireResources 1 resourcePool) $ \[backend] -> do
+    let dom = backend.berDomain


Already addressed by 2e10d80: Test/Migration/MeetingLinks.hs wraps testMeetingJoinCodeBackfill in withLicensedBackend.

Copilot

🔵 Needs a closer look

Critical correctness and compilation issues remain: verified creator emails are not used, retries can send duplicate invitations, and the unit-test helper import is missing.

4 open issues

2 previously missed issues

In code that hasn't changed since last review

Use the creator’s verified email address for meeting mail

libs/wire-subsystems/src/Wire/MeetingsSubsystem/Interpreter.hs:887

This reads the pending/unverified address instead of the creator's current email. User.userEmailUnvalidated is only populated while an email change is awaiting verification; activated users' usable address is exposed by userEmail from userIdentity (see libs/wire-api/src/Wire/API/User.hs:661-662,740-741). As a result, normal creators with a verified email and no pending change are treated as having no email and all meeting mail is skipped. Use the verified userEmail value and update the mock to populate that field.

Send invitations only to newly added email addresses

libs/wire-subsystems/src/Wire/MeetingsSubsystem/Interpreter.hs:1185

addInvitedEmails is state-idempotent (the Postgres store unions and deduplicates invited_emails), but this path sends to the entire request list. Retrying an add request or adding an already-invited address therefore sends a duplicate invitation, unlike the replace path below, which filters to newly added addresses. Compute emails \ storedMeeting.invitedEmails before dispatching.

3 resolved since last review

_{Review effort: Lite}

battermann · 2026-11-09T09:58:33Z

      guard $ qDomain meetingId == tDomain zUser
      lift $ Store.addInvitedEmails (qUnqualified meetingId) emails
+      -- The request's addresses are exactly the newly invited ones.
+      lift $ sendMeetingEmailsFor emailsEnabled (tDomain zUser) storedMeeting emails sendMeetingInvitationEmail


should we check that we do sent emails to already existing recipients? or is it a resend intended?

No resend intended — fixed in the earlier rework commit 56dbc42: only newlyAdded = emails \\ storedMeeting.invitedEmails get an invitation (same for replace). Covered by the tests "does not re-invite an address that is already invited (add)" and "sends invitations only for newly added addresses (add/replace/remove)`.

battermann · 2026-11-09T10:04:34Z

+sendMeetingEmailsFor ::
+  ( Member CodeStore r,
+    Member BrigAPIAccess r,
+    Member Async r,
+    Member Licensing r,
+    Member TinyLog r
+  ) =>
+  -- | Whether @settings.meetings.email@ is configured.
+  Bool ->
+  Domain ->
+  Store.StoredMeeting ->
+  -- | Recipient addresses.
+  [EmailAddress] ->
+  -- | Per-address send operation (invitation / update / cancellation).
+  (EmailAddress -> Text -> EmailAddress -> API.Meeting -> Maybe Locale -> Sem r ()) ->
+  Sem r ()
+sendMeetingEmailsFor emailsEnabled domain sm recipients sendKind =


In the ticket it says that a locale should be optionally provided by the caller, and if absent it should fall back to the creator's locale.

Fixed in 8dd9f16: MeetingEmailsInvitation gained an optional locale (maybe_ (optField "locale" schema), backward compatible). It is threaded through the AddInvitedEmails/ReplaceInvitedEmails effects and sendMeetingEmailsFor, which dispatches with fromMaybe creatorLoc callerLocale; create/update/refresh/delete keep the creator locale. Tests: caller-provided locale is used, and absence falls back to the creator's locale.

battermann · 2026-11-09T10:20:28Z

+          -- Fork+await solely to isolate typed brig failures: a child that
+          -- dies on 'RpcException'/'ParseException' collapses to 'Nothing'
+          -- (see the haddock), it is not for concurrency.
+          mHost <- P.async (meetingHostProfile sm) >>= P.await


Using async here does not seem right. there is no concurrency benefit. can't we catch the error with runError or something similar? and then we might also have to try/catch for untyped IO/HTTP errors.

Fixed in 8dd9f16: the fork+await is gone. meetingHostProfileBestEffort runs the lookup synchronously and collapses failures to Nothing via the same errorToIOFinal @SomeException (fromExceptionSem @SomeException …) composition as pushBestEffort in Wire.NotificationSubsystem.Interpreter — this also covers transport failures, which "Wire.Rpc" throws as IO exceptions that runError cannot catch (and which the async child let escape through await). prepareMeetingEmails now needs Member (Final IO) r instead of Member Async r; a regression test pins that create still succeeds and sends no RPC when brig fails.

battermann · 2026-11-09T10:45:30Z

-          {{- if .useSES }}
-            sesQueue: {{ required "Missing value: galley.config.settings.meetings.email.aws.sesQueue" .aws.sesQueue }}
-            sesEndpoint: {{ .aws.sesEndpoint | quote }}
+          {{- if .sesQueue }}


I think this is a breaking change. Existing overrides would now be ignored and leave sesQueue unset. Needs to be added to release notes, but I think it would be better to keep the old name.

Compat added in 8dd9f16: brig's meeting.replyTo now falls back to the legacy galley.config.settings.meetings.email.replyTo (new key wins), and the galley configmap enables meeting emails when the legacy meetings.email block is present (documented as deprecated in values.yaml, changelog entry changelog.d/1-api-changes/WPB-25932-meeting-email-config.md). Verified with helm template: legacy replyTo renders into brig's meeting block; legacy block + emailsEnabled: false keeps emails on (remove the block to disable); transport/sender/branding keys under the legacy block are ignored.

battermann · 2026-11-09T11:03:24Z

+    sendMeetingUpdateMailImpl meetingTpls branding replyTo email hostName hostEmail meeting changeSummary mLocale
+  SendMeetingCancelEmail email hostName hostEmail meeting mLocale ->
+    sendMeetingCancelMailImpl meetingTpls branding replyTo email hostName hostEmail meeting mLocale
+  _ -> error "meetingEmailSubsystemInterpreter: unsupported email operation"


This is not ideal. But we have talked about it already.

battermann · 2026-11-09T11:10:43Z

+              pure $
+                CodeCheckOk
+                  API.MeetingCodeCheck
+                    { title = meeting.title,
+                      startTime = meeting.startTime,
+                      endTime = meeting.endTime,
+                      tzid = meeting.tzid,
+                      recurrence = meeting.recurrence
+                    }
+            else pure CodeCheckInvalidPassword


nit-pick: pull the pure out

Done in 8dd9f16.

battermann · 2026-11-09T11:25:25Z

+errorBrigAPIAccess :: InterpreterFor BrigAPIAccess r
+errorBrigAPIAccess = interpret \case
+  _ -> error "errorBrigAPIAccess: operation not available in this context"


the simple fact that we need this is a design smell, IMO. Not a problem introduced by this PR, this PR only exposes the symptoms. Can we separate the dependencies so that we do not need the whole meetings interpreter? Or if this change is not feasible, can we move this local to where it is called and not expose it, to not encourage anyone else to use this?

Removed in 8dd9f16: the worker now calls cleanupOldMeetingsImpl directly on its minimal constraint set (MeetingsStore + ConversationSubsystem + Input (Local ()) + CodeStore), so the interpreter, licensing, brig access and email config drop out of the cleanup path entirely; errorBrigAPIAccess is deleted.

battermann · 2026-11-09T12:25:13Z

+-- | Interpreter that does nothing for every operation: used as a cleanup
+-- worker default and wherever emails are intentionally disabled.
+noopEmailSubsystemInterpreter :: InterpreterFor EmailSubsystem r
+noopEmailSubsystemInterpreter = interpret \case


same dependency smell as errorBrigAPIAccess

The production noop interpreter is already gone: commit 56dbc42 dropped the galley email subsystem entirely (rendering/transport moved to brig), so Wire.EmailSubsystem no longer needs a no-op stand-in in production stacks.

battermann · 2026-11-09T12:27:57Z

+  = -- | The deployment's license does not cover invitation links
+    -- (WPB-29643). Checked before link resolution, so it also applies to
+    -- unknown or stale links (surfaced as 403 id-feature-error).
+    JoinMeetingPaywall


Suggested change

JoinMeetingPaywall

JoinMeetingLicenseFailure

This is more neutral, IMO. Paywall describes more the product POV rather than a domain failure. (this is almost a nit-pick, but applies to all occurrences of "Paywall")

Renamed in 8dd9f16: CodeCheckLicenseFailure / JoinMeetingLicenseFailure (all occurrences in libs/services/integration).

battermann · 2026-11-09T12:37:41Z

can we have a test that covers that existing members do not get another email even if they are part of the add request? like this: create a meeting with member A, then add [A,B], check that only B receives the email

Already covered: "does not re-invite an address that is already invited (add)" creates with A, then adds [A,B] and asserts only B gets an RPC (plus the replace/remove variant asserting recipients [A,B,C,D] across lifecycle events).

battermann · 2026-11-10T08:42:47Z

+        runError @RpcException (runError @ParseException (BrigAPIAccess.sendMeetingEmails payload)) >>= \case
+          Right (Right ()) -> pure ()
+          _ ->
+            TinyLog.warn $
+              Log.msg ("meeting email dispatch failed" :: ByteString)
+                . Log.field "meetingId" (toByteString' (qUnqualified mtg.id))
+                . Log.field "kind" (show kind)


I am stil not 100% this catches all IO exceptions.

The reason this is done is to log delivery failures, right? Can we move the logging into the EmailSubsystem interpreter or even into the RPC interpreter to make sure no transport failures bubble up as unhandled IO exceptions.

Delivery-failure logging now lives in the EmailSubsystem interpreter (per-recipient warn with kind/recipient/exception), and the galley dispatch child additionally catches SomeException: transport IO exceptions (brig entirely down) are warn-logged with the failure reason instead of dying silently with the discarded child. Nothing from the RPC escapes unhandled anymore. One honest caveat on test coverage: the meeting-operation-success unit test passes both before and after this change (the detached child's result was already discarded pre-fix), so the behavioral delta here is the warn log itself, not the outcome.

battermann · 2026-11-10T08:59:33Z

@@ -0,0 +1 @@
+Meeting invitation/update/cancel email rendering and transport moved from galley to brig: configure brig's `emailSMS` instead of the legacy `galley.config.settings.meetings.email` block. For backward compatibility the legacy galley block is still honored: its presence enables meeting emails and its `replyTo` feeds brig's `emailSMS.general.meeting.replyTo`; transport/sender/branding keys under it (`from`, `useSES`, `aws.*`, `smtp.*`, `templateDir`, `branding`) are ignored. The new keys are `galley.config.settings.meetings.emailsEnabled` and `brig.config.emailSMS.general.meeting.replyTo`.


this should be a release note, maybe.

Also can we remove the galley email configuration entirely and rely solely on brig? AFAICT it has never been adopted and IMO just adds more unnecessary complexity.

Maybe I am misunderstanding this. Then you may object of course.
May hunch is that the legacy fallbacks are not necessary because this has not been adopted, yet?

Keeping emailsEnabled and the legacy block for now. The flag is a per-operation short-circuit: without it, every meeting lifecycle op on a licensed deployment pays the brig profile RPC, code lookup and internal POST even when the operator doesn't want emails, with no off-switch short of changing the license payload. The legacy block keeps pre-restructure helm values working, and the compat story is already documented in changelog.d/1-api-changes/ (towncrier, compiled into the release notes). Happy to revisit a brig-only configuration before the release train if you'd still prefer it.

As above: keeping emailsEnabled + the legacy block for now — the flag is the only operator off-switch for meeting emails on licensed deployments, and the legacy block keeps old helm values working (compat documented in changelog.d/1-api-changes/). Open to revisiting brig-only config before the release train if you still prefer that.

Galley now sends meeting invitation, update and cancellation emails to invited email addresses via Wire.EmailSubsystem when settings.meetings.email is configured (scheduled meetings only; creator locale; delivery failures logged, never surfaced): - create/addInvitedEmails/replaceInvitedEmails (newly-added only) -> invitation emails - updateMeeting -> update email per actually-changed stored field - refreshMeetingLink -> 'Join link changed' update email - deleteMeeting -> cancellation emails after the deletion preflight Infrastructure: reusable meetingEmailSubsystemInterpreter and errorBrigAPIAccess; MeetingsEmailConfig gains templateDir + branding; galley MeetingEmailEnv wiring with fire-and-forget best-effort transport; meetings cleanup worker satisfies the new constraints with error interpreters. Sends are async best-effort (pushAsyncImpl idiom); cancellation emails go out only after the fallible deletion preflight; empty recipient lists skip brig/code-store lookups entirely. - License gate: sends happen only when the deployment license's meetings.emails_sending flag is on, checked first in sendMeetingEmails; unlicensed deployments get no meeting emails even with settings.meetings.email configured.

blackheaven requested review from a team as code owners October 19, 2026 12:04

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 19, 2026

Base automatically changed from gdifolco/WPB-25930-wire-emails-operations to develop October 19, 2026 20:19

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 16715ed to 5fe8101 Compare October 19, 2026 20:42

blackheaven changed the title ~~[WPB-25932] Send meeting emails on create/update/link-refresh/delete~~ WPB-25932: Send meeting emails on create/update/link-refresh/delete Oct 19, 2026

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch 2 times, most recently from 9d6987b to 6537276 Compare October 19, 2026 22:00

blackheaven requested a balanced review from Copilot October 20, 2026 06:41

Copilot AI reviewed Oct 20, 2026

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch 4 times, most recently from b311481 to f2f6c3a Compare October 27, 2026 10:09

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from f2f6c3a to fdcb2d4 Compare November 5, 2026 09:03

battermann requested a lite review from Copilot November 6, 2026 08:00

Copilot started reviewing on behalf of battermann November 6, 2026 08:00

View session

Copilot AI reviewed Nov 6, 2026

View reviewed changes

battermann requested a lite review from Copilot November 6, 2026 10:17

Copilot started reviewing on behalf of battermann November 6, 2026 10:17

View session

Copilot AI reviewed Nov 6, 2026

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from f949f3f to 2e10d80 Compare November 6, 2026 17:14

battermann requested a lite review from Copilot November 9, 2026 09:11

Copilot started reviewing on behalf of battermann November 9, 2026 09:12

View session

Copilot AI reviewed Nov 9, 2026

View reviewed changes

battermann self-assigned this Nov 9, 2026

battermann requested changes Nov 9, 2026

View reviewed changes

battermann reviewed Nov 9, 2026

View reviewed changes

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 0a68443 to 56dbc42 Compare November 9, 2026 15:30

blackheaven requested a review from battermann November 10, 2026 06:45

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 8dd9f16 to 27f3529 Compare November 10, 2026 06:46

battermann reviewed Nov 10, 2026

View reviewed changes

blackheaven requested a review from battermann November 10, 2026 14:50

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 4b3ee61 to a51c3cd Compare November 10, 2026 16:49

blackheaven changed the base branch from develop to gdifolco/WPB-29643-meeting-guard-join-by-link November 10, 2026 16:54

blackheaven force-pushed the gdifolco/WPB-29643-meeting-guard-join-by-link branch from bf7548e to 45d6a1b Compare November 10, 2026 17:50

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from a51c3cd to d2aec44 Compare November 10, 2026 18:53

Base automatically changed from gdifolco/WPB-29643-meeting-guard-join-by-link to develop November 12, 2026 16:34

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 6d67409 to 5567c2e Compare November 12, 2026 17:19

battermann approved these changes Nov 13, 2026

View reviewed changes

battermann removed their assignment Nov 13, 2026

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 5567c2e to 3a45ce3 Compare November 13, 2026 11:06

blackheaven force-pushed the gdifolco/WPB-25932-meetings-send-emails branch from 3a45ce3 to 6f213a4 Compare November 13, 2026 11:42

blackheaven merged commit ab2e137 into develop Nov 13, 2026
10 checks passed

blackheaven deleted the gdifolco/WPB-25932-meetings-send-emails branch November 13, 2026 12:57

		templates <- loadMeetingTemplates (O._templateDir cfg) def (O._from cfg)
		let meetingBranding = fromMaybe Map.empty (O._branding cfg)

		@@ -0,0 +1 @@
		Meeting invitation/update/cancel email rendering and transport moved from galley to brig: configure brig's `emailSMS` instead of the legacy `galley.config.settings.meetings.email` block. For backward compatibility the legacy galley block is still honored: its presence enables meeting emails and its `replyTo` feeds brig's `emailSMS.general.meeting.replyTo`; transport/sender/branding keys under it (`from`, `useSES`, `aws.`, `smtp.`, `templateDir`, `branding`) are ignored. The new keys are `galley.config.settings.meetings.emailsEnabled` and `brig.config.emailSMS.general.meeting.replyTo`.

Conversation

blackheaven commented Oct 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackheaven commented Nov 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackheaven commented Oct 19, 2026 •

edited

Loading