directory: only create group links after approval - #7356
Conversation
There was a problem hiding this comment.
Summary
This branch moves group-link creation in the directory service from group-join time to first admin approval. The join no longer calls APICreateGroupLink; instead the service goes straight to GRSPendingApproval and creates (or, for legacy/re-registrations, re-syncs) the link inside /approve (approvedGroupLink). The old GroupProfileUpdate/welcome-message-link machinery is replaced by a single linkOnlyChange check that keeps an active group listed when only the bot link or whitespace changes, and a new updateGroupLinksFromApp config flag disables core's automatic link-data refresh so the service issues every update itself. Search/cards can now resolve a submitted link to its registered group. Docs, README, plan, and tests are updated to match.
I read Service.hs in full, plus the Listing.hs, Util.hs, Subscriber.hs, Controller.hs and Chat.hs changes, the plan, docs, and the event definition, and traced the new state transitions and the approval/link-creation paths. The implementation matches the plan and the transition table.
Verified points worth noting:
APICreateGroupLinkembedsGroupShortLinkData {groupProfile, publicGroupData = Nothing}at creation, so the fresh link in the not-found branch carries the profile even though it (correctly) skips the fullsetGroupLinkDatarelay/publishable-data sync — that path is only taken for non-public groups where relays don't apply. The found/re-approval branch does callupdateGroupLinkData. Consistent with §2.2.linkOnlyChangeshort-circuits ondn == dn'before evaluatingdescrWords, so the old-name label used in stripping is safe.- No new code sets
GRSPendingUpdate; it is handled only as a legacy status. The removedGroupProfileUpdate,unexpectedError, andfindimport leave no dangling references. - Non-admin link lookups return "no groups found" for non-active registrations, so registered-but-unlisted groups are not revealed — good privacy behavior.
No blocking issues found. One maintainability note (not blocking): the recommended link line "Link to join the group " <> name <> ": " is now duplicated across four sites — the approval message (Service.hs:1274), the search-result line (Service.hs:1464), the listing line (Listing.hs:116), and, critically, the label that linkOnlyChange strips to detect link-only edits (Service.hs:585). The "adding the recommended line keeps the group listed" behavior depends on the stripping phrase matching the recommended phrase. It works today (and withoutLink removes the actual link token independently, so a whitespace mismatch is harmless), but a future wording change in one place could silently change re-approval behavior. Worth a shared constant if this text is touched again.
Approving.
There was a problem hiding this comment.
The branch moves group-link creation from group-join to admin approval, and makes adding the link to the welcome message optional (recommended, not required). The state machine collapses PendingUpdate into PendingApproval, and the GroupProfileUpdate classification (GPServiceLink*) is replaced by a simpler "is this only a link/whitespace change?" test. This is a coherent, well-executed change; docs (README, DIRECTORY.md) are updated to match.
Things I checked that are correct:
- The new
LEFT JOIN user_contact_linksingroupReqQuerycannot duplicate group rows.user_contact_linkshas a unique index ongroup_idalone (idx_user_contact_links_group_id), so at most one link row per group. This replaces the previous per-groupgetGroupLink(N+1) with a single join — a genuine performance win, andsearchListedGroups' separate count query stays consistent. toMaybeGroupLinkis consistent with the canonicaltoGroupLink. TheMaybe BoolIntwidening onshort_link_data_set/short_link_large_data_setonly matters for the all-NULL (no link) case, which correctly yieldsNothing.updateGroupLinksFromAppis scoped correctly. It defaults toFalse(mobile/CLI keep the automatic background link refresh) and is only setTrueby the directory service, which now refreshes link data explicitly (updateGroupLinkDataat approval and on link-only changes to active groups). No regression for the app.linkOnlyChangesymmetrically strips the recommended link line (link token viamatchesGroupLinkplus thegroupLinkLinePrefixtext) from both old and new descriptions, so adding/removing the recommended line keeps a group Active while any other edit triggers re-approval — matching the documented behavior.- Approval correctly fails closed: if the service can't create the link (not admin / removed),
approvedGroupLinkreturnsLeftand the group staysPendingApprovalrather than being listed without a working link.
Minor:
- One user-facing typo on a modified line ("upgated" → "updated"), suggested inline.
- Cosmetic: the
/linkcommand's menu button description indirectoryCommandsstill reads "View and upgrade group link" (Service.hs:242), while the/helptext was changed to "view group link". Not in the diff, so no inline suggestion — worth aligning.
No correctness, concurrency, or security issues found.
No description provided.