Skip to content

Commit

Permalink
narrow: Add new "with" narrow operator.
Browse files Browse the repository at this point in the history
This commit adds a new narrow operator -- "with" which expects
a string operand of message_id, and returns all the messages
in the same channel and topic of the operand, with the first
unread message of the topic as anchor.

This is done as an effort to provide permalinks for topics in
zulip#21505.
  • Loading branch information
roanster007 committed May 20, 2024
1 parent 44fa39c commit 0f5a750
Show file tree
Hide file tree
Showing 13 changed files with 295 additions and 15 deletions.
10 changes: 10 additions & 0 deletions api_docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with.

## Changes in Zulip 9.0

**Feature level 262**

* [`GET /messages`](/api/get-messages),
[`GET /messages/matches_narrow`](/api/check-messages-match-narrow),
[`POST /messages/flags/narrow`](/api/update-message-flags-for-narrow),
[`POST /register`](/api/register-queue):
Added support for a new [search/narrow filter](/api/construct-narrow),
`with`, which returns messages in the same channel and topic as that
of the operand of filter, with the first unread message as anchor.

**Feature level 261**

* [`POST /invites`](/api/send-invites),
Expand Down
6 changes: 5 additions & 1 deletion api_docs/construct-narrow.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,11 @@ important optimization when fetching messages in certain cases (e.g.
when [adding the `read` flag to a user's personal
messages](/api/update-message-flags-for-narrow)).

**Changes**: In Zulip 9.0 (feature level 250), support was added for
**Changes**: In Zulip 9.0 (feature level 262), support was added for a new
filter, `with`, which returns messages in the same channel and topic as that
of the operand of the filter, with the first unread message as anchor.

In Zulip 9.0 (feature level 250), support was added for
two filters related to stream messages: `channel` and `channels`. The
`channel` operator is an alias for the `stream` operator. The `channels`
operator is an alias for the `streams` operator. Both `channel` and
Expand Down
2 changes: 1 addition & 1 deletion version.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# Changes should be accompanied by documentation explaining what the
# new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`.
API_FEATURE_LEVEL = 261
API_FEATURE_LEVEL = 262

# Bump the minor PROVISION_VERSION to indicate that folks should provision
# only when going from an old version of the code to a newer version. Bump
Expand Down
32 changes: 32 additions & 0 deletions web/src/filter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,20 @@ function message_matches_search_term(message: Message, operator: string, operand
// this is all handled server side
return true;

case "with": {
if (message.type !== "stream") {
return false;
}
const msg_id = Number.parseInt(operand, 10);
const msg = message_store.get(msg_id);
return (
msg !== undefined &&
msg.type === "stream" &&
msg.stream_id === message.stream_id &&
msg.topic === message.topic
);
}

case "id":
return message.id.toString() === operand;

Expand Down Expand Up @@ -484,6 +498,7 @@ export class Filter {
"channels-public",
"channel",
"topic",
"with",
"dm",
"dm-including",
"sender",
Expand Down Expand Up @@ -536,6 +551,8 @@ export class Filter {
return verb + "channels";
case "near":
return verb + "messages around";
case "with":
return verb + "Thread with Message";

// Note: We hack around using this in "describe" below.
case "has":
Expand Down Expand Up @@ -750,6 +767,7 @@ export class Filter {
"channels-web-public",
"not-channels-web-public",
"near",
"with",
]);

for (const term of term_types) {
Expand Down Expand Up @@ -784,6 +802,14 @@ export class Filter {
// it is limited by the user's message history. Therefore, we check "channel"
// and "topic" together to ensure that the current filter will return all the
// messages of a conversation.
if (
_.isEqual(term_types, ["channel", "topic", "with"]) ||
_.isEqual(term_types, ["channel", "with"]) ||
_.isEqual(term_types, ["with"])
) {
return true;
}

if (_.isEqual(term_types, ["channel", "topic"])) {
return true;
}
Expand Down Expand Up @@ -1022,6 +1048,12 @@ export class Filter {
},
);
}
if (term_types.length === 3 && _.isEqual(term_types, ["channel", "topic", "with"])) {
const channel_name = this.operands("channel")[0];
this._sub = stream_data.get_sub_by_name(channel_name);
assert(this._sub !== undefined);
return this._sub.name;
}
if (term_types.length === 1) {
switch (term_types[0]) {
case "in-home":
Expand Down
69 changes: 65 additions & 4 deletions web/src/narrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,56 @@ export function save_narrow(terms, trigger) {
changehash(new_hash, trigger);
}

function change_view_for_narrow_containing_with_operator(message_data, opts) {
let anchor;
const msg_ids = message_data.messages.map((msg) => msg.id);
const msgs = msg_ids.map((msg_id) => message_store.get(msg_id));

if (message_data.anchor === LARGER_THAN_MAX_MESSAGE_ID) {
anchor = Math.max(...msg_ids);
} else {
anchor = message_data.anchor;
}
const message = msgs[0];
assert(message.type === "stream");
const channel_name = message.display_recipient;
const topic_name = message.topic;

const adjusted_terms = [
{operator: "stream", operand: channel_name, negated: false},
{operator: "topic", operand: topic_name, negated: false},
{operator: "with", operand: `${anchor}`, negated: false},
];

const filter = new Filter(adjusted_terms);
const excludes_muted_topics = filter.excludes_muted_topics();
const msg_data = new MessageListData({
filter,
excludes_muted_topics,
});

const id_info = {
target_id: anchor,
local_select_id: anchor,
final_select_id: anchor,
};

maybe_add_local_messages({
id_info,
msg_data,
});

msg_data.add_messages(msgs);
save_narrow(adjusted_terms, opts.trigger);

const msg_list = new message_list.MessageList({
data: msg_data,
});

message_lists.update_current_message_list(msg_list);
handle_post_view_change(msg_list);
}

export function activate(raw_terms, opts) {
/* Main entry point for switching to a new view / message list.
Expand Down Expand Up @@ -216,6 +266,12 @@ export function activate(raw_terms, opts) {
if (filter.has_operator("id")) {
id_info.target_id = Number.parseInt(filter.operands("id")[0], 10);
}
if (filter.has_operator("with")) {
const target_message_id = Number.parseInt(filter.operands("with")[0], 10);
if (message_store.get(target_message_id) !== undefined) {
id_info.target_id = target_message_id;
}
}

// Narrow with near / id operator. There are two possibilities:
// * The user is clicking a permanent link to a conversation, in which
Expand Down Expand Up @@ -324,7 +380,7 @@ export function activate(raw_terms, opts) {
return;
}
}
} else if (!opts.fetched_target_message) {
} else if (!opts.fetched_target_message && !filter.has_operator("with")) {
// If we don't have the target message ID locally and
// haven't attempted to fetch it, then we ask the server
// for it.
Expand Down Expand Up @@ -554,7 +610,10 @@ export function activate(raw_terms, opts) {
}
message_fetch.load_messages_for_narrow({
anchor,
cont() {
cont(data, opts) {
if (filter.has_operator("with")) {
change_view_for_narrow_containing_with_operator(data, opts);
}
if (!select_immediately) {
render_message_list_with_selected_message({
id_info,
Expand Down Expand Up @@ -687,7 +746,7 @@ export function maybe_add_local_messages(opts) {
// Full-text search and potentially other future cases where
// we can't check which messages match on the frontend, so it
// doesn't matter what's in our cache, we must go to the server.
if (id_info.target_id) {
if (!filter.has_operator("with") && id_info.target_id) {
// TODO: Ideally, in this case we should be asking the
// server to give us the first unread or the target_id,
// whichever is first (i.e. basically the `found` logic
Expand All @@ -709,7 +768,9 @@ export function maybe_add_local_messages(opts) {
// first unread message, or the target_id (if any), whichever
// is earlier. See #2091 for a detailed explanation of why we
// need to look at unread here.
id_info.final_select_id = min_defined(id_info.target_id, unread_info.msg_id);
id_info.final_select_id = filter.has_operator("with")
? unread_info.msg_id
: min_defined(id_info.target_id, unread_info.msg_id);

if (!load_local_messages(msg_data)) {
return;
Expand Down
14 changes: 14 additions & 0 deletions web/src/narrow_state.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as blueslip from "./blueslip";
import {Filter} from "./filter";
import * as message_lists from "./message_lists";
import * as message_store from "./message_store";
import {page_params} from "./page_params";
import * as people from "./people";
import type {NarrowTerm} from "./state_data";
Expand Down Expand Up @@ -281,6 +282,19 @@ export function _possible_unread_message_ids(
return unread.get_msg_ids_for_stream(sub.stream_id);
}

if (
current_filter.can_bucket_by("channel", "topic", "with") ||
current_filter.can_bucket_by("channel", "with") ||
current_filter.can_bucket_by("with")
) {
const msg_id = Number.parseInt(current_filter.operands("with")[0], 10);
const msg = message_store.get(msg_id);
if (msg !== undefined && msg.type === "stream") {
return unread.get_msg_ids_for_topic(msg.stream_id, msg.topic);
}
return [];
}

if (current_filter.can_bucket_by("dm")) {
current_filter_pm_string = pm_ids_string(current_filter);
if (current_filter_pm_string === undefined) {
Expand Down
54 changes: 53 additions & 1 deletion web/tests/filter.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,7 @@ test("basics", () => {
assert.ok(!filter.is_conversation_view());

terms = [
{operator: "with", operand: 17},
{operator: "channel", operand: "foo"},
{operator: "topic", operand: "bar"},
{operator: "search", operand: "pizza"},
Expand All @@ -167,6 +168,7 @@ test("basics", () => {
{operator: "channel", operand: "foo"},
{operator: "topic", operand: "bar"},
{operator: "near", operand: 17},
{operator: "with", operand: 17},
];
filter = new Filter(terms);

Expand All @@ -179,8 +181,16 @@ test("basics", () => {
assert.ok(!filter.is_personal_filter());
assert.ok(filter.can_bucket_by("channel"));
assert.ok(filter.can_bucket_by("channel", "topic"));
assert.ok(filter.can_bucket_by("channel", "topic", "with"));
assert.ok(!filter.is_conversation_view());

terms = [
{operator: "channel", operand: "foo"},
{operator: "with", operand: 17},
];
filter = new Filter(terms);
assert.ok(filter.can_bucket_by("channel", "with"));

// If our only channel operator is negated, then for all intents and purposes,
// we don't consider ourselves to have a channel operator, because we don't
// want to have the channel in the tab bar or unsubscribe messaging, etc.
Expand Down Expand Up @@ -753,7 +763,7 @@ test("canonicalization", () => {
assert.equal(term.operand, "link");
});

test("predicate_basics", () => {
test("predicate_basics", ({override}) => {
// Predicates are functions that accept a message object with the message
// attributes (not content), and return true if the message belongs in a
// given narrow. If the narrow parameters include a search, the predicate
Expand All @@ -769,6 +779,8 @@ test("predicate_basics", () => {
["topic", "Bar"],
]);

override(message_store, "get", () => ({type: stream_message, stream_id, topic: "bar"}));

assert.ok(predicate({type: stream_message, stream_id, topic: "bar"}));
assert.ok(!predicate({type: stream_message, stream_id, topic: "whatever"}));
// 9999999 doesn't exist, testing no match
Expand Down Expand Up @@ -844,6 +856,10 @@ test("predicate_basics", () => {
predicate = get_predicate([["near", 5]]);
assert.ok(predicate({}));

predicate = get_predicate([["with", 5]]);
assert.ok(predicate({type: stream_message, stream_id, topic: "bar"}));
assert.ok(!predicate({type: direct_message, stream_id, topic: "bar"}));

predicate = get_predicate([["id", 5]]);
assert.ok(predicate({id: 5}));
assert.ok(!predicate({id: 6}));
Expand Down Expand Up @@ -1251,6 +1267,10 @@ test("unparse", () => {
string = "near:150";
assert.deepEqual(Filter.unparse(terms), string);

terms = [{operator: "with", operand: 150}];
string = "with:150";
assert.deepEqual(Filter.unparse(terms), string);

terms = [{operator: "", operand: ""}];
string = "";
assert.deepEqual(Filter.unparse(terms), string);
Expand Down Expand Up @@ -1335,6 +1355,10 @@ test("describe", ({mock_template}) => {
string = "unknown operator";
assert.equal(Filter.search_description_as_html(narrow), string);

narrow = [{operator: "with", operand: "12"}];
string = "Thread with Message 12";
assert.equal(Filter.search_description_as_html(narrow), string);

narrow = [
{operator: "channel", operand: "devel"},
{operator: "topic", operand: "JS", negated: true},
Expand Down Expand Up @@ -1501,6 +1525,8 @@ test("term_type", () => {
["dm", "near", "is-unread", "has-link"],
);

assert_term_sort(["topic", "channel", "with"], ["channel", "topic", "with"]);

assert_term_sort(["bogus", "channel", "topic"], ["channel", "topic", "bogus"]);
assert_term_sort(["channel", "topic", "channel"], ["channel", "channel", "topic"]);

Expand Down Expand Up @@ -1592,6 +1618,12 @@ function make_web_public_sub(name, stream_id) {
}

test("navbar_helpers", () => {
const sub = {
name: "Foo",
stream_id: 12,
};
stream_data.add_sub(sub);

const stream_id = 43;
make_sub("Foo", stream_id);

Expand Down Expand Up @@ -1696,6 +1728,12 @@ test("navbar_helpers", () => {
{operator: "dm", operand: "[email protected]"},
{operator: "near", operand: "12"},
];
const channel_with = [
{operator: "channel", operand: "foo"},
{operator: "topic", operand: "bar"},
{operator: "with", operand: "12"},
];
const with_op = [{operator: "with", operand: "11"}];

const test_cases = [
{
Expand Down Expand Up @@ -1876,6 +1914,20 @@ test("navbar_helpers", () => {
title: properly_separated_names([joe.full_name]),
redirect_url_with_search: "#",
},
{
terms: with_op,
is_common_narrow: true,
icon: undefined,
title: undefined,
redirect_url_with_search: "#",
},
{
terms: channel_with,
is_common_narrow: true,
zulip_icon: "hashtag",
title: "Foo",
redirect_url_with_search: "#",
},
];

realm.realm_enable_guest_user_indicator = true;
Expand Down
Loading

0 comments on commit 0f5a750

Please sign in to comment.