messages reducer: Sketch a plan for keeping Message.edit_history current#5422
Conversation
chrisbobbe
added
P1 high-priority
server release goal
gnprice
left a comment
gnprice
left a comment
There was a problem hiding this comment.
Thanks @chrisbobbe! This looks good. Small comments below, then please merge at will.
| // TODO(server-5.0): Always present as of FL 114; make required. | ||
| user_id?: UserId | null, |
There was a problem hiding this comment.
| // TODO(server-5.0): Always present as of FL 114; make required. | |
| user_id?: UserId | null, | |
| // Before FL 114, can be absent with the same meaning as null. | |
| // TODO(server-5.0): Make required. | |
| user_id?: UserId | null, |
I.e., include the information of how to interpret the old format.
There was a problem hiding this comment.
Good thought, thanks.
user_id: integer | null
The ID of the user who sent the message.
Null when event is for a rendering update of the original message, such as for an inline URL preview.
Changes: As of Zulip 5.0 (feature level 114), this field is present for all update_message events. Previously, this field was omitted for inline URL preview updates.
Hmm. Read literally, the meaning isn't exactly the same: in the old format, "missing" is for inline URL preview updates; in the new format, null is for any "rendering update," which includes but isn't limited to inline URL preview updates.
In main, I think it's probably true that "URL preview update" is the only kind of "rendering update" we have. And I'm pretty sure that's true of pre-FL 114 servers.
So I guess there isn't anything I would change from your suggestion, but I wanted to point it out.
There was a problem hiding this comment.
Right. The old version of the API didn't have any explicit coherent concept here; when revising it, "rendering update" was the concept we came up with to explain why it made sense for the events that had been missing this field to be missing it.
| // TODO(server-5.0): New in FL 114. | ||
| rendering_only?: boolean, |
There was a problem hiding this comment.
| // TODO(server-5.0): New in FL 114. | |
| rendering_only?: boolean, | |
| // TODO(server-5.0): New in FL 114. On old servers, infer from `user_id`. | |
| rendering_only?: boolean, |
| // - Handle changes to the allow-edit-history setting: | ||
| // - When allow-edit-history is turned off, set all messages' | ||
| // edit_history to `null`. (Otherwise we'd be keeping around | ||
| // edit_history data that admins would rather we didn't have…and also, | ||
| // that data would get irrecoverably stale as messages get updated.) | ||
| // - When allow-edit-history is turned on…probably just leave | ||
| // edit_history `null` on messages we know about, don't force `null` | ||
| // on new or newly fetched messages, and expect a re-/register | ||
| // soon? |
There was a problem hiding this comment.
As we discussed in yesterday's call, let's simplify this:
We'll plan to treat an allow-edit-history change similar to a restart event (in particular, a restart event where the server version changed, which could mean any number of subtle changes in server behavior), and do a re-fetch of server data soon after it.
- To track this task, can add it to the list in Ensure all server events and their operations are handled #3408. Then for now, we don't worry about it because we frequently re-fetch anyway.
There was a problem hiding this comment.
can add it to the list in
Done.
| * Null if it's coming from a server with FL <118; see comment on | ||
| * MessageEdit. |
There was a problem hiding this comment.
nit: "null" is a keyword, so keep it case-sensitive:
| * Null if it's coming from a server with FL <118; see comment on | |
| * MessageEdit. | |
| * This is null if it's coming from a server with FL <118; see comment on | |
| * MessageEdit. |
…r void We decided to go with `null` for the pre-118 case instead of undefined/missing. Update a comment to say that, and update a type cast to not say that servers might send `null`, since they won't. (That type cast, and another like it, still isn't correct; we'll fix another issue soon.)
…mments Looking at the doc again (https://zulip.com/api/get-messages), we missed some cases where we should expect servers not to include this: - message has no edit history - viewing edit history not allowed So, reflect that in the type. And add a jsdoc that mentions that, and some TODOs for how to make this property useful.
chrisbobbe
force-pushed
the
pr-prep-maintain-edit-history
branch
from
c8204df to
fbe1a05
Compare
Thanks for the review! Merged, with those fixes. |
2 participants
For the plan, see the jsdoc and multi-part TODO added to
Message['edit_history']in the second commit.Maintaining an up-to-date
Message['edit_history']is on the path to #5306.