This commit renames the 'tornado_redirected_to_list' context
manager to 'capture_send_event_calls' to improve readability.
It also refactors the function to yield a list of events
instead of passing in a list data structure as a parameter
and appending events to it.
Improve the Notification Bot by adding a hyperlink to the new location
of a moved single message. The link will make it easier for users to
find the message in its new context.
Fixes#24604.
Previously, there was a stale code that didn't verify
if 'muted_topics' and 'user_topic' events are sent correctly.
This commit updates the test to verify if the expected
users are notified via 'muted_topics' and 'user_topic'
events.
This commit updates the move-topic codepath to perform
bulk database operations on the UserTopic record using
user_profiles for each visibility_policy instead of
previously looping over each user_profile one by one.
This commit refactors 'set_user_topic_visibility_policy_in_database'
to perform bulk database operations and the related changes.
There is an increase in database query count because requests
to delete user_topic rows now take two queries instead of one.
This is required for logging the info for a request to delete
a non-existent user_topic row while performing bulk operations
at the same time.
The overall query count will be lower while performing
bulk operations (multiple user_profiles instead of one).
This commit updates the 'do_update_message' codepath to
update the UserTopic records regardless of visibility policy
during the "move-topic" operation.
This is required before offering new visibility policies
in the UI.
Previously, UserTopic records were moved or deleted only
for objects with a MUTED visibility policy.
Fixes: #24574
This is a prep commit that renames 'set_topic_mutes' and
'topic_is_muted' to 'set_topic_visibility_policy' and
'topic_has_visibility_policy' respectively, and refactors
them to work with any visibility_policy, not only MUTED.
Previously, when a user moves a message to another topic, the Notification
bot will post a message saying "This topic was moved here from..." This is
confusing when the topic already contains messages. The changes aims to make
the messages more clear by changing the logic for the Notification bot. When
there is already messages in the topic, the bot will post "A message was
moved here from..." or "N messages were moved here from...". The bot will
post "This topic was moved here from (somewhere) by (someone)." when the
topic is empty.
Fixes#23267.
This commit updates 'set_user_topic_visibility_policy_in_database'
to not raise an error when deleting a UserTopic row and the user
doesn't have a visibility_policy for the topic yet, or when setting
the visibility_policy to its current value.
Also, it includes the changes to not send unnecessary events
in such cases.
In order to support different types of topic visibility policies,
this renames 'add_topic_mute' to
'set_user_topic_visibility_policy_in_database'
and refactors it to accept a parameter 'visibility_policy'.
Create a corresponding UserTopic row for any visibility policy,
not just muting topics.
When a UserTopic row for (user_profile, stream, topic, recipient_id)
exists already, it updates the row with the new visibility_policy.
In the event of a duplicate request, raises a JsonableError.
i.e., new_visibility_policy == existing_visibility_policy.
There is an increase in the database query count in the message-edit
code path.
Reason:
Earlier, 'add_topic_mute' used 'bulk_create' which either
creates or raises IntegrityError -- 1 query.
Now, 'set_user_topic_visibility_policy' uses get_or_create
-- 2 queries in the case of creating new row.
We can't use the previous approach, because now we have to
handle the case of updating the visibility_policy too.
Also, using bulk_* for a single row is not the correct way.
Co-authored-by: Kartik Srivastava <kaushiksri0908@gmail.com>
Co-authored-by: Prakhar Pratyush <prakhar841301@gmail.com>
This commit adds time restriction on moving messages between streams
using the move_messages_between_streams_limit_seconds setting in the
backend. There is no time limit for admins and moderators.
We now use the newly added move_messages_within_stream_limit_seconds
setting to check for how long the user can edit the topic replacing
the previously used 3-day limit. As it was previously, there is no
time limit for admins and moderators.
When 'resolve|unresolve' and 'move stream' actions occurs in
the same api call, 'This topic was marked as resolved|unresolved'
notification is not sent.
Both 'topic moved' and 'topic resolved' notification should be generated.
This commit updates the logic of when and where to send
'topic resolve|unresolve' notification. Unlike previous logic, notification
may be sent even in the case 'new_stream' is not None.
In general, 'topic resolved|unresolved' notification is sent to
'stream_being_edited'. In this particular case ('new_stream' is not None),
notification is sent to the 'new_stream' after check.
Test case is included.
Fixes: #22973
Accessing .realm will cause a fetch query from the database if the
attribute hasn't been fetched already earlier in the codepath. That's
completely redundant if we're just comparing realms, and we should only
access .realm_id attribute. This seems to eliminate a query in some
codepaths, which is nice in this performance-sensitive function.
Additionally, migrate existing EditMessageTest to use this helper
method, with the side effect of migrating the tested flow from a
/json/messages URL to a /api/v1/messages URL.
This commit changes the topic edit permssions to not depend whether the user
editing the message had sent the message or it was sent by someone else.
We only do backend changes in this commit and frontend changes will be done
in further commits.
Previously, we always allowed topic edits when the user themseleves had
sent the message not considering the edit_topic_policy and the 3-day time
limit. But now we consider all messages as same and editing is allowed only
according to edit_topic_policy setting and the time limit of 3 days in
addition for users who are not admins or moderators.
We change the topic and stream edit permssions to not depend on
allow_message_editing setting in the API and are allowed even
if allow_message_editing is set to False based on other settings
like edit_topic_policy and can_move_message_between_streams.
Fixes a part of #21739.
This solves the problem that resolving a topic with a long name (>60
characters) will cause the topic name to be truncated, and thus the edit
message code path thinks that the topic is being moved in addition to
being resolved.
We store the pre-truncation topic and use it to check against the
original topic when determining whether a topic is being moved while
getting (un)resovled or not.
Fixes#23482
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We intended to send both the "topic was resolved" and the "topic was
moved here" notification when resolving and moving a topic at the same
time in #22312.
The previous implementation did not work as expected and it was only
sending the "topic was moved here" notification.
This removes the check for old_topic and new_topic that have
RESOLVED_TOPIC_PREFIX stripped in maybe_send_resolve_notifications, so
that the notification will be sent regardless if the topic name without
the prefix stays the same or not.
Note that weird topic handling ("✔ ✔✔ some topic") in the comments
was added in e231a03eff is unaffected. In case of confusion, the lstrip
check is not essential to detecting topic being unresolved/resolved.
As we mainly have that handled in the latter part of the helper.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This adds a helper based on testing patterns of using the "queries_captured"
context manager with "assert_length" to check the number of queries
executed for preventing performance regression.
It explains the rationale of checking the query count through an
"AssertionError" and prints the queries captured as assert_length does,
but with a format optimized for displaying the queries in a more
readable manner.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Previously we did not send notification for topic-only edits.
Now, we add backend support for sending notification to topic-only
edits as well.
We would add support for this in webapp in further commits since
message edit UI will be updated as well. We just make sure that no
notifications are sent when editing topic using pencil icon in
message header.
We also change the API default for moving a topic to only notify the
new location, not the old one; this matches the current defaults in
the web UI.
Includes many tests.
We also update the puppeteer tests to test only content edit as
we are going to change the UI to not allow topic editing from
message edit UI. Also fixing the existing tests to pass while
doing topic edits is somewhat complex as notification message
is also sent to new topic by default.
Fixes#21712.
Co-authored-by: Aman Agrawal <amanagr@zulip.com>
Co-authored-by: Tim Abbott <tabbott@zulip.com>
This is preparatory commit for #18941.
Importing `do_delete_message` from `message_edit.py` was causing a
circular import error. In order to avoid that, we create a separate
message_delete.py file which has all the functions related to deleting
messages.
The tests for deleting messages are present in
`zerver/tests/test_message_edit.py`.
Fixes a part of #18941
This commit changes the code to consider zero as an invalid value for
message_content_edit_time_limit_seconds. Now to represent the setting that
user can edit the message anytime, the setting value will be "None" in
database and "unlimited" will be passed to API from clients.
Since we not allow enabling public access on limited plan realms,
we set the enable_spectator_access setting to False when downgrading
to a limited plan. Setting is still shown in the UI but it is
disabled.
Since `HttpResponse` is an inaccurate representation of the
monkey-patched response object returned by the Django test client, we
replace it with `_MonkeyPatchedWSGIResponse` as `TestHttpResponse`.
This replaces `HttpResponse` in zerver/tests, analytics/tests, coporate/tests,
zerver/lib/test_classes.py, and zerver/lib/test_helpers.py with
`TestHttpResponse`. Several files in zerver/tests are excluded
from this substitution.
This commit is auto-generated by a script, with manual adjustments on certain
files squashed into it.
This is a part of the django-stubs refactorings.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This commit changes the error message from "Invalid stream id"
to "Invalid stream ID" for cases where invalid stream IDs are
passed to API endpoints to make it consistent with other similar
error messages.
We were not setting the `historical` flag correctly for
messages fetched via `json_fetch_raw_message` when used didn't
have any UserMessage.
Extended relevant tests to fetch check message flags too.
This avoids an error when a user has already muted the new topic name.
We do this by ignoring duplicates, rather than catching the
IntegrityError, because this edit happens in a transaction, and that
would abort the transaction.
Our original implementation of moving muted topic records when a topic
is moved took a shortcut of treating all change_later usage as
something with intent to move the whole topic.
This works OK when moving the whole topic via this interface, but not
when moving a last off-topic message in the topic.
Address this by changing the rule to match the existing
moved_all_visible_messages variable.
Previously, Attachment.is_realm_public and its cousin,
Attachment.is_web_public, were properties that began as False and
transitioned to True only when a message containing a link to the
attachment was sent to the appropriate class of stream, or such a link
was added as part of editing a message.
This pattern meant that neither field was updated in situations where
the access permissions for a message changed:
* Moving the message to a different stream.
* Changing the permissions for a stream containing links to the message.
This correctness issue has limited security impact, because uploaded
files are secured both by a random URL and by these access checks.
To fix this, we reformulate these fields as a cache, with code paths
that change the permissions affecting an attachment responsible for
setting these values to the `None` (uncached) state. We prefer setting
this `None` state over computing the correct permissions, because the
correct post-edit permissions are a function of all messages
containing the attachment, and we don't want to be responsible for
fetching all of those messages in the edit code paths.
Prakhar Pratyush
Akarsh Jain
Joelute
Kartik Srivastava
Lauryn Menard
Sahil Batra
Mateusz Mandera
Anders Kaseorg
Josh Klar
Zixuan James Li
Julia Bichler
madrix01
Zixuan James Li
Alex Vandiver
Aman Agrawal
Tim Abbott