[lib] Introduce new `sendMultimediaMessage` and consume in `native`
ClosedPublic
Actions

Authored by atul on Sep 5 2022, 10:52 PM.

Details

Reviewers

• abosh
tomek
marcin

Commits

rCOMM88eabcde44a5: [lib] Introduce new `sendMultimediaMessage` and consume in `native`

Summary

Introduce a new sendMultimediaMessage in lib/actions/message-actions that hits the create_multimedia_message endpoint with the updated payload (mediaMessageContents instead of mediaIDs).

This endpoint is consumed by input-state-container on native, but not on web. We continue to use the legacySendMultimediaMessage on web to avoid breaking things.

IMPORTANT: While the changes made to keyserver in this stack are backwards compatible, this change to native depends on keyserver being deployed before a release is made. To keep things simple I'll avoid landing this diff until all necessary changes on the keyserver side are made and tested. I'll hold back on landing this diff (and a few others) until we decide to "flip the switch." I'll probably just keep this commit in my local environment as I'm working, but figured it might be helpful for reviewers if they wanted to arc patch or test any part of the stack in their local dev environment.

(As of this diff, the keyserver has everything it needs in multimediaMessageCreationResponder(...) to properly associate uploads with media with messages)

Depends on D5062

Test Plan

Able to send image(s) on native as before without issue
Able to send image(s) on web as before without issue

Also set breakpoints at key parts of the multimedia message flow and ensured that values appeared as expected.

(I'll do some a bunch of additional testing once things are "working" end-to-end)

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Sep 5 2022, 10:52 PM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 5 2022, 10:52 PM

atul edited the summary of this revision. (Show Details)Sep 5 2022, 11:00 PM

Harbormaster completed remote builds in B11903: Diff 16346.Sep 5 2022, 11:02 PM

atul requested review of this revision.Sep 5 2022, 11:02 PM

atul added a reviewer: marcin.Sep 5 2022, 11:02 PM

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

atul removed a reviewer: • jacek.Sep 7 2022, 5:33 PM

atul added 1 blocking reviewer(s): tomek.Sep 7 2022, 10:22 PM

The new and old API differ in only one type, so maybe it would be a good idea to simply have a method that works for both. We're going to throw away the old API so copying code might be a good idea... but now I'm not sure if introducing the new method made sense for legacy shape. We could simply have a function that accepts either type and at the end of process we would delete the old type.

I'm not necessarily requesting to change the code now, but I'm curious about your opinion.

lib/actions/message-actions.js
133 ↗	(On Diff #16346)	It's confusing to see `db` type in an interface between client and server.

This revision now requires changes to proceed.Sep 8 2022, 8:27 AM

atul added inline comments.Sep 8 2022, 9:13 AM

lib/actions/message-actions.js
133 ↗	(On Diff #16346)	That's totally fair. I couldn't think of a better name, but open to changing it to whatever might be clearer. I thought of just `MediaMessageContent` but that might imply that it contains the "contents" (eg blobs) of the MediaMessage? Idk, I just re-used the naming scheme that I used for the message-reducer stuff a while ago but very open to changing name to something clearer

rebase

Harbormaster completed remote builds in B12028: Diff 16525.Sep 8 2022, 2:36 PM

The new and old API differ in only one type, so maybe it would be a good idea to simply have a method that works for both.

We kind of have that on the keyserver side. I put a good amount of thought into supporting old clients with the new keyserver... but didn't think we were too worried about supporting old keyservers with newer clients at this point since there's only one keyserver.

With regards to sendMultimediaMessage and legacySendMultimediaMessage in lib being largely redundant on the client-side, I didn't want to spend too much time refactoring that since legacySendMultimediaMessage will be removed altogether pretty soon in this stack. These functions are only used from the client-side (native and web), so we don't have to worry about "keeping it around" like we have to for functionality on the keyserver side (to support old clients).

We're going to throw away the old API so copying code might be a good idea... but now I'm not sure if introducing the new method made sense for legacy shape. We could simply have a function that accepts either type and at the end of process we would delete the old type.

Yeah we totally could have merged the functionality to handle both types within one function, but I thought it was easier to just split them apart and give them different names rather than add functionality/logic to handle both. If we were keeping both around, I'd certainly refactor to reduce repetition, but going to remove legacy... very quickly so I guess I'm being lazy and this was easier to work with as I'm developing.

(Let me know if I correctly understood your questions, or if there's something I'm missing/should expand on)

atul added a child revision: D5085: [keyserver] Introduce `fetchMediaFromMediaMessageContent(...)`.Sep 8 2022, 11:37 PM

(Let me know if I correctly understood your questions, or if there's something I'm missing/should expand on)

I think we're on the same page

but didn't think we were too worried about supporting old keyservers with newer clients at this point since there's only one keyserver.

Yeah, that's right. But at some point it could be necessary to version the keyserver code. Probably the same applies to services. We can't assume that everything will be always up-to-date, because we should have a way to rollback. So that seems like a quite complicated issue. But as we agreed, in this diff we're fine.

Yeah we totally could have merged the functionality to handle both types within one function, but I thought it was easier to just split them apart and give them different names rather than add functionality/logic to handle both. If we were keeping both around, I'd certainly refactor to reduce repetition, but going to remove legacy... very quickly so I guess I'm being lazy and this was easier to work with as I'm developing.

Sounds reasonable.

lib/actions/message-actions.js
133 ↗	(On Diff #16346)	I think that adding e.g. `descriptor` at the end of the name could differentiate between content and just the ids. What do you think about `MediaMessageContentDescriptor`? (or description, not sure which one sounds more appropriate)