Page MenuHomePhabricator
Differential D12829

[lib] Specify messageID in sendTextMessageSpec
ClosedPublic
Actions

Authored by ashoat on Jul 20 2024, 10:04 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 12, 1:09 AM
Unknown Object (File)
Tue, Mar 11, 5:45 AM
Unknown Object (File)
Feb 27 2025, 9:59 AM
Unknown Object (File)
Feb 24 2025, 6:28 AM
Unknown Object (File)
Feb 21 2025, 1:29 PM
Unknown Object (File)
Feb 21 2025, 2:55 AM
Unknown Object (File)
Feb 21 2025, 2:55 AM
Unknown Object (File)
Feb 21 2025, 2:53 AM
View All 76 Files
Subscribers
None

Details

Reviewers
tomek
kamil
marcin
Commits
rCOMM3980d47ad71f: [lib] Specify messageID in sendTextMessageSpec
Summary

Testing revealed that I had forgotten to specify this.

Depends on D12828

Test Plan

I followed the test plan described in this Linear comment. Before this diff, I saw this error, which disappeared afterwards:

Failed processing Olm P2P message: [Invariant Violation: new messageInfos should have serverID]

Diff Detail

Repository
rCOMM Comm
Branch
ashoat/dmop
Lint
No Lint Coverage
Unit
No Test Coverage

Revision Contents
Changeset List

PathSize
lib/
shared/
dm-ops/
3 lines

Diff 42595

View Options

lib/shared/dm-ops/send-text-message-spec.js

// @flow // @flow
import invariant from 'invariant'; import invariant from 'invariant';
import { import {
pushTypes, pushTypes,
type CreateMessageInfoParams, type CreateMessageInfoParams,
type MessageSpec, type MessageSpec,
type RobotextParams, type RobotextParams,
} from './message-spec.js'; } from './message-spec.js';
import { joinResult } from './utils.js'; import { joinResult } from './utils.js';
import type { PlatformDetails } from '../../types/device-types.js';
import { messageTypes } from '../../types/message-types-enum.js'; import { messageTypes } from '../../types/message-types-enum.js';
import type { import type {
MessageInfo, MessageInfo,
ClientDBMessageInfo, ClientDBMessageInfo,
} from '../../types/message-types.js'; } from '../../types/message-types.js';
import { import {
type ChangeRoleMessageData, type ChangeRoleMessageData,
type ChangeRoleMessageInfo, type ChangeRoleMessageInfo,
type RawChangeRoleMessageInfo, type RawChangeRoleMessageInfo,
rawChangeRoleMessageInfoValidator, rawChangeRoleMessageInfoValidator,
} from '../../types/messages/change-role.js'; } from '../../types/messages/change-role.js';
import type { RawUnsupportedMessageInfo } from '../../types/messages/unsupported';
import type { NotifTexts } from '../../types/notif-types.js'; import type { NotifTexts } from '../../types/notif-types.js';
import type { ThreadInfo } from '../../types/thread-types.js'; import type { ThreadInfo } from '../../types/thread-types.js';
import type { RelativeUserInfo } from '../../types/user-types.js'; import type { RelativeUserInfo } from '../../types/user-types.js';
import { import {
ET, ET,
type EntityText, type EntityText,
pluralizeEntityText, pluralizeEntityText,
} from '../../utils/entity-text.js'; } from '../../utils/entity-text.js';
import { values } from '../../utils/objects.js'; import { values } from '../../utils/objects.js';
import { notifRobotextForMessageInfo } from '../notif-utils.js'; import { notifRobotextForMessageInfo } from '../notif-utils.js';
import { NEXT_CODE_VERSION, hasMinCodeVersion } from '../version-utils.js';
export const changeRoleMessageSpec: MessageSpec< export const changeRoleMessageSpec: MessageSpec<
ChangeRoleMessageData, ChangeRoleMessageData,
RawChangeRoleMessageInfo, RawChangeRoleMessageInfo,
ChangeRoleMessageInfo, ChangeRoleMessageInfo,
> = Object.freeze({ > = Object.freeze({
messageContentForServerDB( messageContentForServerDB(
data: ChangeRoleMessageData | RawChangeRoleMessageInfo, data: ChangeRoleMessageData | RawChangeRoleMessageInfo,
): string { ): string {
return JSON.stringify({ return JSON.stringify({
userIDs: data.userIDs, userIDs: data.userIDs,
newRole: data.newRole, newRole: data.newRole,
roleName: data.roleName,
}); });
}, },
messageContentForClientDB(data: RawChangeRoleMessageInfo): string { messageContentForClientDB(data: RawChangeRoleMessageInfo): string {
return this.messageContentForServerDB(data); return this.messageContentForServerDB(data);
}, },
rawMessageInfoFromServerDBRow(row: Object): RawChangeRoleMessageInfo { rawMessageInfoFromServerDBRow(row: Object): RawChangeRoleMessageInfo {
const content = JSON.parse(row.content); const content = JSON.parse(row.content);
return { return {
type: messageTypes.CHANGE_ROLE, type: messageTypes.CHANGE_ROLE,
id: row.id.toString(), id: row.id.toString(),
threadID: row.threadID.toString(), threadID: row.threadID.toString(),
time: row.time, time: row.time,
creatorID: row.creatorID.toString(), creatorID: row.creatorID.toString(),
userIDs: content.userIDs, userIDs: content.userIDs,
newRole: content.newRole, newRole: content.newRole,
roleName: content.roleName,
}; };
}, },
rawMessageInfoFromClientDB( rawMessageInfoFromClientDB(
clientDBMessageInfo: ClientDBMessageInfo, clientDBMessageInfo: ClientDBMessageInfo,
): RawChangeRoleMessageInfo { ): RawChangeRoleMessageInfo {
invariant( invariant(
clientDBMessageInfo.content !== undefined && clientDBMessageInfo.content !== undefined &&
clientDBMessageInfo.content !== null, clientDBMessageInfo.content !== null,
'content must be defined for ChangeRole', 'content must be defined for ChangeRole',
); );
const content = JSON.parse(clientDBMessageInfo.content); const content = JSON.parse(clientDBMessageInfo.content);
const rawChangeRoleMessageInfo: RawChangeRoleMessageInfo = { const rawChangeRoleMessageInfo: RawChangeRoleMessageInfo = {
type: messageTypes.CHANGE_ROLE, type: messageTypes.CHANGE_ROLE,
id: clientDBMessageInfo.id, id: clientDBMessageInfo.id,
threadID: clientDBMessageInfo.thread, threadID: clientDBMessageInfo.thread,
time: parseInt(clientDBMessageInfo.time), time: parseInt(clientDBMessageInfo.time),
creatorID: clientDBMessageInfo.user, creatorID: clientDBMessageInfo.user,
userIDs: content.userIDs, userIDs: content.userIDs,
newRole: content.newRole, newRole: content.newRole,
roleName: content.roleName,
}; };
return rawChangeRoleMessageInfo; return rawChangeRoleMessageInfo;
}, },
createMessageInfo( createMessageInfo(
rawMessageInfo: RawChangeRoleMessageInfo, rawMessageInfo: RawChangeRoleMessageInfo,
creator: RelativeUserInfo, creator: RelativeUserInfo,
params: CreateMessageInfoParams, params: CreateMessageInfoParams,
): ChangeRoleMessageInfo { ): ChangeRoleMessageInfo {
const members = params.createRelativeUserInfos(rawMessageInfo.userIDs); const members = params.createRelativeUserInfos(rawMessageInfo.userIDs);
return { return {
type: messageTypes.CHANGE_ROLE, type: messageTypes.CHANGE_ROLE,
id: rawMessageInfo.id, id: rawMessageInfo.id,
threadID: rawMessageInfo.threadID, threadID: rawMessageInfo.threadID,
creator, creator,
time: rawMessageInfo.time, time: rawMessageInfo.time,
members, members,
newRole: rawMessageInfo.newRole, newRole: rawMessageInfo.newRole,
roleName: rawMessageInfo.roleName,
}; };
}, },
rawMessageInfoFromMessageData( rawMessageInfoFromMessageData(
messageData: ChangeRoleMessageData, messageData: ChangeRoleMessageData,
id: ?string, id: ?string,
): RawChangeRoleMessageInfo { ): RawChangeRoleMessageInfo {
invariant(id, 'RawChangeRoleMessageInfo needs id'); invariant(id, 'RawChangeRoleMessageInfo needs id');
Show All 9 Lines ): EntityText {
const creator = ET.user({ userInfo: messageInfo.creator }); const creator = ET.user({ userInfo: messageInfo.creator });
const affectedUsers = pluralizeEntityText( const affectedUsers = pluralizeEntityText(
users.map(user => ET`${ET.user({ userInfo: user })}`), users.map(user => ET`${ET.user({ userInfo: user })}`),
); );
const { threadInfo } = params; const { threadInfo } = params;
invariant(threadInfo, 'ThreadInfo should be set for CHANGE_ROLE message'); invariant(threadInfo, 'ThreadInfo should be set for CHANGE_ROLE message');
const roleName = threadInfo.roles[messageInfo.newRole].name;
const threadRoleName = threadInfo.roles[messageInfo.newRole]?.name || null;
ashoatUnsubmitted
Not Done
Inline Actions

What's the || null for? Typically ?? null is preferred is you really want to coerce to null, but I'm not clear on why you want to do that

ashoat: What's the `|| null` for? Typically `?? null` is preferred is you really want to coerce to…
const messageInfoRoleName = messageInfo.roleName;
const roleName = threadRoleName || messageInfoRoleName;
ashoatUnsubmitted
Not Done
Inline Actions

?? instead of ||

ashoat: `??` instead of `||`
invariant(
roleName,
'roleName should either be derived from threadInfo or messageInfo',
);
return ET`${creator} assigned ${affectedUsers} the \"${roleName}\" role`; return ET`${creator} assigned ${affectedUsers} the \"${roleName}\" role`;
}, },
async notificationTexts( async notificationTexts(
messageInfos: $ReadOnlyArray<MessageInfo>, messageInfos: $ReadOnlyArray<MessageInfo>,
threadInfo: ThreadInfo, threadInfo: ThreadInfo,
): Promise<NotifTexts> { ): Promise<NotifTexts> {
Show All 23 Lines ): Promise<NotifTexts> {
})}`; })}`;
return { return {
merged, merged,
title: threadInfo.uiName, title: threadInfo.uiName,
body: robotext, body: robotext,
}; };
}, },
shimUnsupportedMessageInfo(
rawMessageInfo: RawChangeRoleMessageInfo,
platformDetails: ?PlatformDetails,
): RawChangeRoleMessageInfo | RawUnsupportedMessageInfo {
if (hasMinCodeVersion(platformDetails, { native: NEXT_CODE_VERSION })) {
rohanAuthorUnsubmitted
Done
Inline Actions

I'm not sure if this is the right constant to use here / what to do prior to landing.

Before landing, should I replace it like usual with current code version + 2 to include public and staff releases Or is that part of the release process now and I should leave it as is?

rohan: I'm not sure if this is the right constant to use here / what to do prior to landing. Before…
ashoatUnsubmitted
Not Done
Inline Actions

Or is that part of the release process now and I should leave it as is?

That's the idea, but it helps to ping @atul to give him a heads-up

ashoat: > Or is that part of the release process now and I should leave it as is? That's the idea, but…
return rawMessageInfo;
}
const { id } = rawMessageInfo;
invariant(id !== null && id !== undefined, 'id should be set on server');
return {
type: messageTypes.UNSUPPORTED,
id,
threadID: rawMessageInfo.threadID,
creatorID: rawMessageInfo.creatorID,
time: rawMessageInfo.time,
robotext: 'changed the role of some members',
ashoatUnsubmitted
Not Done
Inline Actions

Should we make this look more like the result of the robotext function? Is it possible to factor out the assigned ${affectedUsers} the \"${roleName}\" role part of the robotext method and call it here?

Worth noting that there are some nuances here for converting from`EntityText` to a string since it requires ENS resolution. Some ideas:

  1. Call entityTextToRawString and accept that ENS resolution would not occur
  2. Use "some members" or "a member" instead of affectedUsers for this case, so that ENS resolution doesn't matter. Then you can call entityTextToRawString without worrying
  3. (Best but hardest) call getEntityTextAsString and do ENS resolution, but first you'll need to convert shimUnsupportedMessageInfo and shimUnsupportedRawMessageInfos into async functions
ashoat: Should we make this look more like the result of the `robotext` function? Is it possible to…
rohanAuthorUnsubmitted
Done
Inline Actions

I thought the idea was that older clients won't have access to the new roleName field in the first place, so we'd have to shim the message. The original way we're getting the roleName is by accessing threadInfo.roles[messageInfo.newRole].name, but I took a look at the message-spec and it doesn't look like there's a way we can access params to get the threadInfo.

TLDR of that is I don't think we can access the role name for older clients with just the messageInfo.

Unless maybe I'm misundestanding the problem with 'older clients', and accessing messageInfo.roleName here won't actually be an issue

rohan: I thought the idea was that older clients won't have access to the new `roleName` field in the…
ashoatUnsubmitted
Not Done
Inline Actions

I think shimUnsupportedMessageInfo gets called on the keyserver, not on clients

ashoat: I think `shimUnsupportedMessageInfo` gets called on the keyserver, not on clients
rohanAuthorUnsubmitted
Done
Inline Actions

Ah yeah good point

rohan: Ah yeah good point
unsupportedMessageInfo: rawMessageInfo,
};
},
unshimMessageInfo(
unwrapped: RawChangeRoleMessageInfo,
): RawChangeRoleMessageInfo {
return unwrapped;
},
notificationCollapseKey(rawMessageInfo: RawChangeRoleMessageInfo): string { notificationCollapseKey(rawMessageInfo: RawChangeRoleMessageInfo): string {
return joinResult( return joinResult(
rawMessageInfo.type, rawMessageInfo.type,
rawMessageInfo.threadID, rawMessageInfo.threadID,
rawMessageInfo.creatorID, rawMessageInfo.creatorID,
rawMessageInfo.newRole, rawMessageInfo.newRole,
); );
}, },
generatesNotifs: async () => pushTypes.NOTIF, generatesNotifs: async () => pushTypes.NOTIF,
validator: rawChangeRoleMessageInfoValidator, validator: rawChangeRoleMessageInfoValidator,
}); });