messages state: Maintain edit_history to be reasonably current#5435
messages state: Maintain edit_history to be reasonably current#5435chrisbobbe merged 7 commits intozulip:mainfrom
edit_history to be reasonably current#5435Conversation
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! A few comments below, just about the tests; then please merge at will.
I'm not sure I'm quite happy with the last commit, where we set
edit_historytonullbased onallowEditHistory. I've put the condition onallowEditHistoryout to the edge (eventToAction,api.getMessages)…but that means plumbingallowEditHistoryout to the edge. Is that OK?
Yeah, that isn't beautiful, but I think it's OK.
Effectively it means that the edge becomes a little bit stateful -- that there's a little bit of the data that's previously passed through it which it needs to remember, or be reminded of, in order to correctly interpret the new data coming through. Specifically, there's a little bit of the realm's settings that it needs to know.
That isn't much different in kind from the little bit of such knowledge it already needs, namely the server version. And I think it's natural that there might occasionally be bits of the realm settings (or server settings) that are needed as background information for the kind of processing that we do at the edge, just like there ends up being here.
| subject: 'new topic', | ||
| edit_history: [ | ||
| { | ||
| user_id: message1.sender_id, |
There was a problem hiding this comment.
In this test case, there's three messages getting moved at once, and the user_id of each one's edit_history entry is the sender of that particular message. That seems like not the intended semantics -- it should instead be the same for all three entries, namely the user that did the move.
It looks like the actual implementation gets this right, though.
Part of why the test passes despite that is that the messages all have the same sender, and the action is attributed to that same user.
Probably the right fix is that the mkMoveAction call should pass an explicit user_id, so that these lines in the expected result can match that. And to fully exercise this part of the logic, it should be different from the user ID of the sender.
There was a problem hiding this comment.
I think this also highlights that this expected data is fairly verbose and repetitive -- that makes this detail easy to miss. It'd be good to instead write it with [message0, message1, message2].map(…), so that the ways it's different between messages and the ways it's the same stand out better from each other.
| { | ||
| ...message1, |
There was a problem hiding this comment.
Then here I guess [message1, message2].map(…) would be good. (Probably trying to unify the latter two with the content edit that happens to message0 would be more unhelpful than helpful.)
According to our types, the only case in which a message-update event is missing an edit_timestamp is when it's a rendering-only event that comes from a server pre-FL 114. So, fix some events that we don't mean to be rendering-only but that we haven't given an edit_timestamp.
…vents In FL 114, servers stopped representing rendering-only events with missing edit_timestamp, and started representing them with `rendering_only: true`. Handle both representations, not just the old one.
In particular, all the update-message events we simulate in this file have rendering_only, user_id, and edit_timestamp.
chrisbobbe
force-pushed
the
pr-maintain-edit-history
branch
from
7d207b8 to
77e97fc
Compare
Done, thanks for the review! |
2 participants
Following the plan we made in #5422, toward #5306.
I'm not sure I'm quite happy with the last commit, where we set
edit_historytonullbased onallowEditHistory. I've put the condition onallowEditHistoryout to the edge (eventToAction,api.getMessages)…but that means plumbingallowEditHistoryout to the edge. Is that OK?