diff --git a/docs/client-api.md b/docs/client-api.md index de0141f43..96be97dee 100644 --- a/docs/client-api.md +++ b/docs/client-api.md @@ -801,9 +801,10 @@ Shared by Add Members, Remove Member, and Update Member Role. | `userId` | string | The affected user's internal user ID. Omitted on the org-removal path (only `subscription.u.account` is set there). | | `subscription` | [Subscription](#subscription) | For `added` / `role_updated`: the full Subscription record. For `removed`: a [RemovedSubscriptionRef](#removedsubscriptionref) lean ref (see Remove Member). | | `action` | string | `"added"`, `"removed"`, `"role_updated"`, `"mute_toggled"`, or `"favorite_toggled"`. | +| `roomName` | string | Per-subscriber display label, set only where the server already has the name. On `added`: `channel` → room name; `dm` → counterpart's display name (`engName` + `chineseName`, falling back to account); `botDM` → the bot's app name. On `role_updated`: the channel name. Omitted (`omitempty`) on `mute_toggled` / `favorite_toggled` / `read`, and absent on `removed`. | | `timestamp` | number | Epoch ms (UTC). | -On `added` / `role_updated` / `mute_toggled` / `favorite_toggled` the embedded `Subscription` serializes its ID as `id` (not `_id`) and the user under `u` (not `user`). Non-`omitempty` fields (`id`, `u`, `roomId`, `siteId`, `roles`, `name`, `roomType`, `joinedAt`, `hasMention`, `alert`, `muted`, `favorite`) are always present. `removed` events use a dedicated lean payload (`SubscriptionRemovedEvent`) whose `subscription` carries **only** `roomId`, `roomType`, and `u` — no zero-valued `Subscription` fields are sent. +On `added` / `role_updated` / `mute_toggled` / `favorite_toggled` the embedded `Subscription` serializes its ID as `id` (not `_id`) and the user under `u` (not `user`). Non-`omitempty` fields (`id`, `u`, `roomId`, `siteId`, `roles`, `name`, `roomType`, `joinedAt`, `hasMention`, `alert`, `muted`, `favorite`) are always present — and the envelope's `roomName` is always present as a field (empty on `mute_toggled` / `favorite_toggled`). `removed` events use a dedicated lean payload (`SubscriptionRemovedEvent`) whose `subscription` carries **only** `roomId`, `roomType`, and `u` — no zero-valued `Subscription` fields are sent. ```json { @@ -818,6 +819,7 @@ On `added` / `role_updated` / `mute_toggled` / `favorite_toggled` the embedded ` "joinedAt": "2026-05-06T08:01:23Z" }, "action": "added", + "roomName": "engineering-announcements", "timestamp": 1746518483000 } ``` @@ -1015,6 +1017,7 @@ See [Error envelope](#6-error-envelope-reference). Returned synchronously when v "joinedAt": "2026-05-06T08:01:23Z" }, "action": "role_updated", + "roomName": "engineering-announcements", "timestamp": 1746518483000 } ``` @@ -1239,7 +1242,7 @@ See [Error envelope](#6-error-envelope-reference). Common errors: `"only room me | Field | Type | Required | Notes | |---|---|---|---| -| `limit` | number | no | When omitted, the server uses `min(3, room.userCount)` (so a 2-member room returns 2 rows, an empty room returns an empty list). When supplied, must be `> 0` and `<= room.userCount`. | +| `limit` | number | no | Upper bound on returned rows. When omitted, the server uses `min(3, room.userCount)` (an empty room returns an empty list); when supplied, must be `> 0` and `<= room.userCount`. Fewer rows may come back — members with an empty `statusText` are omitted (see `members`). | ```json { "limit": 5 } @@ -1249,7 +1252,7 @@ See [Error envelope](#6-error-envelope-reference). Common errors: `"only room me | Field | Type | Notes | |---|---|---| -| `members` | array | One entry per room subscription, projected from the joined `users` document. | +| `members` | array | One entry per room subscription **with a non-empty `statusText`**, projected from the joined `users` document. Members without a status set are omitted. | `MemberStatus`: @@ -1259,7 +1262,7 @@ See [Error envelope](#6-error-envelope-reference). Common errors: `"only room me | `engName` | string | English display name. | | `chineseName` | string | Chinese display name. | | `statusIsShow` | boolean | Whether the user has chosen to surface their status text. | -| `statusText` | string | Free-form presence text (e.g. `"available"`, `"in a meeting"`). Empty for users who have never set a status. | +| `statusText` | string | Free-form presence text (e.g. `"available"`, `"in a meeting"`); always non-empty — members without a status are omitted from `members`. | ```json { diff --git a/pkg/model/account.go b/pkg/model/account.go deleted file mode 100644 index 0c3654de3..000000000 --- a/pkg/model/account.go +++ /dev/null @@ -1,13 +0,0 @@ -package model - -import "strings" - -// IsBotAccount reports whether an account name denotes a bot/pseudo user. -// The rule — a ".bot" suffix or a "p_" prefix — is the single source of truth -// for bot classification and is equivalent to the regex `(\.bot$|^p_)` used by -// pkg/pipelines and room-service. Subscriptions store the result in u.isBot so -// member-count reconciliation can split user vs app counts off an indexed field -// instead of evaluating a regex per document on every read. -func IsBotAccount(account string) bool { - return strings.HasSuffix(account, ".bot") || strings.HasPrefix(account, "p_") -} diff --git a/pkg/model/account_test.go b/pkg/model/account_test.go deleted file mode 100644 index d1119ce7c..000000000 --- a/pkg/model/account_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package model_test - -import ( - "testing" - - "github.com/hmchangw/chat/pkg/model" -) - -func TestIsBotAccount(t *testing.T) { - tests := []struct { - name string - account string - want bool - }{ - {name: "dot-bot suffix", account: "weather.bot", want: true}, - {name: "bare dot-bot", account: ".bot", want: true}, - {name: "p_ prefix webhook", account: "p_webhook", want: true}, - {name: "bare p_ prefix", account: "p_", want: true}, - {name: "plain human account", account: "alice", want: false}, - {name: "ends with bot but no dot", account: "robot", want: false}, - {name: "dot-bot not at end", account: "alice.bot.com", want: false}, - {name: "p underscore not at start", account: "alice_p_x", want: false}, - {name: "case sensitive prefix", account: "P_webhook", want: false}, - {name: "case sensitive suffix", account: "weather.BOT", want: false}, - {name: "empty", account: "", want: false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := model.IsBotAccount(tt.account); got != tt.want { - t.Errorf("IsBotAccount(%q) = %v, want %v", tt.account, got, tt.want) - } - }) - } -} diff --git a/pkg/model/event.go b/pkg/model/event.go index 46c050746..c5a7e1c8e 100644 --- a/pkg/model/event.go +++ b/pkg/model/event.go @@ -74,6 +74,7 @@ type SubscriptionUpdateEvent struct { UserID string `json:"userId"` Subscription Subscription `json:"subscription"` Action string `json:"action"` // "added" | "removed" | "role_updated" | "mute_toggled" | "favorite_toggled" | "read" + RoomName string `json:"roomName,omitempty"` Timestamp int64 `json:"timestamp" bson:"timestamp"` } diff --git a/pkg/model/model_test.go b/pkg/model/model_test.go index d8081673c..52474a71d 100644 --- a/pkg/model/model_test.go +++ b/pkg/model/model_test.go @@ -62,6 +62,7 @@ func TestIsPlatformAdmin(t *testing.T) { {"admin role present", &model.User{Account: "alice", Roles: []model.UserRole{model.UserRoleAdmin}}, true}, {"admin among many", &model.User{Account: "alice", Roles: []model.UserRole{model.UserRoleUser, model.UserRoleAdmin, "auditor"}}, true}, {"case-sensitive (Admin not admin)", &model.User{Account: "alice", Roles: []model.UserRole{"Admin"}}, false}, + {"p_ account without admin role is not a role admin", &model.User{Account: "p_webhook"}, false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -70,6 +71,49 @@ func TestIsPlatformAdmin(t *testing.T) { } } +func TestIsPlatformAdminAccount(t *testing.T) { + tests := []struct { + name string + account string + want bool + }{ + {"p_ prefix webhook", "p_webhook", true}, + {"bare p_ prefix", "p_", true}, + {"plain human account", "alice", false}, + {"bot account", "weather.bot", false}, + {"case-sensitive prefix (P_)", "P_upper", false}, + {"p underscore not at start", "alice_p_x", false}, + {"empty", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, model.IsPlatformAdminAccount(tt.account)) + }) + } +} + +func TestIsBot(t *testing.T) { + tests := []struct { + name string + account string + want bool + }{ + {"dot-bot suffix", "weather.bot", true}, + {"bare dot-bot", ".bot", true}, + {"p_ prefix is not a bot", "p_webhook", false}, + {"plain human account", "alice", false}, + {"ends with bot but no dot", "robot", false}, + {"dot-bot not at end", "alice.bot.com", false}, + {"case-sensitive suffix", "weather.BOT", false}, + {"empty", "", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, model.IsBot(tt.account)) + }) + } +} + func TestUserJSON_WithStatus(t *testing.T) { u := model.User{ ID: "u1", Account: "alice", SiteID: "site-a", @@ -3892,3 +3936,26 @@ func TestSubscriptionBaseMetadata_RoundTrip(t *testing.T) { assert.False(t, present, "%q must be omitted when unset", k) } } + +func TestSubscriptionUpdateEvent_RoomNameRoundTrips(t *testing.T) { + evt := model.SubscriptionUpdateEvent{ + UserID: "u1", + Subscription: model.Subscription{ID: "s1", RoomID: "r1", RoomType: model.RoomTypeDM}, + Action: "added", + RoomName: "Alice Wang", + Timestamp: 1735689600000, + } + data, err := json.Marshal(&evt) + require.NoError(t, err) + assert.Contains(t, string(data), `"roomName":"Alice Wang"`) + + var dst model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(data, &dst)) + assert.Equal(t, "Alice Wang", dst.RoomName) +} + +func TestSubscriptionUpdateEvent_RoomNameOmittedWhenEmpty(t *testing.T) { + data, err := json.Marshal(&model.SubscriptionUpdateEvent{Action: "mute_toggled"}) + require.NoError(t, err) + assert.NotContains(t, string(data), "roomName") +} diff --git a/pkg/model/user.go b/pkg/model/user.go index 01922d125..8a8ae313f 100644 --- a/pkg/model/user.go +++ b/pkg/model/user.go @@ -1,5 +1,7 @@ package model +import "strings" + // UserRole is a platform-level role flag on the User record. // Empty Roles reads as ["user"]; only positive marker is "admin". type UserRole string @@ -27,8 +29,7 @@ type User struct { Roles []UserRole `json:"roles,omitempty" bson:"roles,omitempty"` } -// IsPlatformAdmin reports whether u holds the platform admin role. -// Returns false for nil receivers and for users without the role. +// IsPlatformAdmin reports whether u holds the platform admin role. Nil-safe. func IsPlatformAdmin(u *User) bool { if u == nil { return false @@ -41,6 +42,17 @@ func IsPlatformAdmin(u *User) bool { return false } +// IsPlatformAdminAccount reports whether account is a platform-admin account +// (a "p_" prefix). +func IsPlatformAdminAccount(account string) bool { + return strings.HasPrefix(account, "p_") +} + +// IsBot reports whether account is a bot account (a ".bot" suffix). +func IsBot(account string) bool { + return strings.HasSuffix(account, ".bot") +} + // DisplayName renders the user's display label for Drive ownership metadata: // the account when either name is missing, the English name when both names are // identical, otherwise " ". diff --git a/room-service/handler.go b/room-service/handler.go index 587c2ba05..6664e9511 100644 --- a/room-service/handler.go +++ b/room-service/handler.go @@ -188,7 +188,7 @@ func classifyAndValidate(req *model.CreateRoomRequest, requesterAccount string) return "", errChannelNameTooLong } for _, a := range req.Users { - if isBot(a) { + if model.IsBot(a) || model.IsPlatformAdminAccount(a) { return "", errBotInChannel } } @@ -736,7 +736,8 @@ func (h *Handler) updateRole(c *natsrouter.Context, req model.UpdateRoleRequest) return nil, fmt.Errorf("set owner role: %w", err) } - subEvtData, err := h.publishSubscriptionUpdate(ctx, req.Account, "role_updated", sub, now) + // Role updates are channel-only (guarded above); the channel name is already in hand. + subEvtData, err := h.publishSubscriptionUpdate(ctx, req.Account, "role_updated", sub, room.Name, now) if err != nil { return nil, err } @@ -765,16 +766,14 @@ func (h *Handler) updateRole(c *natsrouter.Context, req model.UpdateRoleRequest) return &model.StatusReply{Status: "ok"}, nil } -// publishSubscriptionUpdate marshals a SubscriptionUpdateEvent for sub with the -// given action and best-effort publishes it to the account's subscription.update -// subject over core NATS. A publish failure is logged, not returned — the DB -// write is the source of truth; clients reconcile on next refetch. Returns the -// marshaled event so callers can reuse it (e.g. as a cross-site outbox payload). -func (h *Handler) publishSubscriptionUpdate(ctx context.Context, account, action string, sub *model.Subscription, ts time.Time) ([]byte, error) { +// publishSubscriptionUpdate best-effort publishes a SubscriptionUpdateEvent (sub, action, +// roomName) over core NATS; a publish failure is logged, not returned. Returns the marshaled event. +func (h *Handler) publishSubscriptionUpdate(ctx context.Context, account, action string, sub *model.Subscription, roomName string, ts time.Time) ([]byte, error) { subEvt := model.SubscriptionUpdateEvent{ UserID: sub.User.ID, Subscription: *sub, Action: action, + RoomName: roomName, Timestamp: ts.UnixMilli(), } data, err := json.Marshal(subEvt) @@ -782,7 +781,8 @@ func (h *Handler) publishSubscriptionUpdate(ctx context.Context, account, action return nil, fmt.Errorf("marshal subscription update event: %w", err) } if err := h.publishCore(ctx, subject.SubscriptionUpdate(account), data); err != nil { - slog.Error("subscription update publish failed", "error", err, "account", account) + slog.ErrorContext(ctx, "subscription update publish failed", + "request_id", natsutil.RequestIDFromContext(ctx), "error", err, "account", account) } return data, nil } @@ -825,7 +825,7 @@ func (h *Handler) addMembers(c *natsrouter.Context, req model.AddMembersRequest) // create-channel: a client that explicitly lists a bot must see a hard // error rather than a silent drop. for _, a := range req.Users { - if isBot(a) { + if model.IsBot(a) || model.IsPlatformAdminAccount(a) { return nil, errBotInChannel } } @@ -1294,11 +1294,12 @@ func (h *Handler) messageRead(c *natsrouter.Context) (*model.StatusReply, error) } // Best-effort subscription.update to the reader's account (multi-device sync). - if !isBot(account) { + if !model.IsBot(account) { updatedSub := *sub updatedSub.LastSeenAt = &now updatedSub.Alert = newAlert - if _, err := h.publishSubscriptionUpdate(ctx, account, "read", &updatedSub, now); err != nil { + // roomName omitted: clients don't use it on read events. + if _, err := h.publishSubscriptionUpdate(ctx, account, "read", &updatedSub, "", now); err != nil { slog.Error("subscription update on read failed", "error", err, "request_id", natsutil.RequestIDFromContext(ctx), "account", account) } @@ -1684,7 +1685,7 @@ func (h *Handler) roomRename(c *natsrouter.Context, req model.RoomRenameRequest) return nil, errRenameChannelOnly } - if !isPlatformAdmin(requesterUser) { + if !model.IsPlatformAdmin(requesterUser) { sub, subErr := h.store.GetSubscription(ctx, account, roomID) if subErr != nil { if errors.Is(subErr, mongo.ErrNoDocuments) || errors.Is(subErr, model.ErrSubscriptionNotFound) { @@ -1735,7 +1736,7 @@ func (h *Handler) roomRestricted(c *natsrouter.Context, req model.RoomRestricted if getUserErr != nil && !errors.Is(getUserErr, ErrUserNotFound) { return nil, fmt.Errorf("get user: %w", getUserErr) } - if !isPlatformAdmin(requesterUser) { + if !model.IsPlatformAdmin(requesterUser) { return nil, errOnlyAdmins } @@ -1894,7 +1895,8 @@ func (h *Handler) muteToggle(c *natsrouter.Context) (*model.MuteToggleResponse, return nil, fmt.Errorf("toggle subscription mute: %w", err) } - if _, err := h.publishSubscriptionUpdate(ctx, account, "mute_toggled", sub, now); err != nil { + // roomName omitted: the frontend doesn't use it for mute, so we avoid an extra lookup. + if _, err := h.publishSubscriptionUpdate(ctx, account, "mute_toggled", sub, "", now); err != nil { return nil, err } @@ -1971,7 +1973,8 @@ func (h *Handler) favoriteToggle(c *natsrouter.Context) (*model.FavoriteToggleRe return nil, fmt.Errorf("toggle subscription favorite: %w", err) } - if _, err := h.publishSubscriptionUpdate(ctx, account, "favorite_toggled", sub, now); err != nil { + // roomName omitted: the frontend doesn't use it for favorite, so we avoid an extra lookup. + if _, err := h.publishSubscriptionUpdate(ctx, account, "favorite_toggled", sub, "", now); err != nil { return nil, err } diff --git a/room-service/handler_test.go b/room-service/handler_test.go index af5cfd1af..a89ae4614 100644 --- a/room-service/handler_test.go +++ b/room-service/handler_test.go @@ -81,6 +81,7 @@ func TestHandler_UpdateRole_Success(t *testing.T) { require.NoError(t, json.Unmarshal(coreData, &evt)) assert.Equal(t, "role_updated", evt.Action) assert.Equal(t, []model.Role{model.RoleMember, model.RoleOwner}, evt.Subscription.Roles) + assert.Equal(t, "general", evt.RoomName, "role_updated carries the channel name") } func TestHandler_UpdateRole_NonOwnerRejected(t *testing.T) { @@ -6099,3 +6100,65 @@ func TestHandler_threadUnreadSummary(t *testing.T) { }) } } + +func TestHandler_MuteToggle_OmitsRoomName(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().ToggleSubscriptionMute(gomock.Any(), "dmroom", "alice", gomock.Any()). + Return(&model.Subscription{ + ID: "s1", User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "dmroom", SiteID: "site-a", RoomType: model.RoomTypeDM, Name: "bob", Muted: true, + }, nil) + store.EXPECT().GetUserSiteID(gomock.Any(), "alice").Return("site-a", nil) + + var coreBodies [][]byte + h := &Handler{ + store: store, + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { return nil }, + publishCore: func(_ context.Context, _ string, data []byte) error { + coreBodies = append(coreBodies, data) + return nil + }, + } + + _, err := h.muteToggle(ctxParams(map[string]string{"account": "alice", "roomID": "dmroom"})) + require.NoError(t, err) + + require.Len(t, coreBodies, 1) + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(coreBodies[0], &evt)) + assert.Empty(t, evt.RoomName, "mute must not look up or set roomName") +} + +func TestHandler_FavoriteToggle_OmitsRoomName(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockRoomStore(ctrl) + + store.EXPECT().ToggleSubscriptionFavorite(gomock.Any(), "botroom", "alice", gomock.Any()). + Return(&model.Subscription{ + ID: "s1", User: model.SubscriptionUser{ID: "u1", Account: "alice"}, + RoomID: "botroom", SiteID: "site-a", RoomType: model.RoomTypeBotDM, Name: "helper.bot", Favorite: true, + }, nil) + store.EXPECT().GetUserSiteID(gomock.Any(), "alice").Return("site-a", nil) + + var coreBodies [][]byte + h := &Handler{ + store: store, + siteID: "site-a", + publishToStream: func(_ context.Context, _ string, _ []byte, _ string) error { return nil }, + publishCore: func(_ context.Context, _ string, data []byte) error { + coreBodies = append(coreBodies, data) + return nil + }, + } + + _, err := h.favoriteToggle(ctxParams(map[string]string{"account": "alice", "roomID": "botroom"})) + require.NoError(t, err) + + require.Len(t, coreBodies, 1) + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(coreBodies[0], &evt)) + assert.Empty(t, evt.RoomName, "favorite must not look up or set roomName") +} diff --git a/room-service/helper.go b/room-service/helper.go index 50307b493..2c99c1ffa 100644 --- a/room-service/helper.go +++ b/room-service/helper.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "regexp" "time" "github.com/hmchangw/chat/pkg/errcode" @@ -105,8 +104,6 @@ var ( errResponseTooLarge = errcode.Internal("response payload exceeds maximum size") ) -var botPattern = regexp.MustCompile(`\.bot$|^p_`) - // platformAdminRegex matches platform-admin / webhook accounts by their `p_` // prefix. Mentionable autocomplete hides these accounts entirely so they do // not appear as `@`-mention targets. @@ -137,14 +134,11 @@ func hasRole(roles []model.Role, target model.Role) bool { return false } -// isBot returns true if an account name matches the bot naming pattern. -func isBot(account string) bool { return botPattern.MatchString(account) } - // filterBots removes bot accounts from a slice of account names. func filterBots(accounts []string) []string { var filtered []string for _, a := range accounts { - if !isBot(a) { + if !model.IsBot(a) && !model.IsPlatformAdminAccount(a) { filtered = append(filtered, a) } } @@ -164,26 +158,12 @@ func dedup(items []string) []string { return result } -// isPlatformAdmin returns true when u has the UserRoleAdmin role. Nil-safe. -func isPlatformAdmin(u *model.User) bool { - if u == nil { - return false - } - for _, r := range u.Roles { - if r == model.UserRoleAdmin { - return true - } - } - return false -} - // determineRoomType classifies a post-strip request; caller must guarantee non-empty input. -// Uses the shared isBot predicate so both ".bot" suffix and "p_" prefix accounts -// classify as botDM, matching the bot-pattern guard used elsewhere in the service -// (filterBots, errBotInChannel) and in pkg/pipelines. +// A single-user DM with a bot (".bot") or platform-admin ("p_") counterpart is a botDM — +// the same union enforced by the channel-membership guards (filterBots, errBotInChannel). func determineRoomType(req *model.CreateRoomRequest) model.RoomType { if req.Name == "" && len(req.Orgs) == 0 && len(req.Channels) == 0 && len(req.Users) == 1 { - if isBot(req.Users[0]) { + if model.IsBot(req.Users[0]) || model.IsPlatformAdminAccount(req.Users[0]) { return model.RoomTypeBotDM } return model.RoomTypeDM diff --git a/room-service/helper_test.go b/room-service/helper_test.go index e0bdb7e0d..98d553ef3 100644 --- a/room-service/helper_test.go +++ b/room-service/helper_test.go @@ -33,24 +33,6 @@ func TestHasRole(t *testing.T) { } } -func TestIsBot(t *testing.T) { - tests := []struct { - name string - account string - want bool - }{ - {"bot suffix", "helper.bot", true}, - {"bot prefix", "p_scheduler", true}, - {"normal user", "alice", false}, - {"contains bot but not suffix", "botmaster", false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, isBot(tt.account)) - }) - } -} - func TestFilterBots(t *testing.T) { input := []string{"alice", "helper.bot", "bob", "p_scheduler"} got := filterBots(input) @@ -158,23 +140,6 @@ func TestStripAccount(t *testing.T) { } } -func TestIsPlatformAdmin(t *testing.T) { - tests := []struct { - name string - user *model.User - want bool - }{ - {"nil", nil, false}, - {"empty roles", &model.User{Account: "alice"}, false}, - {"user only", &model.User{Account: "a", Roles: []model.UserRole{model.UserRoleUser}}, false}, - {"admin", &model.User{Account: "a", Roles: []model.UserRole{model.UserRoleAdmin}}, true}, - {"mixed", &model.User{Account: "a", Roles: []model.UserRole{model.UserRoleUser, model.UserRoleAdmin}}, true}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { assert.Equal(t, tt.want, isPlatformAdmin(tt.user)) }) - } -} - func TestIsURLSafeIDToken(t *testing.T) { tests := []struct { name string diff --git a/room-service/integration_test.go b/room-service/integration_test.go index 6568c491d..e5f5300fc 100644 --- a/room-service/integration_test.go +++ b/room-service/integration_test.go @@ -3195,12 +3195,62 @@ func TestMongoStore_ListMemberStatuses_Integration(t *testing.T) { }, byAcct["bob"]) }) + t.Run("members with empty statusText are excluded", func(t *testing.T) { + db := setupMongo(t) + store := NewMongoStore(db) + mustInsertUser(t, db, &model.User{ + ID: "u-alice", Account: "alice", EngName: "Alice", ChineseName: "愛", + StatusIsShow: true, StatusText: "available", + }) + mustInsertUser(t, db, &model.User{ + ID: "u-bob", Account: "bob", EngName: "Bob", ChineseName: "博", StatusText: "", + }) + mustInsertSub(t, db, &model.Subscription{ + ID: "sub-a", User: model.SubscriptionUser{ID: "u-alice", Account: "alice"}, + RoomID: "r1", SiteID: "site-a", + }) + mustInsertSub(t, db, &model.Subscription{ + ID: "sub-b", User: model.SubscriptionUser{ID: "u-bob", Account: "bob"}, + RoomID: "r1", SiteID: "site-a", + }) + + got, err := store.ListMemberStatuses(ctx, "r1", 5) + require.NoError(t, err) + require.Len(t, got, 1, "member with empty statusText must be excluded") + assert.Equal(t, "alice", got[0].Account) + }) + + t.Run("member whose user doc lacks statusText is excluded", func(t *testing.T) { + db := setupMongo(t) + store := NewMongoStore(db) + mustInsertUser(t, db, &model.User{ + ID: "u-alice", Account: "alice", EngName: "Alice", ChineseName: "愛", + StatusIsShow: true, StatusText: "available", + }) + // Raw insert with no statusText field at all (bypasses the model's zero value). + _, err := db.Collection("users").InsertOne(ctx, bson.M{"_id": "u-dave", "account": "dave", "engName": "Dave"}) + require.NoError(t, err) + mustInsertSub(t, db, &model.Subscription{ + ID: "sub-a", User: model.SubscriptionUser{ID: "u-alice", Account: "alice"}, + RoomID: "r1", SiteID: "site-a", + }) + mustInsertSub(t, db, &model.Subscription{ + ID: "sub-d", User: model.SubscriptionUser{ID: "u-dave", Account: "dave"}, + RoomID: "r1", SiteID: "site-a", + }) + + got, err := store.ListMemberStatuses(ctx, "r1", 5) + require.NoError(t, err) + require.Len(t, got, 1, "member whose user doc lacks statusText must be excluded") + assert.Equal(t, "alice", got[0].Account) + }) + t.Run("limit caps the result count", func(t *testing.T) { db := setupMongo(t) store := NewMongoStore(db) for i := 0; i < 5; i++ { acct := fmt.Sprintf("user%d", i) - mustInsertUser(t, db, &model.User{ID: "u-" + acct, Account: acct, EngName: acct, ChineseName: acct}) + mustInsertUser(t, db, &model.User{ID: "u-" + acct, Account: acct, EngName: acct, ChineseName: acct, StatusText: acct}) mustInsertSub(t, db, &model.Subscription{ ID: "sub-" + acct, User: model.SubscriptionUser{ID: "u-" + acct, Account: acct}, RoomID: "r1", SiteID: "site-a", @@ -3251,7 +3301,7 @@ func TestMongoStore_ListMemberStatuses_Integration(t *testing.T) { // Then 3 live subs. for i := 0; i < 3; i++ { acct := fmt.Sprintf("live%d", i) - mustInsertUser(t, db, &model.User{ID: "u-" + acct, Account: acct, EngName: acct}) + mustInsertUser(t, db, &model.User{ID: "u-" + acct, Account: acct, EngName: acct, StatusText: acct}) mustInsertSub(t, db, &model.Subscription{ ID: fmt.Sprintf("sub-live%d", i), User: model.SubscriptionUser{ID: "u-" + acct, Account: acct}, diff --git a/room-service/store_mongo.go b/room-service/store_mongo.go index e429f2ba5..7f4bce677 100644 --- a/room-service/store_mongo.go +++ b/room-service/store_mongo.go @@ -17,10 +17,8 @@ import ( "github.com/hmchangw/chat/pkg/pipelines" ) -// botAccountRegex matches bot/app accounts by the ".bot" suffix. -// Distinct from helper.go::botPattern which also matches "^p_" — that -// clause is a pre-existing bug (p_ accounts are platform admins, not -// bots) and is out of scope for this change. +// botAccountRegex matches bot/app accounts by the ".bot" suffix only — it excludes +// "p_" platform-admin accounts, which have user records and are looked up as users here. const botAccountRegex = `\.bot$` var botAccountPattern = regexp.MustCompile(botAccountRegex) @@ -1329,14 +1327,8 @@ func (s *MongoStore) ListActiveCmdMenus(ctx context.Context, assistantNames []st return menus, nil } -// ListMemberStatuses returns up to `limit` members of roomID, each projected -// from the joined users document as MemberStatus. Subscriptions whose user -// document has been deleted are dropped by the $unwind with -// preserveNullAndEmptyArrays:false rather than returned half-populated. -// $limit runs AFTER the join so the wire contract ("up to limit live rows") -// holds even when the room contains orphan subscriptions whose user document -// has been hard-deleted. Pre-join $limit would silently under-deliver in that -// case. Mirrors ListReadReceipts. +// ListMemberStatuses returns up to limit members of roomID (projected from the joined +// user doc); orphan subs and empty-statusText members are dropped before the post-join $limit. func (s *MongoStore) ListMemberStatuses(ctx context.Context, roomID string, limit int) ([]model.MemberStatus, error) { pipeline := mongo.Pipeline{ {{Key: "$match", Value: bson.M{"roomId": roomID}}}, @@ -1363,6 +1355,8 @@ func (s *MongoStore) ListMemberStatuses(ctx context.Context, roomID string, limi }}}, {{Key: "$unwind", Value: bson.M{"path": "$user", "preserveNullAndEmptyArrays": false}}}, {{Key: "$replaceWith", Value: "$user"}}, + // Exclude members with no status set; an empty statusText is not a presence to surface. + {{Key: "$match", Value: bson.M{"statusText": bson.M{"$nin": bson.A{"", nil}}}}}, {{Key: "$limit", Value: int64(limit)}}, } cursor, err := s.subscriptions.Aggregate(ctx, pipeline) diff --git a/room-worker/debug_log_test.go b/room-worker/debug_log_test.go index eec1e053d..7a0649a1d 100644 --- a/room-worker/debug_log_test.go +++ b/room-worker/debug_log_test.go @@ -85,25 +85,29 @@ func TestPublishSubscriptionUpdates_TraceFanout(t *testing.T) { {User: model.SubscriptionUser{ID: "u1", Account: "alice"}}, {User: model.SubscriptionUser{ID: "u2", Account: "bob"}}, } + users := []*model.User{ + {ID: "u1", Account: "alice"}, + {ID: "u2", Account: "bob"}, + } rec := installRecorder(t) t.Run("trace: one delivery line per subscriber + the flow count", func(t *testing.T) { rec.reset() - h.publishSubscriptionUpdates(admitRung("trace"), subs, "req-1") + h.publishSubscriptionUpdates(admitRung("trace"), subs, users, "req-1") assert.Equal(t, 2, rec.count(logctx.LevelTrace, "room-worker subscription delivered")) assert.True(t, rec.has(logctx.LevelFlow, "room-worker subscription fan-out")) }) t.Run("flow: count only, no per-subscriber lines", func(t *testing.T) { rec.reset() - h.publishSubscriptionUpdates(admitRung("flow"), subs, "req-1") + h.publishSubscriptionUpdates(admitRung("flow"), subs, users, "req-1") assert.True(t, rec.has(logctx.LevelFlow, "room-worker subscription fan-out")) assert.False(t, rec.hasLevel(logctx.LevelTrace)) }) t.Run("unadmitted: nothing", func(t *testing.T) { rec.reset() - h.publishSubscriptionUpdates(context.Background(), subs, "req-1") + h.publishSubscriptionUpdates(context.Background(), subs, users, "req-1") assert.False(t, rec.hasLevel(logctx.LevelFlow)) assert.False(t, rec.hasLevel(logctx.LevelTrace)) }) diff --git a/room-worker/handler.go b/room-worker/handler.go index 4dde1c817..3a71b60af 100644 --- a/room-worker/handler.go +++ b/room-worker/handler.go @@ -1024,11 +1024,13 @@ func (h *Handler) processAddMembers(ctx context.Context, data []byte) (err error h.bustRoomMeta(ctx, req.RoomID) // Publish subscription.update BEFORE room.key so clients have a sub entry to store the key under. + // Channel-only handler: roomName is the already-fetched channel name. for _, sub := range subs { subEvt := model.SubscriptionUpdateEvent{ UserID: sub.User.ID, Subscription: *sub, Action: "added", + RoomName: room.Name, Timestamp: now.UnixMilli(), } subEvtData, _ := json.Marshal(subEvt) @@ -1229,7 +1231,7 @@ func newSub(id string, user *model.User, room *model.Room, roles []model.Role, name string, isSubscribed bool, joinedAt time.Time) *model.Subscription { return &model.Subscription{ ID: id, - User: model.SubscriptionUser{ID: user.ID, Account: user.Account, IsBot: model.IsBotAccount(user.Account)}, + User: model.SubscriptionUser{ID: user.ID, Account: user.Account, IsBot: model.IsBot(user.Account) || model.IsPlatformAdminAccount(user.Account)}, RoomID: room.ID, SiteID: room.SiteID, Roles: roles, @@ -1397,12 +1399,11 @@ func (h *Handler) existingRoomKey(ctx context.Context, roomID string, fallbackPa return &roomkeystore.VersionedKeyPair{Version: ver, KeyPair: *fallbackPair}, nil } -// determineRoomTypeFromPayload mirrors room-service's determineRoomType on the -// canonical payload. model.IsBotAccount classifies webhook-style bots (".bot" -// suffix or "p_" prefix) consistently with room-service/helper.go and pkg/pipelines. +// determineRoomTypeFromPayload mirrors room-service's determineRoomType: a single +// ".bot"/"p_" counterpart is a botDM (consistent with room-service + pkg/pipelines). func determineRoomTypeFromPayload(req *model.CreateRoomRequest) model.RoomType { if req.Name == "" && len(req.Orgs) == 0 && len(req.Channels) == 0 && len(req.Users) == 1 { - if model.IsBotAccount(req.Users[0]) { + if model.IsBot(req.Users[0]) || model.IsPlatformAdminAccount(req.Users[0]) { return model.RoomTypeBotDM } return model.RoomTypeDM @@ -1500,11 +1501,16 @@ func (h *Handler) finishCreateRoom(ctx context.Context, req *model.CreateRoomReq h.bustRoomMeta(ctx, room.ID) // Task 35: subscription.update fan-out per sub + userByAccount := make(map[string]*model.User, len(allUsers)) + for i := range allUsers { + userByAccount[allUsers[i].Account] = &allUsers[i] + } for _, sub := range subs { evt := model.SubscriptionUpdateEvent{ UserID: sub.User.ID, Subscription: *sub, Action: "added", + RoomName: h.resolveSubUpdateRoomName(ctx, sub, userByAccount), Timestamp: now.UnixMilli(), } data, err := json.Marshal(evt) @@ -1806,7 +1812,7 @@ func (h *Handler) serverCreateDM(c *natsrouter.Context, req model.SyncCreateDMRe return nil, fmt.Errorf("re-read DM subs after write: %w", err) } - h.publishSubscriptionUpdates(ctx, []*model.Subscription{requesterSub, otherSub}, requestID) + h.publishSubscriptionUpdates(ctx, []*model.Subscription{requesterSub, otherSub}, []*model.User{requester, other}, requestID) // Outbox failure means the remote site won't learn about the room; fail the request. if err := h.publishSyncDMOutbox(ctx, room, requester, other, requesterSub.JoinedAt); err != nil { @@ -1848,7 +1854,7 @@ func (h *Handler) createSelfDM(ctx context.Context, requester *model.User, reque } // No read-back: a fresh room id means a pure insert, so sub is what persisted. - h.publishSubscriptionUpdates(ctx, []*model.Subscription{sub}, requestID) + h.publishSubscriptionUpdates(ctx, []*model.Subscription{sub}, []*model.User{requester}, requestID) return &model.SyncCreateDMReply{Success: true, Subscription: *sub}, nil } @@ -1868,12 +1874,44 @@ func validateSyncCreateDMShape(req *model.SyncCreateDMRequest) error { return nil } -func (h *Handler) publishSubscriptionUpdates(ctx context.Context, subs []*model.Subscription, requestID string) { +// resolveSubUpdateRoomName computes a subscription.update's roomName: the room name for +// channels; for dm/botDM, app.Name when the counterpart is a bot account, else its display name. +func (h *Handler) resolveSubUpdateRoomName(ctx context.Context, sub *model.Subscription, userByAccount map[string]*model.User) string { + switch sub.RoomType { + case model.RoomTypeDM, model.RoomTypeBotDM: + cp := sub.Name + // Platform-admin (p_) accounts are users, not bots, for naming — only .bot takes the app path. + if model.IsBot(cp) { + app, err := h.store.GetApp(ctx, cp) + if err == nil && app.Name != "" { + return app.Name + } + if err != nil && !errors.Is(err, ErrAppNotFound) { + slog.WarnContext(ctx, "resolve roomName: GetApp failed, using account fallback", + "request_id", natsutil.RequestIDFromContext(ctx), "botAccount", cp, "error", err) + } + return cp + } + if u, ok := userByAccount[cp]; ok { + return displayName(u) + } + return cp + default: + return sub.Name + } +} + +func (h *Handler) publishSubscriptionUpdates(ctx context.Context, subs []*model.Subscription, users []*model.User, requestID string) { + userByAccount := make(map[string]*model.User, len(users)) + for _, u := range users { + userByAccount[u.Account] = u + } for _, sub := range subs { evt := model.SubscriptionUpdateEvent{ UserID: sub.User.ID, Subscription: *sub, Action: "added", + RoomName: h.resolveSubUpdateRoomName(ctx, sub, userByAccount), Timestamp: time.Now().UTC().UnixMilli(), } data, err := json.Marshal(evt) diff --git a/room-worker/handler_test.go b/room-worker/handler_test.go index 431d03cc5..3b310c22b 100644 --- a/room-worker/handler_test.go +++ b/room-worker/handler_test.go @@ -2071,6 +2071,10 @@ func TestProcessCreateRoom_BotDM_HasIsSubscribed(t *testing.T) { return capturedSubs[0], capturedSubs[1], nil }) + // finishCreateRoom calls resolveSubUpdateRoomName per sub; the human sub has + // Name="helper.bot" (RoomTypeBotDM), so GetApp is invoked once. + mockStore.EXPECT().GetApp(gomock.Any(), "helper.bot").Return(&model.App{Name: "Helper Bot"}, nil) + mockStore.EXPECT().ReconcileMemberCounts(gomock.Any(), "room-bot-1").Return(nil) body := makeCreateRoomBody(t, &model.CreateRoomRequest{ @@ -2095,6 +2099,28 @@ func TestProcessCreateRoom_BotDM_HasIsSubscribed(t *testing.T) { assert.False(t, botSub.IsSubscribed) assert.Empty(t, messagesCanonical(getPublished(), "site-A"), "botDM must emit no sys-messages") + + // human (alice) subscription.update must carry the resolved app name. + humanSubj := subject.SubscriptionUpdate("alice") + var humanEvt model.SubscriptionUpdateEvent + for _, p := range getPublished() { + if p.subj == humanSubj { + require.NoError(t, json.Unmarshal(p.data, &humanEvt)) + break + } + } + assert.Equal(t, "Helper Bot", humanEvt.RoomName) + + // bot (helper.bot) subscription.update must carry the human's display name. + botSubj := subject.SubscriptionUpdate("helper.bot") + var botEvt model.SubscriptionUpdateEvent + for _, p := range getPublished() { + if p.subj == botSubj { + require.NoError(t, json.Unmarshal(p.data, &botEvt)) + break + } + } + assert.Equal(t, "Alice A 艾麗斯", botEvt.RoomName) } // ---- Task 34: Channel branch tests ---- @@ -5138,3 +5164,155 @@ func TestHandler_bustRoomMeta_FailOpen(t *testing.T) { // The Del was attempted (error swallowed inside BustMeta), not skipped. assert.Equal(t, []string{roommetacache.MetaKey("r123")}, fake.dels) } + +func TestHandler_resolveSubUpdateRoomName(t *testing.T) { + alice := &model.User{Account: "alice", EngName: "Alice", ChineseName: "愛麗絲"} + users := map[string]*model.User{"alice": alice} + + tests := []struct { + name string + sub model.Subscription + userMap map[string]*model.User + setupMock func(s *MockSubscriptionStore) + want string + }{ + { + name: "channel uses sub.Name", + sub: model.Subscription{RoomType: model.RoomTypeChannel, Name: "general"}, + want: "general", + }, + { + name: "dm resolves counterpart from map", + sub: model.Subscription{RoomType: model.RoomTypeDM, Name: "alice"}, + userMap: users, + want: "Alice 愛麗絲", + }, + { + name: "dm counterpart missing from map falls back to account", + sub: model.Subscription{RoomType: model.RoomTypeDM, Name: "carol"}, + userMap: users, + want: "carol", + }, + { + name: "botDM resolves app name", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "helper.bot"}, + setupMock: func(s *MockSubscriptionStore) { + s.EXPECT().GetApp(gomock.Any(), "helper.bot").Return(&model.App{Name: "Helper Bot"}, nil) + }, + want: "Helper Bot", + }, + { + name: "botDM GetApp error falls back to bot account", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "broken.bot"}, + setupMock: func(s *MockSubscriptionStore) { + s.EXPECT().GetApp(gomock.Any(), "broken.bot").Return(nil, ErrAppNotFound) + }, + want: "broken.bot", + }, + { + name: "botDM GetApp infra error falls back to bot account", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "flaky.bot"}, + setupMock: func(s *MockSubscriptionStore) { + s.EXPECT().GetApp(gomock.Any(), "flaky.bot").Return(nil, context.DeadlineExceeded) + }, + want: "flaky.bot", + }, + { + name: "botDM empty app name falls back to bot account", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "nameless.bot"}, + setupMock: func(s *MockSubscriptionStore) { + s.EXPECT().GetApp(gomock.Any(), "nameless.bot").Return(&model.App{Name: ""}, nil) + }, + want: "nameless.bot", + }, + { + name: "botDM bot-side sub resolves human from map", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "alice"}, + userMap: users, + want: "Alice 愛麗絲", + }, + { + name: "botDM platform-admin (p_) counterpart resolves as a user from map", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "p_admin"}, + userMap: map[string]*model.User{"p_admin": {Account: "p_admin", EngName: "Pat", ChineseName: "派特"}}, + want: "Pat 派特", + }, + { + name: "botDM platform-admin (p_) counterpart missing from map falls back to account", + sub: model.Subscription{RoomType: model.RoomTypeBotDM, Name: "p_admin"}, + userMap: users, + want: "p_admin", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + store := NewMockSubscriptionStore(ctrl) + if tt.setupMock != nil { + tt.setupMock(store) + } + h := &Handler{store: store} + got := h.resolveSubUpdateRoomName(context.Background(), &tt.sub, tt.userMap) + assert.Equal(t, tt.want, got) + }) + } +} + +// decodeSubUpdate finds the subscription.update published to account and decodes it. +func decodeSubUpdate(t *testing.T, captured []dmCapturedPublish, account string) model.SubscriptionUpdateEvent { + t.Helper() + want := subject.SubscriptionUpdate(account) + for _, p := range captured { + if p.subject == want { + var evt model.SubscriptionUpdateEvent + require.NoError(t, json.Unmarshal(p.data, &evt)) + return evt + } + } + t.Fatalf("no subscription.update published to %s", account) + return model.SubscriptionUpdateEvent{} +} + +func TestServerCreateDM_DM_SetsCounterpartRoomName(t *testing.T) { + h, store, capture := newSyncDMTestHandler(t) + + requester := &model.User{ID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice"} + other := &model.User{ID: "u-bob", Account: "bob", SiteID: "site-a", EngName: "Bob"} + store.EXPECT().FindUsersByAccounts(gomock.Any(), gomock.Any()).Return([]model.User{*requester, *other}, nil) + store.EXPECT().CreateRoom(gomock.Any(), gomock.Any(), gomock.Any()).Return(true, nil) + store.EXPECT().BulkCreateSubscriptions(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().FindDMSubscriptionPair(gomock.Any(), gomock.Any(), "alice").Return( + &model.Subscription{User: model.SubscriptionUser{ID: "u-alice", Account: "alice"}, RoomType: model.RoomTypeDM, Name: "bob"}, + &model.Subscription{User: model.SubscriptionUser{ID: "u-bob", Account: "bob"}, RoomType: model.RoomTypeDM, Name: "alice"}, + nil) + + req := model.SyncCreateDMRequest{RoomType: model.RoomTypeDM, RequesterAccount: "alice", OtherAccount: "bob"} + _, err := h.serverCreateDM(dmCtx(), req) + require.NoError(t, err) + + assert.Equal(t, "Bob", decodeSubUpdate(t, capture.captured, "alice").RoomName) + assert.Equal(t, "Alice", decodeSubUpdate(t, capture.captured, "bob").RoomName) +} + +func TestServerCreateDM_BotDM_SetsAppNameForHuman(t *testing.T) { + h, store, capture := newSyncDMTestHandler(t) + + requester := &model.User{ID: "u-alice", Account: "alice", SiteID: "site-a", EngName: "Alice"} + bot := &model.User{ID: "u-bot", Account: "helper.bot", SiteID: "site-a"} + store.EXPECT().FindUsersByAccounts(gomock.Any(), gomock.Any()).Return([]model.User{*requester, *bot}, nil) + store.EXPECT().CreateRoom(gomock.Any(), gomock.Any(), gomock.Any()).Return(true, nil) + store.EXPECT().BulkCreateSubscriptions(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().FindDMSubscriptionPair(gomock.Any(), gomock.Any(), "alice").Return( + &model.Subscription{User: model.SubscriptionUser{ID: "u-alice", Account: "alice"}, RoomType: model.RoomTypeBotDM, Name: "helper.bot"}, + &model.Subscription{User: model.SubscriptionUser{ID: "u-bot", Account: "helper.bot"}, RoomType: model.RoomTypeBotDM, Name: "alice"}, + nil) + store.EXPECT().GetApp(gomock.Any(), "helper.bot").Return(&model.App{Name: "Helper Bot"}, nil) + + req := model.SyncCreateDMRequest{RoomType: model.RoomTypeBotDM, RequesterAccount: "alice", OtherAccount: "helper.bot"} + _, err := h.serverCreateDM(dmCtx(), req) + require.NoError(t, err) + + assert.Equal(t, "Helper Bot", decodeSubUpdate(t, capture.captured, "alice").RoomName) + assert.Equal(t, "Alice", decodeSubUpdate(t, capture.captured, "helper.bot").RoomName) +} diff --git a/room-worker/integration_test.go b/room-worker/integration_test.go index 8bf014c6e..8890823da 100644 --- a/room-worker/integration_test.go +++ b/room-worker/integration_test.go @@ -451,8 +451,8 @@ func TestReconcileMemberCountsSplitsBots(t *testing.T) { mustInsertSub(t, db, &model.Subscription{ ID: "s3", User: model.SubscriptionUser{Account: "carol"}, RoomID: "r1", }) - // Bot subs carry u.isBot=true in production (stamped by newSub via - // model.IsBotAccount); set it explicitly here since the test inserts the + // Bot subs carry u.isBot=true in production (stamped by newSub for + // ".bot"/"p_" accounts); set it explicitly here since the test inserts the // document directly. ReconcileMemberCounts now counts off this flag. mustInsertSub(t, db, &model.Subscription{ ID: "s4", User: model.SubscriptionUser{Account: "weather.bot", IsBot: true}, RoomID: "r1", @@ -1966,3 +1966,23 @@ func TestMongoStore_UpdateSubscriptionNamesForRoom_StampsTimestamp(t *testing.T) assert.Equal(t, "newest", gotName) assert.Equal(t, newer.UnixMilli(), gotTs.UnixMilli()) } + +func TestMongoStore_GetApp_Integration(t *testing.T) { + db := setupMongo(t) + store := NewMongoStore(db) + ctx := context.Background() + + _, err := db.Collection("apps").InsertOne(ctx, model.App{ + ID: "app1", + Name: "Helper Bot", + Assistant: &model.AppAssistant{Enabled: true, Name: "helper.bot"}, + }) + require.NoError(t, err) + + app, err := store.GetApp(ctx, "helper.bot") + require.NoError(t, err) + assert.Equal(t, "Helper Bot", app.Name) + + _, err = store.GetApp(ctx, "missing.bot") + assert.ErrorIs(t, err, ErrAppNotFound) +} diff --git a/room-worker/mock_store_test.go b/room-worker/mock_store_test.go index f49c99c57..275b8509d 100644 --- a/room-worker/mock_store_test.go +++ b/room-worker/mock_store_test.go @@ -161,6 +161,21 @@ func (mr *MockSubscriptionStoreMockRecorder) FindUsersByAccounts(ctx, accounts a return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FindUsersByAccounts", reflect.TypeOf((*MockSubscriptionStore)(nil).FindUsersByAccounts), ctx, accounts) } +// GetApp mocks base method. +func (m *MockSubscriptionStore) GetApp(ctx context.Context, botAccount string) (*model.App, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetApp", ctx, botAccount) + ret0, _ := ret[0].(*model.App) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetApp indicates an expected call of GetApp. +func (mr *MockSubscriptionStoreMockRecorder) GetApp(ctx, botAccount any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetApp", reflect.TypeOf((*MockSubscriptionStore)(nil).GetApp), ctx, botAccount) +} + // GetOrgMembersWithIndividualStatus mocks base method. func (m *MockSubscriptionStore) GetOrgMembersWithIndividualStatus(ctx context.Context, roomID, orgID string) ([]OrgMemberStatus, error) { m.ctrl.T.Helper() diff --git a/room-worker/store.go b/room-worker/store.go index adc43a1d8..4dfbe7e30 100644 --- a/room-worker/store.go +++ b/room-worker/store.go @@ -16,6 +16,7 @@ var ( ErrRoomNotFound = errors.New("room not found") ErrNotChannelRoom = errors.New("not a channel room") ErrOwnerNotSubscribed = errors.New("owner account is no longer subscribed") + ErrAppNotFound = errors.New("app not found") // GetApp: no matching bot account ) //go:generate mockgen -destination=mock_store_test.go -package=main . SubscriptionStore,RoomKeyStore @@ -71,6 +72,8 @@ type SubscriptionStore interface { GetRoom(ctx context.Context, roomID string) (*model.Room, error) GetSubscription(ctx context.Context, account, roomID string) (*model.Subscription, error) GetUser(ctx context.Context, account string) (*model.User, error) + // GetApp returns the app whose Assistant.Name == botAccount, or ErrAppNotFound. + GetApp(ctx context.Context, botAccount string) (*model.App, error) // FindDMSubscriptionPair returns both subs of a DM/botDM room in a // single query. The first return value is the sub owned by // requesterAccount, the second is the counterpart's. Returns diff --git a/room-worker/store_mongo.go b/room-worker/store_mongo.go index 0d83d8785..1dfb6a46b 100644 --- a/room-worker/store_mongo.go +++ b/room-worker/store_mongo.go @@ -21,6 +21,7 @@ type MongoStore struct { rooms *mongo.Collection roomMembers *mongo.Collection users *mongo.Collection + apps *mongo.Collection } func NewMongoStore(db *mongo.Database) *MongoStore { @@ -29,6 +30,7 @@ func NewMongoStore(db *mongo.Database) *MongoStore { rooms: db.Collection("rooms"), roomMembers: db.Collection("room_members"), users: db.Collection("users"), + apps: db.Collection("apps"), } } @@ -51,7 +53,7 @@ func (s *MongoStore) ListByRoom(ctx context.Context, roomID string) ([]model.Sub // ReconcileMemberCounts recomputes the room's AppCount (bot subs) and UserCount // (everyone else) and writes both back in a single updateOne. AppCount is an // index-backed CountDocuments on {roomId, u.isBot} (the flag is stamped at -// sub-creation via model.IsBotAccount) and UserCount is total minus bots — both +// sub-creation for ".bot"/"p_" accounts) and UserCount is total minus bots — both // counts use the index and no per-document regex runs. Deriving UserCount by // subtraction also means legacy docs written before u.isBot existed (and any // missing the field) correctly fall into UserCount rather than being dropped. @@ -100,6 +102,22 @@ func (s *MongoStore) GetUser(ctx context.Context, account string) (*model.User, return &u, nil } +// GetApp reads the apps collection, which room-service owns and indexes +// (assistant.name); room-worker only reads it and creates no index of its own. +// Projects only name — the one field callers use (the botDM roomName). +func (s *MongoStore) GetApp(ctx context.Context, botAccount string) (*model.App, error) { + var a model.App + err := s.apps.FindOne(ctx, bson.M{"assistant.name": botAccount}, + options.FindOne().SetProjection(bson.M{"name": 1})).Decode(&a) + if errors.Is(err, mongo.ErrNoDocuments) { + return nil, ErrAppNotFound + } + if err != nil { + return nil, fmt.Errorf("get app for bot %q: %w", botAccount, err) + } + return &a, nil +} + func (s *MongoStore) CreateRoom(ctx context.Context, room *model.Room, key *roomkeystore.RoomKeyPair) (bool, error) { // Marshal the room struct (honouring omitempty) into a document so an optional // encKey field can be attached and the whole thing written in one upsert.