Skip to content

Commit

Permalink
narrow: Make getNarrowForReply stop returning stream narrows.
Browse files Browse the repository at this point in the history
There's no reason we would sometimes want a stream narrow here. This
is just how it was when this function was first written (as
`getNarrowFromMessage`), for narrowing to a message's conversation,
in 34ab1f9 (zulip#333) in 2017.

This case also should be impossible anyway, because message topics
should always be nonempty -- we convert empty to "(no topic)".

Co-authored-by: Chris Bobbe <cbobbe@zulip.com>
  • Loading branch information
gnprice and chrisbobbe committed Dec 9, 2020
1 parent ffb2146 commit f3ce1ce
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 21 deletions.
10 changes: 0 additions & 10 deletions src/utils/__tests__/narrow-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -327,16 +327,6 @@ describe('getNarrowForReply', () => {
expect(actualNarrow).toEqual(expectedNarrow);
});

test('for stream message with empty topic, returns a stream narrow', () => {
// TODO this behavior seems pretty dubious
const message = eg.streamMessage({ subject: '' });
const expectedNarrow = streamNarrow(eg.stream.name);

const actualNarrow = getNarrowForReply(message, eg.selfUser);

expect(actualNarrow).toEqual(expectedNarrow);
});

test('for stream message with nonempty topic, returns a topic narrow', () => {
const message = eg.streamMessage();
const expectedNarrow = topicNarrow(eg.stream.name, message.subject);
Expand Down
15 changes: 4 additions & 11 deletions src/utils/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,22 +318,15 @@ export const canSendToNarrow = (narrow: Narrow): boolean =>
/**
* Answers the question, "Where should my reply to a message go?"
*
* Careful: quirky behavior on a stream message with empty topic.
*
* If you think you want to reuse this function: study carefully; maybe
* refactor it to something cleaner first; if not, then definitely document
* its quirky behavior.
* For stream messages, chooses a topic narrow over a stream narrow.
*/
// TODO: do that, or just make this a private local helper of its one caller
// TODO: probably make this a private local helper of its one caller,
// now that it's free of fiddly details from the Narrow data structure
export const getNarrowForReply = (message: Message | Outbox, ownUser: User) => {
if (message.type === 'private') {
return pmNarrowFromEmails(pmKeyRecipientsFromMessage(message, ownUser).map(x => x.email));
} else {
const streamName = streamNameOfStreamMessage(message);
const topic = message.subject;
if (topic && topic.length) {
return topicNarrow(streamName, topic);
}
return streamNarrow(streamName);
return topicNarrow(streamName, message.subject);
}
};

0 comments on commit f3ce1ce

Please sign in to comment.