Page MenuHomePhabricator

[keyserver] Add function for fetching RawMessageInfo for rows and their related messages
ClosedPublic

Authored by inka on Apr 24 2023, 2:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 9, 7:19 PM
Unknown Object (File)
Tue, Jul 9, 10:33 AM
Unknown Object (File)
Mon, Jul 1, 8:57 AM
Unknown Object (File)
Mon, Jul 1, 8:57 AM
Unknown Object (File)
Mon, Jul 1, 8:57 AM
Unknown Object (File)
Mon, Jul 1, 8:57 AM
Unknown Object (File)
Mon, Jul 1, 8:51 AM
Unknown Object (File)
Jun 14 2024, 9:17 AM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-3689/implement-fetching-edits-sidebar-source-and-reactions
This function takes rows from messages table, and returns RawMessageInfo for those rows and messages related, ie edits, reactions and sidebar_sources.
For sidebar_source, edit_message and reaction messages there are no sidebar_source, edit_message and reaction messages (user cannot react to a reaction message etc). So those messages are not passed to
fetchRelatedMessages function.

Test Plan

Called the function on some rows, checked that all related messages are returned (and the messages from the rows).

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Apr 24 2023, 2:25 AM
Harbormaster failed remote builds in B18683: Diff 25589!
inka requested review of this revision.Apr 25 2023, 4:15 AM
keyserver/src/fetchers/message-fetchers.js
750

Realizing this error message should be updated to mention TOGGLE_PIN

(Please adjust to keep under 80 chars)

900–902

Since we use this also on line 747, can we factor it out into a function in lib/types/message-types.js?

(Also look for other cases where it should be used in the codebase)

keyserver/src/fetchers/message-fetchers.js
750

Or maybe make this error message more generic to avoid having to update the message if we added other message types in the future.
The extracted function name of this check (comment below on L900) could potentially be used.

keyserver/src/fetchers/message-fetchers.js
900–902

I added D7632 to address this, with an explanation why I did it the way I did it, but honestly I think it's worse. I will change this if D7632 is accepted.

keyserver/src/fetchers/message-fetchers.js
900–902

@bartek showed me a better way of doing that (in D7632), and now I think it's fine.
I will still wait for D7632 to be accepted, in case the reviewers don't like the functions name or something

This revision is now accepted and ready to land.Apr 27 2023, 1:29 PM