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');
}