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.

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.

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.

gnprice · 2024-06-04T17:53:52Z

api_docs/construct-narrow.md

+for a new `with` operator, designed to be used in links that should
+follow a topic across being moved in a single API request. This operator
+takes in message ID as operand. If the message -- stream/private recipient
+type corresponding to the message ID exists and can be accessed by the
+user, then the narrow terms till the `with` operator is updated to point
+to the narrow containing the message.


This detailed description of what the with operator means and what it's intended for should go in the main description of the operator, in the "Message IDs" section below. Then this "Changes" entry can just say something like:

Changes: In Zulip 9.0 (feature level 263), the with operator was introduced.

In general, the principle with "Changes" entries is that if you just want to understand how the API works today, with the latest version of the server, you can ignore the "Changes" entries completely and just read the rest of the description. And then if you want to understand some previous version of the API, you start by reading up on the current version, then start reading the "Changes" entries to go back through time and get diffs that tell you about the previous versions.

So in particular when something didn't exist at a previous version, the "Changes" entry doesn't need to include any details about it — the only new information you need for understanding that previous version is the fact that such-and-such didn't exist yet.

(This is different from the central API changelog file. That one is meant to be read forward, not backward, for a reader who had some idea of a previous API to catch up on what's changed since then. So when a new feature is introduced, it tends to have a few more words about what it means or what it's for, although the details are still left for the feature's own documentation.)

gnprice · 2024-06-05T04:31:58Z

api_docs/construct-narrow.md

+**Changes**: In Zulip 9.0 (feature level 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.


(continuing from #30114 (comment) )

This version is much closer, but it can still be improved. Like this, from my previous comment:

Suggested change

**Changes**: In Zulip 9.0 (feature level 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.

**Changes**: In Zulip 9.0 (feature level 263), the `with` operator

was introduced.

Mentioning "narrows gained support" is redundant — this is the page about narrows.

The motivation isn't needed here. Remember (from my previous comment) that "Changes" entries are meant to be read from the present and going backward: so the reader already knows, from reading the main description, absolutely everything they want to learn right now about with operators as they are in the current API. The only information that's relevant for the reader when they're here in the "Changes" section is information about the old API, described in terms of the current one; so just the fact that this operator didn't exist then.

Conversely, this information "designed to be used in links that should follow a topic across being moved in a single API request" seems like useful context for understanding the current API. That means it should be in the main description, not in the "Changes" entries — because the reader should be able to totally ignore the "Changes" entries when they're focused on understanding the current API.

oh, ok ~~I'll update it~~ Updated!.

gnprice · 2024-06-05T04:36:49Z

api_docs/construct-narrow.md

+The `with`, `near` and `id` operators, documented in the help center,
+use message IDs for their operands.


Is it true that with is documented in the help center? I think it isn't, in this version.

Which is probably appropriate, since our plan is to not put it in the search box.

Probably this paragraph can just remain as it is, and with can be documented in its own paragraphs separate from the paragraphs about near and id.

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

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 Outdated 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

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 5 times, most recently from 7bf2538 to 28d97c2 Compare June 1, 2024 13:50

zulipbot added the has conflicts label Jun 3, 2024

roanster007 force-pushed the iss-21505 branch from 28d97c2 to 9d3924e Compare June 3, 2024 12:19

zulipbot added has conflicts and removed has conflicts labels Jun 3, 2024

roanster007 force-pushed the iss-21505 branch from 9d3924e to b589e7b Compare June 4, 2024 16:18

zulipbot removed the has conflicts label Jun 4, 2024

gnprice reviewed Jun 4, 2024

View reviewed changes

roanster007 force-pushed the iss-21505 branch from b589e7b to 0337260 Compare June 4, 2024 20:40

gnprice reviewed Jun 5, 2024

View reviewed changes

roanster007 force-pushed the iss-21505 branch from 0337260 to ef0ddf9 Compare June 5, 2024 13:12

alya added maintainer review PR is ready for review by Zulip maintainers. integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels Jun 5, 2024

zulipbot added the has conflicts label Jun 5, 2024

roanster007 added 3 commits June 6, 2024 01:25

left_sidebar: Change topic links to use new permalinks.

0a48d6d

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 ef0ddf9 to 0a48d6d Compare June 5, 2024 19:59

zulipbot removed the has conflicts label Jun 5, 2024

		The `with`, `near` and `id` operators, documented in the help center,
		use message IDs for their operands.

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

timabbott May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 commented Jun 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roanster007 Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

roanster007 Jun 5, 2024 •

edited