Skip to content

Commit

Permalink
msglist: Handle topic permalinks (/with/<id>) just as we do /near/ links
Browse files Browse the repository at this point in the history 
Explanation in a comment, which quotes Greg in zulip#5866.

Fixes: zulip#5866
  • Loading branch information
@chrisbobbe
chrisbobbe committed Jun 10, 2024
1 parent d30f94e commit 9562778
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 5 deletions.
15 changes: 15 additions & 0 deletions src/message/__tests__/messageActions-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,21 @@ describe('messageActions', () => {
pmNarrowFromUsersUnsafe([user1, user2]),
);
});

test('handles /with/ links', async () => {
const stream = eg.makeStream({ stream_id: 1, name: 'test' });
const user1 = eg.makeUser({ user_id: 1, full_name: 'user 1' });
const user2 = eg.makeUser({ user_id: 2, full_name: 'user 2' });
const { store } = prepare({ streams: [stream], users: [user1, user2] });

await checkLink(store, '#narrow/stream/1-test/topic/hello/with/1', topicNarrow(1, 'hello'));
await checkLink(store, '#narrow/dm/1-user-1/with/1', pmNarrowFromUsersUnsafe([user1]));
await checkLink(
store,
'#narrow/dm/1,2-group/with/1',
pmNarrowFromUsersUnsafe([user1, user2]),
);
});
});

describe('doNarrow', () => {
Expand Down
57 changes: 57 additions & 0 deletions src/utils/__tests__/internalLinks-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,10 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
[
'/#narrow/channel/1-jest/topic/test',
'/#narrow/channel/4-mobile/subject/topic/near/378333',
'/#narrow/channel/4-mobile/subject/topic/with/378333',
'/#narrow/channel/4-mobile/topic/topic/',
'/#narrow/channel/3-stream/topic/topic/near/1',
'/#narrow/channel/3-stream/topic/topic/with/1',
].forEach(hash => check(hash));
});

Expand All @@ -204,9 +206,12 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
[
'/#narrow/stream/jest/topic/test',
'/#narrow/stream/mobile/subject/topic/near/378333',
'/#narrow/stream/mobile/subject/topic/with/378333',
'/#narrow/stream/mobile/topic/topic/',
'/#narrow/stream/stream/topic/topic/near/1',
'/#narrow/stream/stream/topic/topic/with/1',
'/#narrow/stream/stream/subject/topic/near/1',
'/#narrow/stream/stream/subject/topic/with/1',
'/#narrow/stream/stream/subject/topic',
].forEach(hash => check(hash));
});
Expand All @@ -216,7 +221,9 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
[
'/#narrow/dm/1,2-group',
'/#narrow/dm/1,2-group/near/1',
'/#narrow/dm/1,2-group/with/1',
'/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3',
'/#narrow/dm/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/3',
].forEach(hash => check(hash));
});

Expand All @@ -225,7 +232,9 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
[
'/#narrow/pm-with/1,2-group',
'/#narrow/pm-with/1,2-group/near/1',
'/#narrow/pm-with/1,2-group/with/1',
'/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/near/3',
'/#narrow/pm-with/a.40b.2Ecom.2Ec.2Ed.2Ecom/with/3',
].forEach(hash => check(hash));
});

Expand All @@ -245,6 +254,9 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
// `near` with no operand
'/#narrow/channel/stream/topic/topic/near/',

// `with` with no operand
'/#narrow/channel/stream/topic/topic/with/',

// `is` with invalid operand
'/#narrow/is/men',
'/#narrow/is/men/channel',
Expand Down Expand Up @@ -472,6 +484,31 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
get(`https://example.com/#narrow/stream/${eg.stream.name}/subject/test/near/1`, [eg.stream]),
).toEqual(topicNarrow(eg.stream.stream_id, 'test'));
});

test('on a /with/ link', () => {
const ids = `${userB.user_id},${userC.user_id}`;
expect(get(`https://example.com/#narrow/dm/${ids}-group/with/2`, [])).toEqual(
pmNarrowFromUsersUnsafe([userB, userC]),
);
expect(get(`https://example.com/#narrow/pm-with/${ids}-group/with/2`, [])).toEqual(
pmNarrowFromUsersUnsafe([userB, userC]),
);

expect(
get(
`https://example.com/#narrow/channel/${eg.stream.stream_id}-${eg.stream.name}/topic/test/with/1`,
[eg.stream],
),
).toEqual(topicNarrow(eg.stream.stream_id, 'test'));

expect(
get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/test/with/1`, [eg.stream]),
).toEqual(topicNarrow(eg.stream.stream_id, 'test'));

expect(
get(`https://example.com/#narrow/stream/${eg.stream.name}/subject/test/with/1`, [eg.stream]),
).toEqual(topicNarrow(eg.stream.stream_id, 'test'));
});
});

describe('getNearOperandFromLink', () => {
Expand All @@ -485,13 +522,24 @@ describe('getNearOperandFromLink', () => {
expect(getNearOperandFromLink(new URL('/#narrow/near/1', realm), realm)).toBe(1);
});

test('`with` is the only operator', () => {
expect(getNearOperandFromLink(new URL('/#narrow/with/1', realm), realm)).toBe(1);
});

test('when link is a group link, return anchor message id', () => {
expect(getNearOperandFromLink(new URL('/#narrow/dm/1,3-group/near/1/', realm), realm)).toBe(1);
expect(
getNearOperandFromLink(new URL('/#narrow/pm-with/1,3-group/near/1/', realm), realm),
).toBe(1);
});

test('when link is a group link with /with/, return anchor message id', () => {
expect(getNearOperandFromLink(new URL('/#narrow/dm/1,3-group/with/1/', realm), realm)).toBe(1);
expect(
getNearOperandFromLink(new URL('/#narrow/pm-with/1,3-group/with/1/', realm), realm),
).toBe(1);
});

test('when link is a topic link, return anchor message id', () => {
expect(
getNearOperandFromLink(new URL('/#narrow/channel/1-jest/topic/test/near/1', realm), realm),
Expand All @@ -500,4 +548,13 @@ describe('getNearOperandFromLink', () => {
getNearOperandFromLink(new URL('/#narrow/stream/jest/topic/test/near/1', realm), realm),
).toBe(1);
});

test('when link is a topic link with /with/, return anchor message id', () => {
expect(
getNearOperandFromLink(new URL('/#narrow/channel/1-jest/topic/test/with/1', realm), realm),
).toBe(1);
expect(
getNearOperandFromLink(new URL('/#narrow/stream/jest/topic/test/with/1', realm), realm),
).toBe(1);
});
});
19 changes: 14 additions & 5 deletions src/utils/internalLinks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export const getNarrowFromNarrowLink = (
(hashSegments.length === 2 && (hashSegments[0] === 'pm-with' || hashSegments[0] === 'dm'))
|| (hashSegments.length === 4
&& (hashSegments[0] === 'pm-with' || hashSegments[0] === 'dm')
&& hashSegments[2] === 'near')
&& (hashSegments[2] === 'near' || hashSegments[2] === 'with'))
) {
// TODO: This case is pretty useless in practice, due to basically a
// bug in the webapp: the URL that appears in the location bar for a
Expand All @@ -167,7 +167,7 @@ export const getNarrowFromNarrowLink = (
|| (hashSegments.length === 6
&& (hashSegments[0] === 'stream' || hashSegments[0] === 'channel')
&& (hashSegments[2] === 'subject' || hashSegments[2] === 'topic')
&& hashSegments[4] === 'near')
&& (hashSegments[4] === 'near' || hashSegments[4] === 'with'))
) {
const streamId = parseStreamOperand(hashSegments[1], streamsById, streamsByName);
return streamId != null ? topicNarrow(streamId, parseTopicOperand(hashSegments[3])) : null;
Expand Down Expand Up @@ -219,9 +219,18 @@ export const getNearOperandFromLink = (url: URL, realm: URL): number | null => {
(str, i) =>
// This is a segment where we expect an operator to be specified.
i % 2 === 0
// The operator is 'near' and its meaning is not negated (`str` does
// not start with "-").
&& str === 'near',
// The operator is 'near' or 'with' and its meaning is not negated
// (`str` does not start with "-").
//
// Quoting Greg from #5866:
// > Currently the app doesn't interpret the "anchor" meaning of
// > `/near/` links at all (that's #3604) — we already effectively
// > give `/near/` links exactly the meaning that the upcoming
// > `/with/` links will have. So to handle the new links, we just
// > need to parse them and then handle them the way we already handle
// > `/near/` links.
// Which makes sense for this legacy codebase.
&& (str === 'near' || str === 'with'),
);
if (nearOperatorIndex < 0) {
return null;
Expand Down

0 comments on commit 9562778

Please sign in to comment.