web] switch the promise type in sendReactionMessage action from SendReactionMessageResult to SendMessageResult
ClosedPublic
Actions

Authored by ginsu on Jan 3 2023, 12:22 PM.

Details

Reviewers

atul
tomek

Commits

rCOMM73a31647f96a: [lib/native/web] switch the promise type in sendReactionMessage action from…

Summary

switch the promise type in sendReactionMessage action from SendReactionMessageResult to SendMessageResult. Since we have a lot of the message info already stored in the local message we no longer need to return a whole new newMessageInfos and instead can follow the pattern of text and multimedia messages of returning an updated time and id which is now the server id

Linear Task: ENG-2599
Depends on D6123

Test Plan

flow and this will be tested futher in subsequent diffs

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ginsu created this revision.Jan 3 2023, 12:22 PM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptJan 3 2023, 12:22 PM

ginsu edited the summary of this revision. (Show Details)Jan 3 2023, 12:27 PM

ginsu edited the test plan for this revision. (Show Details)

ginsu added reviewers: atul, tomek.

ginsu edited the summary of this revision. (Show Details)

ginsu added a parent revision: D6123: [lib] add sendReactionMessageAction.started cases to message and local id reducers.

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 3 2023, 12:36 PM

Harbormaster failed remote builds in B15048: Diff 20529!

ginsu added a child revision: D6153: [lib] change the Send Reaction Message Success Payload and also change its reducer success case.Jan 3 2023, 12:50 PM

Harbormaster completed remote builds in B15048: Diff 20529.Jan 3 2023, 12:51 PM

ginsu requested review of this revision.Jan 3 2023, 12:51 PM

tomek accepted this revision.Jan 5 2023, 8:09 AM

tomek added inline comments.

lib/actions/message-actions.js
236	Could you reming why do we need an `interface`?

This revision is now accepted and ready to land.Jan 5 2023, 8:09 AM

ginsu added inline comments.Jan 5 2023, 11:34 AM

lib/actions/message-actions.js
236	The`SendMessageResult` type, which is used for all locally composed message info has the `interface` field, and since now we are considering reaction messages as a locally composed message we should include this in the return

tomek added inline comments.Jan 9 2023, 1:28 AM

lib/actions/message-actions.js
236	Ok, it makes sense to be consistent. But I was curious about why do we need this field. Is it used anywhere? I've searched a bit and it seems like this field was introduced in D511 and then the usage was removed in D4645, so maybe we don't need this field at all. It doesn't hurt to keep it for some time, though.

ginsu added inline comments.Jan 9 2023, 9:38 AM

lib/actions/message-actions.js
236	Is it used anywhere? Also seems like there are a couple of instances of `interface` in `lib/utils/call-server-endpoint.js` when we call a server endpoint. It doesn't hurt to keep it for some time, though. I think we should keep it for now, especially if it doesn't hurt anything, but if we truly are not using this field, we should remove it from all messages that use it (text/multimedia/reaction) so that we can have all the local message types be consistent with each other

rebase before landing

Harbormaster completed remote builds in B15204: Diff 20719.Jan 9 2023, 9:56 AM

Closed by commit rCOMM73a31647f96a: [lib/native/web] switch the promise type in sendReactionMessage action from… (authored by ginsu). · Explain WhyJan 9 2023, 10:05 AM

This revision was automatically updated to reflect the committed changes.

ginsu added a commit: rCOMM73a31647f96a: [lib/native/web] switch the promise type in sendReactionMessage action from….

ginsu added inline comments.Jan 11 2023, 1:19 PM

lib/actions/message-actions.js
235–236 ↗	(On Diff #20727)	I believe this is why message liking is broken on build 172. In the future, we should have taken into account that removing the `newMessageInfo` field, which was expected by build 172, from the return, could break functionality

ashoat added inline comments.Jan 11 2023, 1:42 PM

lib/actions/message-actions.js
235–236 ↗	(On Diff #20727)	I don't think this is right – this is a change in client code. The problem isn't the change in client code, the problem is that you changed keyserver code to break support for old clients. Can you try to find where you did that?