[lib] Accept keyserver ID in blob service upload action
ClosedPublic
Actions

Authored by bartek on Nov 7 2023, 12:14 AM.

Details

Reviewers

inka
michal
tomek
ashoat

Commits

rCOMM7a0c568d895a: [lib] Accept keyserver ID in blob service upload action

Summary

Due to changes in D9399 the blob service upload action now requires threadID to extract keyserver ID from it. It is needed to determine which keyserver we want to send upload metatata to.

The thread ID is unavailable in some context, e.g. for user avatars. However, the extraction function is a no-op if we passed keyserver ID already so renamed the input argument to indicate that.

More context in ENG-5673.

Test Plan

Created another unit test for extractKeyserverIdFromID()
Flow passes
Multimedia uplaods still work
Avatars tested together with other diffs in this stack (two child diffs)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Nov 7 2023, 12:14 AM

bartek held this revision as a draft.

Herald added a subscriber: will. · View Herald TranscriptNov 7 2023, 12:14 AM

bartek added child revisions: D9669: [native] Support avatar uploads to blob service, D9170: [web] Support avatar uploads to blob service.Nov 7 2023, 12:15 AM

I've posted alternative solution proposal inline

lib/actions/upload-actions.js
107 ↗	(On Diff #32841)	The other idea is to make this action accept `keyserverID` directly and force its callers to call `extractKeyserverIDFromID()` on their own. This might be less convenient but perhaps more correct: const keyserverID = extractKeyserverIDFromID(threadID); const uploadBlob = useBlobServiceUpload(); await uploadBlob({ // ... keyserverID, });

Harbormaster failed remote builds in B23825: Diff 32841!Nov 7 2023, 12:31 AM

inka accepted this revision.Nov 7 2023, 1:04 AM

inka added inline comments.

lib/actions/upload-actions.js
107 ↗	(On Diff #32841)	I think it's better if the call to `extractKeyserverIDFromID` is inside of the action. We could also make the `threadID` optional and if not present, use `ashoatKeyserverID` in the action directly: const keyserverID = threadID ? extractKeyserverIDFromID(threadID) : ashoatKeyserverID; But I don't think it matters much since it's a "hack" either way. This would be just simpler

This revision is now accepted and ready to land.Nov 7 2023, 1:04 AM

bartek added inline comments.Nov 7 2023, 5:36 AM

lib/actions/upload-actions.js
107 ↗	(On Diff #32841)	Thanks for your opinion 😄 I wouldn't make this optional though. Since this is a temporary "hack", we shouldn't hide it behind optional arguments. This way, any possible future usage will require explicit declaration and indicate the requirement of either specifying the hosting keyserver for media metadata or choosing another way of storing it.

Rebase on latest master

Is reverting this hack tracked in any task? We should have a task that explicitly references the part of the code where the hack needs to be reverted.

Harbormaster completed remote builds in B23847: Diff 32865.Nov 7 2023, 7:05 AM

In D9738#284946, @ashoat wrote:

Is reverting this hack tracked in any task? We should have a task that explicitly references the part of the code where the hack needs to be reverted.

There is no real hack here - this diff is just a rename to make naming more accurate. The thing we called a "hack" is hardcoding keyserver ID in D9170 and D9669 because this action now requires it. Currently, avatar metadata is stored on the keyserver but in the future it won't. So basically "reverting the hack" means implementing ENG-4139, not just reverting a few lines of code.