Paths

Table of Contentst

-
keyserver/src/
-
src/
-
fetchers/
2/4
message-fetchers.js
-
updaters/
2/4
thread-updaters.js
-
lib/
-
shared/
2/4
message-utils.js
-
messages/
2/4
message-specs.js
-
toggle-pin-message-spec.js
-
types/
-
message-types.js
-
messages/
-
toggle-pin.js
-
utils/
-
message-ops-utils.js

[keyserver/lib] Introduce a new message type and spec for toggling pins
ClosedPublic
Actions

Authored by rohan on Mar 22 2023, 9:38 AM.

Details

Reviewers

atul
ginsu
tomek
ashoat
• kuba

Commits

rCOMM196ba465b204: [keyserver/lib] Introduce a new message type and spec for toggling pins

Summary

Part of the way we want to alert clients that a message has been pinned / unpinned is by introducing a new message type and message spec to represent toggling a pin. I've decided to consolidate the two actions into one spec and handle the robotext / notif texts that are sent within the functions to avoid lots of duplicate code (much like update-relationship-message-spec.js). We could probably indicate a message preview like 'you pinned "This is a message to b..."', but if we want to do that I'd prefer to handle that in a later diff once the core keyserver implementation is done.

Depends on D7112

Linear: https://linear.app/comm/issue/ENG-3190/introduce-a-new-message-type-and-spec-for-toggling-pins

Test Plan

Confirmed that hasMinCodeVersion successfully works for both cases where the client should be able to see the new message type and vice versa.
Ran several pin / unpin actions on a variety of messages (text, images, videos) to confirm that the robotext is as expected

Sorry for the table view, I figured it'll make it easier to see specific images

	Text	One Image	Two Images	Video	Mixed Media
Original
Robotext
Database

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a subscriber: ashoat. · View Herald TranscriptMar 22 2023, 9:38 AM

rohan edited the test plan for this revision. (Show Details)Mar 22 2023, 9:46 AM

rohan attached a referenced file: F440873: Mixed Media (DB).png. (Show Details)

rohan attached a referenced file: F440872: Video (DB).png. (Show Details)

rohan attached a referenced file: F440871: Two Images (DB).png. (Show Details)

rohan attached a referenced file: F440870: One Image (DB).png. (Show Details)

rohan attached a referenced file: F440869: Text (DB).png. (Show Details)

rohan attached a referenced file: F440868: Mixed Media (Robotext).png. (Show Details)

rohan attached a referenced file: F440867: Video (Robotext).png. (Show Details)

rohan attached a referenced file: F440866: Two Images (Robotext).png. (Show Details)

rohan edited the test plan for this revision. (Show Details)Mar 22 2023, 9:50 AM

rohan edited the test plan for this revision. (Show Details)Mar 22 2023, 9:53 AM

Harbormaster completed remote builds in B17530: Diff 23998.Mar 22 2023, 9:54 AM

rohan requested review of this revision.Mar 22 2023, 9:54 AM

rohan edited the summary of this revision. (Show Details)Mar 22 2023, 9:54 AM

rohan edited the summary of this revision. (Show Details)

rohan added inline comments.Mar 22 2023, 10:01 AM

keyserver/src/updaters/thread-updaters.js
892 ↗	(On Diff #23998)	This is because (from my experience), one video will be `MULTIMEDIA` but so will a message with several mixed media (3 photos and 1 video for example).
907 ↗	(On Diff #23998)	I don't have a `return` value here yet since I still need to figure out what kind of update should be returned to notify the client of a message pin. Regardless though that won't be part of this diff

bartek added a subscriber: bartek.Mar 22 2023, 10:45 AM

bartek added inline comments.

keyserver/src/updaters/thread-updaters.js
892 ↗	(On Diff #23998)	This will not be always true - when encrypted images are introduced, they are gonna be of type MULTIMEDIA regardless of being image/video. I'd rather use the approach from contentStringFromMediaArray() to distinguish between image/video

rohan added inline comments.Mar 22 2023, 11:14 AM

keyserver/src/updaters/thread-updaters.js
892 ↗	(On Diff #23998)	Thanks for pointing that out, I wasn't aware! I'll update the diff with `contentStringForMediaArray` soon

Use contentStringForMediaArray and confirm robotext is still as expected

Screenshot 2023-03-22 at 2.22.31 PM.png (510×1 px, 61 KB)

rohan attached a referenced file: F440923: Screenshot 2023-03-22 at 2.22.31 PM.png. (Show Details)Mar 22 2023, 11:24 AM

Harbormaster completed remote builds in B17531: Diff 24001.Mar 22 2023, 11:41 AM

ashoat requested changes to this revision.Mar 22 2023, 3:37 PM

ashoat added a subscriber: • kuba.

ashoat added inline comments.

keyserver/src/updaters/thread-updaters.js
902 ↗	(On Diff #23998)	We shouldn't need this in `MessageData` here at all – we could fetch it on the keyserver at query time, by looking up the `targetMessageID` (or am I missing something?)
lib/shared/messages/toggle-pin-message-spec.js
35 ↗	(On Diff #23998)	Let's avoid storing this in the database if possible
45 ↗	(On Diff #23998)	If we make sure to fetch the pinned message in `fetchDerivedMessages`, then we should be able to access it here via `params.derivedMessages`. You should make sure to test the `IMAGES`/`MULTIMEDIA` case for that, not sure if `fetchDerivedMessages` handles that correctly but I'm guessing it does
lib/types/messages/toggle-pin.js
6 ↗	(On Diff #23998)	@kuba is using type 20 in D6961. Can you make sure there is no conflict?

This revision now requires changes to proceed.Mar 22 2023, 3:37 PM

Rest of the comments were discussed on Comm

lib/types/messages/toggle-pin.js
6 ↗	(On Diff #23998)	There shouldn't be a conflict until one of the stacks is landed, and then the other stack should just have to change the type from 20 to 21. I'll make sure to let him know if I end up landing ahead of him

Adding @kuba here – he is looking at fetchDerivedMessages right now in D7141

lib/types/messages/toggle-pin.js
5–29 ↗	(On Diff #24001)	Please make all of these types `$ReadOnly` by prefixing all parameters with `+`
6 ↗	(On Diff #23998)	Thank you!!

Discussed the revision mainly offline with @ashoat, so I'll copy the changes we discussed from our chat here, as well as outline any additional changes.

Main changes:

We should not store pinnedContent in MySQL, so it should be excluded from messageContentForServerDB
We should update fetchDerivedMessages to get the target message for message pinning
We should update rawMessageInfoFromServerDBRow to look at derivedMessages to determine pinnedContent
We do NOT want to change TogglePinMessageData

Additional Changes:

Extracted the logic of determining pinnedContent to a helper function
Make types in toggle-pin.js read only
Update rawMessageInfoFromClientDB to use a new helper function that takes in ClientDBMessageInfo and returns the pinnedContent (I can't re-use the same helper function since ClientDBMessageInfo is structured differently from RawMessageInfo (both with type being different types and the way media is stored)

Harbormaster completed remote builds in B17588: Diff 24077.Mar 24 2023, 7:38 AM

Really close!

keyserver/src/updaters/thread-updaters.js
883 ↗	(On Diff #24077)	When applying my feedback below, please try as hard as possible NOT to introduce any additional `await` keywords to this file
883–886 ↗	(On Diff #24077)	Can we check this before calling the `await dbQuery` on line 881, so that we don't cause the database to get in an inconsistent state?
lib/shared/messages/toggle-pin-message-spec.js
182–186 ↗	(On Diff #24077)	This looks wrong. `body` isn't usable by itself (has a trailing "in" and trailing "from"), you're including the `prefix` directly in the `body`, and you're not even setting the `prefix`. Please make sure you test both iOS and Android notifs Please make sure you're setting these correctly
198 ↗	(On Diff #24077)	Given you're not generating any notifs from this function, why are bothering to define `notificationTexts`?

This revision now requires changes to proceed.Mar 24 2023, 12:21 PM

rohan added inline comments.Mar 24 2023, 2:51 PM

keyserver/src/updaters/thread-updaters.js
883–886 ↗	(On Diff #24077)	Yeah that makes sense. I'm a little confused about When applying my feedback below, please try as hard as possible NOT to introduce any additional await keywords to this file I've moved this check above declaring `togglePinQuery` to make sure that only happens if targetMessage != null, and that definitely doesn't introduce any additional `await` keywords - were you suggesting something else or is that pretty much it? For what it's worth, I made the change and tested to confirm that the dbQuery is only executed if targetMessage exists.
lib/shared/messages/toggle-pin-message-spec.js
198 ↗	(On Diff #24077)	That's fair - I'll push handling the notifications down to when I take on https://linear.app/comm/issue/ENG-3391/handle-push-notifications-for-message-pinning (so I'll remove `notificationTexts` here).

ashoat added inline comments.Mar 24 2023, 2:56 PM

keyserver/src/updaters/thread-updaters.js
883–886 ↗	(On Diff #24077)	I'd like you to call `fetchMessageInfoByID` without introducing any new `await` keywords. Your diff should add a total of one new `await`, which is the one on line 898

Use Promise.all([...]) for checkThreadPermission and fetchMessageInfoByID to reduce the number of awaits (instead of await checkThreadPermission and await fetchMessageInfoByID)

Harbormaster completed remote builds in B17613: Diff 24107.Mar 24 2023, 6:47 PM

Please apply the requested change before landing!

keyserver/src/updaters/thread-updaters.js

855–868 ↗

(On Diff #24107)

Rethinking this, I don't think we need to bother with querying the messages table twice. How about this:

const targetMessage = await fetchMessageInfoByID(viewer, messageID);
if ((!targetMessage) {
  throw new ServerError('invalid_parameters');
}

const { threadID } = targetMessage;
const hasPermission = await checkThreadPermission(
  viewer,
  threadID,
  threadPermissions.MANAGE_PINS,
);
if (!hasPermission) {
  throw new ServerError('invalid_credentials');
}

This revision is now accepted and ready to land.Mar 25 2023, 3:00 AM

Incorporate requested feedback + test to make sure message pinning still works

Harbormaster completed remote builds in B17630: Diff 24127.Mar 25 2023, 5:07 AM

rohan added a child revision: D7175: [lib] Include an isPinned flag in ChatMessageInfoItem.Mar 25 2023, 6:44 AM

Rebase

Harbormaster completed remote builds in B17881: Diff 24496.Mar 31 2023, 4:37 AM

Change hasMinCodeVersion to version 205

Harbormaster completed remote builds in B18045: Diff 24726.Apr 6 2023, 7:42 AM

Rebase

Harbormaster completed remote builds in B18108: Diff 24808.Apr 7 2023, 9:10 AM

Code version 205 -> 207

Harbormaster completed remote builds in B18138: Diff 24840.Apr 7 2023, 1:43 PM

Rebase before landing

Harbormaster completed remote builds in B18149: Diff 24851.Apr 7 2023, 2:46 PM

Code version 207 --> 209 (not landing yet)

Harbormaster completed remote builds in B18167: Diff 24871.Apr 7 2023, 3:57 PM

Rebase before landing

This revision was landed with ongoing or failed builds.Apr 11 2023, 9:00 AM

Closed by commit rCOMM196ba465b204: [keyserver/lib] Introduce a new message type and spec for toggling pins (authored by RohanK6 <rohan.khanderia@gmail.com>). · Explain Why

This revision was automatically updated to reflect the committed changes.

RohanK6 <rohan.khanderia@gmail.com> added a commit: rCOMM196ba465b204: [keyserver/lib] Introduce a new message type and spec for toggling pins.

Harbormaster completed remote builds in B18243: Diff 24986.Apr 11 2023, 9:07 AM

Revision Contents
Changeset List

Path

Size

keyserver/

src/

fetchers/

message-fetchers.js

3 lines

updaters/

thread-updaters.js

27 lines

lib/

shared/

message-utils.js

16 lines

messages/

message-specs.js

2 lines

toggle-pin-message-spec.js

158 lines

types/

message-types.js

18 lines

messages/

toggle-pin.js

29 lines

utils/

message-ops-utils.js

17 lines

Diff 24871

View Options

keyserver/src/fetchers/message-fetchers.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js';		import { dbQuery, SQL } from '../database/database.js';
import {		import {
fetchThreadInfos,		fetchThreadInfos,
fetchServerThreadInfos,		fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos,		rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js';		} from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';		import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js';		import type { Viewer } from '../session/viewer.js';
		import { updateRole } from '../updaters/thread-updaters.js';

type InitialRoles = {		type InitialRoles = {
+default: RoleInfo,		+default: RoleInfo,
+creator: RoleInfo,		+creator: RoleInfo,
};		};
async function createInitialRolesForNewThread(		async function createInitialRolesForNewThread(
threadID: string,		threadID: string,
threadType: ThreadType,		threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	const hasPermission = await checkThreadPermission(
viewer,		viewer,
request.community,		request.community,
threadPermissions.CHANGE_ROLE,		threadPermissions.CHANGE_ROLE,
);		);
if (!hasPermission) {		if (!hasPermission) {
throw new ServerError('invalid_credentials');		throw new ServerError('invalid_credentials');
}		}

const { community, name, permissions, action } = request;		const { community, existingRoleID, name, permissions, action } = request;

for (const permission of permissions) {		for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) {		if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters');		throw new ServerError('invalid_parameters');
}		}
}		}

const [id] = await createIDs('roles', 1);		const [id] = await createIDs('roles', 1);
Show All 31 Lines	): Promise<RoleModificationResult> {

let query = SQL``;		let query = SQL``;
if (action === 'create_role') {		if (action === 'create_role') {
query = SQL`		query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time)		INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row})		VALUES (${row})
`;		`;
} else if (action === 'edit_role') {		} else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet");		query = SQL`
		UPDATE roles
		SET name = ${name}, permissions = ${permissionsBlob}
		WHERE id = ${existingRoleID}
		`;
}		}

await dbQuery(query);		await dbQuery(query);

		// The `updateRole` needs to occur after the role has been updated
		// in the database because it will want the most up to date role info
		// (permissions / name)
		if (action === 'edit_role') {
		atulUnsubmitted Not Done Inline Actions Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could move `await dbQuery(query);` within both the `(action === 'create_role')` (line 139) and `else if (action === 'edit_role')` (line 144) blocks? Not a huge deal, whatever you prefer atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
		rohanAuthorUnsubmitted Done Inline Actions Yeah we can, that should be cleaner too rohan: Yeah we can, that should be cleaner too
		if (!existingRoleID) {
		throw new ServerError('invalid_parameters');
		}

		const membersWithRole = threadInfo.members.filter(
		memberInfo => memberInfo.role === existingRoleID,
		);
		await updateRole(
		viewer,
		{
		threadID: community,
		role: existingRoleID,
		memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
		atulUnsubmitted Not Done Inline Actions Should we add this `.map` to the end of the `.filter` on lines 162-164? atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
		rohanAuthorUnsubmitted Done Inline Actions Sure that makes sense rohan: Sure that makes sense
		},
		{ silenceNewMessages: true },
		);
		}

const fetchServerThreadInfosResult = await fetchServerThreadInfos({		const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community,		threadID: community,
});		});
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;		const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community];		const serverThreadInfo = serverThreadInfos[community];

if (!serverThreadInfo) {		if (!serverThreadInfo) {
throw new ServerError('internal_error');		throw new ServerError('internal_error');
Show All 32 Lines

View Options

keyserver/src/updaters/thread-updaters.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js';		import { dbQuery, SQL } from '../database/database.js';
import {		import {
fetchThreadInfos,		fetchThreadInfos,
fetchServerThreadInfos,		fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos,		rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js';		} from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';		import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js';		import type { Viewer } from '../session/viewer.js';
		import { updateRole } from '../updaters/thread-updaters.js';

type InitialRoles = {		type InitialRoles = {
+default: RoleInfo,		+default: RoleInfo,
+creator: RoleInfo,		+creator: RoleInfo,
};		};
async function createInitialRolesForNewThread(		async function createInitialRolesForNewThread(
threadID: string,		threadID: string,
threadType: ThreadType,		threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	const hasPermission = await checkThreadPermission(
viewer,		viewer,
request.community,		request.community,
threadPermissions.CHANGE_ROLE,		threadPermissions.CHANGE_ROLE,
);		);
if (!hasPermission) {		if (!hasPermission) {
throw new ServerError('invalid_credentials');		throw new ServerError('invalid_credentials');
}		}

const { community, name, permissions, action } = request;		const { community, existingRoleID, name, permissions, action } = request;

for (const permission of permissions) {		for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) {		if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters');		throw new ServerError('invalid_parameters');
}		}
}		}

const [id] = await createIDs('roles', 1);		const [id] = await createIDs('roles', 1);
Show All 31 Lines	): Promise<RoleModificationResult> {

let query = SQL``;		let query = SQL``;
if (action === 'create_role') {		if (action === 'create_role') {
query = SQL`		query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time)		INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row})		VALUES (${row})
`;		`;
} else if (action === 'edit_role') {		} else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet");		query = SQL`
		UPDATE roles
		SET name = ${name}, permissions = ${permissionsBlob}
		WHERE id = ${existingRoleID}
		`;
}		}

await dbQuery(query);		await dbQuery(query);

		// The `updateRole` needs to occur after the role has been updated
		// in the database because it will want the most up to date role info
		// (permissions / name)
		if (action === 'edit_role') {
		atulUnsubmitted Not Done Inline Actions Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could move `await dbQuery(query);` within both the `(action === 'create_role')` (line 139) and `else if (action === 'edit_role')` (line 144) blocks? Not a huge deal, whatever you prefer atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
		rohanAuthorUnsubmitted Done Inline Actions Yeah we can, that should be cleaner too rohan: Yeah we can, that should be cleaner too
		if (!existingRoleID) {
		throw new ServerError('invalid_parameters');
		}

		const membersWithRole = threadInfo.members.filter(
		memberInfo => memberInfo.role === existingRoleID,
		);
		await updateRole(
		viewer,
		{
		threadID: community,
		role: existingRoleID,
		memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
		atulUnsubmitted Not Done Inline Actions Should we add this `.map` to the end of the `.filter` on lines 162-164? atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
		rohanAuthorUnsubmitted Done Inline Actions Sure that makes sense rohan: Sure that makes sense
		},
		{ silenceNewMessages: true },
		);
		}

const fetchServerThreadInfosResult = await fetchServerThreadInfos({		const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community,		threadID: community,
});		});
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;		const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community];		const serverThreadInfo = serverThreadInfos[community];

if (!serverThreadInfo) {		if (!serverThreadInfo) {
throw new ServerError('internal_error');		throw new ServerError('internal_error');
Show All 32 Lines

View Options

lib/shared/message-utils.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js';		import { dbQuery, SQL } from '../database/database.js';
import {		import {
fetchThreadInfos,		fetchThreadInfos,
fetchServerThreadInfos,		fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos,		rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js';		} from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';		import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js';		import type { Viewer } from '../session/viewer.js';
		import { updateRole } from '../updaters/thread-updaters.js';

type InitialRoles = {		type InitialRoles = {
+default: RoleInfo,		+default: RoleInfo,
+creator: RoleInfo,		+creator: RoleInfo,
};		};
async function createInitialRolesForNewThread(		async function createInitialRolesForNewThread(
threadID: string,		threadID: string,
threadType: ThreadType,		threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	const hasPermission = await checkThreadPermission(
viewer,		viewer,
request.community,		request.community,
threadPermissions.CHANGE_ROLE,		threadPermissions.CHANGE_ROLE,
);		);
if (!hasPermission) {		if (!hasPermission) {
throw new ServerError('invalid_credentials');		throw new ServerError('invalid_credentials');
}		}

const { community, name, permissions, action } = request;		const { community, existingRoleID, name, permissions, action } = request;

for (const permission of permissions) {		for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) {		if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters');		throw new ServerError('invalid_parameters');
}		}
}		}

const [id] = await createIDs('roles', 1);		const [id] = await createIDs('roles', 1);
Show All 31 Lines	): Promise<RoleModificationResult> {

let query = SQL``;		let query = SQL``;
if (action === 'create_role') {		if (action === 'create_role') {
query = SQL`		query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time)		INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row})		VALUES (${row})
`;		`;
} else if (action === 'edit_role') {		} else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet");		query = SQL`
		UPDATE roles
		SET name = ${name}, permissions = ${permissionsBlob}
		WHERE id = ${existingRoleID}
		`;
}		}

await dbQuery(query);		await dbQuery(query);

		// The `updateRole` needs to occur after the role has been updated
		// in the database because it will want the most up to date role info
		// (permissions / name)
		if (action === 'edit_role') {
		atulUnsubmitted Not Done Inline Actions Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could move `await dbQuery(query);` within both the `(action === 'create_role')` (line 139) and `else if (action === 'edit_role')` (line 144) blocks? Not a huge deal, whatever you prefer atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
		rohanAuthorUnsubmitted Done Inline Actions Yeah we can, that should be cleaner too rohan: Yeah we can, that should be cleaner too
		if (!existingRoleID) {
		throw new ServerError('invalid_parameters');
		}

		const membersWithRole = threadInfo.members.filter(
		memberInfo => memberInfo.role === existingRoleID,
		);
		await updateRole(
		viewer,
		{
		threadID: community,
		role: existingRoleID,
		memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
		atulUnsubmitted Not Done Inline Actions Should we add this `.map` to the end of the `.filter` on lines 162-164? atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
		rohanAuthorUnsubmitted Done Inline Actions Sure that makes sense rohan: Sure that makes sense
		},
		{ silenceNewMessages: true },
		);
		}

const fetchServerThreadInfosResult = await fetchServerThreadInfos({		const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community,		threadID: community,
});		});
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;		const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community];		const serverThreadInfo = serverThreadInfos[community];

if (!serverThreadInfo) {		if (!serverThreadInfo) {
throw new ServerError('internal_error');		throw new ServerError('internal_error');
Show All 32 Lines

View Options

lib/shared/messages/message-specs.js

Show All 21 Lines
import { dbQuery, SQL } from '../database/database.js';		import { dbQuery, SQL } from '../database/database.js';
import {		import {
fetchThreadInfos,		fetchThreadInfos,
fetchServerThreadInfos,		fetchServerThreadInfos,
rawThreadInfosFromServerThreadInfos,		rawThreadInfosFromServerThreadInfos,
} from '../fetchers/thread-fetchers.js';		} from '../fetchers/thread-fetchers.js';
import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';		import { checkThreadPermission } from '../fetchers/thread-permission-fetchers.js';
import type { Viewer } from '../session/viewer.js';		import type { Viewer } from '../session/viewer.js';
		import { updateRole } from '../updaters/thread-updaters.js';

type InitialRoles = {		type InitialRoles = {
+default: RoleInfo,		+default: RoleInfo,
+creator: RoleInfo,		+creator: RoleInfo,
};		};
async function createInitialRolesForNewThread(		async function createInitialRolesForNewThread(
threadID: string,		threadID: string,
threadType: ThreadType,		threadType: ThreadType,
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines	const hasPermission = await checkThreadPermission(
viewer,		viewer,
request.community,		request.community,
threadPermissions.CHANGE_ROLE,		threadPermissions.CHANGE_ROLE,
);		);
if (!hasPermission) {		if (!hasPermission) {
throw new ServerError('invalid_credentials');		throw new ServerError('invalid_credentials');
}		}

const { community, name, permissions, action } = request;		const { community, existingRoleID, name, permissions, action } = request;

for (const permission of permissions) {		for (const permission of permissions) {
if (!userSurfacedPermissionsSet.has(permission)) {		if (!userSurfacedPermissionsSet.has(permission)) {
throw new ServerError('invalid_parameters');		throw new ServerError('invalid_parameters');
}		}
}		}

const [id] = await createIDs('roles', 1);		const [id] = await createIDs('roles', 1);
Show All 31 Lines	): Promise<RoleModificationResult> {

let query = SQL``;		let query = SQL``;
if (action === 'create_role') {		if (action === 'create_role') {
query = SQL`		query = SQL`
INSERT INTO roles (id, thread, name, permissions, creation_time)		INSERT INTO roles (id, thread, name, permissions, creation_time)
VALUES (${row})		VALUES (${row})
`;		`;
} else if (action === 'edit_role') {		} else if (action === 'edit_role') {
throw new ServerError("unimplemented: can't edit roles yet");		query = SQL`
		UPDATE roles
		SET name = ${name}, permissions = ${permissionsBlob}
		WHERE id = ${existingRoleID}
		`;
}		}

await dbQuery(query);		await dbQuery(query);

		// The `updateRole` needs to occur after the role has been updated
		// in the database because it will want the most up to date role info
		// (permissions / name)
		if (action === 'edit_role') {
		atulUnsubmitted Not Done Inline Actions Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could move `await dbQuery(query);` within both the `(action === 'create_role')` (line 139) and `else if (action === 'edit_role')` (line 144) blocks? Not a huge deal, whatever you prefer atul: Instead of two separate `(action === 'edit_role')` blocks, could we merge the two? We could…
		rohanAuthorUnsubmitted Done Inline Actions Yeah we can, that should be cleaner too rohan: Yeah we can, that should be cleaner too
		if (!existingRoleID) {
		throw new ServerError('invalid_parameters');
		}

		const membersWithRole = threadInfo.members.filter(
		memberInfo => memberInfo.role === existingRoleID,
		);
		await updateRole(
		viewer,
		{
		threadID: community,
		role: existingRoleID,
		memberIDs: membersWithRole.map(memberInfo => memberInfo.id),
		atulUnsubmitted Not Done Inline Actions Should we add this `.map` to the end of the `.filter` on lines 162-164? atul: Should we add this `.map` to the end of the `.filter` on lines 162-164?
		rohanAuthorUnsubmitted Done Inline Actions Sure that makes sense rohan: Sure that makes sense
		},
		{ silenceNewMessages: true },
		);
		}

const fetchServerThreadInfosResult = await fetchServerThreadInfos({		const fetchServerThreadInfosResult = await fetchServerThreadInfos({
threadID: community,		threadID: community,
});		});
const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;		const { threadInfos: serverThreadInfos } = fetchServerThreadInfosResult;
const serverThreadInfo = serverThreadInfos[community];		const serverThreadInfo = serverThreadInfos[community];

if (!serverThreadInfo) {		if (!serverThreadInfo) {
throw new ServerError('internal_error');		throw new ServerError('internal_error');
Show All 32 Lines