topics: Convert topic links to permalinks. #30114

roanster007 · 2024-05-16T12:46:22Z

Commit 1: 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.

Commit 2: Converts #-mention of topics to permalinks.

Commit 3: Converts left sidebar topic links and Copy link to topic to permalinks.

Fixes: #21505

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

gnprice · 2024-05-16T19:46:02Z

Chat threads in #design and #api design, for cross-reference:
https://chat.zulip.org/#narrow/stream/101-design/topic/redirects.20from.20near.20links/near/1774401
https://chat.zulip.org/#narrow/stream/378-api-design/topic/topic.20permalinks/near/1801198

roanster007 · 2024-05-20T14:51:24Z

Updated narrow with operator as per the api design thread --

If message is found in local cache, use it to return the message list.
If message not found in local cache, don't fetch the message_id attached to with operator recursively, but send the entire narrow to backend and to correct the operator and fetch the message list.

timabbott · 2024-05-20T22:59:39Z

zerver/openapi/zulip.yaml

-            **Changes**: In Zulip 9.0 (feature level 250), narrows gained support
+            **Changes**: In Zulip 9.0 (feature level 262), narrows gained support 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.


You should redo this with some of the more precise language from the API design conversation.

timabbott · 2024-05-20T23:00:38Z

zerver/lib/narrow.py

@@ -919,6 +927,30 @@ def get_channel_from_narrow_access_unchecked(
    return None


+def update_narrow_terms_containing_with_operator(
+    realm: Realm, narrow: OptionalNarrowListT
+) -> OptionalNarrowListT:


Please add a nice docstring explaining this function; this feature is an important and somewhat strange thing we're doing, and so deserves some nice context-setting. It should probably mention the extra RTT considerations for why we're doing this in the server, for example.

timabbott · 2024-05-20T23:01:09Z

zerver/lib/narrow.py

+def update_narrow_terms_containing_with_operator(
+    realm: Realm, narrow: OptionalNarrowListT
+) -> OptionalNarrowListT:
+    if narrow is not None:


You can use the early-return pattern to reduce nesting; just do if narrow is None: return narrow at the start.

oh ya, right. updated!

timabbott · 2024-05-20T23:02:51Z

zerver/lib/message.py

+    and not send it out to unauthorized recipients.
+    """
+    message = Message.objects.filter(realm=realm, id=message_id).first()
+    return message


Why aren't you using access_message_by_id? It's almost always a security bug to have any behavior depend on the state of the database without checking access. I'm 98% confident that this code would in fact introduce a security vulnerability.

Once you fix this, please make sure test_message_fetch.py has a test of someone trying to access the current stream/topic of a message that they do not have access to using this new operator.

I did this thing similar to -- get_stream_by_narrow_operand_access_unchecked. Since just like this function we use the method just to prepare the response -- update the narrow and not send the response to unauthorized users.

def get_stream_by_narrow_operand_access_unchecked(operand: Union[str, int], realm: Realm) -> Stream: """This is required over access_stream_* in certain cases where we need the stream data only to prepare a response that user can access and not send it out to unauthorized recipients. """

Plus, so as to use access_message we won't be able to update the narrow if the maybe_user_profile is None in views/message_fetch. I guess it is fine?

That other code path has a very explicit comment about what it is doing with the data fetched this way that is safe:

# Note: It is okay here to not check access to channel # because we are only using the channel ID to exclude data, # not to include results. channel = get_channel_from_narrow_access_unchecked(narrow, user_profile.realm)

The code path that you're adding this to does not -- if a message was moved to a private channel that you can't access, you shouldn't be able to access the new location's stream/topic.

If maybe_user_profile is None, this this request is on behalf of a public access option request; in that case, we should be able to check if the message should be available correctly; I've not tried to look at the logic here, but I think that would be a good corner case to have an explicit test for.

timabbott · 2024-05-20T23:05:29Z

zerver/lib/narrow.py

+
+            narrow.remove(with_term)
+
+    return narrow


I think we will want this function to also return at least a boolean for whether the narrow was in fact modified from its original state; I think there's a good chance we'll want to have the API response do something to reflect whether that in fact occurred.

we can decide it in the client side whether the narrow has been updated or not. I updated it to use the client side implementation.

timabbott · 2024-05-20T23:08:45Z

zerver/lib/narrow.py

+                for term in narrow:
+                    if term["operator"] == "channel":
+                        stream_id = message.recipient.type_id
+                        term["operand"] = get_stream_by_id_in_realm(stream_id, realm).name


Having to do this lookup is pretty ugly; but I guess the fix for that will require moving to generally sending stream IDs rather than names over the wire. Maybe worth adding a TODO comment that ideally we'd not have to translate back to a stream name here.

The API does accept stream/channel IDs:
https://zulip.com/api/construct-narrow#stream-and-user-ids

So it should be possible to write this code to stay in terms of channel IDs too.

Updated to rather use channel id, so lookup would be simpler

timabbott · 2024-05-20T23:09:46Z

api_docs/construct-narrow.md

-**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.


You should also document the with parameter semantics in the section on ### Message IDs, below.

timabbott · 2024-05-20T23:10:33Z

web/src/filter.ts

@@ -536,6 +551,8 @@ export class Filter {
                return verb + "channels";
            case "near":
                return verb + "messages around";
+            case "with":
+                return verb + "Thread with Message";


Capitalization here is not in line with the standard for this file. I think you want something like "conversation containing" here.

timabbott · 2024-05-20T23:12:13Z

web/src/narrow.js

-        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);


Why is this the right special case?

well, in case of this with operator, we populate the target_id with the operand. The final_select_id in our case should always be the first unread if found locally (else undefined, so that we can fetch it from server). It is however possible that out target_id (basically the with operand) might be < first unread msg_id theroetically

web/src/narrow.js

timabbott · 2024-05-20T23:15:00Z

web/src/narrow.js

@@ -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) {


I'm not sure I see anywhere where this logic would avoid doing a bunch of work in the very common case that the target message has never moved. Possibly the message_fetch API response suggested elsewhere would be helpful to you here for doing this check well.

I added the logic so that it would avoid the work in case it is in the same narrow.

timabbott · 2024-05-20T23:16:20Z

web/src/narrow.js

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


Is there a way, perhaps involving some extracted functions, that this can share code with the main narrowing code path? I worry a lot that this duplicates a bunch of fiddly logic and will bitrot into incorrectness quickly.

Prep commit - #30172

updated to use the common method - create_and_update_message_list.

timabbott · 2024-05-20T23:17:40Z

web/tests/filter.test.js

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


Is it correct to say we can bucket by those operators? We don't actually know which messages are in the current conversation... We might need to be careful here.

web/tests/narrow_unread.test.js

This commit extracts the method to create and update message lists in the narrow. This is particularly useful in cases when we need to update the narrow and view with our own message lists. This is a preparatory commit to zulip#30114, since it involves creating a new message list from the messages fetched, which may be of a different narrow from that of the initial filter, and updating the view and narrow with it.

This commit extracts the method to create and update message lists in the narrow. This is particularly useful in cases when we need to update the narrow and view with our own message lists. This is a preparatory commit to #30114, since it involves creating a new message list from the messages fetched, which may be of a different narrow from that of the initial filter, and updating the view and narrow with it.

roanster007 · 2024-05-23T13:47:41Z

web/src/narrow.js

+    // raw_terms with both `dm` and with operators are redundant, since dms
+    // don't have topics. Hence, we remove the `with` operator.
+    if (raw_terms.some((term) => term.operator === "dm")) {
+        raw_terms = raw_terms.filter((term) => term.operator !== "with");
+    }


I assume private messages contaning with operators would be redundant since they can't be moved?

roanster007 · 2024-05-23T19:24:11Z

addressed the above reviews.

roanster007 · 2024-05-27T18:56:40Z

Addressed reviews, added commit to convert topic #-mentions to permalinks.

gnprice

Thanks! A couple of comments purely on the API.

gnprice · 2024-05-28T17:38:46Z

api_docs/construct-narrow.md

+The `with` operator uses message ID, which must be encoded as a string
+for its operand.


This should accept the ID as a number, not only as a string. That is, it should accept "operand":12345, not only "operand":"12345".

The use of a string here is kind of messy, and ideally I'd say it would only accept a number, not a string. But IIUC getting the web client to send a number instead of a string would be a nontrivial refactor, and I'm fine with leaving that out of scope for this issue and having the server accept a string as a workaround for that behavior of the web client.

The example below contradicts this description — it has the operand as a number after all.

gnprice · 2024-05-28T17:47:17Z

zerver/openapi/zulip.yaml

+            **Changes**: In Zulip 9.0 (feature level 263), narrows gained support for a new
+            filter - `with`, taking a message ID as operand. If the message corresponding to
+            the message ID exists and is visible to the user, then it replaces any "conversation"
+            filters — channel/stream, topic conversation — with filters corresponding to the
+            conversation the message is in. Else, the `with` operator is just ignored.


Rather than duplicate the details of these changes here and in the other endpoints that take a narrow, I think it'd be best to just have the changelog in one place and point to there. So for example:

Suggested change

**Changes**: In Zulip 9.0 (feature level 263), narrows gained support for a new

filter - `with`, taking a message ID as operand. If the message corresponding to

the message ID exists and is visible to the user, then it replaces any "conversation"

filters — channel/stream, topic conversation — with filters corresponding to the

conversation the message is in. Else, the `with` operator is just ignored.

**Changes**: See changelog at [Construct a narrow](/api/construct-narrow).

but as a prep commit replacing the existing changelog here (and at the other endpoints).

That avoids the risk of the details getting out of sync between the different descriptions, and reduces the burden for updating or editing them. Conversely, I think anyone who's getting into the details of constructing a narrow is going to have to follow the link to /api/construct-narrow in any case.

Preparatory PR - #30244

roanster007 · 2024-05-29T20:29:32Z

Updates:

This should accept the ID as a number, not only as a string. That is, it should accept "operand":12345, not only "operand":"12345".

now accepts both string and number

removed the operator from search typeahead.
updated to make sure that in case of with operator, the "hash change" is set to false initially, and set to true, before the view is changed (because hash change triggers another activate(), affecting anchor position)

This commit extracts various methods called post the current message list is updated in "narrow.js" such as update_unread_banner, handle_post_view_change, etc. into a separate method -- "handle_post_message_list_change". This allows us to be able to call these methods independently in case we need to update the filter and the corresponding message lists. This is a preparatory commit to zulip#30114.

This commit extracts various methods called post the current message list is updated in "narrow.js" such as update_unread_banner, handle_post_view_change, etc. into a separate method -- "handle_post_message_list_change". This allows us to be able to call these methods independently in case we need to update the filter and the corresponding message lists. This is a preparatory commit to #30114.

timabbott · 2024-05-30T18:38:49Z

api_docs/construct-narrow.md


+* `with:12345`: Search for the conversation (channel and topic) which
+contains the message with ID `12345`.


Where do we explain the precise semantics of the operator -- that it results in the query

Also, this description doesn't explain what happens for a with operator in a DM conversation -- what happens then?

I added them!

timabbott · 2024-05-30T18:39:08Z

api_docs/construct-narrow.md

@@ -51,7 +51,13 @@ 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 263), narrows gained support for a new
+filter - `with`, taking a message ID as operand. If the message corresponding to


Maybe /filter/operator, since it does more than just filtering?

timabbott · 2024-05-30T18:41:36Z

api_docs/construct-narrow.md

+filter - `with`, taking a message ID as operand. If the message corresponding to
+the message ID exists and is visible to the user, then it replaces any "conversation"
+filters — channel/stream, topic conversation — with filters corresponding to the
+conversation the message is in. Else, the `with` operator is just ignored.


I think this doesn't clearly specify what happens if no channel/topic operator was present. That's probably OK here, since this is the changelog, not the full specification of the operator.

Also, not need to mention channel/stream, just say "channel" since that's how we refer to that in prose.

So here we should place something like:

263), narrows gained support for a new with operator, designed to be used in links that should follow a topic across being moved in a single API request.

And then the full details for what it does should be in the actual documentation for it, below.

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.

This commit converts the links generated by the markdown of the "#-mention" of topics to permalinks -- the links containing the "with" narrow operator, the operand being the last message of the channel and topic of the mention. Partly Fixes zulip#21505

This commit updates the topic links obtained from clicking the topics and those obtained from "Copy link to topic" to use the new topic permalinks. Fixes zulip#21505

roanster007 · 2024-06-01T13:58:14Z

Updates:

refined the update_narrow_terms_containing_with_operator method (which corrects the narrow operator when with is present) as per the review
Added commit to use the topic permalinks in left-sidebar topic links, and Copy topic link of topic popover
Addressed the reviews of construct-narrow.

zulipbot added the size: XL label May 16, 2024

roanster007 force-pushed the iss-21505 branch from 6a99ef3 to 24a5aad Compare May 16, 2024 12:52

roanster007 force-pushed the iss-21505 branch from 24a5aad to 0f5a750 Compare May 20, 2024 14:48

roanster007 force-pushed the iss-21505 branch 2 times, most recently from a6b737a to df4829a Compare May 20, 2024 17:00

timabbott reviewed May 20, 2024

View reviewed changes

web/src/narrow.js Show resolved Hide resolved

timabbott reviewed May 20, 2024

View reviewed changes

web/tests/narrow_unread.test.js Show resolved Hide resolved

roanster007 mentioned this pull request May 22, 2024

refactor: Extract method to create and update message lists in narrow. #30172

Merged

12 tasks

roanster007 force-pushed the iss-21505 branch from df4829a to 756e993 Compare May 23, 2024 13:36

roanster007 commented May 23, 2024

View reviewed changes

roanster007 force-pushed the iss-21505 branch 2 times, most recently from 43535b5 to 95c4e72 Compare May 27, 2024 13:25

roanster007 force-pushed the iss-21505 branch from 95c4e72 to 28ff2c1 Compare May 27, 2024 18:53

roanster007 changed the title ~~narrow: Add new "with" narrow operator.~~ topics: Convert topic links to permalinks. May 27, 2024

gnprice reviewed May 28, 2024

View reviewed changes

roanster007 force-pushed the iss-21505 branch 3 times, most recently from 0223270 to e53140a Compare May 29, 2024 20:26

roanster007 mentioned this pull request May 29, 2024

Preparatory commits to #30114 #30244

Merged

12 tasks

timabbott reviewed May 30, 2024

View reviewed changes

zulipbot added the has conflicts label May 30, 2024

roanster007 force-pushed the iss-21505 branch from e53140a to 3d0812d Compare May 30, 2024 19:12

zulipbot removed the has conflicts label May 30, 2024

roanster007 force-pushed the iss-21505 branch 4 times, most recently from f7bad58 to c607da2 Compare June 1, 2024 10:10

roanster007 added 3 commits June 1, 2024 19:20

left_sidebar: Change topic links to use new permalinks.

28d97c2

This commit updates the topic links obtained from clicking the topics and those obtained from "Copy link to topic" to use the new topic permalinks. Fixes zulip#21505

roanster007 force-pushed the iss-21505 branch from 7bf2538 to 28d97c2 Compare June 1, 2024 13:50

		The `with` operator uses message ID, which must be encoded as a string
		for its operand.


		* `with:12345`: Search for the conversation (channel and topic) which
		contains the message with ID `12345`.

topics: Convert topic links to permalinks. #30114

Are you sure you want to change the base?

topics: Convert topic links to permalinks. #30114

Conversation

roanster007 commented May 16, 2024 • edited

gnprice commented May 16, 2024 • edited

roanster007 commented May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott May 20, 2024 • edited

Choose a reason for hiding this comment

roanster007 May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 31, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 May 23, 2024 • edited

Choose a reason for hiding this comment

roanster007 commented May 23, 2024

roanster007 commented May 27, 2024

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented Jun 1, 2024

roanster007 commented May 16, 2024 •

edited

gnprice commented May 16, 2024 •

edited

roanster007 commented May 20, 2024 •

edited

timabbott May 20, 2024 •

edited

roanster007 May 23, 2024 •

edited

roanster007 May 23, 2024 •

edited

roanster007 May 31, 2024 •

edited

roanster007 May 22, 2024 •

edited

roanster007 May 23, 2024 •

edited

roanster007 May 23, 2024 •

edited

timabbott May 30, 2024 •

edited