Add skeleton of handling a move UpdateMessageEvent

[gnprice]

This PR is stacked atop #751.

Main commit:

The way the Zulip server API expresses whether there was a move,
and if so what kind of move, is a bit quirky. This change adds
logic to interpret that information into a more canonical form.

The logic here is translated from what we have in zulip-mobile:
https://github.com/zulip/zulip-mobile/blob/e352f563e/src/api/misc.js#L26

This doesn't yet do anything to actually update our data structures,
and it's an NFC change for release builds. The only behavior change
is new asserts that fire when the event from the server is malformed.

---

This is based on part of my draft branch for handling moved messages, #150. This covers the portion of that branch that's involved in just interpreting what an UpdateMessageEvent is saying about a message move, which is logic that will be needed in #171 / #750 too — so @PIG208 you'll want to rebase atop this.

The remainder of the draft branch is about actually updating Message.topic and Message.streamId to reflect a message move, and starting to update the data structures in MessageListView accordingly. So those changes will be needed for #150 but aren't involved in #171, and I'll leave them be for now and return to them sometime later.

[gnprice]

@PIG208 as part of #171 you'll be writing a helper for conveniently making UpdateMessageEvent values representing a move (an eg.updateMessageMoveEvent similar to the existing eg.updateMessageEditEvent). So then you can follow that with a small commit fixing this TODO.

[PIG208]

I found this check a bit confusing (as well as the following one on origStreamId) before looking at the API doc. It could be helpful to spell out what we are expecting these checks (something that I can add in my PRs stacked atop this).

[gnprice]

Hmm yeah, these are a bit terse.

Would this version explain it?

@@ -139,6 +139,9 @@ class MessageStoreImpl with MessageStore {
   }
 
   void _handleUpdateMessageEventMove(UpdateMessageEvent event) {
+    // The interaction between the fields of these events are a bit tricky.
+    // For reference, see: https://zulip.com/api/get-events#update_message
+
     if (event.origTopic == null) {
       // There was no move.
       assert(() {
@@ -154,10 +157,12 @@ class MessageStoreImpl with MessageStore {
     }
 
     if (event.newTopic == null) {
-      assert(debugLog('Malformed UpdateMessageEvent: move but no topic')); // TODO(log)
+      // The `subject` field (aka newTopic) is documented to be present on moves.
+      assert(debugLog('Malformed UpdateMessageEvent: move but no newTopic')); // TODO(log)
       return;
     }
     if (event.origStreamId == null) {
+      // The `stream_id` field (aka origStreamId) is documented to be present on moves.
       assert(debugLog('Malformed UpdateMessageEvent: move but no origStreamId')); // TODO(log)
       return;
     }

(That also fixes "no topic" to "no newTopic" — that's from a draft where I'd renamed that field only to "topic" rather than "newTopic".)

[PIG208]

Yeah this works.

[chrisbobbe]

GitHub is showing that this has a conflict; Greg, could you resolve that please?

[gnprice]

Sure — rebased.

[chrisbobbe]

Cool; thanks! One small comment below; if you think it's not an issue, or it's just an API-doc issue, please merge at will.

[chrisbobbe]

message: Update all edit timestamps on UpdateMessageEvent

So in this commit, message lists check event.messageIds to see if they need to notify listeners, and they don't check event.messageId for that purpose. I think the API doc isn't quite explicit that message_ids will include the message ID for a pure content edit: https://zulip.com/api/get-events#update_message

I might be splitting hairs, though. On the message_id field, the doc does have this:

[…] the separate message_ids field, which is guaranteed to include message_id.

…but then again, here's the full sentence that quote is from:

If the channel or topic was changed, the set of moved messages is encoded in the separate message_ids field, which is guaranteed to include message_id.

I did just test a pure content edit, though, and the message ID did appear in message_ids.

[PIG208]

It does appear to be the case that message_ids always show up.

However, this feels like one of the cases where it will be more convenient if we know that the field should never be missing or nullable. Having our OpenAPI spec properly defined so that a "only present if ..." field is marked as nullable/required explicitly will definitely help.

[chrisbobbe]

Ah, so I'm not actually looking at whether the message_ids field is present or not; I'm assuming it is present. I'm looking for an API guarantee that, in a pure content-edit event, message_ids will include the ID of the edited message, instead of being an empty array. If it can be an empty array, then I expect a message list would fail to show the new content immediately, because we wouldn't be calling MessageListView.notifyListeners.

[gnprice]

Hmm, I agree it'd be good to have that better nailed down. We have this in our API types in zulip-mobile:

  // Any stream/topic changes apply to all of message_ids, which is
  //   guaranteed to include message_id.
  // TODO(server-future): It'd be nice for these to be sorted; NB they may
  //   not be.  As of server 5.0-dev-3868-gca17a452f (2022-02), there's no
  //   guarantee of that in the API nor, apparently, in the implementation.
  message_ids: $ReadOnlyArray<number>,

Looks like that dates to zulip/zulip-mobile@4ce9e20 . That's actually after the era where we'd started having good API docs outside the zulip-mobile type definitions. But I don't see in that PR thread zulip/zulip-mobile#5155 , or the chat thread it links to, any discussion of this particular question.

Anyway, I do think we intend to make that guarantee. I'll ask in the thread and see about making it explicit in the API docs.

→ done: https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1833971

[gnprice]

Updated to add an assert checking the condition discussed above.

[chrisbobbe]

Thanks, LGTM! Merging.