Details
Add the following to the textMessageCreationResponder in keyserver/src/responders/message-responders.js, right before return:
const reportedMessageID = rawMessageInfos[0].id;
if(reportedMessageID){
await createMessageReport(viewer, { messageID: reportedMessageID });
}And see that when a user sends a text message, ashoat user recievies a message from commbot saying
user user_name reported user_name’s message in chat titled chat_name
message
Add this to legacyMultimediaMessageCreationResponder right before return:
await createMessageReport(viewer, { messageID: id });And see that when a user sends a multimedia message, ashoat user recievies a message from commbot saying
user user_name reported user_name’s message in chat titled chat_name
non-text message
So basically the message that is sent is also reported by the author
See that when chat between commbot and ashoat user doesn't exists it gets created, and when it exists it isn't created again, just a new message appears in the chat.
I also checked that when fetching the keyserver admin or creating the message with the report fail the creator throws an error. I did that by setting keyserverAdminID = null, result = [] (in createMessageReport), respectively, by hand. This error will be handled on the client side and is meant to be an indicator of whether the report was successful or not. In all other cases the report should succeed.
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 42 | We shouldn't replace falsy threadID by [null] because that would result in a query with t.id = [null] which shouldn't be executed at all - we know that there are no such threads. | |
| 45–48 | ||
| 53–54 | In the proposed approach it might be easier to use promiseAll from promises.js | |
Addressing Tomek's comments.
In the process I realized that the responder will need the message info as well, so it will fetch it and pass it to the creator
The reducer will need the threadInfo as well, although probably only as long as hack from ENG-1707 doesn't get reverted. For now we'll pass it to the creator to avoid multiple database calls for the same data.
Looks great! Added just a couple of small comments.
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 33–35 ↗ | (On Diff #16196) | We can start by assigning it |
| 43 ↗ | (On Diff #16196) | It's better to keep it null / undefined here and deal with it in getCommbotMessage |
| 73–83 ↗ | (On Diff #16196) | Maybe something like this? |
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 33–35 ↗ | (On Diff #16196) | I don't think I actually can, since I want to add something to it later. From Flow documentation:
link |
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 33–35 ↗ | (On Diff #16196) | Ok, that makes sense |
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 46 ↗ | (On Diff #16579) | Could you check what we usually do with the result of createMessages call? And what is the meaning of the third parameter of this function? |
| 63 ↗ | (On Diff #16579) | This might be really confusing to use because it's not obvious what is the meaning of the following array elements. A lot better approach would be to return an object, because it has named fields. |
| 106–110 ↗ | (On Diff #16579) | We try to use two good practices: returning early if possible and avoiding mutability. In this case, you could return if fetchPersonalThreadID returned truthy value and then return the result of createCommbotThread. |
| keyserver/src/fetchers/thread-fetchers.js | ||
| 245 ↗ | (On Diff #16579) | Why do we allow optional RawMessageInfo but use required MessageInfo? It this case it shouldn't matter that much, but if the function body had more complicated logic, this type might cause issues. You can also consider having an if and assign to promises conditionally, instead of introducing this function. if (reportedMessage) {
promises.reportedThread = await fetchServerThreadInfos(SQL`t.id = ${reportedMessage.threadID}`);
} |
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 63 ↗ | (On Diff #16579) | Right, sorry, forgot to clean this up |
| 106–110 ↗ | (On Diff #16579) | I'll change it to return commbotThreadID ?? createCommbotThread(userID); I checked what happens with this code: let a = 0; const b = null ?? (a=1); console.log(a); const c = 1 ?? (a=2); console.log(a); and it logs 1 1 So it seems if the left operator is not null or unndefined then the right side is not run. (from MDN docs:
) |
| 124 ↗ | (On Diff #16579) | Sorry, I don't understand. Do you mean that createMessageReply appends '\n\n'? Because it also adds > so that the text is displayed as a markdown quote... And the '\n\n' are as far as I understand our way of determining the end of the quote. From native/markdown/rules.react.js: // match end of blockQuote by either \n\n or end of string |
| 126 ↗ | (On Diff #16579) | The 's is added in lines 120-122 (if we successfully fetched the username). Is this what you meant? |
| keyserver/src/fetchers/thread-fetchers.js | ||
| 245 ↗ | (On Diff #16579) | I'd rather leave this function. I feel like having to unpack the threadInfos array returned by the fetchServerThreadInfos in the creator is messy. It seems to me like this is what the '-fetchers' files are for? To provide some abstraction over this kind of actions? I don't know, I'm open to discussion. |
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 124 ↗ | (On Diff #16579) |
If the last thing in the message is the quote, there is no reason to append \n\n |
| 126 ↗ | (On Diff #16579) | Oops, I missed that! Thanks for pointing it out |
Great, thank you!! Assuming you'll update this to use your new function before landing
| keyserver/src/creators/message-report-creator.js | ||
|---|---|---|
| 134 ↗ | (On Diff #16974) | Update to createMessageQuote before landing |