-
Notifications
You must be signed in to change notification settings - Fork 13.8k
fix: direct messages addressed by username never resolve #41822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| '@rocket.chat/rest-typings': minor | ||
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| Adds `POST /v1/rooms.getOrCreate`, an idempotent endpoint that resolves a room by type and name, creating the direct message when it does not exist yet. | ||
|
|
||
| Uses it to fix direct messages addressed by username, such as the ones opened with **Reply in direct message**, not being found on the first lookup — which made opening a conversation cost an extra request and log an avoidable `Invalid Room` error. | ||
|
ricardogarim marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import type { IOmnichannelRoom, IRoom, Serialized } from '@rocket.chat/core-typings'; | ||
|
|
||
| import { mapMessageFromApi } from './mapMessageFromApi'; | ||
|
|
||
| // Livechat rooms are opened through the same flow, so their own date fields have to be covered too. | ||
| type SerializedRoom = Serialized<IRoom> & Partial<Serialized<Pick<IOmnichannelRoom, 'queuedAt' | 'closedAt'>>>; | ||
|
|
||
| // REST serializes Date fields to strings; the Rooms store is Date-typed. Every date-bearing key | ||
| // published by `roomFields` has to be revived, or it lands in the store as a string. | ||
| export const mapRoomFromApi = ({ _updatedAt, ts, lm, queuedAt, closedAt, lastMessage, ...room }: SerializedRoom): IRoom => | ||
| ({ | ||
| ...room, | ||
| ...(_updatedAt && { _updatedAt: new Date(_updatedAt) }), | ||
| ...(ts && { ts: new Date(ts) }), | ||
| ...(lm && { lm: new Date(lm) }), | ||
| ...(queuedAt && { queuedAt: new Date(queuedAt) }), | ||
| ...(closedAt && { closedAt: new Date(closedAt) }), | ||
| ...(lastMessage && { lastMessage: mapMessageFromApi(lastMessage) }), | ||
| }) as unknown as IRoom; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { | |
| isPrivateRoom, | ||
| isPublicRoom, | ||
| type IUser, | ||
| type RoomType, | ||
| } from '@rocket.chat/core-typings'; | ||
| import { Messages, Rooms, Users, Uploads, Subscriptions } from '@rocket.chat/models'; | ||
| import type { Notifications } from '@rocket.chat/rest-typings'; | ||
|
|
@@ -60,8 +61,10 @@ import { FileUpload } from '../../lib/media/file-upload'; | |
| import { notifyOnSubscriptionChanged } from '../../lib/notifyListener'; | ||
| import { openRoom } from '../../lib/openRoom'; | ||
| import type { RoomRoles } from '../../lib/roles/getRoomRoles'; | ||
| import { resolveDirectRoomTargets } from '../../lib/rooms/findDirectRoomByIdentifier'; | ||
| import { syncRolePrioritiesForRoomIfRequired } from '../../lib/rooms/syncRolePrioritiesForRoomIfRequired'; | ||
| import { unbanUserFromRoom } from '../../lib/unbanUserFromRoom'; | ||
| import { createDirectMessage } from '../../meteor-methods/messages/createDirectMessage'; | ||
| import { createDiscussion } from '../../meteor-methods/messages/createDiscussion'; | ||
| import { sendFileMessage } from '../../meteor-methods/messages/sendFileMessage'; | ||
| import { executeArchiveRoom } from '../../meteor-methods/rooms/archiveRoom'; | ||
|
|
@@ -76,7 +79,7 @@ import { executeUnarchiveRoom } from '../../meteor-methods/rooms/unarchiveRoom'; | |
| import { unmuteUserInRoom } from '../../meteor-methods/rooms/unmuteUserInRoom'; | ||
| import { saveNotificationSettingsMethod } from '../../meteor-methods/users/saveNotificationSettings'; | ||
| import type { NotificationFieldType } from '../../meteor-methods/users/saveNotificationSettings'; | ||
| import { roomsGetMethod } from '../../publications/room'; | ||
| import { findRoomByTypeAndName, roomsGetMethod } from '../../publications/room'; | ||
| import { settings } from '../../settings'; | ||
| import type { ExtractRoutesFromAPI } from '../ApiClass'; | ||
| import { API } from '../api'; | ||
|
|
@@ -109,7 +112,7 @@ export async function findRoomByIdOrName({ | |
| }): Promise<IRoom> { | ||
| if ( | ||
| (!('roomId' in params) && !('roomName' in params)) || | ||
| ('roomId' in params && !(params as { roomId?: string }).roomId && 'roomName' in params && !(params as { roomName?: string }).roomName) | ||
| ('roomId' in params && !params.roomId && 'roomName' in params && !(params as { roomName?: string }).roomName) | ||
| ) { | ||
| throw new Meteor.Error('error-roomid-param-not-provided', 'The parameter "roomId" or "roomName" is required'); | ||
| } | ||
|
|
@@ -516,6 +519,63 @@ API.v1.post( | |
| }, | ||
| ); | ||
|
|
||
| API.v1.post( | ||
| 'rooms.getOrCreate', | ||
| { | ||
| authRequired: false, | ||
| body: ajv.compile<{ type: RoomType; name: string }>({ | ||
| type: 'object', | ||
| properties: { | ||
| type: { type: 'string', enum: ['c', 'd', 'p', 'l'] }, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it doesn't make much sense to accept the 4 room types and then throwing on !d. |
||
| name: { type: 'string', minLength: 1 }, | ||
| }, | ||
| required: ['type', 'name'], | ||
| additionalProperties: false, | ||
| }), | ||
|
hacktron-app[bot] marked this conversation as resolved.
|
||
| response: { | ||
| 200: ajv.compile<{ room: IRoom }>({ | ||
| type: 'object', | ||
| properties: { | ||
| room: { type: 'object' }, | ||
| success: { type: 'boolean', enum: [true] }, | ||
| }, | ||
| required: ['room', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { type, name } = this.bodyParams; | ||
|
|
||
| const room = await findRoomByTypeAndName(this.userId ?? null, type, name); | ||
|
ricardogarim marked this conversation as resolved.
|
||
| if (room) { | ||
| return API.v1.success({ room }); | ||
| } | ||
|
|
||
| // Only direct messages can be created on demand; every other type must already exist. The | ||
| // caller may be anonymous here, since the route stays readable without a session. | ||
| if (type !== 'd' || !this.userId) { | ||
| return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room'); | ||
| } | ||
|
|
||
| const targets = await resolveDirectRoomTargets(name); | ||
| if (!targets) { | ||
| return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room'); | ||
| } | ||
|
|
||
| const { rid } = await createDirectMessage(targets, this.userId); | ||
|
coderabbitai[bot] marked this conversation as resolved.
ricardogarim marked this conversation as resolved.
|
||
|
|
||
| const created = await findRoomByTypeAndName(this.userId ?? null, type, rid); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why we can't return the result of createDirectMessage directly? 🤔 that would save a find and we know that if the function ends the room was created. |
||
| if (!created) { | ||
| return API.v1.failure('Invalid room [error-invalid-room]', 'error-invalid-room'); | ||
| } | ||
|
|
||
| return API.v1.success({ room: created }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }, | ||
| ); | ||
|
|
||
| API.v1.get( | ||
| 'rooms.info', | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import type { IRoom, IUser } from '@rocket.chat/core-typings'; | ||
| import { Rooms, Users } from '@rocket.chat/models'; | ||
|
|
||
| export const parseDirectRoomTargets = (identifier: string): string[] => identifier.split(',').map((username) => username.trim()); | ||
|
|
||
| const resolveUsernames = async (usernames: string[]): Promise<Pick<IUser, '_id' | 'username'>[] | null> => { | ||
| const users = await Users.findUsersByUsernames<Pick<IUser, '_id' | 'username'>>(usernames, { | ||
|
ricardogarim marked this conversation as resolved.
|
||
| projection: { _id: 1, username: 1 }, | ||
| }).toArray(); | ||
|
|
||
| return users.length === usernames.length ? users : null; | ||
| }; | ||
|
|
||
| export const resolveDirectRoomTargets = async (identifier: string): Promise<string[] | null> => { | ||
| const targets = [...new Set(parseDirectRoomTargets(identifier))]; | ||
|
|
||
| return (await resolveUsernames(targets)) ? targets : null; | ||
| }; | ||
|
|
||
| export const findDirectRoomByIdentifier = async (identifier: string, user: Pick<IUser, '_id' | 'username'>): Promise<IRoom | null> => { | ||
| const targets = parseDirectRoomTargets(identifier); | ||
|
|
||
| if (targets.length === 1) { | ||
| const byId = await Rooms.findByTypeAndNameOrId('d', targets[0]); | ||
| if (byId) { | ||
| return byId; | ||
|
ricardogarim marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| if (!user.username) { | ||
| return null; | ||
| } | ||
|
|
||
| const members = await resolveUsernames([...new Set([user.username, ...targets])]); | ||
| if (!members) { | ||
| return null; | ||
| } | ||
|
|
||
| const uids = members.map(({ _id }) => _id).sort(); | ||
|
|
||
| return Rooms.findOneDirectRoomContainingAllUserIDs(uids); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since its a minor the PR should be a feat