[keyserver] Merge updated and legacy `multimediaMessageCreationResponder(...)`
ClosedPublic
Actions

Authored by atul on Sep 20 2022, 4:21 AM.

Details

Reviewers

tomek
• abosh
marcin

Commits

rCOMM243ec858b6f8: [keyserver] Merge updated and legacy `multimediaMessageCreationResponder(...)`

Summary

Addresses @tomek's feedback here: https://phab.comm.dev/D5127#150659

Basically cuts a bunch of redundant code and just branches where the two request payloads vary (mediaIDs vs mediaMessageContents)

Thought it was simpler to put up a refactor diff rather than go back and change D5127, hope that's okay.

Depends on D5127

Test Plan

Things continue to work as expected. Set breakpoints and observed correct values at each step.

Diff Detail

Repository

rCOMM Comm

Branch

arcpatch-D5191 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Sep 20 2022, 4:21 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 20 2022, 4:21 AM

atul mentioned this in D5127: [keyserver] Introduce updated `multimediaMessageCreationResponder(...)` to handle updated payload.Sep 20 2022, 4:23 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 20 2022, 4:23 AM

Harbormaster failed remote builds in B12308: Diff 16896!

be more explicit to help out flow

Harbormaster completed remote builds in B12309: Diff 16897.Sep 20 2022, 4:55 AM

atul requested review of this revision.Sep 20 2022, 4:55 AM

atul added a child revision: D5193: [keyserver] Introduce `constructMediaFromMediaMessageContentsAndUploadRows`.Sep 20 2022, 7:24 AM

Thought it was simpler to put up a refactor diff rather than go back and change D5127, hope that's okay.

From review point of view, it might make more sense to have a single diff. The reason is that the previous diff introduces a new function and this one deletes it. But let's keep the current structure.

keyserver/src/responders/message-responders.js
144–145 ↗	(On Diff #16897)	Another idea for simplification: what do you think about merging these functions?
152 ↗	(On Diff #16897)	This check is weaker than the previous one. Can we keep checking if the lengths match?

This revision is now accepted and ready to land.Sep 21 2022, 6:18 AM

atul added inline comments.Sep 21 2022, 12:58 PM

keyserver/src/responders/message-responders.js
144–145 ↗	(On Diff #16897)	I think it makes sense to keep these two distinct
144–145 ↗	(On Diff #16897)	I tend to personally prefer branching at the level of a function rather than within a function
152 ↗	(On Diff #16897)	Based on discussion with @ashoat this is equivalent to the previous check. We weren't actually trying to account for situations where only some of the media was already assigned a container. The expectation was that either media.length would be 0 (if all the media were previously assigned to another message) or !0 in which case the assumption was that all the media could be assigned to the media message being created in this function. This also simplifies the logic for the `request.mediaIDs` and `request.mediaMessageContents` cases