Details
See that when a user reports a message using the tooltip "Report" button, ashoat user recieves a report message.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- inka/message_report
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/types/redux-types.js | ||
|---|---|---|
| 805–816 ↗ | (On Diff #16578) | It seems that every other request type has at least loadingInfo. Can we make these types consistent with the others? |
| native/chat/message-report-utils.js | ||
| 18 ↗ | (On Diff #16578) | It's not related to this diff but an overall observation. When we export a function with complicated signature, it is a good idea to export its type next to the function, so that using this function is more convenient. In this case it would be great if we had <F>(serverCall: ActionFunc<F>) => F defined somewhere (in action utils?) and used here and in all the other places. |
| 21 ↗ | (On Diff #16578) | In this case it shouldn't make any difference in behavior, but in most places we're using lambdas when defining inner async functions, so it's better to be consistent. |
| 30 ↗ | (On Diff #16578) | What is the reason behind using false here? |
| 35 ↗ | (On Diff #16578) | I think that having this code in try would make it less confusing - currently it's easy to miss a rethrow and conclude that we're always displaying the modal. |
| native/chat/multimedia-message-tooltip-modal.react.js | ||
| 30 ↗ | (On Diff #16578) | This is definitely not a good idea to provide a "null" string when there's no id. Even simple null would be better than an arbitrary string. What is the possible reason for a missing id? Should we report anything in that case? Maybe we should display an Alert in such case? Please investigate this a bit deeper. |
| native/chat/message-report-utils.js | ||
|---|---|---|
| 18 ↗ | (On Diff #16578) | I'll change it in a different diff, since it needs to change other parts of the code. Issue |
| 30 ↗ | (On Diff #16578) | |
| native/chat/multimedia-message-tooltip-modal.react.js | ||
| 30 ↗ | (On Diff #16578) | As far as I can see it is null when local copy without ID yet so when we have sent a message but not yet gotten a response from the server. I'm not quite sure what is the best thing to do in this case, it's a weird scenario when someone reports a message've they just sent... I suppose the best thing I can do is display an Alert that something went wrong? |
Looks great! The only important thing to improve is about local messages.
| lib/types/redux-types.js | ||
|---|---|---|
| 819 ↗ | (On Diff #17003) | In most of other types, the Error is mandatory |
| native/chat/message-report-utils.js | ||
| 23 ↗ | (On Diff #17003) | How about renaming to messageID? |
| 38 ↗ | (On Diff #17003) | Maybe reportMessage? |
| native/chat/multimedia-message-tooltip-modal.react.js | ||
| 30 ↗ | (On Diff #16578) | That sounds reasonable, but maybe we can disable report actions for messages without id? We can also consider disabling report action on user's own messages. |
| native/chat/multimedia-message-tooltip-modal.react.js | ||
|---|---|---|
| 30 ↗ | (On Diff #16578) | Thanks! That makes sense. |