Skip to content

Commit

Permalink
internalLinks: Recognize "channel" as operator in narrow links
Browse files Browse the repository at this point in the history 
Fixes: #5860
  • Loading branch information
@chrisbobbe
chrisbobbe committed Apr 24, 2024
1 parent 5fa3ce6 commit 4e86013
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 32 deletions.
105 changes: 75 additions & 30 deletions src/utils/__tests__/internalLinks-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,61 +44,71 @@ describe('isNarrowLink', () => {
[true, 'legacy: stream name, no ID', urlOnRealm('#narrow/stream/jest')],
[true, 'legacy: stream name, no ID, topic', urlOnRealm('#narrow/stream/jest/topic/topic1')],

[true, 'with numeric stream ID', urlOnRealm('#narrow/stream/123-jest')],
[true, 'with numeric stream ID and topic', urlOnRealm('#narrow/stream/123-jest/topic/topic1')],
[true, 'with numeric channel ID', urlOnRealm('#narrow/channel/123-jest')],
[true, 'with numeric channel ID (old operator)', urlOnRealm('#narrow/stream/123-jest')],
[
true,
'with numeric channel ID and topic',
urlOnRealm('#narrow/channel/123-jest/topic/topic1'),
],
[
true,
'with numeric channel ID (old operator) and topic',
urlOnRealm('#narrow/stream/123-jest/topic/topic1'),
],

[true, 'with numeric pm user IDs (new operator)', urlOnRealm('#narrow/dm/123-mark')],
[true, 'with numeric pm user IDs (old operator)', urlOnRealm('#narrow/pm-with/123-mark')],

[false, 'wrong fragment', urlOnRealm('#nope')],
[false, 'wrong path', urlOnRealm('/user_uploads/#narrow/stream/jest')],
[false, 'wrong domain', new URL('https://another.com/#narrow/stream/jest')],
[false, 'wrong path', urlOnRealm('/user_uploads/#narrow/channel/jest')],
[false, 'wrong domain', new URL('https://another.com/#narrow/channel/jest')],

[false, '#narrowly', urlOnRealm('#narrowly/stream/jest')],
[false, '#narrowly', urlOnRealm('#narrowly/channel/jest')],

[false, 'double slash', new URL(`${realm.origin}//#narrow/stream/jest`)],
[false, 'triple slash', new URL(`${realm.origin}///#narrow/stream/jest`)],
[false, 'double slash', new URL(`${realm.origin}//#narrow/channel/jest`)],
[false, 'triple slash', new URL(`${realm.origin}///#narrow/channel/jest`)],

[
true,
'with port',
new URL('#narrow/stream/jest', 'https://example.com:444/'),
new URL('#narrow/channel/jest', 'https://example.com:444/'),
new URL('https://example.com:444/'),
],

// This one fails because our polyfilled URL implementation has IDNA stripped out.
// [
// true,
// 'same domain, punycoded host',
// new URL('https://example.xn--h2brj9c/#narrow/stream/jest'),
// new URL('https://example.xn--h2brj9c/#narrow/channel/jest'),
// new URL('https://example.भारत/'),
// ], // FAILS
[
true,
'punycodable host',
new URL('#narrow/stream/jest', 'https://example.भारत/'),
new URL('#narrow/channel/jest', 'https://example.भारत/'),
new URL('https://example.भारत/'),
],

// This one fails because our polyfilled URL implementation has IDNA stripped out.
// [
// true,
// 'same domain, IDNA-mappable',
// new URL('https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/stream/jest'),
// new URL('https://ℯⅩªm🄿ₗℰ.ℭᴼⓂ/#narrow/channel/jest'),
// new URL('https://example.com'),
// ], // FAILS

[
true,
'ipv4 address',
new URL('#narrow/stream/jest', 'http://192.168.0.1/'),
new URL('#narrow/channel/jest', 'http://192.168.0.1/'),
new URL('http://192.168.0.1/'),
],
// This one fails because our polyfilled URL implementation has IDNA stripped out.
// [
// true,
// 'same IPv4 address, IDNA-mappable',
// new URL('http://１𝟗𝟚。①⁶🯸．₀｡𝟭/#narrow/stream/jest'),
// new URL('http://１𝟗𝟚。①⁶🯸．₀｡𝟭/#narrow/channel/jest'),
// new URL('http://192.168.0.1/'),
// ], // FAILS

Expand All @@ -111,17 +121,17 @@ describe('isNarrowLink', () => {
// This one, except possibly the fragment, is a 100% realistic link
// for innocent normal use. The buggy old version narrowly avoided
// accepting it... but would accept all the variations below.
new URL(`https://web.archive.org/web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
new URL(`https://web.archive.org/web/*/${urlOnRealm('#narrow/channel/jest').toString()}`),
],
[
false,
'odd scheme, wrong domain, realm-like path, narrow-like fragment',
new URL(`ftp://web.archive.org/web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
new URL(`ftp://web.archive.org/web/*/${urlOnRealm('#narrow/channel/jest').toString()}`),
],
[
false,
'same domain, realm-like path, narrow-like fragment',
urlOnRealm(`web/*/${urlOnRealm('#narrow/stream/jest').toString()}`),
urlOnRealm(`web/*/${urlOnRealm('#narrow/channel/jest').toString()}`),
],
];

Expand All @@ -138,10 +148,10 @@ describe('isNarrowLink', () => {
describe('getNarrowFromNarrowLink (part 1)', () => {
const mkCheck = (narrowExpectation: (Narrow => boolean) | null) => hash => {
const streams = [
eg.makeStream({ name: 'jest' }),
eg.makeStream({ name: 'stream' }),
eg.makeStream({ name: 'topic' }),
eg.makeStream({ name: 'mobile' }),
eg.makeStream({ stream_id: 1, name: 'jest' }),
eg.makeStream({ stream_id: 2, name: 'stream' }),
eg.makeStream({ stream_id: 3, name: 'topic' }),
eg.makeStream({ stream_id: 4, name: 'mobile' }),
];
const baseState = eg.reduxStatePlus({ streams });
const narrow = getNarrowFromNarrowLink(
Expand All @@ -163,6 +173,13 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
}
};

describe('"/#narrow/channel/<…>" is a channel link', () => {
const check = mkCheck(isStreamNarrow);
['/#narrow/channel/1-jest', '/#narrow/channel/2-stream/', '/#narrow/channel/3-topic/'].forEach(
hash => check(hash),
);
});

describe('"/#narrow/stream/<…>" is a stream link', () => {
const check = mkCheck(isStreamNarrow);
['/#narrow/stream/jest', '/#narrow/stream/stream/', '/#narrow/stream/topic/'].forEach(hash =>
Expand All @@ -172,6 +189,16 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
// TODO: Test with modern-style stream links that use stream IDs
});

describe('"/#narrow/channel/<…>/topic/<…>" is a topic link', () => {
const check = mkCheck(isTopicNarrow);
[
'/#narrow/channel/1-jest/topic/test',
'/#narrow/channel/4-mobile/subject/topic/near/378333',
'/#narrow/channel/4-mobile/topic/topic/',
'/#narrow/channel/3-stream/topic/topic/near/1',
].forEach(hash => check(hash));
});

describe('"/#narrow/stream/<…>/topic/<…>" is a topic link', () => {
const check = mkCheck(isTopicNarrow);
[
Expand Down Expand Up @@ -216,14 +243,14 @@ describe('getNarrowFromNarrowLink (part 1)', () => {
const check = mkCheck(null);
[
// `near` with no operand
'/#narrow/stream/stream/topic/topic/near/',
'/#narrow/channel/stream/topic/topic/near/',

// `is` with invalid operand
'/#narrow/is/men',
'/#narrow/is/men/stream',
'/#narrow/is/men/channel',

// invalid operand `are`; `stream` operator with no operand
'/#narrow/are/men/stream',
// invalid operand `are`; `channel` operator with no operand
'/#narrow/are/men/channel',
].forEach(hash => check(hash));
});
});
Expand Down Expand Up @@ -263,8 +290,11 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
eg.selfUser.user_id,
);

describe('on stream links', () => {
describe('on channel links', () => {
const expectStream = (operand, streams, expectedStream: null | Stream) => {
expect(get(`#narrow/channel/${operand}`, streams)).toEqual(
expectedStream === null ? null : streamNarrow(expectedStream.stream_id),
);
expect(get(`#narrow/stream/${operand}`, streams)).toEqual(
expectedStream === null ? null : streamNarrow(expectedStream.stream_id),
);
Expand All @@ -278,12 +308,12 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
expectStream(`${streamGeneral.stream_id}-general`, [], streamGeneral);
});

test('on stream link with wrong name: ID wins', () => {
test('on channel link with wrong name: ID wins', () => {
expectStream(`${streamGeneral.stream_id}-nonsense`, [streamGeneral], streamGeneral);
expectStream(`${streamGeneral.stream_id}-`, [streamGeneral], streamGeneral);
});

test('on malformed stream link: reject', () => {
test('on malformed channel link: reject', () => {
expectStream(`-${streamGeneral.stream_id}`, [streamGeneral], null);
expectStream(`${streamGeneral.stream_id}nonsense-general`, [streamGeneral], null);
});
Expand Down Expand Up @@ -340,17 +370,21 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
describe('on topic links', () => {
test('basic', () => {
const expectBasic = (operand, expectedTopic) => {
const url = `#narrow/stream/${streamGeneral.stream_id}-general/topic/${operand}`;
const url = `#narrow/channel/${streamGeneral.stream_id}-general/topic/${operand}`;
const legacyUrl = `#narrow/stream/${streamGeneral.stream_id}-general/topic/${operand}`;
expect(get(url, [streamGeneral])).toEqual(
topicNarrow(streamGeneral.stream_id, expectedTopic),
);
expect(get(legacyUrl, [streamGeneral])).toEqual(
topicNarrow(streamGeneral.stream_id, expectedTopic),
);
};

expectBasic('(no.20topic)', '(no topic)');
expectBasic('lunch', 'lunch');
});

test('on old topic link, with dot-encoding', () => {
test('on old topic link (no stream ID), with dot-encoding', () => {
expect(
get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/(no.20topic)`, [eg.stream]),
).toEqual(topicNarrow(eg.stream.stream_id, '(no topic)'));
Expand All @@ -376,7 +410,7 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
).toEqual(topicNarrow(eg.stream.stream_id, 'topic'));
});

test('on old topic link, without realm info', () => {
test('on old topic link (no stream ID), without realm info', () => {
expect(get(`/#narrow/stream/${eg.stream.name}/topic/topic`, [eg.stream])).toEqual(
topicNarrow(eg.stream.stream_id, 'topic'),
);
Expand Down Expand Up @@ -423,6 +457,13 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
pmNarrowFromUsersUnsafe([userB, userC]),
);

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

expect(
get(`https://example.com/#narrow/stream/${eg.stream.name}/topic/test/near/1`, [eg.stream]),
).toEqual(topicNarrow(eg.stream.stream_id, 'test'));
Expand All @@ -436,6 +477,7 @@ describe('getNarrowFromNarrowLink (part 2)', () => {
describe('getNearOperandFromLink', () => {
test('not message link', () => {
expect(getNearOperandFromLink(new URL('/#narrow/is/private', realm), realm)).toBe(null);
expect(getNearOperandFromLink(new URL('/#narrow/channel/jest', realm), realm)).toBe(null);
expect(getNearOperandFromLink(new URL('/#narrow/stream/jest', realm), realm)).toBe(null);
});

Expand All @@ -451,6 +493,9 @@ describe('getNearOperandFromLink', () => {
});

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),
).toBe(1);
expect(
getNearOperandFromLink(new URL('/#narrow/stream/jest/topic/test/near/1', realm), realm),
).toBe(1);
Expand Down
9 changes: 7 additions & 2 deletions src/utils/internalLinks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,19 @@ export const getNarrowFromNarrowLink = (

if (
(hashSegments.length === 4 || hashSegments.length === 6)
&& hashSegments[0] === 'stream'
// 'channel' is new in server-9.0; means the same as 'stream'
&& (hashSegments[0] === 'stream' || hashSegments[0] === 'channel')
&& (hashSegments[2] === 'subject' || hashSegments[2] === 'topic')
) {
const streamId = parseStreamOperand(hashSegments[1], streamsById, streamsByName);
return streamId != null ? topicNarrow(streamId, parseTopicOperand(hashSegments[3])) : null;
}

if (hashSegments.length === 2 && hashSegments[0] === 'stream') {
if (
hashSegments.length === 2
// 'channel' is new in server-9.0; means the same as 'stream'
&& (hashSegments[0] === 'stream' || hashSegments[0] === 'channel')
) {
const streamId = parseStreamOperand(hashSegments[1], streamsById, streamsByName);
return streamId != null ? streamNarrow(streamId) : null;
}
Expand Down

0 comments on commit 4e86013

Please sign in to comment.