Page MenuHomePhabricator

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

Authored by rohan on Jan 31 2023, 10:25 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 22, 12:17 PM
Unknown Object (File)
Wed, Nov 13, 5:37 AM
Unknown Object (File)
Fri, Nov 8, 12:20 PM
Unknown Object (File)
Fri, Nov 8, 3:30 AM
Unknown Object (File)
Fri, Nov 8, 3:30 AM
Unknown Object (File)
Fri, Nov 8, 3:12 AM
Unknown Object (File)
Fri, Nov 8, 12:30 AM
Unknown Object (File)
Fri, Nov 8, 12:30 AM
Subscribers

Details

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:

  1. Upload a single media on web and native
  2. Upload multiple media on web
  3. Use the queue feature to upload multiple media on native
  4. 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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat requested changes to this revision.Jan 31 2023, 12:12 PM
  1. 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?
  2. 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

Can we make this $ReadOnly?

keyserver/src/uploads/media-utils.js
33–38

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

Why files.length === 1?

lib/actions/upload-actions.js
14

Put this at the bottom along with loop... it's weird having ...Dimensions in the middle

14–16

Can this be $ReadOnly?

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

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)
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
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.

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

This revision is now accepted and ready to land.Feb 3 2023, 10:53 AM
This revision was landed with ongoing or failed builds.Feb 26 2023, 11:46 AM
This revision was automatically updated to reflect the committed changes.