-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
topics: Convert topic links to permalinks. #30114
base: main
Are you sure you want to change the base?
Conversation
Chat threads in |
Updated narrow
|
a6b737a
to
df4829a
Compare
zerver/openapi/zulip.yaml
Outdated
**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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should redo this with some of the more precise language from the API design conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
zerver/lib/narrow.py
Outdated
def update_narrow_terms_containing_with_operator( | ||
realm: Realm, narrow: OptionalNarrowListT | ||
) -> OptionalNarrowListT: | ||
if narrow is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use the early-return pattern to reduce nesting; just do if narrow is None: return narrow
at the start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ya, right. updated!
zerver/lib/message.py
Outdated
and not send it out to unauthorized recipients. | ||
""" | ||
message = Message.objects.filter(realm=realm, id=message_id).first() | ||
return message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
|
||
narrow.remove(with_term) | ||
|
||
return narrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
zerver/lib/narrow.py
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to rather use channel id, so lookup would be simpler
api_docs/construct-narrow.md
Outdated
**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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also document the with
parameter semantics in the section on ### Message IDs
, below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
web/src/filter.ts
Outdated
@@ -536,6 +551,8 @@ export class Filter { | |||
return verb + "channels"; | |||
case "near": | |||
return verb + "messages around"; | |||
case "with": | |||
return verb + "Thread with Message"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalization here is not in line with the standard for this file. I think you want something like "conversation containing" here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this the right special case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the logic so that it would avoid the work in case it is in the same narrow.
|
||
message_lists.update_current_message_list(msg_list); | ||
handle_post_view_change(msg_list); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prep commit - #30172
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to use the common method - create_and_update_message_list
.
web/tests/filter.test.js
Outdated
{operator: "with", operand: 17}, | ||
]; | ||
filter = new Filter(terms); | ||
assert.ok(filter.can_bucket_by("channel", "with")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 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.
// 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"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume private messages contaning with
operators would be redundant since they can't be moved?
addressed the above reviews. |
43535b5
to
95c4e72
Compare
Addressed reviews, added commit to convert topic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A couple of comments purely on the API.
api_docs/construct-narrow.md
Outdated
The `with` operator uses message ID, which must be encoded as a string | ||
for its operand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
zerver/openapi/zulip.yaml
Outdated
**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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
**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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preparatory PR - #30244
0223270
to
e53140a
Compare
Updates:
now accepts both string and number
|
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 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.
api_docs/construct-narrow.md
Outdated
|
||
* `with:12345`: Search for the conversation (channel and topic) which | ||
contains the message with ID `12345`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added them!
api_docs/construct-narrow.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe /filter/operator
, since it does more than just filtering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
api_docs/construct-narrow.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
cb944eb
to
c0a5db0
Compare
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
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.
Pending: Converting left sidebar topic links to these links.
#21505
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: