Skip to content

Commit

Permalink
topics: Change topic links to use new permalinks.
Browse files Browse the repository at this point in the history
This commit updates the topic links obtained from clicking
the topics in the left sidebar, recent view and inbox, and
those obtained from "Copy link to topic" to use the new
topic permalinks.

Fixes zulip#21505
  • Loading branch information
roanster007 committed Oct 11, 2024
1 parent c3d897d commit 2d4425c
Show file tree
Hide file tree
Showing 13 changed files with 160 additions and 8 deletions.
25 changes: 25 additions & 0 deletions web/src/echo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import render_message_controls_failed_msg from "../templates/message_controls_fa

import * as alert_words from "./alert_words";
import * as blueslip from "./blueslip";
import * as browser_history from "./browser_history";
import * as compose_notifications from "./compose_notifications";
import * as compose_ui from "./compose_ui";
import * as echo_state from "./echo_state";
import * as hash_util from "./hash_util";
import * as local_message from "./local_message";
import * as markdown from "./markdown";
import * as message_events_util from "./message_events_util";
Expand Down Expand Up @@ -431,6 +433,28 @@ export function edit_locally(message: Message, request: LocalEditRequest): Messa
return message;
}

export function update_topic_hash_to_contain_with_term(message: Message): void {
// If the current filter consists only of channel and topic terms
// and the incoming message belongs to same narrow, we try to
// update the hash, to convert to permalink.
if (message.type !== "stream") {
return;
}
const filter = message_lists.current!.data.filter;
if (
filter.is_in_channel_topic_narrow() &&
filter.has_operand("channel", message.stream_id.toString()) &&
filter.has_operand("topic", message.topic)
) {
const narrow_terms = filter.terms();
const with_term = {operator: "with", operand: message.id.toString()};

narrow_terms.push(with_term);
const new_hash = hash_util.search_terms_to_hash(narrow_terms);
browser_history.set_hash(new_hash);
}
}

export function reify_message_id(local_id: string, server_id: number): void {
const message = echo_state.get_message_waiting_for_id(local_id);
echo_state.remove_message_from_waiting_for_id(local_id);
Expand Down Expand Up @@ -462,6 +486,7 @@ export function reify_message_id(local_id: string, server_id: number): void {
message_id: message.id,
});
}
update_topic_hash_to_contain_with_term(message);
}

export function update_message_lists({old_id, new_id}: {old_id: number; new_id: number}): void {
Expand Down
11 changes: 11 additions & 0 deletions web/src/filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1047,6 +1047,17 @@ export class Filter {
return this._terms.length === 1 && this.has_operand("in", "home");
}

is_in_channel_topic_narrow(): boolean {
if (
this.terms().length === 2 &&
this.has_operator("channel") &&
this.has_operator("topic")
) {
return true;
}
return false;
}

is_keyword_search(): boolean {
return this.has_operator("search");
}
Expand Down
17 changes: 17 additions & 0 deletions web/src/hash_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as people from "./people";
import * as settings_data from "./settings_data";
import type {NarrowTerm} from "./state_data";
import * as stream_data from "./stream_data";
import {get_latest_message_id_in_topic} from "./stream_topic_history";
import * as sub_store from "./sub_store";
import type {StreamSubscription} from "./sub_store";
import * as user_groups from "./user_groups";
Expand Down Expand Up @@ -84,6 +85,22 @@ export function by_stream_topic_url(stream_id: number, topic: string): string {
return internal_url.by_stream_topic_url(stream_id, topic, sub_store.maybe_get_stream_name);
}

export function by_channel_topic_permalink(stream_id: number, topic: string): string {
const channel_topic_url = internal_url.by_stream_topic_url(
stream_id,
topic,
sub_store.maybe_get_stream_name,
);
const latest_msg_in_topic = get_latest_message_id_in_topic(stream_id, topic);

if (latest_msg_in_topic === undefined) {
return channel_topic_url;
}

const topic_permalink = channel_topic_url + `/with/${latest_msg_in_topic}`;
return topic_permalink;
}

// Encodes a term list into the
// corresponding hash: the # component
// of the narrow URL
Expand Down
2 changes: 1 addition & 1 deletion web/src/inbox_ui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ function format_topic(
topic_name: topic,
unread_count: topic_unread_count,
conversation_key: get_topic_key(stream_id, topic),
topic_url: hash_util.by_stream_topic_url(stream_id, topic),
topic_url: hash_util.by_channel_topic_permalink(stream_id, topic),
is_hidden: filter_should_hide_stream_row({stream_id, topic}),
is_collapsed: collapsed_containers.has(STREAM_HEADER_PREFIX + stream_id),
mention_in_unread: unread.topic_has_any_unread_mentions(stream_id, topic),
Expand Down
2 changes: 1 addition & 1 deletion web/src/recent_view_ui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -606,7 +606,7 @@ function format_conversation(conversation_data: ConversationData): ConversationC
const is_web_public = stream_info.is_web_public;
// Topic info
const topic = last_msg.topic;
const topic_url = hash_util.by_stream_topic_url(stream_id, topic);
const topic_url = hash_util.by_channel_topic_permalink(stream_id, topic);

// We hide the row according to filters or if it's muted.
// We only supply the data to the topic rows and let jquery
Expand Down
2 changes: 1 addition & 1 deletion web/src/stream_list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ export function set_event_handlers({
const topic_list_info = topic_list_data.get_list_info(stream_id, false, "");
const topic_item = topic_list_info.items[0];
if (topic_item !== undefined) {
const destination_url = hash_util.by_stream_topic_url(
const destination_url = hash_util.by_channel_topic_permalink(
stream_id,
topic_item.topic_name,
);
Expand Down
8 changes: 8 additions & 0 deletions web/src/stream_topic_history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,14 @@ export function get_max_message_id(stream_id: number): number {
return history.get_max_message_id();
}

export function get_latest_message_id_in_topic(
stream_id: number,
topic_name: string,
): number | undefined {
const history = stream_dict.get(stream_id);
return history?.topics.get(topic_name)?.message_id;
}

export function reset(): void {
// This is only used by tests.
stream_dict.clear();
Expand Down
7 changes: 3 additions & 4 deletions web/src/topic_popover.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import render_delete_topic_modal from "../templates/confirm_dialog/confirm_delet
import render_left_sidebar_topic_actions_popover from "../templates/popovers/left_sidebar/left_sidebar_topic_actions_popover.hbs";

import * as confirm_dialog from "./confirm_dialog";
import * as hash_util from "./hash_util";
import {$t_html} from "./i18n";
import * as message_edit from "./message_edit";
import * as popover_menus from "./popover_menus";
import * as popover_menus_data from "./popover_menus_data";
import * as starred_messages_ui from "./starred_messages_ui";
import {realm} from "./state_data";
import * as stream_popover from "./stream_popover";
import * as ui_util from "./ui_util";
import * as unread_ops from "./unread_ops";
Expand All @@ -28,23 +28,22 @@ export function initialize() {
popover_menus.on_show_prep(instance);
let stream_id;
let topic_name;
let url;

if (instance.reference.classList.contains("inbox-topic-menu")) {
const $elt = $(instance.reference);
stream_id = Number.parseInt($elt.attr("data-stream-id"), 10);
topic_name = $elt.attr("data-topic-name");
url = new URL($elt.attr("data-topic-url"), realm.realm_url);
} else {
const $elt = $(instance.reference)
.closest(".topic-sidebar-menu-icon")
.expectOne();
const $stream_li = $elt.closest(".narrow-filter").expectOne();
topic_name = $elt.closest("li").expectOne().attr("data-topic-name");
url = $elt.closest("li").find(".sidebar-topic-name").expectOne().prop("href");
stream_id = stream_popover.elem_to_stream_id($stream_li);
}

const url = hash_util.by_channel_topic_permalink(stream_id, topic_name);

instance.context = popover_menus_data.get_topic_popover_content_context({
stream_id,
topic_name,
Expand Down
8 changes: 8 additions & 0 deletions web/src/ui_init.js
Original file line number Diff line number Diff line change
Expand Up @@ -658,10 +658,18 @@ export function initialize_everything(state_data) {
topic_list.initialize({
on_topic_click(stream_id, topic) {
const sub = sub_store.get(stream_id);
const latest_msg_id = stream_topic_history.get_latest_message_id_in_topic(
stream_id,
topic,
);

assert(latest_msg_id !== undefined);

message_view.show(
[
{operator: "channel", operand: sub.stream_id.toString()},
{operator: "topic", operand: topic},
{operator: "with", operand: latest_msg_id},
],
{trigger: "sidebar"},
);
Expand Down
71 changes: 71 additions & 0 deletions web/tests/echo.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {mock_esm, zrequire} = require("./lib/namespace");
const {make_stub} = require("./lib/stub");
const {run_test, noop} = require("./lib/test");

const browser_history = mock_esm("../src/browser_history");
const compose_notifications = mock_esm("../src/compose_notifications");
const markdown = mock_esm("../src/markdown");
const message_lists = mock_esm("../src/message_lists");
Expand Down Expand Up @@ -44,6 +45,13 @@ message_lists.current = {
can_apply_locally() {
return true;
},
is_in_channel_topic_narrow() {
return true;
},
has_operand() {
return true;
},
terms: () => [],
},
},
change_message_id: noop,
Expand All @@ -68,6 +76,7 @@ message_lists.non_rendered_data = () => [];

const echo = zrequire("echo");
const echo_state = zrequire("echo_state");
const {Filter} = zrequire("filter");
const people = zrequire("people");
const {set_current_user} = zrequire("state_data");
const stream_data = zrequire("stream_data");
Expand Down Expand Up @@ -368,6 +377,7 @@ run_test("test reify_message_id", ({override}) => {

override(markdown, "render", noop);
override(markdown, "get_topic_links", noop);
override(browser_history, "set_hash", noop);

const message_request = {
type: "stream",
Expand Down Expand Up @@ -404,6 +414,67 @@ run_test("test reify_message_id", ({override}) => {
assert.equal(history.topics.get("test").message_id, 110);
});

run_test("test update_topic_hash_to_contain_with_term", ({override}) => {
const local_id_float = 103.01;
let initial_hash;
let new_hash;

override(markdown, "render", noop);
override(markdown, "get_topic_links", noop);
override(message_store, "reify_message_id", noop);
override(compose_notifications, "reify_message_id", noop);

const message_request = {
type: "stream",
stream_id: general_sub.stream_id,
topic: "test",
};

// Test normal topic link converted to permalink upon arrival of a new
// message if it belongs to the same narrow.
let topic = "test";
initial_hash = `#narrow/stream/${general_sub.stream_id}-${general_sub.name}/topic/${topic}`;
new_hash = initial_hash;

override(browser_history, "set_hash", (updated_hash) => {
new_hash = updated_hash;
});

message_lists.current.data.filter = new Filter([
{operator: "channel", operand: general_sub.stream_id.toString()},
{operator: "topic", operand: topic},
]);

echo.insert_local_message(message_request, local_id_float, (messages) => {
messages.map((message) => echo.track_local_message(message));
return messages;
});
echo.reify_message_id(local_id_float.toString(), 110);

assert.equal(
new_hash,
`#narrow/stream/${general_sub.stream_id}-${general_sub.name}/topic/${topic}/with/110`,
);

// No change in current hash if incoming message is of different
// narrow.
topic = "testing";
initial_hash = `#narrow/stream/${general_sub.stream_id}-${general_sub.name}/topic/${topic}`;
new_hash = initial_hash;

message_lists.current.data.filter = new Filter([
{operator: "channel", operand: general_sub.stream_id.toString()},
{operator: "topic", operand: topic},
]);

echo.insert_local_message(message_request, local_id_float, (messages) => {
messages.map((message) => echo.track_local_message(message));
return messages;
});
echo.reify_message_id(local_id_float.toString(), 110);
assert.equal(new_hash, initial_hash);
});

run_test("reset MockDate", () => {
MockDate.reset();
});
3 changes: 3 additions & 0 deletions web/tests/filter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ test("basics", () => {
assert.ok(!filter.is_personal_filter());
assert.ok(filter.is_conversation_view());
assert.ok(!filter.is_conversation_view_with_near());
assert.ok(!filter.is_in_channel_topic_narrow());

terms = [
{operator: "dm", operand: "[email protected]"},
Expand All @@ -364,6 +365,7 @@ test("basics", () => {
assert.ok(!filter.is_personal_filter());
assert.ok(filter.is_conversation_view());
assert.ok(!filter.is_conversation_view_with_near());
assert.ok(!filter.is_in_channel_topic_narrow());

terms = [
{operator: "dm", operand: "[email protected],[email protected]"},
Expand Down Expand Up @@ -450,6 +452,7 @@ test("basics", () => {
assert.ok(!filter.is_personal_filter());
assert.ok(filter.is_conversation_view());
assert.ok(!filter.is_conversation_view_with_near());
assert.ok(filter.is_in_channel_topic_narrow());

terms = [
{operator: "channel", operand: foo_stream_id.toString()},
Expand Down
4 changes: 3 additions & 1 deletion web/tests/recent_view.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {page_params} = require("./lib/zpage_params");

window.scrollTo = noop;
const test_url = () => "https://www.example.com";
const test_permalink = () => "https://www.example.com/with/12";

// We assign this in our test() wrapper.
let messages;
Expand Down Expand Up @@ -94,6 +95,7 @@ mock_esm("../src/compose_closed_ui", {
mock_esm("../src/hash_util", {
by_stream_url: test_url,
by_stream_topic_url: test_url,
by_channel_topic_permalink: test_permalink,
by_conversation_and_time_url: test_url,
});
mock_esm("../src/message_list_data", {
Expand Down Expand Up @@ -410,7 +412,7 @@ function generate_topic_data(topic_info_array) {
stream_url: "https://www.example.com",
topic,
conversation_key: get_topic_key(stream_id, topic),
topic_url: "https://www.example.com",
topic_url: "https://www.example.com/with/12",
unread_count,
mention_in_unread: false,
visibility_policy,
Expand Down
8 changes: 8 additions & 0 deletions web/tests/stream_topic_history.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,14 @@ test("server_history_end_to_end", () => {
const history = stream_topic_history.get_recent_topic_names(stream_id);
assert.deepEqual(history, ["topic3", "topic2", "topic1"]);

for (const topic of topics) {
const last_msg_id_in_topic = stream_topic_history.get_latest_message_id_in_topic(
stream_id,
topic.name,
);
assert.deepEqual(last_msg_id_in_topic, topic.max_id);
}

// Try getting server history for a second time.

/* istanbul ignore next */
Expand Down

0 comments on commit 2d4425c

Please sign in to comment.