-
-
Notifications
You must be signed in to change notification settings - Fork 676
unreadModel: Handle moving messages between topics/streams. #4980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,7 +1,11 @@ | ||||||
| /* @flow strict-local */ | ||||||
| import Immutable from 'immutable'; | ||||||
|
|
||||||
| import { ACCOUNT_SWITCH, EVENT_UPDATE_MESSAGE_FLAGS } from '../../actionConstants'; | ||||||
| import { | ||||||
| ACCOUNT_SWITCH, | ||||||
| EVENT_UPDATE_MESSAGE_FLAGS, | ||||||
| EVENT_UPDATE_MESSAGE, | ||||||
| } from '../../actionConstants'; | ||||||
| import { reducer } from '../unreadModel'; | ||||||
| import { type UnreadState } from '../unreadModelTypes'; | ||||||
| import * as eg from '../../__tests__/lib/exampleData'; | ||||||
|
|
@@ -63,6 +67,114 @@ describe('stream substate', () => { | |||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| describe('EVENT_UPDATE_MESSAGE', () => { | ||||||
| const mkAction = args => { | ||||||
| const { | ||||||
| message_ids, | ||||||
| stream_id = 123, | ||||||
| new_stream_id = undefined, | ||||||
| orig_subject = 'foo', | ||||||
| subject = 'foo', | ||||||
| } = args; | ||||||
| return { | ||||||
| id: 1, | ||||||
| type: EVENT_UPDATE_MESSAGE, | ||||||
| user_id: eg.selfUser.user_id, | ||||||
| message_id: message_ids[0], | ||||||
| message_ids, | ||||||
| edit_timestamp: 10000, | ||||||
| stream_id, | ||||||
| new_stream_id, | ||||||
| propagate_mode: 'change_later', | ||||||
| orig_subject, | ||||||
| subject, | ||||||
| subject_links: [], | ||||||
| orig_content: '', | ||||||
| orig_rendered_content: '', | ||||||
| prev_rendered_content_version: 0, | ||||||
| content: '', | ||||||
| rendered_content: '', | ||||||
| is_me_message: false, | ||||||
| flags: [], | ||||||
| }; | ||||||
| }; | ||||||
|
|
||||||
| const baseState = (() => { | ||||||
| const streamAction = args => eg.mkActionEventNewMessage(eg.streamMessage(args)); | ||||||
| const r = (state, action) => reducer(state, action, eg.plusReduxState); | ||||||
| let state = initialState; | ||||||
| state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 1 })); | ||||||
| state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 2 })); | ||||||
| state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 3 })); | ||||||
| state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 4 })); | ||||||
| state = r(state, streamAction({ stream_id: 456, subject: 'zzz', id: 6 })); | ||||||
| state = r(state, streamAction({ stream_id: 456, subject: 'zzz', id: 7 })); | ||||||
| state = r(state, streamAction({ stream_id: 123, subject: 'foo', id: 15 })); | ||||||
| return state; | ||||||
| })(); | ||||||
|
|
||||||
| test('(base state, for comparison)', () => { | ||||||
| // prettier-ignore | ||||||
| expect(summary(baseState)).toEqual(Immutable.Map([ | ||||||
| [123, Immutable.Map([['foo', [1, 2, 3, 4, 15]]])], | ||||||
| [456, Immutable.Map([['zzz', [6, 7]]])], | ||||||
| ])); | ||||||
| }); | ||||||
|
|
||||||
| test('if topic/stream not updated, return original state', () => { | ||||||
| const state = reducer(baseState, mkAction({ message_ids: [5] }), eg.plusReduxState); | ||||||
| expect(state.streams).toBe(baseState.streams); | ||||||
| }); | ||||||
|
|
||||||
| test('if topic updated, but no unreads, return original state', () => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| const state = reducer( | ||||||
| baseState, | ||||||
| mkAction({ message_ids: [100], orig_subject: 'foo', subject: 'bar' }), | ||||||
| eg.plusReduxState, | ||||||
| ); | ||||||
| expect(state.streams).toBe(baseState.streams); | ||||||
| }); | ||||||
|
|
||||||
| test('if topic updated, move unreads', () => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| const state = reducer( | ||||||
| baseState, | ||||||
| mkAction({ message_ids: [3, 4, 15], orig_subject: 'foo', subject: 'bar' }), | ||||||
| eg.plusReduxState, | ||||||
| ); | ||||||
| // prettier-ignore | ||||||
| expect(summary(state)).toEqual(Immutable.Map([ | ||||||
| [123, Immutable.Map([['foo', [1, 2]], ['bar', [3, 4, 15]]])], | ||||||
| [456, Immutable.Map([['zzz', [6, 7]]])], | ||||||
| ])); | ||||||
| }); | ||||||
|
|
||||||
| test('if stream updated, move unreads', () => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| const state = reducer( | ||||||
| baseState, | ||||||
| mkAction({ message_ids: [3, 4, 15], new_stream_id: 456 }), | ||||||
| eg.plusReduxState, | ||||||
| ); | ||||||
| // prettier-ignore | ||||||
| expect(summary(state)).toEqual(Immutable.Map([ | ||||||
| [123, Immutable.Map([['foo', [1, 2]]])], | ||||||
| [456, Immutable.Map([['zzz', [6, 7]], ['foo', [3, 4, 15]]])], | ||||||
| ])); | ||||||
| }); | ||||||
|
|
||||||
| test('if moved to topic with existing unreads, ids stay sorted', () => { | ||||||
| const state = reducer( | ||||||
| baseState, | ||||||
| mkAction({ message_ids: [3, 4, 15], new_stream_id: 456, subject: 'zzz' }), | ||||||
| eg.plusReduxState, | ||||||
| ); | ||||||
| // prettier-ignore | ||||||
| expect(summary(state)).toEqual(Immutable.Map([ | ||||||
| [123, Immutable.Map([['foo', [1, 2]]])], | ||||||
| [456, Immutable.Map([['zzz', [3, 4, 6, 7, 15]]])], | ||||||
| ])); | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| describe('EVENT_NEW_MESSAGE', () => { | ||||||
| const action = eg.mkActionEventNewMessage; | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import { | |
| ACCOUNT_SWITCH, | ||
| EVENT_MESSAGE_DELETE, | ||
| EVENT_NEW_MESSAGE, | ||
| EVENT_UPDATE_MESSAGE, | ||
| EVENT_UPDATE_MESSAGE_FLAGS, | ||
| LOGOUT, | ||
| MESSAGE_FETCH_COMPLETE, | ||
|
|
@@ -203,6 +204,48 @@ function streamsReducer( | |
| return deleteMessages(state, action.messages); | ||
| } | ||
|
|
||
| case EVENT_UPDATE_MESSAGE: { | ||
| const { stream_id } = action; | ||
| if (stream_id == null) { | ||
| // Not stream messages, or else a pure content edit (no stream/topic change.) | ||
| // | ||
| // The docs actually promise this field for all updates to stream | ||
| // messages. As of 2021-12 (circa feature level 111): | ||
| // https://chat.zulip.org/#narrow/stream/378-api-design/topic/.60update_message.60.20event/near/1296823 | ||
| // empirically it's present just on edits affecting either the | ||
| // stream or topic (so, absent on pure content edits), and the plan | ||
| // is to make it indeed present for all updates to stream messages. | ||
| return state; | ||
| } | ||
|
|
||
| if ( | ||
| (action.subject === action.orig_subject || action.orig_subject == null) | ||
| && (action.new_stream_id === stream_id || action.new_stream_id == null) | ||
| ) { | ||
|
Comment on lines
+221
to
+224
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated that above comment to reflect the further discussion in that thread: we plan to make |
||
| // Stream and topic didn't change. | ||
| return state; | ||
| } | ||
|
|
||
| const actionIds = new Set(action.message_ids); | ||
| const matchingIds = state | ||
| .getIn([stream_id, action.orig_subject ?? action.subject], Immutable.List()) | ||
| .filter(id => actionIds.has(id)); | ||
| if (matchingIds.size === 0) { | ||
| // None of the updated messages were unread. | ||
| return state; | ||
| } | ||
|
|
||
| return state | ||
| .updateIn( | ||
| [stream_id, action.orig_subject ?? action.subject], | ||
| (messages = Immutable.List()) => messages.filter(id => !actionIds.has(id)), | ||
| ) | ||
| .updateIn( | ||
| [action.new_stream_id ?? stream_id, action.subject], | ||
| (messages = Immutable.List()) => messages.push(...matchingIds).sort(), | ||
| ); | ||
| } | ||
|
|
||
| default: | ||
| return state; | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this default parameter means that
mkAction's callers can't request an action that omitsstream_id: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Default_parameters#passing_undefined_vs._other_falsy_valuesWhether 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 inunreadModel:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Also, I see from your other comment that we've settled that servers will start always including
stream_idfor stream messages, so there's no reason for this modernized streams-focused part of the model to be tested with an event that's missingstream_id.