WPB-28697 add federated reminder events with origin user - #5540
Conversation
zebot
added
the
ok-to-test
There was a problem hiding this comment.
馃煛 Changes recommended
Add the WPB-28697 changelog entry and fix the Haskell formatting issue.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds v4 federation support for adminless reminder events while preserving the originating user across backends.
Changes:
- Adds the federation endpoint, payload, and subsystem handling.
- Routes origin-aware reminders locally and remotely.
- Adds integration coverage and updates changelog metadata.
File summaries
| File | Reviewed change |
|---|---|
services/galley/src/Galley/API/Federation.hs |
Registers the federation route. |
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs |
Sends origin-aware reminders. |
libs/wire-subsystems/src/Wire/ConversationSubsystem/Notify.hs |
Queues federated reminder bundles. |
libs/wire-subsystems/src/Wire/ConversationSubsystem/Interpreter.hs |
Dispatches the new effect. |
libs/wire-subsystems/src/Wire/ConversationSubsystem/Federation.hs |
Handles incoming reminders. |
libs/wire-subsystems/src/Wire/ConversationSubsystem.hs |
Defines the new subsystem operation. |
libs/wire-api-federation/src/Wire/API/Federation/API/Util.hs |
Builds the federation bundle. |
libs/wire-api-federation/src/Wire/API/Federation/API/Galley/Notifications.hs |
Defines the v4 endpoint and payload. |
libs/wire-api-federation/src/Wire/API/Federation/API.hs |
Re-exports the bundle builder. |
integration/test/Test/AdminlessGroups.hs |
Tests origin propagation. |
changelog.d/2-features/WPB-26650 |
Updates the existing changelog entry. |
Review notes: add a separate WPB-28697 changelog entry (nit, 3 votes) and align the record-field indentation with repository convention (nit, 1 vote).
Review details
Suppressed comments (1)
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1581
- The fields in this record are aligned with the opening brace instead of being indented beneath it, unlike the surrounding records; please format this block according to the repository's Haskell formatting convention.
conversation = tUnqualified lcnv,
reminder = AdminlessReminder deletionScheduledFor,
-- Filled per remote backend by Notify.sendSystemAdminlessReminder.
alreadyPresentUsers = []
}
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
馃數 Needs a closer look
The moderate retry-ordering issue remains unresolved and can cause duplicate reminders.
Review details
Suppressed comments (2)
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1581
- The fields in this record are indented two spaces less than the other record literals in this function (for example, lines 1565-1571), so this changed block is not formatter-compliant and
make formatwill rewrite it. Please align the fields with the opening record field.
conversation = tUnqualified lcnv,
reminder = AdminlessReminder deletionScheduledFor,
-- Filled per remote backend by Notify.sendSystemAdminlessReminder.
alreadyPresentUsers = []
}
libs/wire-subsystems/src/Wire/ConversationSubsystem/Update.hs:1562
- Because
runAdminlessReminderJobretries the whole effect when this federation enqueue fails (services/background-worker/src/Wire/AdminlessJobsWorker.hs:95-105), pushing the local event first can deliver duplicate reminders on a transient RabbitMQ/federation failure: the retry executespushConversationEventagain before retrying the remote enqueue. Queue the remote notification before the local push, as the system-reminder branch below andNotify.notifyConversationActionImpldo.
Notify.sendAdminlessReminder
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
| resp.status `shouldMatchInt` 200 | ||
|
|
||
| testAdminlessSetupSendsReminderWithOriginUserAndRemoteMembers :: (HasCallStack) => App () | ||
| testAdminlessSetupSendsReminderWithOriginUserAndRemoteMembers = do |
There was a problem hiding this comment.
this smells like a sentence, but i can't parse it. :)
|
|
||
| removeMember alice conv alice >>= assertSuccess | ||
|
|
||
| withWebSockets [app, remoteUser] $ \[wsApp, wsRemoteUser] -> do |
There was a problem hiding this comment.
why not open this before you do the stuff that triggers the events? if you only open the web sockets after, isn't there race condition and you might miss events?
There was a problem hiding this comment.
it is the enabling of the team feature that causes the events
|
|
||
| conv <- createTeamMLSConversation alice tid alice1 [remoteUser] | ||
| let newApp = def {name = "adminless-federated-origin-reminder-app", description = "not eligible for promotion"} | ||
| (app, _) <- createAndAddAppMember alice tid alice1 conv newApp |
There was a problem hiding this comment.
is there a reason why you add an app here, not a regular user? (just curious)
There was a problem hiding this comment.
yes, apps are not eligible for promotion
https://wearezeta.atlassian.net/browse/WPB-28697
Added to fed API version v4, since this is unreleased.
Checklist
changelog.d