Page MenuHomePhabricator
Differential D5197

[lib] Implement `messageContentForServerDB` for `messageTypes.MULTIMEDIA`
ClosedPublic
Actions

Authored by atul on Sep 20 2022, 9:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 30, 5:29 PM
Unknown Object (File)
Tue, Apr 30, 5:29 PM
Unknown Object (File)
Tue, Apr 30, 5:29 PM
Unknown Object (File)
Tue, Apr 30, 5:29 PM
Unknown Object (File)
Tue, Apr 30, 5:28 PM
Unknown Object (File)
Sun, Apr 28, 9:39 PM
Unknown Object (File)
Sun, Apr 28, 9:39 PM
Unknown Object (File)
Sun, Apr 28, 9:39 PM
View All 87 Files
Subscribers
ashoat

Details

Reviewers
• abosh
tomek
marcin
Commits
rCOMM4597a9a14843: [lib] Implement `messageContentForServerDB` for `messageTypes.MULTIMEDIA`
Summary

The messageContentForServerDB(...) function determines what gets put in the content column of the messages table.

The current content column for media messages is a "stringified" list of uploadIDs. However, the content column is never used after it's been stored in the DB. Rather, messages are joined with corresponding uploads via the container column of the uploads table. Therefore this change isn't "destructive."

Depends on D5195

Test Plan

Rows in the messages and uploads column appear as expected:

20a7c2.png (54×3 px, 66 KB)

8ddede.png (194×2 px, 214 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D5197 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Sep 20 2022, 9:01 AM
Herald added a subscriber: ashoat. · View Herald TranscriptSep 20 2022, 9:01 AM
atul edited the test plan for this revision. (Show Details)Sep 20 2022, 9:04 AM
atul added 1 blocking reviewer(s): tomek.Sep 20 2022, 9:06 AM
Harbormaster completed remote builds in B12327: Diff 16919.Sep 20 2022, 9:09 AM
atul requested review of this revision.Sep 20 2022, 9:09 AM
atul edited the summary of this revision. (Show Details)Sep 20 2022, 9:38 AM
atul added a parent revision: D5195: [keyserver] Construct `MULTIMEDIA` message media correctly in `rawMessageInfoFromRows(...)`.
atul added a child revision: D5198: [keyserver] Introduce `getMediaMessageServerDBContentsFromMedia(...)`.Sep 20 2022, 9:38 AM
ashoat added a comment.EditedSep 20 2022, 11:55 AM

I would reverse these conditions to overall reduce indentation level

EDIT

Never mind, it's cleaner in D5198. I probably would've submitted that first and this diff second

tomek accepted this revision.Sep 21 2022, 6:59 AM

Is it safe for both server and client DB?

Should we have migrations on both platforms that update the values?

This revision is now accepted and ready to land.Sep 21 2022, 6:59 AM
atul added a comment.Sep 21 2022, 2:03 PM
In D5197#152670, @tomek wrote:

Is it safe for both server and client DB?

Should we have migrations on both platforms that update the values?

Yeah it's safe! There aren't any media messages in the database of type 15 (cc @ashoat who checked recently) so we're starting from a clean slate.

There certainly are issues if someone has video messages in their local environment, but that should only affect @abosh and I.

atul updated this revision to Diff 16964.Sep 21 2022, 2:04 PM

rebase before landing

Harbormaster completed remote builds in B12356: Diff 16964.Sep 21 2022, 2:15 PM
Closed by commit rCOMM4597a9a14843: [lib] Implement `messageContentForServerDB` for `messageTypes.MULTIMEDIA` (authored by atul). · Explain WhySep 22 2022, 6:03 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM4597a9a14843: [lib] Implement `messageContentForServerDB` for `messageTypes.MULTIMEDIA`.

Revision Contents
Changeset List

PathSize
lib/
shared/
messages/
17 lines

Diff 16964

View Options

lib/shared/messages/multimedia-message-spec.js

Loading...