unreadModel: Handle moving messages between topics/streams. #4980

WesleyAC · 2021-08-30T23:32:33Z

gnprice

Thanks! Comments below on the first commit. The second commit is more interesting, and I'll return to it after looking at some other things, but wanted to get these out without waiting 🙂

gnprice · 2021-09-01T21:35:33Z

src/actionTypes.js

+  message_ids: number[],
+  stream_id: number,
+  new_stream_id?: number,


Let's accompany this kind of change by having a comment at the top of the event link to the relevant bit of docs:
https://zulip.com/api/get-events#update_message
That helps a lot in reviewing a change here, or in comparing in the future to the possibly-by-then-further-changed API. It also makes it easy to look up what each of these properties is documented as being supposed to mean.

We mostly don't have those now (in fact it looks like there's just one such example, RestartEvent in eventTypes.js -- which not coincidentally looks like the latest event we've added to our types), mainly because the bulk of these type definitions were written down in the reverse-engineering era, when there were no API docs to link to.

gnprice · 2021-09-01T21:39:33Z

src/actionTypes.js

  message_id: number,
+  message_ids: number[],


I just mentioned linking to the API docs, and I had the thought that that substitutes for writing down any documentation here about what the properties mean... but as I read this event's docs, I see that this message_id vs. message_ids thing is odd enough, and enough of a potential gotcha, that it probably merits making an exception and writing something down here.

In particular, any content changes apply to just message_id, and any stream or topic changes apply to all of message_ids, which is guaranteed to include message_id.

The docs also kind of make it sound like message_ids isn't always present -- emphasis added:

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

even though the message_ids bit doesn't say it's optional. That might be good to clarify in chat on #api-design.

Confirmed in chat that we do expect it to be always present.

gnprice · 2021-09-01T21:40:36Z

src/actionTypes.js

  edit_timestamp: number,
  message_id: number,
+  message_ids: number[],
+  stream_id: number,


This should be optional, according to the docs. (It's there only when the message was sent to a stream.)

gnprice

And read the main commit. The logic generally looks good! Small comments below.

One commit-message nit:

unreadModel: Handle moving messages between topics/streams.

Fixes #4840.

See https://github.com/zulip/zulip-mobile/blob/master/docs/style.md#fixes-format .

gnprice · 2021-09-01T23:25:41Z

src/unread/unreadModel.js

+            )
+            .updateIn(
+              [action.new_stream_id ?? action.stream_id, action.subject],
+              (messages = Immutable.List()) => messages.push(...updated_unreads),


This data structure has an invariant that these lists are sorted (see jsdoc at the type definition, in unreadModelTypes.js), so this should sort.

gnprice · 2021-09-01T23:51:36Z

src/unread/unreadModel.js

+        const moved_ids = new Set(action.message_ids);
+        const updated_unreads = state


nit: we use camelCase for internal names (when not C_MACRO_CASE), following the usual convention for JS.

The exceptions are in names that come straight from the server API, like properties on objects that are taken straight from, or modified only lightly from, some JSON the server sent us.

gnprice · 2021-09-02T00:02:44Z

src/unread/unreadModel.js

+        if (updated_unreads.size > 0) {
+          return state


These conditionals (this, and the if above) have the feeling that the main line of the code's logic gets nested deeper and deeper as the code goes on. This pattern also has the effect that what happens in the alternative case (here, when updated_unreads is empty) is left in suspense for the reader until after all the other code.

I tend to prefer the "early return" pattern for arranging code flow: exceptional cases like errors and fast-path shortcuts go off to the side and then return, so that the main narrative of the code gets to continue vertically all in the same column. This also means that the condition for each exceptional case, and what we do in that case, get to stay right next to each other.

I can try demonstrating that rearrangement for this code at merge time, or after merge.

gnprice · 2021-09-02T00:04:55Z

src/unread/__tests__/unreadModel-test.js

+    test('if topic updated, but no unreads, return original state', () => {
+      const state = reducer(baseState, mkAction({ message_id: 100 }), eg.plusReduxState);
+      expect(state.streams).toBe(baseState.streams);
+    });


Does this test match its description? It doesn't look like a topic is updated.

gnprice · 2021-09-02T00:06:26Z

src/unread/__tests__/unreadModel-test.js

+      // prettier-ignore
+      expect(summary(state)).toEqual(Immutable.Map([
+        [123, Immutable.Map([['foo', [1, 2]]])],
+        [456, Immutable.Map([['zzz', [6, 7]], ['foo', [3, 4, 5]]])],


Let's also have a test case where they get moved to an existing topic (like 456 > 'zzz'), showing that the list gets properly combined with the existing one.

chrisbobbe · 2021-12-07T01:56:49Z

FYI I've just sent #5155; it might be convenient for that to be reviewed/merged before this.

gnprice · 2021-12-13T21:15:41Z

I've just pushed a revision of this PR, rebasing atop the API types from #5155 and applying my comments above. @chrisbobbe please take a look!

chrisbobbe

Thanks! See a few small comments/suggestions below. Let me know if I can be clearer on any of them.

chrisbobbe · 2021-12-13T22:16:41Z

src/unread/__tests__/unreadModel-test.js

+    const mkAction = args => {
+      const {
+        message_ids,
+        stream_id = 123,


I think this default parameter means that mkAction's callers can't request an action that omits stream_id: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters#passing_undefined_vs._other_falsy_values

Whether those callers will end up wanting to do that will depend on whether servers want to convey something relevant by omitting stream_id, I guess. You've written down the state of that question in unreadModel:

if (stream_id == null) { // Not stream messages, or else a pure content edit (no stream/topic change.) // // As of this writing, the docs actually promise this field for all // updates to stream messages. Empirically it's absent on pure // content edits, but present on edits affecting either the stream // or topic: // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1296823 return state; }

True. At present this test file is just for the "streams" part of the unreads data, because that's the part that has been modernized. In the future when we similarly modernize the rest and have its tests in the same place, we'll want to either add a different helper for those tests or modify this one.

Makes sense. Also, I see from your other comment that we've settled that servers will start always including stream_id for stream messages, so there's no reason for this modernized streams-focused part of the model to be tested with an event that's missing stream_id.

chrisbobbe · 2021-12-13T22:20:08Z

src/unread/__tests__/unreadModel-test.js

+      ]));
+    });
+
+    test('if topic not updated, return original state', () => {


Suggested change

test('if topic not updated, return original state', () => {

test('if topic/stream not updated, return original state', () => {

chrisbobbe · 2021-12-13T22:20:39Z

src/unread/__tests__/unreadModel-test.js

+      expect(state.streams).toBe(baseState.streams);
+    });
+
+    test('if topic updated, but no unreads, return original state', () => {


Suggested change

test('if topic updated, but no unreads, return original state', () => {

test('if topic updated (and not stream), but no unreads, return original state', () => {

chrisbobbe · 2021-12-13T22:21:03Z

src/unread/__tests__/unreadModel-test.js

+      expect(state.streams).toBe(baseState.streams);
+    });
+
+    test('if topic updated, move unreads', () => {


Suggested change

test('if topic updated, move unreads', () => {

test('if topic updated (and not stream), move unreads', () => {

chrisbobbe · 2021-12-13T22:21:25Z

src/unread/__tests__/unreadModel-test.js

+      ]));
+    });
+
+    test('if stream updated, move unreads', () => {


Suggested change

test('if stream updated, move unreads', () => {

test('if stream updated (and not topic), move unreads', () => {

chrisbobbe · 2021-12-13T22:25:12Z

src/unread/__tests__/unreadModel-test.js

+      ]));
+    });
+
+    test('if moved to existing topic, ids stay sorted', () => {


Suggested change

test('if moved to existing topic, ids stay sorted', () => {

test('if stream/topic both updated, and stream/topic thread has existing unreads, ids stay sorted', () => {

chrisbobbe · 2021-12-13T22:28:44Z

src/unread/unreadModel.js

+      if (
+        (action.subject === action.orig_subject || action.orig_subject == null)
+        && (action.new_stream_id === stream_id || action.new_stream_id == null)
+      ) {


I think a check like this makes a lot of sense, given that we don't have confirmation on what we've observed empirically; see above:

// As of this writing, the docs actually promise this field for all // updates to stream messages. Empirically it's absent on pure // content edits, but present on edits affecting either the stream // or topic: // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1296823

And even if we did, I still like it because it's nice and intuitive, even though I guess it would be kind of belt-and-suspenders, right. We would've already returned before this, on pure content edits, because of the absent stream_id.

Updated that above comment to reflect the further discussion in that thread: we plan to make stream_id present on all updates to stream messages. Then this check will be directly needed in order to skip pure content edits.

Fixes: zulip#4840 Co-authored-by: Greg Price <[email protected]>

gnprice · 2021-12-16T04:51:24Z

Thanks for the review!

I applied some of the test-name suggestions, but left out the "(and not stream)" ones; I think it's generally fine for the test names to highlight the thing that does happen and leave implicit that other things that would be interesting don't happen.

The revision is only in comments and test names, so going ahead and merging.

WesleyAC requested review from chrisbobbe and gnprice August 30, 2021 23:32

WesleyAC force-pushed the unreads-reducer-handle-topic-moves branch from 2106517 to 80401b9 Compare August 30, 2021 23:33

gnprice reviewed Sep 1, 2021

View reviewed changes

gnprice reviewed Sep 2, 2021

View reviewed changes

chrisbobbe mentioned this pull request Dec 7, 2021

unreadModel: respond to message topic/stream edits #4840

Closed

gnprice mentioned this pull request Dec 8, 2021

action types: Sync update-message event type with doc #5155

Merged

chrisbobbe assigned gnprice Dec 13, 2021

gnprice force-pushed the unreads-reducer-handle-topic-moves branch from 80401b9 to c9a38be Compare December 13, 2021 21:14

gnprice added a-data-sync Zulip's event system, event queues, staleness/liveness a-redux P1 high-priority labels Dec 13, 2021

chrisbobbe reviewed Dec 13, 2021

View reviewed changes

unread: Handle moving messages between topics/streams

50ea26e

Fixes: zulip#4840 Co-authored-by: Greg Price <[email protected]>

gnprice force-pushed the unreads-reducer-handle-topic-moves branch from c9a38be to 50ea26e Compare December 16, 2021 04:51

gnprice merged commit 50ea26e into zulip:main Dec 16, 2021

chrisbobbe mentioned this pull request Apr 11, 2022

streams: Show alert when stream name already exists. #5331

Merged

		const moved_ids = new Set(action.message_ids);
		const updated_unreads = state

	test('if topic not updated, return original state', () => {
	test('if topic/stream not updated, return original state', () => {

	test('if topic updated, but no unreads, return original state', () => {
	test('if topic updated (and not stream), but no unreads, return original state', () => {

	test('if topic updated, move unreads', () => {
	test('if topic updated (and not stream), move unreads', () => {

	test('if stream updated, move unreads', () => {
	test('if stream updated (and not topic), move unreads', () => {

	test('if moved to existing topic, ids stay sorted', () => {
	test('if stream/topic both updated, and stream/topic thread has existing unreads, ids stay sorted', () => {

Uh oh!

Uh oh!

unreadModel: Handle moving messages between topics/streams. #4980

unreadModel: Handle moving messages between topics/streams. #4980

Uh oh!

Conversation

WesleyAC commented Aug 30, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Dec 7, 2021

Uh oh!

gnprice commented Dec 13, 2021

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Dec 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe Dec 13, 2021 •

edited

Loading