Skip to content

Commit

Permalink
message: Handle update_message making stream/topic changes
Browse files Browse the repository at this point in the history 
This handles part of zulip#3408, closely related to zulip#2688.  In particular
this means that if you find one of these messages in an interleaved
view where recipient headers are shown, the recipient header will
take you to the correct narrow.

We actually did handle topic edits, but only for a single message,
and didn't handle moves between streams.
  • Loading branch information
@gnprice @chrisbobbe
gnprice authored and chrisbobbe committed Mar 2, 2022
1 parent 23a44bc commit 4260400
Show file tree
Hide file tree
Showing 2 changed files with 148 additions and 21 deletions.
103 changes: 97 additions & 6 deletions src/message/__tests__/messagesReducer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,18 @@ describe('messagesReducer', () => {
});
};

const mkMoveAction = args => {
const { message, ...restArgs } = args;
return mkAction({
message,
// stream_id and orig_subject are always present when either
// the stream or the topic was changed.
stream_id: message.stream_id,
orig_subject: message.subject,
...restArgs,
});
};

test('if a message does not exist no changes are made', () => {
const message1 = eg.streamMessage();
const message2 = eg.streamMessage();
Expand All @@ -199,6 +211,88 @@ describe('messagesReducer', () => {
expect(newState).toBe(prevState);
});

describe('move', () => {
test('edited topic', () => {
const message = eg.streamMessage();
const newTopic = `${message.subject}abc`;
const action = mkMoveAction({ message, subject: newTopic });
expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
eg.makeMessagesState([{ ...message, subject: newTopic }]),
);
});

test('other messages in conversation are unaffected', () => {
const topic = 'some topic';
const message1 = eg.streamMessage({ subject: topic });
const message2 = eg.streamMessage({ subject: topic });
const message3 = eg.streamMessage({ subject: topic });
const newTopic = 'some revised topic';
const action = mkMoveAction({
message: message1,
message_ids: [message1.id, message2.id],
subject: newTopic,
});
expect(
messagesReducer(
eg.makeMessagesState([message1, message2, message3]),
action,
eg.plusReduxState,
),
).toEqual(
eg.makeMessagesState([
{ ...message1, subject: newTopic },
{ ...message2, subject: newTopic },
message3,
]),
);
});

test('new stream', () => {
const message = eg.streamMessage();
const action = mkMoveAction({ message, new_stream_id: eg.otherStream.stream_id });
expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
eg.makeMessagesState([
{
...message,
stream_id: eg.otherStream.stream_id,
display_recipient: eg.otherStream.name,
},
]),
);
});

test('new stream + edited topic', () => {
const message = eg.streamMessage();
const newTopic = `${message.subject}abc`;
const action = mkMoveAction({
message,
new_stream_id: eg.otherStream.stream_id,
subject: newTopic,
});
expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
eg.makeMessagesState([
{
...message,
stream_id: eg.otherStream.stream_id,
display_recipient: eg.otherStream.name,
subject: newTopic,
},
]),
);
});

test('new, unknown stream', () => {
const message = eg.streamMessage();
const unknownStream = eg.makeStream();
const action = mkMoveAction({ message, new_stream_id: unknownStream.stream_id });
expect(messagesReducer(eg.makeMessagesState([message]), action, eg.plusReduxState)).toEqual(
eg.makeMessagesState([
{ ...message, stream_id: unknownStream.stream_id, display_recipient: 'unknown' },
]),
);
});
});

test('when a message exists in state, it is updated', () => {
const message1 = eg.streamMessage();
const message2 = eg.streamMessage();
Expand Down Expand Up @@ -237,11 +331,9 @@ describe('messagesReducer', () => {
last_edit_timestamp: 123,
};
const prevState = eg.makeMessagesState([message1Old]);
const action = mkAction({
const action = mkMoveAction({
edit_timestamp: 123,
message: message1New,
stream_id: message1Old.stream_id,
orig_subject: message1Old.subject,
message: message1Old,
subject: message1New.subject,
});
const expectedState = eg.makeMessagesState([message1New]);
Expand Down Expand Up @@ -270,15 +362,14 @@ describe('messagesReducer', () => {
};

const prevState = eg.makeMessagesState([message1Old]);
const action = mkAction({
const action = mkMoveAction({
edit_timestamp: 456,
message: message1Old,
orig_content: message1Old.content,
orig_rendered_content: message1Old.content,
rendered_content: message1New.content,
content: message1New.content,
subject: message1New.subject,
orig_subject: message1Old.subject,
prev_rendered_content_version: 1,
});
const expectedState = eg.makeMessagesState([message1New]);
Expand Down
66 changes: 51 additions & 15 deletions src/message/messagesReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
EVENT_UPDATE_MESSAGE,
} from '../actionConstants';
import { getNarrowsForMessage, keyFromNarrow } from '../utils/narrow';
import * as logging from '../utils/logging';
import { getStreamsById } from '../selectors';

const initialState: MessagesState = Immutable.Map([]);

Expand Down Expand Up @@ -137,32 +139,66 @@ export default (
return state.deleteAll(action.messageIds);

case EVENT_UPDATE_MESSAGE: {
const { event } = action;
return state.update(event.message_id, <M: Message>(oldMessage: M): M => {
const { event, move } = action;
let result = state;

result = result.update(event.message_id, <M: Message>(oldMessage: M): M => {
if (!oldMessage) {
return oldMessage;
}

const messageWithNewCommonFields: M = {
return {
...(oldMessage: M),
content: event.rendered_content ?? oldMessage.content,
last_edit_timestamp: event.edit_timestamp ?? oldMessage.last_edit_timestamp,
// TODO(#3408): Update edit_history, too. This is OK for now
// because we don't actually have any UI to expose it: #4134.
};

// FlowIssue: https://github.com/facebook/flow/issues/8833
// The cast `: 'stream'` is silly but harmless, and works
// around a Flow issue which causes an error.
return messageWithNewCommonFields.type === ('stream': 'stream')
? {
...messageWithNewCommonFields,
subject: event.subject ?? messageWithNewCommonFields.subject,
// TODO(#3408): Update topic_links. This is OK for now
// because we don't have any UI to expose it.
}
: messageWithNewCommonFields;
});

if (move) {
const update: { subject: string, stream_id?: number, display_recipient?: string } = {
subject: move.new_topic,
// TODO(#3408): Update topic_links. This is OK for now
// because we don't have any UI to expose it.
// TODO(#3408): Update last_edit_timestamp, probably. But want to
// say "moved" in the UI in this case, not "edited".
};
if (move.new_stream_id !== move.orig_stream_id) {
update.stream_id = move.new_stream_id;
const newStream = getStreamsById(globalState).get(move.new_stream_id);
// It's normal for newStream to potentially be missing here: it
// happens when the move was to a stream our user can't see.
// TODO(i18n): Not sure this "unknown" ever reaches the UI, but
// it'd be nice to somehow translate it in case it can.
update.display_recipient = newStream?.name ?? 'unknown';
}

// eslint-disable-next-line no-shadow
result = result.withMutations(state => {
for (const id of event.message_ids) {
state.update(id, <M: Message>(oldMessage: M | void): M | void => {
if (!oldMessage) {
return oldMessage;
}

// FlowIssue: https://github.com/facebook/flow/issues/8833
// The cast `: 'stream'` is silly but harmless, and works
// around a Flow issue which causes an error.
if (oldMessage.type !== ('stream': 'stream')) {
logging.warn('messagesReducer: got update_message with stream/topic move on PM');
return oldMessage;
}

// TODO(#3408): Update edit_history, too. This is OK for now
// because we don't actually have any UI to expose it: #4134.
return { ...oldMessage, ...update };
});
}
});
}

return result;
}

default:
Expand Down

0 comments on commit 4260400

Please sign in to comment.