Page MenuHomePhabricator

[keyserver] Add message report responder
ClosedPublic

Authored by inka on Sep 1 2022, 7:42 AM.
Tags
None
Referenced Files
F3524731: D5022.id16975.diff
Mon, Dec 23, 2:15 PM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Unknown Object (File)
Tue, Dec 17, 10:12 AM
Subscribers

Details

Summary

Linear issue: ENG-160, ENG-1700
We need the keyserver to send a message report to the keyserver admin from the commbot user when a user reports a message. This diff introdueces the endpoint responder.

Test Plan

Add the following code to lib/actions/message-actions.js sendTextMessage right after the other fetchJSON:

const messageID = response.newMessageInfo.id;
await fetchJSON('create_message_report', { messageID });

And see that when a user sends a message, ashoat user recievies a message from commbot saying

user user_name reported user_name’s message in chat titled chat_name

message

So basically the message that is sent is also reported by the author

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Returning on !threadInfo might seem weird as we should be able to proceed with the raport ex when the thread had been deleted in the meantime, but this is due to a hack used for now to fetch the keyserver admins info. More context: ENG-1707. We indeed cannot proceed without the keyserver admins info as we wouldn't know whom to send the report to.

inka requested review of this revision.Sep 1 2022, 7:52 AM
tomek requested changes to this revision.Sep 5 2022, 6:55 AM
tomek added inline comments.
keyserver/src/responders/message-report-responder.js
28 ↗(On Diff #16207)

It is quite expensive to fetch all the users every time we call this function. Can we somehow make it cheaper or use other existing function?

28–30 ↗(On Diff #16207)

fetchUserInfos and reportedMessage can be run independently. Also, it might be a good idea to modify serverThreadInfoFromMessageInfo so that it requires just the messageID - by doing that we will be able to call all three functions at the same time.

This revision now requires changes to proceed.Sep 5 2022, 6:55 AM

Refactoring to match the projects convention of reponder/creator responsibilities

Make message report responder return a boolean

*Make message report responder return a wrapped boolean

inka planned changes to this revision.Sep 7 2022, 7:33 AM

Make the responder not return anything. Instead, if the creator fails it will throw, and in turn the responder will throw

This revision is now accepted and ready to land.Sep 9 2022, 4:27 AM

Changes in reponse to code review on D4941

keyserver/src/responders/message-report-responder.js
31 ↗(On Diff #16975)

Is this message always present or only when a viewer is a member of admin - commbot thread?

keyserver/src/responders/message-report-responder.js
31 ↗(On Diff #16975)

Always... createMessageReport throws if result it is about to return is of length 0. It gets its result from createMessages that having updatesForCurrentSession === 'return' and non empty messageDatas, as in this case, returns a non empty array as far as I can see. I also checked it, and being logged onto a different user, and having added console.log(rawMessageInfos); here, I got a one element array. It this not right?

keyserver/src/responders/message-report-responder.js
31 ↗(On Diff #16975)

I guess this is fine. But on the other hand it doesn't make too much sense to return a message to a viewer that isn't a member of the thread. I think that the reducer should handle it, but it's worth checking.

When a new message is created, message creator could check if a viewer is a member of a thread and send the message back only when that's the case - that would save a little of network traffic. But keeping this also makes sense, as a client could do something different than just adding it to messages store. So this is probably safe to keep as is, but creating a task to have a discussion might be the best solution for now.

keyserver/src/responders/message-report-responder.js
31 ↗(On Diff #16975)

ENG-1925 .
Message reducer handling sendMessageReportActionTypes.success calls mergeNewMessages that filters messages by isThreadWatched, so it should indeed filter this message out.