Details

Reviewers

atul
ginsu
tomek
kamil
ashoat

Commits

rCOMMbe43778193f8: [keyserver] Attach a threadID to each media (image/video) upload

Summary

We need to attach a thread id to each upload that is made. The best way to do this would be on the keyserver, as this gives us the flexibility of shareable code betweewn the web and native clients. This also means we don't need to propagate the thread id from the individual clients to the keyserver.

Originally, I planned to put this query in message-creator, as mentioned in the Linear task. However, this seemed to cause some problems where the query was being executed prior to a container being assigned to an upload, thus the query found no results. To resolve this, I looked around for where we assign a container to each upload, and thought that this would be the best place to attach a thread to each upload.

Depends on D6467

https://linear.app/comm/issue/ENG-2871/attach-a-threadid-to-each-media-imagevideo-upload

Test Plan

Upload several media (images and videos) and check the uploads table to confirm that the thread column is populated. Both web and native were successful. Tested the following scenarios:

Upload a single media on web and native
Upload multiple media on web
Use the queue feature to upload multiple media on native
Tested with both videos and photos

Screenshot 2023-02-02 at 9.44.15 AM.png (686×1 px, 436 KB)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jan 31 2023, 10:25 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJan 31 2023, 10:25 AM

rohan edited the summary of this revision. (Show Details)Jan 31 2023, 10:26 AM

rohan edited the test plan for this revision. (Show Details)

rohan added a parent revision: D6467: [native] Render the media gallery in the thread settings on native.

rohan edited reviewers, added: kamil; removed: • przemek.

rohan attached a referenced file: F353951: Attach a threadID to each media (image:video) upload on native.png. (Show Details)

Harbormaster completed remote builds in B15894: Diff 21695.Jan 31 2023, 10:40 AM

rohan requested review of this revision.Jan 31 2023, 10:40 AM

Rebase

rohan added a child revision: D6485: [keyserver] Create a function to query for all uploads from a provided threadID.Jan 31 2023, 11:04 AM

Harbormaster completed remote builds in B15897: Diff 21698.Jan 31 2023, 11:06 AM

Do we really need to do all of this "prop drilling"? In some places it seems like we pass thread in just to make sure we return it... can we avoid passing it in entirely in cases where a function doesn't need to use it, and then merge it in after the result comes back?
I don't see why the client needs to pass this up to the keyserver at all... it already passes in the message when the file is attached to a message, so can't we query for that message's thread on the keyserver, and just associate it then? Note: this would involve adding thread at the same time as message (when the upload is attached to the message), rather than when the upload is created

keyserver/src/creators/upload-creator.js
19–25 ↗	(On Diff #21698)	Can we make this `$ReadOnly`?
keyserver/src/uploads/media-utils.js
33–38 ↗	(On Diff #21698)	This is a lot of parameters... wonder if it would better taking an object, so the parameters would be named at the callsite
keyserver/src/uploads/uploads.js
61 ↗	(On Diff #21698)	Why `files.length === 1`?
lib/actions/upload-actions.js
14 ↗	(On Diff #21698)	Put this at the bottom along with `loop`... it's weird having `...Dimensions` in the middle
14–16 ↗	(On Diff #21698)	Can this be `$ReadOnly`?

This revision now requires changes to proceed.Jan 31 2023, 12:12 PM

ashoat mentioned this in D6465: [keyserver] Add thread column to uploads table.Feb 1 2023, 2:52 AM

Handle the logic on the keyserver, and update the summary / test plan with the new implementation.

rohan retitled this revision from [keyserver/native] Attach a threadID to each media (image/video) upload on native to [keyserver] Attach a threadID to each media (image/video) upload.Feb 2 2023, 6:54 AM

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

rohan edited the test plan for this revision. (Show Details)

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

rohan attached a referenced file: F357838: Screenshot 2023-02-02 at 9.44.15 AM.png. (Show Details)

Harbormaster completed remote builds in B16004: Diff 21848.Feb 2 2023, 7:03 AM

ashoat requested changes to this revision.Feb 2 2023, 10:55 AM

ashoat added inline comments.

keyserver/src/updaters/upload-updaters.js
38 ↗	(On Diff #21848)	I don't understand why this is a separate query. We're already doing an `UPDATE uploads` for the `messageID` – what's the point of doing two?
42–43 ↗	(On Diff #21848)	No need for this inner query, we already know the `threadID`

This revision now requires changes to proceed.Feb 2 2023, 10:55 AM

rohan added inline comments.Feb 2 2023, 12:41 PM

keyserver/src/updaters/upload-updaters.js
38 ↗	(On Diff #21848)	Considering either `assignMedia` or `assignMessageContainerToMedia` is called based on the conditional, are you suggesting to merge this query into both of the existing queries?
42–43 ↗	(On Diff #21848)	If I'm merging this query into the two above, I can most likely pass in the `threadID` since `request` is of type `SendReactionMessageRequest` and it has a threadID.

Update the thread column for uploads when we update the container column. Testing this works locally. Adding the AND thread NOT NULL should be ok because when a new entry is added to the upload table,
it's thread value is defaulted to be null. So when we're updating the container, thread will be NULL at the time of update.

rohan added inline comments.Feb 2 2023, 3:42 PM

keyserver/src/updaters/upload-updaters.js
19 ↗	(On Diff #21924)	Used indentation here to indicate that it's part of the same `WHERE` clause, just on a new line to prevent a long line. Same as below