Page MenuHomePhabricator
Differential D13490

[lib] Handle a case where viewer is added to a thread while being its member
ClosedPublic
Actions

Authored by tomek on Sep 26 2024, 4:16 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 26, 6:03 PM
Unknown Object (File)
Feb 21 2025, 1:58 PM
Unknown Object (File)
Feb 18 2025, 10:11 AM
Unknown Object (File)
Feb 16 2025, 9:05 AM
Unknown Object (File)
Jan 31 2025, 4:03 PM
Unknown Object (File)
Jan 31 2025, 4:03 PM
Unknown Object (File)
Jan 31 2025, 4:03 PM
Unknown Object (File)
Jan 31 2025, 4:03 PM
View All 63 Files
Subscribers
ashoat

Details

Reviewers
kamil
will
ashoat
Commits
rCOMMa432a967f452: [lib] Handle a case where viewer is added to a thread while being its member
Summary

It is possible that we receive an operation about viewer being added to a thread while the thread is already present in the store and viewer is its member. We need to make sure that we don't override the whole thread and instead update it as little as possible.

https://linear.app/comm/issue/ENG-8720/add-protection-against-thick-thread-creation-overwriting-existing

Depends on D13483

Test Plan

Create a sidebar, add, remove, and add a user and check on that user's device if the thread is displayed and updated correctly.

Diff Detail

Repository
rCOMM Comm
Branch
improvements2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek created this revision.Sep 26 2024, 4:16 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 26 2024, 4:16 AM
tomek added inline comments.Sep 26 2024, 4:18 AM
lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
183–189

The message is already included in JOIN_THREAD

Harbormaster completed remote builds in B31881: Diff 44599.Sep 26 2024, 4:33 AM
tomek requested review of this revision.Sep 26 2024, 4:33 AM
ashoat accepted this revision.Sep 26 2024, 8:13 AM
ashoat added inline comments.
lib/shared/dm-ops/add-members-spec.js
58

Nit: shorthand

lib/shared/dm-ops/add-viewer-to-thread-members-spec.js
117

Unrelated to this diff, but I did some investigation as to why we still have permissions for members here.

The context is that in D12789, we removed member permissions from the SQLite DB on native clients.

I know Atul never finished it, but I was under the impression that the only thing left was to implement an equivalent migration on web.

However, looking at the types it seems like the changes were made for thin threads but NOT for thick threads.

I'm guessing Atul's work intersected with the thick threads work, and he ended up making the changes only for thin threads.

I'm not 100% sure, but I suspect we should probably do the same for thick threads as thin threads (calculate the member permissions "on the fly") instead of storing them in the DB. I created ENG-9404 to track this.

This revision is now accepted and ready to land.Sep 26 2024, 8:13 AM
tomek updated this revision to Diff 44623.Sep 27 2024, 3:28 AM

Shorthand

Harbormaster completed remote builds in B31898: Diff 44623.Sep 27 2024, 3:49 AM
Closed by commit rCOMMa432a967f452: [lib] Handle a case where viewer is added to a thread while being its member (authored by tomek). · Explain WhySep 27 2024, 3:54 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMa432a967f452: [lib] Handle a case where viewer is added to a thread while being its member.

Revision Contents
Changeset List

PathSize
lib/
shared/
dm-ops/
73 lines
58 lines

Diff 44599

View Options

lib/shared/dm-ops/add-members-spec.js

Loading...
View Options

lib/shared/dm-ops/add-viewer-to-thread-members-spec.js

// @flow // @flow
import uuid from 'uuid'; import uuid from 'uuid';
import { createPermissionsForNewMembers } from './add-members-spec.js';
import { createThickRawThreadInfo } from './create-thread-spec.js'; import { createThickRawThreadInfo } from './create-thread-spec.js';
import type { import type {
DMOperationSpec, DMOperationSpec,
ProcessDMOperationUtilities, ProcessDMOperationUtilities,
} from './dm-op-spec.js'; } from './dm-op-spec.js';
import { import {
type DMAddViewerToThreadMembersOperation, type DMAddViewerToThreadMembersOperation,
dmAddViewerToThreadMembersValidator, dmAddViewerToThreadMembersValidator,
} from '../../types/dm-ops.js'; } from '../../types/dm-ops.js';
import { messageTypes } from '../../types/message-types-enum.js'; import { messageTypes } from '../../types/message-types-enum.js';
import type { RawMessageInfo } from '../../types/message-types.js'; import type { RawMessageInfo } from '../../types/message-types.js';
import { messageTruncationStatus } from '../../types/message-types.js'; import { messageTruncationStatus } from '../../types/message-types.js';
import type { AddMembersMessageData } from '../../types/messages/add-members.js'; import type { AddMembersMessageData } from '../../types/messages/add-members.js';
import {
minimallyEncodeMemberInfo,
minimallyEncodeThreadCurrentUserInfo,
} from '../../types/minimally-encoded-thread-permissions-types.js';
import { joinThreadSubscription } from '../../types/subscription-types.js'; import { joinThreadSubscription } from '../../types/subscription-types.js';
import type { ThickMemberInfo } from '../../types/thread-types.js';
import { updateTypes } from '../../types/update-types-enum.js'; import { updateTypes } from '../../types/update-types-enum.js';
import { rawMessageInfoFromMessageData } from '../message-utils.js'; import { rawMessageInfoFromMessageData } from '../message-utils.js';
import { userIsMember } from '../thread-utils.js'; import { userIsMember } from '../thread-utils.js';
function createAddViewerToThreadMembersMessageDataWithInfoFromDMOp( function createAddViewerToThreadMembersMessageDataWithInfoFromDMOp(
dmOperation: DMAddViewerToThreadMembersOperation, dmOperation: DMAddViewerToThreadMembersOperation,
): { ): {
+messageData: AddMembersMessageData, +messageData: AddMembersMessageData,
Show All 24 Lines ) => {
), ),
], ],
}; };
}, },
processDMOperation: async ( processDMOperation: async (
dmOperation: DMAddViewerToThreadMembersOperation, dmOperation: DMAddViewerToThreadMembersOperation,
utilities: ProcessDMOperationUtilities, utilities: ProcessDMOperationUtilities,
) => { ) => {
const { time, messageID, addedUserIDs, existingThreadDetails } = const { time, messageID, addedUserIDs, existingThreadDetails, editorID } =
dmOperation; dmOperation;
const { threadInfos } = utilities; const { threadInfos } = utilities;
const { rawMessageInfo } = const { rawMessageInfo } =
createAddViewerToThreadMembersMessageDataWithInfoFromDMOp(dmOperation); createAddViewerToThreadMembersMessageDataWithInfoFromDMOp(dmOperation);
const rawMessageInfos = messageID ? [rawMessageInfo] : []; const rawMessageInfos = messageID ? [rawMessageInfo] : [];
const threadID = existingThreadDetails.threadID; const threadID = existingThreadDetails.threadID;
Show All 27 Lines ) => {
isMember: time, isMember: time,
}; };
if (!userIsMember(currentThreadInfo, userID)) { if (!userIsMember(currentThreadInfo, userID)) {
newMembers.push(userID); newMembers.push(userID);
} }
} }
if (currentThreadInfo) {
const { membershipPermissions, roleID } =
createPermissionsForNewMembers(currentThreadInfo, utilities);
const newMemberInfos = newMembers.map(userID =>
minimallyEncodeMemberInfo<ThickMemberInfo>({
id: userID,
role: roleID,
permissions: membershipPermissions,
ashoatUnsubmitted
Not Done
Inline Actions

Unrelated to this diff, but I did some investigation as to why we still have permissions for members here.

The context is that in D12789, we removed member permissions from the SQLite DB on native clients.

I know Atul never finished it, but I was under the impression that the only thing left was to implement an equivalent migration on web.

However, looking at the types it seems like the changes were made for thin threads but NOT for thick threads.

I'm guessing Atul's work intersected with the thick threads work, and he ended up making the changes only for thin threads.

I'm not 100% sure, but I suspect we should probably do the same for thick threads as thin threads (calculate the member permissions "on the fly") instead of storing them in the DB. I created ENG-9404 to track this.

ashoat: Unrelated to this diff, but I did some investigation as to why we still have `permissions` for…
isSender: editorID === utilities.viewerID,
subscription: joinThreadSubscription,
}),
);
const resultThreadInfo = {
...currentThreadInfo,
members: [...currentThreadInfo.members, ...newMemberInfos],
currentUser: minimallyEncodeThreadCurrentUserInfo({
role: roleID,
permissions: membershipPermissions,
subscription: joinThreadSubscription,
unread: true,
}),
timestamps: {
...currentThreadInfo.timestamps,
members: {
...currentThreadInfo.timestamps.members,
...memberTimestamps,
},
},
};
const updateInfos = [
{
type: updateTypes.UPDATE_THREAD,
id: uuid.v4(),
time,
threadInfo: resultThreadInfo,
},
];
return {
rawMessageInfos,
updateInfos,
blobOps: [],
};
}
const resultThreadInfo = createThickRawThreadInfo( const resultThreadInfo = createThickRawThreadInfo(
{ {
...existingThreadDetails, ...existingThreadDetails,
allMemberIDsWithSubscriptions: [ allMemberIDsWithSubscriptions: [
...existingThreadDetails.allMemberIDsWithSubscriptions, ...existingThreadDetails.allMemberIDsWithSubscriptions,
...newMembers.map(id => ({ ...newMembers.map(id => ({
id, id,
subscription: joinThreadSubscription, subscription: joinThreadSubscription,
Show All 10 Lines ) => {
utilities, utilities,
); );
const updateInfos = [ const updateInfos = [
{ {
type: updateTypes.JOIN_THREAD, type: updateTypes.JOIN_THREAD,
id: uuid.v4(), id: uuid.v4(),
time, time,
threadInfo: resultThreadInfo, threadInfo: resultThreadInfo,
rawMessageInfos, rawMessageInfos,
truncationStatus: messageTruncationStatus.EXHAUSTIVE, truncationStatus: messageTruncationStatus.EXHAUSTIVE,
rawEntryInfos: [], rawEntryInfos: [],
}, },
]; ];
return { return {
rawMessageInfos, rawMessageInfos: [],
tomekAuthorUnsubmitted
Done
Inline Actions

The message is already included in JOIN_THREAD

tomek: The message is already included in `JOIN_THREAD`
updateInfos, updateInfos,
blobOps: [], blobOps: [],
}; };
}, },
canBeProcessed: async ( canBeProcessed: async (
dmOperation: DMAddViewerToThreadMembersOperation, dmOperation: DMAddViewerToThreadMembersOperation,
utilities: ProcessDMOperationUtilities, utilities: ProcessDMOperationUtilities,
) => { ) => {
Show All 26 Lines