Page MenuHomePhabricator
Differential D3793

Add amqp-cpp
ClosedPublic
Actions

Authored by • jon on Apr 20 2022, 2:12 PM.
Tags
None
Referenced Files
F5081784: D3793.id11679.diff
Sat, Mar 29, 6:37 AM
Unknown Object (File)
Fri, Mar 28, 4:27 PM
Unknown Object (File)
Tue, Mar 25, 3:55 PM
Unknown Object (File)
Thu, Mar 20, 10:00 PM
Unknown Object (File)
Wed, Mar 19, 7:26 PM
Unknown Object (File)
Mon, Mar 17, 11:56 AM
Unknown Object (File)
Thu, Mar 13, 8:12 AM
Unknown Object (File)
Mon, Mar 10, 1:40 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
• jim
tomek
varun

Details

Reviewers
varun
• max
• jim
Commits
rCOMM9122b99941b2: Add amqp-cpp
Summary

Add tunnelbroker's amqp-cpp dependency.

This isn't currently available in nixpkgs, so we need to package it locally.

Depends on D3784

Test Plan

nix build .#amqp-cpp

Diff Detail

Repository
rCOMM Comm
Branch
joringer/add-nix-clean (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• jon created this revision.Apr 20 2022, 2:12 PM
Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 20 2022, 2:12 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Apr 20 2022, 2:13 PM
Harbormaster failed remote builds in B8364: Diff 11676!
• jon updated this revision to Diff 11677.Apr 20 2022, 2:16 PM

Accidental inclusion of unstaged files

Harbormaster completed remote builds in B8365: Diff 11677.Apr 20 2022, 2:21 PM
• jon requested review of this revision.Apr 20 2022, 2:21 PM
• jon updated this revision to Diff 11679.Apr 20 2022, 2:24 PM

Fix build with tcp extension

Harbormaster completed remote builds in B8367: Diff 11679.Apr 20 2022, 2:29 PM
• jon updated this revision to Diff 11998.Apr 27 2022, 10:09 AM

Remove accidental tunnelbroker mention

Harbormaster completed remote builds in B8614: Diff 11998.Apr 27 2022, 10:14 AM
varun added a reviewer: varun.Apr 27 2022, 10:28 AM
varun added a subscriber: varun.

think i should be a reviewer on this

ashoat added a reviewer: • max.Apr 27 2022, 1:11 PM
ashoat added a reviewer: • jim.
ashoat added a subscriber: • jim.

Adding @geekbrother because AMQP pertains to Tunnelbroker, and adding @jimpo because there's some CMake stuff here

• jon added a child revision: D3865: Add protobuf with corrected cmake build.Apr 27 2022, 2:33 PM
• max added a comment.May 2 2022, 4:10 AM

I think the package is available as libamqpcpp in nix packages. I'm not sure it's built with the -DAMQP-CPP_LINUX_TCP=ON.
But maybe it would be easier to use it and just pass the flags to the nix build, and nix rebuilds it. @jonringer-comm, what do you think?

• max requested changes to this revision.May 2 2022, 4:10 AM
This revision now requires changes to proceed.May 2 2022, 4:10 AM
• jon added a comment.EditedMay 2 2022, 12:38 PM

@geekbrother you're right, but it looks like it just uses the exported makeFile to build the project https://github.com/NixOS/nixpkgs/blob/634141959076a8ab69ca2cca0f266852256d79ee/pkgs/development/libraries/libamqpcpp/default.nix

The implications is that it doesn't export a cmake/amqpcppConfig.cmake

ashoat added a comment.May 2 2022, 12:51 PM

@jonringer-comm make sure to hit "Re-request review" in the dropdown at the bottom of the page, to make sure this diff goes back to @geekbrother's queue and he sees your response!

• jon requested review of this revision.May 2 2022, 12:54 PM
• max accepted this revision.May 4 2022, 11:35 AM
In D3793#108796, @jonringer-comm wrote:

@geekbrother you're right, but it looks like it just uses the exported makeFile to build the project https://github.com/NixOS/nixpkgs/blob/634141959076a8ab69ca2cca0f266852256d79ee/pkgs/development/libraries/libamqpcpp/default.nix

The implications is that it doesn't export a cmake/amqpcppConfig.cmake

That seems like a solution here. Thank you for the clarification @jonringer-comm !

varun accepted this revision.May 5 2022, 10:58 AM
This revision is now accepted and ready to land.May 5 2022, 10:58 AM
Closed by commit rCOMM9122b99941b2: Add amqp-cpp (authored by • jon). · Explain WhyMay 6 2022, 2:15 AM
This revision was automatically updated to reflect the committed changes.
• jon added a commit: rCOMM9122b99941b2: Add amqp-cpp.
• karol removed a subscriber: • karol.May 6 2022, 3:55 AM

Revision Contents
Changeset List

PathSize
nix/
35 lines
4 lines

Diff 11679

View Options

nix/amqp-cpp.nix

// @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,
}); });
View Options

nix/overlay.nix

// @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,
}); });