This commit adds a new endpoint 'users/me/onboarding_steps'
deprecating the older 'users/me/hotspots' to mark hotspot as read.
We also renamed the view `mark_hotspot_as_read` to
`mark_onboarding_step_as_read`.
Reason: Our plan is to make this endpoint flexible to support
other types of UI elements not just restricted to hotspots.
This commit adds code to include original name, email and avatar
for inaccessible users which can happen when a user sends message
to an unsubscribed stream.
This commit adds code to not allow Zulip Cloud organizations that are not
on the Plus plan to change the "can_access_all_users_group" setting.
Fixes#27877.
1. When we get data and it includes realm info, we should automatically
link the new records with the appropriate RemoteRealm.
2. For old records, when we receive realm data, we have an opportunity
to update those old record to link them to the right RemoteRealm.
This logic doesn't need to always run, just after a remote server
upgrade, since that's when this shift in remote server behavior will
occur.
This is a prep commit to return, for each remote realm, the 'uuid',
'can_push', and 'expected_end_timestamp'.
This data will be used in 'initialize_push_notifications'.
This consists of the following pieces:
1. Makes servers using the bouncer send realm_uuid in requests for token
registration. (Sidenote: realm_uuid is already sent in the "send
notification" codepath as of
48db4bf854)
2. This allows the bouncer to tie RemotePushDeviceToken to the
RemoteRealm with matching realm_uuid at registration time.
3. Introduce handling of some potential weird edge cases around the
realm_uuid and RemoteRealm objects in get_remote_realm_helper.
This default setup will be more realistic, matching the ordinary
conditions for a modern server.
Especially needed as we add bouncer code that will expect to have
RemoteRealm entries for realm_uuid values for which it receives
requests.
[squash]: Update sponsorsip and question boxes for Cloud.
[squash]: Update tabs subtitles.
[squash]: Content for info boxes for self-hosted plans.
[squash]: Adjust content to fit design.
portico: Tweak /plans text.
This reduces the query time by an order of magnitude, since it is able
to switch from a raw `stream_id` index to an index over all of
`realm_id, property, end_time`.
These metadata are essentially all publicily available anyway, and
making uploading them unconditional will simplify some things.
The documentation is not quite accurate in that it claims the server
will upload some metadata that is not actually uploaded yet (but will
by soon). This seems harmless.
Currently, the sender names for outgoing emails sent by Zulip
are hardcoded. It should be configurable for self-hosted systems.
This commit makes the 'Zulip' part a variable in the following
email sender names: 'Zulip Account Security', 'Zulip Digest',
and 'Zulip Notifications' by introducing a settings variable
'SERVICE_NAME' with the default value as f"{EXTERNAL_HOST} Zulip".
Fixes: #23857
This commit updates get_fake_email_domain to accept realm.host as
argument instead of the Realm object since we only use realm.host
to get the fake email domain.
This is a preparatory commit for the limited guest feature as we
would be sending the fake email of the message sender in message
event object to a guest user who cannot access the sender and
there we would need to compute the fake email.
549dd8a4c4 changed the regex that we build to contain whitespace for
readability, and strip that back out before returning it.
Unfortunately, this also serves to strip out whitespace in the source
linkifier, causing it to not match expected strings.
Revert 549dd8a4c4.
Fixes: #27854.
Adds details about the requested organization URL and type to the
registration confirmation email that's sent when creating a new
Zulip organization.
Fixes#25899.
If the request's `Accept:` header signals a preference for serving
images over text, return an image representing the 404/403 instead of
serving a `text/html` response.
Fixes: #23739.
Previously, we weren't able to mute the cross realm bots. This was
because, for muting the users, we access only those profiles which are
in realm, excluding the cross realm system bots.
This is fixed by replacing the access_user_by_id method with a new
method access_user_by_id_including_cross_realm for this specific test.
Fixes#27823
Earlier, for the push notifications having latex math
like "$$1 \oplus 0 = 1$$, the notification had the math
included multiple times.
This commit fixes the incorrect behavior by replacing
the KaTeX with the raw LaTeX source.
Fixes part of #25289.
This commit refactors the current hotspot subsytem to use a more
robust dataclass `Hotspot` defined in `lib/hotspots.py`. This fixes
mypy errors as well as make code more readable.
This commit introduces non-intro hotspots.
They are a bit different than intro hotspots in the
following ways:
* All the non-intro hotspots are sent at once instead of
sending them one by one like intro hotspots.
* They only activate when a specific event occurs,
unlike intro hotspot where they activate after the
previous hotspot is read.
Now, the topic wildcard mention follows the following
rules:
* If the topic has less than 15 participants , anyone
can use @ topic mentions.
* For more than 15, the org setting 'wildcard_mention_policy'
determines who can use @ topic mentions.
Earlier, topic wildcard mentions followed the same restriction
as stream wildcard mentions, which was incorrect.
Fixes part of #27700.
This commit updates the backend code to allow changing
can_access_all_users_group setting in development environment
and also adds a dropdown in webapp UI which is only shown in
development environment.
This makes it possible for a self-hosted realm administrator to
directly access a logged-page on the push notifications bouncer
service, enabling billing, support contacts, and other administrator
for enterprise customers to be managed without manual setup.
We previously did not allow setting signup_notifications_stream and
notifications_stream settings to private streams that admin is not
subscribed to, even when admins have access to metadata of all the
streams in the realm and can see them in the dropdown options as well.
This commit fixes it to allow admins to set these settings to private
streams that the admin is not subscribed to.
Guests might lose access to deactivated users if the user
is not involved in any DM with guest. This commit adds
code to send "realm_user/remove" events for such cases.
We now send user creation events to recipient users
when sending DMs if recipients gain access to either
sender or other pariticpating users in the DM.
This commit adds code to send "realm_user/remove" event
when a guest user loses access to a user due to the user
being unsubscribed from one or more streams.
This commit adds code to send user creation events to
guests who gain access to new subscribers and to the
new guest subscribers who gain access to existing
stream subscribers.
The presence and user status update events are only sent to accessible
users, i.e. guests do not receive presence and user status updates for
users they cannot access.
This commit adds code to make sure that update events for changing
a user's role, email, etc. are not sent to guests who cannot access
the modified user.
We do not send the original user data in user creation events
to guests if user access is restricted in realm, as they would
receive the information about user if user is subscribed to some
common streams after account creation.
This commit adds code to update access_user_by_id to raise
error if guest tries to access an inaccessible user.
One notable behavioral change due to this is that we do
not allow guest to mute or unmute a deactivated user if
that user was not involved in DMs.
This may happen if there are multiple servers with the same UUID
submitting data (e.g. if they were cloned after initial creation), or
if there is one server, but `./manage.py clear_analytics_tables` was
used to truncate the analytics tables.
In the case of `clear_analytics_tables`, the data submitted likely has
identical historical values with new remote `id` values; preserving
the originally-submitted contemporaneous data is the best option. For
the case of submissions from multiple servers, there is no completely
sensible outcome, so the best we can do is detect the case and move
on.
Since we have a lock on the RemoteZulipServer, we know that no other
inserts are happening, so counting before and after will return the
true number of rows inserted (which `bulk_create` cannot do in the
face of `ignore_conflicts`[^1]). We compare this to the expected
number of new inserted rows to detect dropped duplicates.
[^1]: See https://code.djangoproject.com/ticket/30138.
Earlier, for the emails having latex math like
"$$d^* = +\infty$$", the bad rendering led to the math
being included multiple times in the email body.
This was due to displaying KaTeX HTML without the CSS.
This commit fixes the incorrect behavior by replacing
the KaTeX with the raw LaTex source.
Fixes part of #25289.
This is a useful helper using the same API as
send_analytics_to_push_bouncer(), but uploading only realms info. This
is useful to upload realms info without the risk of taking a long time
to process the request due to too much of the *Count analytics data.
The original behavior of this setting was to disable LDAP
authentication for any realms not configured to use it. This was an
arbitrary choice, and its only value was to potentially help catch
typos for users who are lazy about testing their configuration.
Since it makes it a very inconvenient to potentially host multiple
organizations with different LDAP configurations, remove that
behavior.
This makes it possible to send notifications to more than one app ID
from the same server: for example, the main Zulip mobile app and the
new Flutter-based app, which has a separate app ID for use through its
beta period so that it can be installed alongside the existing app.
This commit adds code to send stream deletion events when
unsubscribing non-admin users from private streams and
when unsubscribing guests from public streams since
non-admins cannot access unsubscribed private streams
and guests cannot access unsubscribed public streams.
It was discovered by the Zulip development team that active users who
had previously been subscribed to a stream incorrectly continued being
able to use the Zulip API to access metadata for that stream. As a
result, users who had been removed from a stream, but still had an
account in the organization, could still view metadata for that
stream (including the stream name, description, settings, and an email
address used to send emails into the stream via the incoming email
integration). This potentially allowed users to see changes to a
stream’s metadata after they had lost access to the stream.
This bug was present in all Zulip releases prior to today's Zulip
Server 7.5.
This commit adds new API endpoint to get stream email which is
used by the web-app as well to get the email when a user tries
to open the stream email modal.
The stream email is returned only to the users who have access
to it. Specifically for private streams only subscribed users
have access to its email. And for public streams, all non-guest
users and only subscribed guests have access to its email.
All users can access email of web-public streams.
This commit removes "email_address" field from Subscription objects
and we would instead a new endpoint in next commit to get email
address for stream with proper access check.
This change also fixes the bug where we would include email address
for the unsubscribed private stream as well when user did not have
permission to send message to the stream, and having email allowed
the unsubscribed user to send message to the stream.
Note that the unsubscribed user can still send message to the stream
if the user had noted down the email before being unsubscribed
and the stream token is not changed after unsubscribing the user.
We now pass bogus data for inaccessible users when sending
the users data in "realm_users" field of "register" response
or when using endpoints like "GET /users" to get data of
all the users in realm.
We would add a client capability field in future commits
such that new clients would receive data only for accessible
users and they can form the bogus data by themselves.
This commit adds new setting for controlling who can access
all users in the realm which would have "Everyone" and
"Members only" option.
Fixes part of #10970.
This is a CountStat for tracking how many mobile notifications the
server requested.
1. On a self-hosted server, that means requesting from the push bouncer.
2. On a server that's its own push bouncer, that's just the number
directly sent.
This number has room for inaccuracy due to incrementing by the number of
user devices on a self-hosted server, as it doesn't account for errors
that may occur in the GCM/APNs low-level sending codepaths on the bouncer.
Also tests that a server that's its own push bouncer correctly
increments its mobile_pushes_sent::day CountStat, by basing it on the
values returned from the send_apple/android_push_notification functions
which tell us the actual number of successfully sent notifications.
Since the return values of send_..._push_notification are now
used in those codepaths, we need to tweak our mocks in some unrelated
tests to set up some return value to avoid errors.
Rename the existing 'wildcard_mentioned' flag to
'stream_wildcard_mentioned'.
The 'wildcard_mentioned' flag is deprecated and exists for
backwards compatibility.
We have two separate flags for stream and topic wildcard mentions,
i.e., 'stream_wildcard_mentioned' and 'topic_wildcard_mentioned',
respectively.
* stream wildcard mentions: `@all`, `@everyone`, and `@stream`
* topic wildcard mentions: `@topic`
The `wildcard_mentioned` flag is included in the events and
API response if either `stream_wildcard_mentioned` or
`topic_wildcard_mentioned` is set.
In c37871ac3a, we renamed the
two unused and historical bits of the 'flags' bitfield of
the 'UserMessage' table:
* 'summarize_in_home' to 'topic_wildcard_mentioned'
* 'summarize_in_stream' to 'group_mentioned'
This commit clears out the old data for those bits.
Additionally, we are clearing 'force_expand' and 'force_collapse'
unused flags to save future work.
Add the new model for recording basic information about Realms on remote
server, to go with the other analytics data. Also adds necessary changes
to the bouncer endpoint and the send_analytics_to_push_bouncer()
function to submit such Realm information.
Previously, when a deactivated user was mentioned, he wasn't
rendered as a Pill. This is because the dataset for validating mentions
only included active users, which is fixed by removing that filter.
To allow only silent mentions of them, an extra is_active property
added to FullNameInfo class, which is populated from the query,
which tells if user is deactivated. This is used to convert any
mentions of them to silent mentions in the backend markdown.
Fixes#26857
This is a prep commit for adding feature of restricting
user access to guests such that we can keep the code
easy to read and understand when that feature is added.
We'll need this information in order to properly direct APNs
notifications. Happily, the Zulip server always sends it when
registering an APNs token; and it appears it always has done so
since the commit:
cddee49e7 Add support infrastructure for push notification bouncer service.
back in 2016. So there's no compatibility issue from requiring it.
This missing `REQ` call has meant we just drop this parameter:
even though the remote Zulip server passes it (for all APNs tokens),
we never notice and never store it. Fix that.
We're going to need to use this information, so we shouldn't just
assume a value; the client should tell us the actual value.
Conveniently, the Zulip mobile app does already pass this parameter
and has since forever. So we can just start requiring it, with no
compatibility constraint.
We already always pass this parameter from the mobile client,
so this makes the tests more realistic already. And we'll shortly
be making this parameter required.
Earlier, email message notifications included prior messages sent
to the same topic for context. This is more confusing than helpful
for messages that the user is likely to have received notifications
for all the prior messages in the conversation already (or read them
in the Zulip UI).
Now, we include prior context only when the user is mentioned via
personal, group, stream or topic wildcard mention.
Fixes#27479.
This commit improves the test to explicitly verify that multiple
messages that were sent in quick succession to a topic are included
in the email body when we have email notifications enabled for a
given stream.
Earlier, the test was only verifying the email subject and the fact
that only one email was sent.
It is important to verify the fact that all the messages sent to a
topic in quick succession should be included in the email body.
The event for stream typing notifications is no longer sent
to the long_term_idle subscribers of the stream.
This helps to reduce the tornado's work of parsing super-long
JSON-encoded lists of user IDs in large streams. Now the lists
are shorter.
This will be used in gear menu to inform admin of their
sponsorship application status.
This includes some additional tweaks for when to show
billing and plans to users.
There order of group ids doesn't matter here and thus the
compared values can have the ids in different order and test
should still pass. So, using `set` for comparing unordered
lists seems like the right fix here.
Earlier, the 'wildcard_mentioned' flag was set for both the
stream and topic wildcard mentions.
Now, the 'topic_wildcard_mentioned' flag is set for topic
wildcard mentions, and the 'wildcard_mentioned' flag is set for
stream wildcard mentions.
We will rename the 'wildcard_mentioned' flag to
'stream_wildcard_mentioned' in a later commit.
This commit renames the two unused and historical bits of the
'fields' bitfield of the 'UserMessage' and 'ArchivedUserMessage'
tables.
* 'summarize_in_home' to 'topic_wildcard_mentioned'
* 'summarize_in_stream' to 'group_mentioned'
The 'group_mentioned' flag doesn't affect the feature,
but completing the work here helps to save future migration
and indexing efforts on the UserMessage table, as we plan to
use this flag in the future for group mentions.
The unused bits may have old data; we'll clear that in
a separate commit.
It creates the 'zerver_usermessage_any_mentioned_message_id'
index concurrently.
We now send "realm_user/update" (and "realm_bot/update" for bots)
events with "is_active" field when deactivating and reactivating
users, including bots.
We would want to use "remove" event for a user losing access
to another user for #10970, so it is better to use "update"
event for deactivation as we only update "is_active" field
in the user objects and the clients still have the data for
deactivated users.
Previously, we used to send "add" event for reactivation along
with complete user objects, but clients should have the data
for deactivated users as well, so an "update" event is enough
like we do when deactivating users.
38f2a2f475 updated the comment but not the code. Using
`self.client.post` instead of `self.client_post` means that we do not
set the host headers correctly.
This commit adds code to pass configuration objects for group
permission settings in register response to clients such that
we do need to duplicate that data in clients and can avoid
future bugs due to inconsistency.
The "server_supported_permission_settings" field is included
in the response if "realm" is present in "fetch_event_types",
as this is what we do for other server-related fields.
This commit moves constants for system group names to a new
"SystemGroups" class so that we can use these group names
in multiple classes in models.py without worrying about the
order of defining them.
This commit renames permissions_configuration variable to
permission_configuration since the object contains config for
a single permission setting and thus permission_configuration
seems like a better name.
Previous behavior-
- Guest did not receive stream creation events for new
web-public streams.
- Guest did not receive peer_add and peer_remove events
for web-public and subscribed public streams.
This commit fixes the behavior to be -
- Guests now receive stream creation events for new
web-public streams.
- Guest now receive peer_add and peer_remove events for
web-public and subscribed public streams.
This commit updates code in bulk_remove_subscriptions and
bulk_add_subscriptions to return early if there are no
subscribers to remove or add to the streams.
This change helps us in avoiding unnecessary queries like the
one used to get subscribers list of streams, which is then used
to send events but we would not send any events if no subscribers
are added or removed and some more similar queries.
We use `Realm.default_language` value, which is set by selecting
the 'Organization language', to internationalize the introductory
messages of the initial streams.
Fixes#25729.
In this commit, we add a new dropdown 'Organization language' on
the `/new` and `/realm/register` pages. This dropdown allows setting
the language of the organization during its creation. This allows
messages from Welcome Bot and introductory messages in streams to be
internationalized.
Fixes a part of #25729.
This commit renames default_view and escape_navigates_to_default_view
settings to web_home_view and web_escape_navigates_to_home_view in
database and API to match with our recent renaming of user facing
strings related to this.
We also rename the variables, functions, comments in code and class
names and IDs for elements related to this.
Adds a new onboarding email `onboarding_team_to_zulip` for the user
who created the new Zulip organization.
Co-authored by: Alya Abbott <alya@zulip.com>
The former name is kind of misleading - this function is for the remote
server to send analytics to the push bouncer. Under our usual
terminology, a "remote server" is a self-hosted Zulip server. So data is
sent FROM not TO a remote server.
Originally, this was how the notification emails worked, but that was changed
in 797a7ef97b, with this old behavior
available as an option.
The footer and from address of emails that are sent when this
setting is set to True are confusing, especially when more people
are involved in a stream and since we have changed the way we send
emails, it should be removed. It’s also not widely used.
Fixes#26609.
Thisi and the following commit follow the approach used in
3e2ad84bbe.
First migration requires a server restart - after that any new realms
will be created with the columns set.
The following migrations are in the next commit:
Second migration does a backfill for older realms and can run in the
background while the server is operating normally.
Third migration enforces null=False now that all realms have the columns
set.
Add an optional `automatic_new_visibility_policy` enum field
in the success response to indicate the new visibility policy
value due to the `automatically_follow_topics_policy` and
`automatically_unmute_topics_in_muted_streams_policy` user settings
during the send message action.
Only present if there is a change in the visibility policy.
Python evaluates function parameter defaults at definition time, not
call time. This function wouldn’t work with other realms anyway.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit adds support to allow bot-owners to delete messages
sent by their bots if they are allowed to delete their own messages
as per "delete_own_message_policy" setting and the message delete
time limit has not passed.
`stack_info` shows the stack between where the error was raised and
where it was captured -- which is not interesting when we
intentionally raised it, and know where it will be captured.
Omit the `stack_info` when it will just fill the logs with
uninteresting data.
This commit adds a 'stream_id' parameter to the 'POST /typing'
endpoint.
Now, 'to' is used only for "direct" type. In the case of
"stream" type, 'stream_id' and 'topic' are used.
Earlier, 'MissedMessageHookTest' didn't have 'super().setUp()'
and 'super().tearDown()' in the overrided methods 'setUp' and
'tearDown', respectively, that resulted in cached objects being
used between tests and hence flaky test failures.
This commit adds 'super().setUp()' and 'super().tearDown()'.
232eb8b7cf changed how these pages work, to render inline instead of
serving from a URL, but did not update the SMTP use case; this made
SMTP failures redirect to a 404.
Two registration requests for the same email address can race,
leading to an IntegrityError when making the second user.
Catch this and redirect them to the login page for their existing
email.
When the `type` of the message being composed is "stream",
this commit updates the `to` parameter to accept the ID of
the stream in which the message is being typed.
Earlier, it accepted a single-element list containing the ID
of the stream.
Sending the element instead of a list containing the single element
makes more sense.
This is a prep commit that extracts the following two methods
from '/actions/scheduled_messages' to reuse in the next commit.
* extract_stream_id
* extract_direct_message_recipient_ids
The 'to' parameter for 'POST /typing' will follow the same pattern
in the next commit as we currently have for the 'to' parameter in
'POST /scheduled_messages', so we can reuse these functions.
This commit removes the compatibility support for "private"
being a valid value for the 'type' parameter in 'POST /typing'.
"direct" and "stream" are the only valid values.
This commit includes the message's sender id in the
'topic_participant_user_ids' set.
The 'participants_for_topic' function doesn't include the sender_id,
if the user is sending their first message in the topic, because
'participants_for_topic' queries the 'Message' table, but the message
is actually sent at a later stage in the codepath, resulting in
missing the sender_id in this case.
This is needed to set the 'wildcard_mentioned' flag for the sender's
user message in the case of topic wildcard mentions.
This doesn't lead to sending email and push notifications to the
sender because we have a check to skip notifications if the user
to receive notifications is the sender itself.
This should have been included in c0c30bc.
This commit adds two user settings, named
* `automatically_follow_topics_policy`
* `automatically_unmute_topics_in_muted_streams_policy`
The settings control the user's preference on which topics they
will automatically 'follow' or 'unmute in muted streams'.
The policies offer four options:
1. Topics I participate in
2. Topics I send a message to
3. Topics I start
4. Never (default)
There is no support for configuring the settings through the UI yet.
Earlier, when we used 'self.send_message()' in the backend tests,
the sent message was not marked as read for the sender.
Reason: To set the read flag, we have to check if
'message.sent_by_human()'. It returns False because the
'sending_client' for tests is "test suite" and the 'sent_by_human'
function doesn't enlist the "test suite" client name as a human client.
This commit adds "test suite" to that list.
Also fixes a bug in when apply_unread_message_event was called that
was revealed by this change.
Updates the testing for draft event schemas to be fully checked by
`zerver/tests/test_events.py` and `tools/check-schema`.
Also, corrects the type for the timestamp field in Draft objects
in the OpenAPI documentation.
Updates the testing for scheduled message event schemas to be fully
checked by `zerver/tests/test_events.py` and `tools/check-schema`.
Adds the missing 'failed' field to the scheduled message events
in `web/tests/lib/events.js` as well.
This change adds support for importing guest users from a Mattermost
export file into Zulip. The function now checks the user's teams and
roles to determine whether the user is a guest on the team, and sets
the user's role accordingly. This ensures that the imported user data
includes the correct role for each user.
Fixes#23720.
This fixes a regression introduced in
9954db4b59, where the realm's default
language would be ignored for users created via API/LDAP/SAML,
resulting in all such users having English as their default language.
The API/LDAP/SAML account creation code paths don't have a request,
and thus cannot pull default language from the user's browser.
We have the `realm.default_language` field intended for this use case,
but it was not being passed through the system.
Rather than pass `realm.default_language` through from each caller, we
make the low-level user creation code set this field, as that seems
more robust to the creation of future callers.
Making request a mandatory kwarg avoids confusion about the meaning of
parameters, especially with `request` acquiring the ability to be None
in the upcoming next commit.
None of these tests seem to want to have tick=True, which is the
default. Letting the clock tick without a reason introduces the
possibility of nondeterministic test failures depending on the execution
time.
This reverts b8581e2895. The mobile
client on Android parses this field using:
```kotlin
timeMs = data.require("time").parseLong("time") * 1000
```
This throws an error if value is not `long` (i.e. an integer),
resulting in dropped notifications on Android from servers which had
deployed b8581e2895.
Switch back to sending an integer, but keep the behaviour from
fd6091ad17 where we send the timestamp in the payload of both
Android and Apple push notifications.
Rather than fetch all UserMessage rows for all streams, and subtract
those out in Python-space from the list of all Message rows the user
may have received -- do this via a "NOT EXISTS" subquery. This is
much better indexed (performing in fractions of milliseconds rather
than hundreds), and also consumes much less memory.
Adds support for bulk-adjusting a single user's membership in multiple
user groups in a single transaction in the low-level actions
functions, for future use by work on #9957.
This kind of payload that's loaded from json in the body of the request
is not only used for webhooks, but also in the push bouncer, and may get
used elsewhere too - so a general name is better.
Earlier, 'is_row_muted' returned 'true' if the message was in
a muted stream or muted topic.
If the message is in an unmuted or followed topic in a muted
stream, such topics should be treated as not muted topics
in an unmuted stream.
This commit fixes the incorrect behavior.
Now, for wildcard mentions, 'unread_msgs.mentions' exclude
the IDs in muted streams only if the message is in default or
muted topic.
Also, 'unread_msgs.count' takes into account the unreads in unmuted
or followed topics in muted streams too.
Documents that this bug was fixed in the API changelog.
Update 'get_muted_stream_ids' to return a set of IDs
instead of a list.
This will help to avoid linear time search operations later
while using 'if stream_id in muted_streams_ids'.
A comment was added in f797604 to convey that the unread count
at that time doesn't exclude the unreads in muted topics.
848c080 added the support to exclude the muted topic;
however, the comment was not updated.
This commit updates the comment to reflect the current behavior.
This is an exception that we should be generally catching like the
others, which will give our standard /login/ redirect and proper logging
- as opposed to a 500 if we don't catch.
Addresses directly a bug we occurred in the wild, where a SAMLResponse
was submitted without issuers specified in a valid way, causing this
exception. The added test tests this specific type of scenario.
These queries benefit from the increased specificity of using the
realm / recipient / sender indexes. The argument from 11a1cb9630
does not apply in these cases, since there are only 2 usermessage rows
for each matching message row for DMs, and few more than that for
huddles.
This commit adds a `jitsi_server_url` field to the Realm model, which
will be used to save the URL of the custom Jitsi Meet server. In
the database, `None` will encode the server-level default. We can't
readily use `None` in the API, as it could be confused with "field not
sent". Therefore, we will use the string "default" for this purpose.
We have also introduced `server_jitsi_server_url` in the `/register`
API. This will be used to display the server's default Jitsi server
URL in the settings UI.
The existing `jitsi_server_url` will now be calculated as
`realm_jitsi_server_url || server_jitsi_server_url`.
Fixes a part of #17914.
Co-authored-by: Gaurav Pandey <gauravguitarrocks@gmail.com>
The unique index on `(user_id, message_id)` that is the
`zerver_usermessage` table is rather specific, and even the PostgreSQL
extended statistics are not enough for it to realize there is a
correlation between the `realm_id` in the message table and the
`user_id` in the usermessage table. This means that adding the
`realm_id` limit when there is a join to `zerver_usermessage` flips
the query plan from a nested loop of unique usermessage index-only
scan, with an index scan of the messages pkey -- to a parallel hash
join of the messages limit with a index scan of just the user_id limit
on usermessages. It thinks this is necessary because it thinks that
the `realm_id` limit may remove a large number of messages from the
usermessage set -- which is totally untrue.
Remove the `realm_id` limit if we have a usermessage join.
This endpoint verifies that the services that Zulip needs to function
are running, and Django can talk to them. It is designed to be used
as a readiness probe[^1] for Zulip, either by Kubernetes, or some other
reverse-proxy load-balancer in front of Zulip. Because of this, it
limits access to only localhost and the IP addresses of configured
reverse proxies.
Tests are limited because we cannot stop running services (which would
impact other concurrent tests) and there would be extremely limited
utility to mocking the very specific methods we're calling to raising
the exceptions that we're looking for.
[^1]: https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/
The `expected` flag was incredibly confusing, as you
couldn't tell from the calling code what you were
actually expecting to happen.
I avoid the context manager idiom in order to force
the callers to create simple helper functions, and
I de-duplicate some code in some places.
I also force the caller to explicitly soft-deactivate
the user with one simple line of code, so that the
person reading the test doesn't have to research
the side effects of the helper. (And I make it
very easy for new authors to follow the practice
going forward.)
This is also somewhat of a prep commit to avoid
the obfuscated use of refresh_from_db.
I add a bunch of cute helper methods to make
the test a bit more readable.
And then I make sure to get clean objects,
which precludes the need for our callback
functions to refresh the user objects.
And finally I make sure that our validation
functions don't cause any round trips (assuming
we have fetched objects using a standard
Zulip helper, which example_user ensures.)
In feature levels 153 and 154, a new value of "partially_completed"
for `result` in a success (HTTP status code 200) was added for two
endpoints that process messages in batches: /api/delete-topic and
/api/mark-all-as-read.
Prior to these changes, `result` was either "success" or "error" for
all responses, which was a useful API invariant to have for clients.
So, here we remove "partially_completed" as a potential value for
"result" in a response. And instead, for the two endpoints noted
above, we return a boolean field "complete" to indicate if the
response successfully deleted/marked as read all the targeted
messages (complete: true) or if only some of the targeted messages
were processed (complete: false).
The "code" field for an error string that was also returned as part
of a partially completed response is removed in these changes as
well.
The web app does not currently use the /api/mark-all-as-read
endpoint, but it does use the /api/delete-topic endpoint, so these
changes update that to check the `complete` boolean instead of the
string value for `result`.
This adds support for syncing user role via the newly added "role"
attribute, which can be set to either of
['owner', 'administrator', 'moderator', 'member', 'guest'].
Removes durable=True from the atomic decorator of do_change_user_role,
as django-scim2 runs PATCH operations in an atomic block.
This is a prep commit to separate the single test
'test_stream_send_message_events' into two separate tests named
'test_stream_send_message_events' & test_stream_update_message_events'
to verify the events related to send and update message, respectively.
As a part of introducing two new user settings
* 'automatically_follow_topics_policy'
* 'automatically_unmute_topics_policy'
in the next commit, we will extend 'test_stream_send_message_events'.
This logical separation helps in avoiding a single, super-long test.
This commit removes the stray values, i.e., [1, 2, 3], used
in the tests for desktop_icon_count_display.
We use 'UserProfile.DESKTOP_ICON_COUNT_DISPLAY_CHOICES' instead.
'test_change_user_settings' in 'UserDisplayActionTest' excludes
the notification settings and tests only the display settings.
The code block excluding the notification settings doesn't exclude
'modern_notification_settings'. It only excludes the
'notification_settings_legacy'.
This commit replaces 'notification_settings_legacy' with
'notification_setting_types', which consists of all the
notification settings.
The query plan for fetching recent messages from the arbitrary set of
streams formed by the intersection of 30 random users can be quite
bad, and can descend into a sequential scan on `zerver_recipient`.
Worse, this work of pulling recent messages out is redone if the
stream appears in the next batch of 30 users.
Instead, pull the recent messages for a stream on a one-by-one basis,
but cache them in an in-memory cache. Since digests are enqueued in
30-user batches but still one-realm-at-a-time, work will be saved both
in terms of faster query plans whose results can also be reused across
batches.
This requires that we pull the stream-id to stream-name mapping for
_all_ streams in the realm at once, but that is well-indexed and
unlikely to cause performance issues -- in fact, it may be faster
than pulling a random subset of the streams in the realm.
This is designed to help PostgreSQL have better specificity and
locality in its indexes. Subsequent commits will adjust the code to
make sure that we use these indexes rather than the `realm_id`-less
versions.
We do not add a `realm_id` variation to the full-text index, since
it is a GIN index; multi-column GIN indexes are not terribly
performant, require the `btree_gin` extension for `int` types (which
requires superuser privileges on PostgreSQL 12 and earlier), and
cannot be consistently added concurrently on running instances.
After all indexes have been made, we also run `CREATE STATISTICS` in
order to give PostgreSQL the opportunity to realize that recipient and
sender are highly correlated with message realm, allowing it to
estimate that `(realm_id, recipient_id)` is likely as specific as
matching a given `recipient_id`, instead of as likely as matching
`realm_id` times matching a `recipient_id`. Finally, those statistics
must be filled by `ANALYZE zerver_message`, which is run last.
We now have a `realm_id` on Message; use it, rather than having to
check the sender's realm. This is theoretically different for
cross-realm bots, but these changes are all in tests where that does
not apply.
When searching for links inside a topic name, the question mark (?)
was used to split the topic. If a URL had a query after the URL
(e.g., "?foo=bar"), then the query was trimmed from the URL.
Removing the question mark from `basic_link_splitter` is sufficient
to fix this issue. The `get_web_link_regex` function then removes
the trailing punctuation if any, including literal question marks.
Fixes#26368.
Transifex has parameters that need to be parsed from JSON and converted
to int. Note that we use Optional[Json[int]] instead of
Json[Optional[int]] to replicate the behavior of json_validator. This
caveat is explained in a new test called test_json_optional.
This demonstrates the use of BaseModel to replace a check_dict_only
validator.
We also add support to referring to $defs in the OpenAPI tests. In the
future, we can descend down each object instead of mapping them to dict
for more accurate checks.
This demonstrates some basic use cases of the Json[...] wrapper with
@typed_endpoint.
Along with this change we extend test_openapi so that schema checking
based on function signatures will still work with this new decorator.
Pydantic's TypeAdapter supports dumping the JSON schema of any given type,
which is leveraged here to validate against our own OpenAPI definitions.
Parts of the implementation will be covered in later commits as we
migrate more functions to use @typed_endpoint.
See also:
https://docs.pydantic.dev/latest/api/type_adapter/#pydantic.type_adapter.TypeAdapter.json_schema
For the OpenAPI schema, we preprocess it mostly the same way. For the
parameter types though, we no longer need to use
get_standardized_argument_type to normalize type annotation, because
Pydantic dumps a JSON schema that is compliant with OpenAPI schema
already, which makes it a lot convenient for us to compare the types
with our OpenAPI definitions.
Do note that there are some exceptions where our definitions do not match
the generated one. For example, we use JSON to parse int and bool parameters,
but we don't mark them to use "application/json" in our definitions.
We want to reject ambiguous type annotations that set ApiParamConfig
inside a Union. If a parameter is Optional and has a default of None, we
prefer Annotated[Optional[T], ...] over Optional[Annotated[T, ...]].
This implements a check that detects Optional[Annotated[T, ...]] and
raise an assertion error if ApiParamConfig is in the annotation. It also
checks if the type annotation contains any ApiParamConfig objects that
are ignored, which can happen if the Annotated type is nested inside
another type like List, Union, etc.
Note that because
param: Annotated[Optional[T], ...] = None
and
param: Optional[Annotated[Optional[T], ...]] = None
are equivalent in runtime prior to Python 3.11, there is no way for us
to distinguish the two. So we cannot detect that in runtime.
See also: https://github.com/python/cpython/issues/90353
The goal of typed_endpoint is to replicate most features supported by
has_request_variables, and to improve on top of it. There are some
unresolved issues that we don't plan to work on currently. For example,
typed_endpoint does not support ignored_parameters_supported for 400
responses, and it does not run validators on path-only arguments.
Unlike has_request_variables, typed_endpoint supports error handling by
processing validation errors from Pydantic.
Most features supported by has_request_variables are supported by
typed_endpoint in various ways.
To define a function, use a syntax like this with Annotated if there is
any metadata you want to associate with a parameter, do note that
parameters that are not keyword-only are ignored from the request:
```
@typed_endpoint
def view(
request: HttpRequest,
user_profile: UserProfile,
*,
foo: Annotated[int, ApiParamConfig(path_only=True)],
bar: Json[int],
other: Annotated[
Json[int],
ApiParamConfig(
whence="lorem",
documentation_status=NTENTIONALLY_UNDOCUMENTED
)
] = 10,
) -> HttpResponse:
....
```
There are also some shorthands for the commonly used annotated types,
which are encouraged when applicable for better readability and less
typing:
```
WebhookPayload = Annotated[Json[T], ApiParamConfig(argument_type_is_body=True)]
PathOnly = Annotated[T, ApiParamConfig(path_only=True)]
```
Then the view function above can be rewritten as:
```
@typed_endpoint
def view(
request: HttpRequest,
user_profile: UserProfile,
*,
foo: PathOnly[int],
bar: Json[int],
other: Annotated[
Json[int],
ApiParamConfig(
whence="lorem",
documentation_status=INTENTIONALLY_UNDOCUMENTED
)
] = 10,
) -> HttpResponse:
....
```
There are some intentional restrictions:
- A single parameter cannot have more than one ApiParamConfig
- Path-only parameters cannot have default values
- argument_type_is_body is incompatible with whence
- Arguments of name "request", "user_profile", "args", and "kwargs" and
etc. are ignored by typed_endpoint.
- positional-only arguments are not supported by typed_endpoint. Only
keyword-only parameters are expected to be parsed from the request.
- Pydantic's strict mode is always enabled, because we don't want to
coerce input parsed from JSON into other types unnecessarily.
- Using strict mode all the time also means that we should always use
Json[int] instead of int, because it is only possible for the request
to have data of type str, and a type annotation of int will always
reject such data.
typed_endpoint's handling of ignored_parameters_unsupported is mostly
identical to that of has_request_variables.
This is important because the "guests" value isn't one that we'd
expect anyone to pick intentionally, and in particular isn't an
available option for the similar/adjacent "email invitations" setting.
This commit rename the existing setting `Who can invite users to this
organization` to `Who can send email invitations to new users` and
also renames all the variables related to this setting that do not
require a change to the API.
This was done for better code readability as a new setting
`Who can create invite links` will be added in future commits.
This commit does the backend changes required for adding a realm
setting based on groups permission model and does the API changes
required for the new setting `Who can create multiuse invite link`.
Previously (with ERROR_REPORTING = True), we’d stuff the entire
traceback of the initial exception into the subject line of an error
email, and then also send a separate email for the JSON 500 response.
Instead, log one error with the standard Django format.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
'test_get_message_payload_gcm_stream_message' verifies the payload
for notifications generated (for stream messages) due to any of the
push notification triggers, including
'NotificationTriggers.STREAM_PUSH'.
Earlier, 'test_get_message_payload_gcm_stream_notifications' tested
the same thing as 'test_get_message_payload_gcm_stream_message' with
the only difference that it included content that was not truncated.
This commit removes the test
'test_get_message_payload_gcm_stream_notifications' and updates
the test 'test_get_message_payload_gcm_stream_message' to cover
both the cases, i.e., truncated as well as not truncated content.
This commit removes the 'alert' field from the payload for
Android via GCM/FCM.
The alert strings generated do not get used at all and have
not been used since at least 2019. On Android, we construct
the notification UI ourselves in the client, and we ignore
the alert string.
Creates process for demo organization owners to add an email address
and password to their account.
Uses the same flow as changing an email (via user settings) at the
beginning, but then sends a different email template to the user
for the email confirmation process.
We also encourage users to set their full name field in the modal for
adding an email in a demo organization. We disable the submit button
on the form if either input is empty, email or full name.
When the user clicks the 'confirm and set password' button in the
email sent to confirm the email address sent via the form, their
email is updated via confirm_email_change, but the user is redirected
to the reset password page for their account (instead of the page for
confirming an email change has happened).
Once the user successfully sets a password, then they will be
prompted to log in with their newly configured email and password.
Since an email address is not required to create a demo organization,
we need a Zulip API email address for the web-app to use until the
owner configures an email for their account.
Here, we set the owner's `email_address_visibility` to "Nobody" when
the owner's account is created so that the Zulip API email field in
their profile is a fake email address string.
To make creation of demo organizations feel lightweight for users,
we do not want to require an email address at sign-up. Instead an
empty string will used for the new realm owner's email. Currently
implements that for new demo organizations in the development
environment.
Because the user's email address does not exist, we don't enqueue
any of the welcome emails upon account/realm creation, and we
don't create/send new login emails.
This is a part of #19523.
Co-authored by: Tim Abbott <tabbott@zulip.com>
Co-authored by: Lauryn Menard <lauryn@zulip.com>
Updates the API error response when there is an unknown or
deactivated user in the `principals` parameter for either the
`/api/subscribe` or `/api/unsubscribe` endpoints. We now use
the `access_user_by_email` and `access_user_by_id` code paths,
which return an HTTP response of 400 and a "BAD_REQUEST" code.
Previously, an HTTP response of 403 was returned with a special
"UNAUTHORIZED_PRINCIPAL" code in the error response. This code
was not documented in the API documentation and is removed as
a potential JsonableError code with these changes.
Fixes#26593.
The 'startinline' option is utilized in the `CodeHilite` instantiation
to indicate that the provided PHP code snippet should be highlighted
even if it doesn't start with the opening tag or marker of the
associated programming language (which will rarely be the case in
Zulip, since one discusses a section of a file much more often than a
whole file).
Fixes: https://chat.zulip.org/#narrow/stream/137-feedback/topic/php.20syntax.20highlighting.20should.20not.20require.20.60.3C.3Fphp.60.
Signed-off-by: Akshat <akshat25iiit@gmail.com>
This commit adds a test to verify the payload
'get_message_payload_apns' returns when the notification trigger is
'NotificationTriggers.FOLLOWED_TOPIC_PUSH'.
This commit updates the 'get_apns_alert_subtitle' function to
return a common subtitle, i.e., "{full_name} mentioned everyone:"
for wildcard mentions.
The triggers for the stream or topic wildcard mentions include:
* NotificationTriggers.TOPIC_WILDCARD_MENTION_IN_FOLLOWED_TOPIC
* NotificationTriggers.STREAM_WILDCARD_MENTION_IN_FOLLOWED_TOPIC
* NotificationTriggers.TOPIC_WILDCARD_MENTION
* NotificationTriggers.STREAM_WILDCARD_MENTION
This PR implements the audio call feature for Zoom. This is done by explicitly
telling Zoom to create a meeting where the host's video and participants' video
are off by default.
Another key change is that when creating a video call, the host's and
participants' video will be on by default. The old code doesn't specify that
setting, so meetings actually start with video being off. This new behavior has
less work for users to do. They don't have to turn on video when joining a call
advertised as "video call". It still respects users' preferences because they
can still configure their own personal setting that overrides the meeting
defaults.
The Zoom API documentation can be found at
https://developers.zoom.us/docs/api/rest/reference/zoom-api/methods/#operation/meetingCreateFixes#26549.
We now send stream creation and stream deletion events on
changing a user's role because a user can gain or lose
access to some streams on changing their role.
There was a bug in apply_event code where only a stream which
is not private is added to the "never_subscribed" data after
a stream creation event. Instead, it should be added to the
"never_subscribed" data irrespective of permission policy of
the stream as we already send stream creation events only to
those users who can access the stream. Due to the current
bug, private streams were not being added to "never_subscribed"
data in apply_event for admins as well. This commit fixes it
and also makes sure the "never_subscribed" list is sorted
which was not done before and was also a bug.
The bugs mentioned above were unnoticed as the tests did not
cover these cases and this commit also adds tests for those
cases.
Users who used to be subscribed to a private stream and have been
removed from it since retain the ability to edit messages/topics, and
delete messages that they used to have access to, if other relevant
organization permissions allow these actions. For example, a user may be
able to edit or delete their old messages they posted in such a private
stream. An administrator will be able to delete old messages (that they
had access to) from the private stream.
We fix this by fixing the logic in has_message_access (which lies at the
core of our message access checks - access_message() and
bulk_access_messages())
to not rely on only a UserMessage row for checking access but also
verify stream type and subscription status.
**Background**
User groups are expected to comply with the DAG constraint for the
many-to-many inter-group membership. The check for this constraint has
to be performed recursively so that we can find all direct and indirect
subgroups of the user group to be added.
This kind of check is vulnerable to phantom reads which is possible at
the default read committed isolation level because we cannot guarantee
that the check is still valid when we are adding the subgroups to the
user group.
**Solution**
To avoid having another transaction concurrently update one of the
to-be-subgroup after the recursive check is done, and before the subgroup
is added, we use SELECT FOR UPDATE to lock the user group rows.
The lock needs to be acquired before a group membership change is about
to occur before any check has been conducted.
Suppose that we are adding subgroup B to supergroup A, the locking protocol
is specified as follows:
1. Acquire a lock for B and all its direct and indirect subgroups.
2. Acquire a lock for A.
For the removal of user groups, we acquire a lock for the user group to
be removed with all its direct and indirect subgroups. This is the special
case A=B, which is still complaint with the protocol.
**Error handling**
We currently rely on Postgres' deadlock detection to abort transactions
and show an error for the users. In the future, we might need some
recovery mechanism or at least better error handling.
**Notes**
An important note is that we need to reuse the recursive CTE query that
finds the direct and indirect subgroups when applying the lock on the
rows. And the lock needs to be acquired the same way for the addition and
removal of direct subgroups.
User membership change (as opposed to user group membership) is not
affected. Read-only queries aren't either. The locks only protect
critical regions where the user group dependency graph might violate
the DAG constraint, where users are not participating.
**Testing**
We implement a transaction test case targeting some typical scenarios
when an internal server error is expected to happen (this means that the
user group view makes the correct decision to abort the transaction when
something goes wrong with locks).
To achieve this, we add a development view intended only for unit tests.
It has a global BARRIER that can be shared across threads, so that we
can synchronize them to consistently reproduce certain potential race
conditions prevented by the database locks.
The transaction test case lanuches pairs of threads initiating possibly
conflicting requests at the same time. The tests are set up such that exactly N
of them are expected to succeed with a certain error message (while we don't
know each one).
**Security notes**
get_recursive_subgroups_for_groups will no longer fetch user groups from
other realms. As a result, trying to add/remove a subgroup from another
realm results in a UserGroup not found error response.
We also implement subgroup-specific checks in has_user_group_access to
keep permission managing in a single place. Do note that the API
currently don't have a way to violate that check because we are only
checking the realm ID now.
Adds typing notification constants to the response given by
`POST /register`. Until now, these were hardcoded by clients
based on the documentation for implementing typing notifications
in the main endpoint description for `api/set-typing-status`.
This change also reflects updating the web-app frontend code
to use the new constants from the register response.
Co-authored-by: Samuel Kabuya <samuel.mwangikabuya@kibo.school>
Co-authored-by: Wilhelmina Asante <wilhelmina.asante@kibo.school>
Fixes#11767.
Previously multi-character emoji sequences weren't matched in the
emoji regex, so we'd convert the characters to separate images,
breaking the intended display.
This change allows us to match the full emoji sequence, and
therefore show the correct image.
There is no need to get realm for sender as ScheduledMessage
object also has realm field.
There is no direct benefit of this change but it is nice to
maintain the pattern which we want to follow in the code
in tests as well.
We can directly get the realm object from Message object now
and there is no need to get the realm object from "sender"
field of Message object.
After this change, we would not need to fetch "sender__realm"
field using "select_related" and instead only passing "realm"
to select_related when querying Message objects would be enough.
This commit also updates a couple of cases to directly access
realm ID from message object and not message.sender. Although
we have fetched sender object already, so accessing realm_id
from message directly or from message.sender should not matter,
but we can be consistent to directly get realm from Message
object whenever possible.
We do not set realm to Message objects defined for markdown tests
and this works because we currently access realm from sender object.
This commit changes the code to set realm in Message objects as
we would be accessing realm from Message object directly in further
commits.
We set stream_weekly_traffic field to "null" for Subscription
objects in zephyr mirror realm as we do not need stream traffic
data in zephyr mirror realm. This makes the subscription data
consistent with steams data.
This commit also udpates test to check never_subscribed data for
zephyr mirror realm.
Migrates existing ScheduledEmails for onboarding emails that have
either "zerver/emails/followup_day1" or "zerver/emails/followup_day2"
as the email template prefix to instead use the new template
prefixes "zerver/emails/account_registered" and
"zerver/emails/zulip_onboarding_topics".
The "followup_day2" email template name is not clear or descriptive
about the purpose of the email. Creates a duplicate of those email
template files with the template name "zulip_onboarding_topics".
Because any existing scheduled emails that use the "followup_day2"
templates will need to be updated before the current templates can
be removed, we don't do a simple file rename here.
The "followup_day1" email template name is not clear or descriptive
about the purpose of the email. Creates a duplicate of those email
template files with the template name "account_registered".
Because any existing scheduled emails that use the "followup_day1"
templates will need to be updated before the current templates can
be removed, we don't do a simple file rename here.
This migration applies under the assumption that extra_data_json has
been populated for all existing and coming audit log entries.
- This removes the manual conversions back and forth for extra_data
throughout the codebase including the orjson.loads(), orjson.dumps(),
and str() calls.
- The custom handler used for converting Decimal is removed since
DjangoJSONEncoder handles that for extra_data.
- We remove None-checks for extra_data because it is now no longer
nullable.
- Meanwhile, we want the bouncer to support processing RealmAuditLog entries for
remote servers before and after the JSONField migration on extra_data.
- Since now extra_data should always be a dict for the newer remote
server, which is now migrated, the test cases are updated to create
RealmAuditLog objects by passing a dict for extra_data before
sending over the analytics data. Note that while JSONField allows for
non-dict values, a proper remote server always passes a dict for
extra_data.
- We still test out the legacy extra_data format because not all
remote servers have migrated to use JSONField extra_data.
This verifies that support for extra_data being a string or None has not
been dropped.
Co-authored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This commit removes the private stream suscriptions of the bot if the
original owner is deactivated and we change the owner to the user who
is reactivating the bot. We unsusbcribe the bot from private streams
that the new owner is not subscribed to.
Fixes part of #21700.
We remove bot's subscriptions for private streams to which the
new owner is not subscribed and keep the ones to which the new
owner is subscribed on changing owner.
This commit also changes the code for sending subscription
remove events to use transaction.on_commit since we call
the function inside a transactopn in do_change_bot_owner and
this also requires some changes in tests in test_events.
Fixes the `/api/register-queue` endpoint documentation so that the
`realm_emoji` has the correct type, object that contains objects.
By correcting the API documentation, we also fix an error in the
test for the events system, which had been relying on the API
documentation having a list as a possible type for `realm_emoji`
in the register response.
Earlier, for topic wildcard mentions, the 'wildcard_mentioned'
flag was set for all the user-messages. (similar to stream wildcard
mention).
The flag should be set for the topic participants only.
The bug was introduced in 4c9d26c.
For topic wildcard mentions, the 'wildcard_mentioned' flag is set
for those user messages having 'user_profile_id' in
'topic_participant_user_ids', i.e. all topic participants.
Earlier, the flag was set if the 'user_profile_id' exists in
'all_topic_wildcard_mention_user_ids'.
'all_topic_wildcard_mention_user_ids' contains the ids of those
users who are topic participants and have enabled notifications
for '@topic' mentions.
The earlier approach was incorrect, as it would set the
'wildcard_mentioned' flag only for those topic participants
who have enabled the notifications for '@topic' mention instead
of setting the flag for all the topic participants.
The bug was introduced in 4c9d26c.
This commit addresses the issue where the topic highlighting
in search results was offset by one character when an
apostrophe was present. The problem stemmed from the disparity
in HTML escaping generated by the function `func.escape_html` which
is used to obtain `topic_matches` differs from the escaping performed
by the function `django.utils.html.escape` for apostrophes (').
func.escape_html | django.utils.html.escape
-----------------+--------------------------
' | '
To fix this SQL query is changed to return the HTML-escaped
topic name generated by the function `func.escape_html`.
Fixes: #25633.
Currently, we are displaying the "Complete the organization profile"
banner immediately after the organization was created. It's important to
strongly encourage orgs to configure their profile, so we should delay
showing the banner if the profile has not been configured after 15 days.
Thus also allows the users to check out Zulip and see how it works before
configuring the organization settings.
Fixes: #24122.
Previoulsy, test_openapi_arguments had assumed that an endpoint
not using rest_dispatch used the GET method for the request. This
was not the case for the "/fetch_api_key" and "/dev_fetch_api_key"
endpoints, which is why those endpoints were marked as pending
even though they were documented in `zerver/openapi/zulip.yaml`.
Updates test_openapi_arguments to check a set of endpoints that
are documented and don't use the GET method so that these endpoints
can be tested and removed from the pending_endpoints set.
This adds API support to reorder linkifiers and makes sure that the
returned lists of linkifiers from `GET /events`, `POST /register`, and
`GET /realm/linkifiers` are always sorted with the order that they
should processed when rendering linkifiers.
We set the new `order` field to the ID with the migration. This
preserves the order of the existing linkifiers.
New linkifiers added will always be ordered the last. When reordering,
the `order` field of all linkifiers in the same realm is updated, in
a manner similar to how we implement ordering for
`custom_profile_fields`.
The curl examples of reordering linkifiers require there to be some
linkifiers in the database to be reordered. This adjusts some test cases
so they do not assume that there is no linkifier in the test db.
Each unittest subTest can fail without interrupting the other subTests.
By wrapping the test for each view function, we can get all validation
errors at once, which can be useful if multiple endpoints are updated.
More importantly, if the test fails anywhere inside test_openapi but
before the formatted output is printed, we will not lose the information
of which view function fails the validation. Because we attach the name
of the function to the subTest:
```
FAIL: test_openapi_arguments (zerver.tests.test_openapi.OpenAPIArgumentsTest) [zerver.views.alert_words.add_alert_words]
```
We have historically cached two types of values
on a per-request basis inside of memory:
* linkifiers
* display recipients
Both of these caches were hand-written, and they
both actually cache values that are also in memcached,
so the per-request cache essentially only saves us
from a few memcached hits.
I think the linkifier per-request cache is a necessary
evil. It's an important part of message rendering, and
it's not super easy to structure the code to just get
a single value up front and pass it down the stack.
I'm not so sure we even need the display recipient
per-request cache any more, as we are generally pretty
smart now about hydrating recipient data in terms of
how the code is organized. But I haven't done thorough
research on that hypotheseis.
Fortunately, it's not rocket science to just write
a glorified memoize decorator and tie it into key
places in the code:
* middleware
* tests (e.g. asserting db counts)
* queue processors
That's what I did in this commit.
This commit definitely reduces the amount of code
to maintain. I think it also gets us closer to
possibly phasing out this whole technique, but that
effort is beyond the scope of this PR. We could
add some instrumentation to the decorator to see
how often we get a non-trivial number of saved
round trips to memcached.
Note that when we flush linkifiers, we just use
a big hammer and flush the entire per-request
cache for linkifiers, since there is only ever
one realm in the cache.
The get_display_recipient helper is a clumsy way to get
stream names, and it's not even representative of how
most of our code retrieves stream names.
The new helper also double-checks that the Stream
object has the correct recipient id.
This commit renames the keyword 'pm' to 'dm' in the
'pm_mention_email_disabled_user_ids' and
'pm_mention_push_disabled_user_ids' attributes of the
'RecipientInfoResult' dataclass.
'pm' and 'dm' are the acronyms for 'private message' and
'direct message' respectively.
It includes 'TODO/compatibility' code to support the old format
fields in the tornado queues during the Zulip server upgrades.
This commit renames the 'PRIVATE_MESSAGE' attribute of the
'NotificationTriggers' class to 'DIRECT_MESSAGE'.
Custom migration to update the existing value in the database.
It includes 'TODO/compatibility' code to support the old
notification trigger value 'private_message' in the
push notification queue during the Zulip server upgrades.
Earlier 'private_message' was one of the possible values for the
'trigger' property of the '[`POST /zulip-outgoing-webhook`]' response;
Update the docs to reflect the change in the above-mentioned trigger
value.
This commit adds code to pass all the required arguments to
select_related call for Message objects such that only the
required related fields are fetched from the database.
Previously, we did not pass any arguments to select_related,
so all the directly and indirectly related fields were fetched
when many of them were actually not being used and made the
query unnecessarily complex.
This commit adds code to pass all the required arguments to
select_related call for MissedMessageEmailAddress such that
only the required related fields are fetched from the database.
Previously, we did not pass any arguments to select_related,
so all the directly and indirectly related fields were fetched
when many of them were actually not being used and made the
query unnecessarily complex.
This commit updates the code to pass "realm" and "recipient" as
arguments to select_related call in get_stream_by_id_in_realm.
Previously, since there was no arguments, it fetched
can_remove_subscribers_group and the related fields of
"Realm" model as well which were not being used, but
did not fetch "recipient" as it is a nullable field.
This commit removes get_huddle_recipient function and we now use
get_or_create_huddle in get_recipient_from_user_profiles.
As a result of this change, we do not fetch the recipient from
Huddle object but instead get it using the "id" and "recipient_id"
fields available from Huddle object like we do for a personal
message. This change allows us to not fetch recipient object
using select_related when querying the Huddle object.
The set of objects in the `users` object can be very large (in some
cases, literally every object in the database) and making them into a
giant `id in (...)` to handle the one tiny corner case which we never
use is silly.
Switch the `--users` codepath to returning a QuerySet as well, so it
can be composed. We pass a QuerySet into send_custom_email as well,
so it can ensure that the realm is `select_related` in as well, no
matter how the QuerySet was generated.
Substituting the rendered body via Jinja2 means that it cannot
perform any interpolation itself. While the string replacement is
hacky, it is the only solution which avoids running Jinja2 more than
once, and also allows the user-supplied content to have Jinja2
substitutions in it.
In this commit, we introduce a new option in the stream creation
UI - a 'Default stream for new users' checkbox. By default, the
checkbox is set to 'off' and is only visible to admins. This
allow admins to easily designate a stream as the default stream
for new users during stream creation.
Fixes#24048.
This commit adds a 'Default stream for new users' checkbox in
the stream editing UI to allow admins to easily add or remove
a stream as the default stream for new users. Previously, this
functionality required navigating to separate menu.
Fixes a part of #24048.
Adds a test for when a value for a user's custom profile field is
removed and not set to a new value. The omission of this event in
the tests was noted as a possibility in #22103, which updated the
API documentation for these events having `null` for the field
value.
When adding the test discovered that the events logic was not
deleting the field from the user object and instead setting it to
`None`, so fixes that logic as well. There was a similar bug fixed
in commit 96c61a1a41 for when custom profile fields are removed
from a realm.
This commit explicitly sets the following user settings:
* 'enable_followed_topic_email_notifications'
* 'enable_followed_topic_push_notifications'
to True.
Collectively, this improves the readability of the test and
the following two tests.
In 'test_copy_default_settings_from_another_user', we verify that
'cordelia' and 'iago' have the same values for their user settings,
but 'hamlet' has the defaults.
Earlier, we explicitly set the 'color_scheme' setting for 'hamlet' as
'UserProfile.COLOR_SCHEME_NIGHT', which is not needed.
As we verify, 'hamlet' should have the defaults.
So just verifying if the 'color_scheme' setting for 'hamlet' is
'UserProfile.COLOR_SCHEME_AUTOMATIC' (default) fulfils our purpose.
The extra line of code was introduced in b10f156.
This commit adds code to pass stream traffic data using
the "stream_weekly_traffic" field in stream objects.
We already include the traffic data in Subscription objects,
but the traffic data does not depend on the user to stream
relationship and is stream-only information, so it's better
to include it in Stream objects. We may remove the traffic
data and other stream information fields for Subscription
objects in future.
This will help clients to correctly display the stream
traffic data in case where client receives a stream
creation event and no subscription event, for an already
existing stream which the user did not have access to before.
Previously, this code:
```python3
old_archived_attachments = ArchivedAttachment.objects.annotate(
has_other_messages=Exists(
Attachment.objects.filter(id=OuterRef("id"))
.exclude(messages=None)
.exclude(scheduled_messages=None)
)
).filter(messages=None, create_time__lt=delta_weeks_ago, has_other_messages=False)
```
...protected from removal any ArchivedAttachment objects where there
was an Attachment which had _both_ a message _and_ a scheduled
message, instead of _either_ a message _or_ a scheduled message.
Since files are removed from disk when the ArchivedAttachment rows are
deleted, this meant that if an upload was referenced in two messages,
and one was deleted, the file was permanently deleted when the
ArchivedMessage and ArchivedAttachment were cleaned up, despite being
still referenced in live Messages and Attachments.
Switch from `.exclude(messages=None).exclude(scheduled_messages=None)`
to `.exclude(messages=None, scheduled_messages=None)` which "OR"s
those conditions appropriately.
Pull the relevant test into its own file, and expand it significantly
to cover this, and other, corner cases.
The most expensive thing for adding user groups is sending
all the notification messages, but we at least want to make
sure that the basic stuff runs in constant time.
The cross-realm bots rarely change, and there are only
a few of them, so we just query them all at once and
put them in the cache.
Also, we put the dictionaries in the cache, instead of
the user objects, since there is nothing time-sensitive
about the dictionaries, and they are small. This saves
us a little time computing the avatar url and things
like that, not to mention marshalling costs.
This commit also fixes a theoretical bug where we would
have stale cache entries if somebody somehow modified
the cross-realm bots without bumping KEY_PREFIX.
Internally we no longer pre-fetch the realm objects for
the bots, but we don't get overly precise about picking
individual fields from UserProfile, since we rarely hit
the database and since we don't store raw ORM objects
in the cache.
The test diffs make it look like we are hitting the
cache an extra time, but the tests weren't counting
bulk fetches. Now we only use a single key for all
bots rather a key per bot.
The bulk_get_users() function was only being used to
get cross-realm bots.
It appears that it was introduced in
f02e5b90f6 for that
specific use case.
Now we make the function more specific and test it more
accurately.
We also eliminate a lot of janky code and comments,
including some code that never had test coverage.
Incidentally, it appears that we did not have any code
to invalidate the cache keys here, and that is still
the case. In practice I assume people rarely
re-configure their cross-realm bots unless they are
upgrading the server, and then KEY_PREFIX comes into
play. 25fd4c5508 seems
to have caused that hopefully harmless regression.
A further step will be to make this cache more coarse,
since there are only a few cross-realm bots. The next
commit will hopefully simplify the code and address the
validation pitfall.
Earlier the API endpoints related to user_group accepts and returns a
field `can_mention_group_id` which represents the ID
of user_group whose members can mention the group.
This commit renames this field to `can_mention_group`.
Earlier the API endpoints related to streams accepts and returns a
field `can_remove_subscribers_group_id` which represents the ID
of user_group whose members can remove subscribers from stream.
This commit renames this field to `can_remove_subscribers_group`.
Previously, the view function was responsible for doing a first pass of
the validations done for RealmPlayground. It is no longer true now. This
refactors do_add_realm_playground to check_add_realm_playground and make
it responsible for validating the playground fields and doing error
handling for the ValidationError raised.
Dropping support for url_prefix for RealmPlayground, the server now uses
url_template instead only for playground creation, retrieval and audit
logging upon removal.
This does the necessary handling so that url_template is expanded with
the extracted code.
Fixes#25723.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This commit removes the stray strings used to refer to
various types of notification triggers.
We use the attributes of the 'NotificationTriggers' class instead.
We populate url_template by simply escaping "{" and "}" as well as
appending "{code}" to the end of the legacy url_prefix.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
As an intermediate step before we fully support url_template for realm
playgrounds, we populate url_template in the backend ensuring that all
the new entries will be validated. With a later backfilling migration,
we prepare the database such that all the records will have a valid URL
template.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Having a more precise type annotation helps with ensuring the migration
to use URL templates gets type checked.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Updates the realm field `default_code_block_language` to have a default
value of an empty string instead of None. Also updates the web-app to
check for the empty string and not `null` to indicate no default is set.
This means that both new realms and existing realms that have no default
set will have the same value for this setting: an empty string.
Previously, new realms would have None if no default was set, while realms
that had set and then unset a value for this field would have an empty
string when no default was set.
Expands support for the message ID operand for id" operator to be either
a string or an integer. Previously, this operand was always validated as
a string.
Restore the default django.utils.log.AdminEmailHandler when
ERROR_REPORTING is enabled. Those with more sophisticated needs can
turn it off and use Sentry or a Sentry-compatible system.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit updates the select_related calls in queries to get
UserProfile objects in get_user, get_user_by_delivery_email,
get_user_profile_by_id, get_user_profile_by_id_in_realm and
get_user_profile_by_api_key functions to pass "realm" and
"bot_owner" as arguments to select_related call.
These functions are used in different parts of code to get
the UserProfile object and realm is accessed using the user
object at many places.
"bot_owner" field is also used in some places like to check
whether a bot can access a stream, to check whether a user
can change modify another user, in webhooks code to send the
message to the bot owner, and in tests as well. There can be
some places where the bot owner is not required and in most
such cases the code would only be accessed for human users,
which means the bot_owner will be null for these cases and
would avoid complexity and performance issues.
Note that previously, no arguments were passed to select_related
and thus only realm field was fetched during the query.
This code removes a lot of complexity with very likely
positive overall impact on system performance and
negligible downside.
We already cache display recipients on a per-user
level, so there's no need for another cache layer on
top of that that keys them with recipient ids.
We avoid strange things where Alice/Bob and Bob/Charlie
get put into the top layer cache and then we still have
a cache miss on Alice/Charlie despite the lower level
cache being able to support per-user lookups.
This change does introduce an extra database round trip
if any of our messages have a huddle, but the query is
extremely cheap, and we can always try to cache that
function more directly or try to re-use some of our
other huddle-based caches.
As part of this, we clean up the names for the
lower-level per-user cache of display recipients, and
we simplify the cache keys.
We also stop passing in a full Recipient object to the
`bulk_get_huddle_user_ids` functions.
The local impact of this change should be easy to
measure (at least approximately), since we use this
function every time a user gets messages via the
/messages endpoint.
We restrict the columns, avoid quadratic looping,
and don't bother with order_by.
We also return the user ids (per recipient) as
sets, since that's how the only caller uses the
info (albeit implicitly via set.union accepting
a list).
The long-term idle topic participants are soft-reactivated
after email/push notifications are sent due to @topic mention.
The reason being that, generally, @topic mentions are going to
reach a small set of users who have a decent chance of being
reactivated by the notifications.
This commit completes the notifications part of the @topic
wildcard mention feature.
Notifications are sent to the topic participants for the
@topic wildcard mention.
The previous function was poorly named, asked for a
Realm object when realm_id sufficed, and returned a
tuple of strings that had different semantics.
I also avoid calling it duplicate times in a couple
places, although it was probably rarely the case that
both invocations actually happened if upstream
validations were working.
Note that there is a TypedDict called EmojiInfo, so I
chose EmojiData here. Perhaps a better name would be
TinyEmojiData or something.
I also simplify the reaction tests with a verify
helper.
The active realm emoji are just a subset of all your
realm emoji, so just use a single cache entry per
realm.
Cache misses should be very infrequent per realm.
If a realm has lots of deactivated realm emoji, then
there's a minor expense to deserialize them, but that
is gonna be dwarfed by all the other more expensive
operations in message-send.
I also renamed the two related functions. I erred on
the side of using somewhat verbose names, as we don't
want folks to confuse the two use cases. Fortunately
there are somewhat natural affordances to use one or
the other, and mypy helps too.
Finally, I use realm_id instead of realm in places
where we don't need the full Realm object.
This migration is reasonably complex because of various anomalies in existing
data.
Note that there are cases when extra_data does not contain data that is
proper json with possibly single quotes. Thus we need to use
"ast.literal_eval" to cover that.
There is also a special case for "event_type == USER_FULL_NAME_CHANGED",
where extra_data is a plain str. This event_type is only used for
RealmAuditLog, so the zilencer migration script does not need to handle
it.
The migration does not handle "event_type == REALM_DISCOUNT_CHANGED"
because ast.literal_eval only allow Python literals. We expect the admin
to populate the jsonified extra_data for extra_data_json manually
beforehand.
This chunks the backfilling migration to reduce potential block time.
The migration for zilencer is mostly similar to the one for zerver; except that
the backfill helper is added in a wrapper and unrelated events are
removed.
**Logging and error recovery**
We print out a warning when the extra_data_json field of an entry
would have been overwritten by a value inconsistent with what we derived
from extra_data. Usually this only happens when the extra_data was
corrupted before this migration. This prevents data loss by backing up
possibly corrupted data in extra_data_json with the keys
"inconsistent_old_extra_data" and "inconsistent_old_extra_data_json".
More roundtrips to the database are needed for inconsistent data, which are
expected to be infrequent.
This also outputs messages when there are audit log entries with decimals,
indicating that such entries are not backfilled. Do note that audit log
entries with decimals are not populated with "inconsistent_old_extra_data_*"
in the JSONField, because they are not overwritten.
For such audit log entries with "extra_data_json" marked as inconsistent,
we skip them in the migration. Because when we have discovered anomalies in a
previous run, there is no need to overwrite them again nesting the extra keys
we added to it.
**Testing**
We create a migration test case utilizing the property of bulk_create
that it doesn't call our modified save method.
We extend ZulipTestCase to support verifying console output at the test
case level. The implementation is crude but the use case should be rare
enough that we don't need it to be too elaborate.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We did not send the stream creation events when subscribing
guests to public streams while we do send them when subscribing
non-admin users to private streams.
This commit adds code to send the stream creation events when
subscribing guests to public streams, so the clients can know
that the stream exists and fixes the bug where client tries
to process a subscription add event for a stream which it does
not know about.
This tracks user group membership changes when the realm is first set
up, either through an import or not. This happens when we add users to
the system user groups by their roles.
For an imported realm, we do extra handling when the data doesn't include
user groups. This gets audited as well.
The MissedMessage queue worker is the single callsite of
`handle_missedmessage_emails`, which immediately transforms the list
of events into a dict keyed by message-id.
Skip the intermediate list step, and use defaultdict and a dataclass
to simplify and make explicit the pieces. This removes the unused
user_profile_id and message_id pieces of the data structure.
This commit adds a boolean field `mentions_topic_wildcard`
to the `MessageRenderingResult` dataclass.
The field is set to true only if message rendering determines
the message has an actual topic wildcard mention in it (and not,
e.g., topic wildcard mention syntax inside a code block).
The rendered content for topic wildcard mention is
'<span class="topic-mention">{wildcard}</span>'.
The 'topic-mention' class is the identifier for the wildcard
mention being a topic wildcard mention.
We don't use 'data-user-id="*"' and "user-mention" class for
topic wildcard mentions and eventually plan to remove them for
stream wildcard mentions too in a separate mini-project.
This prep commit merges separate tests for '**@all**',
'**@stream**' and '**@everyone**' stream wildcard mentions
into a single test named 'test_mention_stream_wildcard'.
Similarly, it merges separate tests for '@all', '@stream',
and '@everyone' stream wildcard mentions into a single test
named 'test_mention_at_stream_wildcard'.
The aim is to finally have two separate tests for stream and
topic wildcard mentions (when we introduce topic wildcards)
instead of having separate tests for each mention text
(i.e. all, everyone, stream, topic).
This commit adds the 'topic_wildcard_mention_user_ids' and
'topic_wildcard_mention_in_followed_topic_user_ids'
attributes to the 'RecipientInfoResult' dataclass.
Only topic participants are notified of @topic mentions.
Topic participants are anyone who sent a message to a topic
or reacted to a message on the topic.
'topic_wildcard_mention_in_followed_topic_user_ids' stores the
ids of the topic participants who follow the topic and have
enabled the wildcard mention notifications for followed topics.
'topic_wildcard_mention_user_ids' stores the ids of the topic
participants for whom 'user_allows_notifications_in_StreamTopic'
with setting 'wildcard_mentions_notify' returns True.
This commit adds a 'has_topic_wildcards' instance variable
to the 'MentionData' class for the detection of
- possible topic wildcards mentions.
Fixes part of #22829.
Co-authored-by: Prakhar Pratyush <prakhar841301@gmail.com>
Co-authored-by: orientor <aditya.verma@students.iiit.ac.in>
This commit updates the existing tests in 'test_email_notifications'
and 'test_push_notifications' to properly configure user settings
and visibility policies before running the actual tests.
Earlier, the tests were passing, but the corner case expected
to be covered wasn't covered.
This should have been included in
d80779435a.
These comments should not have been included in
a8fd9eb701.
We covered the case "Private message should soft reactivate
the user" earlier in the test. So the comment was rightly added
there.
During stream wildcard or group mention, no such personal mention
is involved; hence, the comments are not needed.
This is a prep commit to replace 'wildcard' with 'stream_wildcard'.
This wasn't included in 179d5cb because we didn't decide to
use a different rendered_content for topic wildcard mention,
i.e., ''<span class="user-mention topic-mention">{wildcard}</span>'.
Our intention was not to create separate tests for both stream
and topic wildcard mentions, as they were expected to have the
same rendered content format.
This includes changing the URL to #settings/preferences, with a
transparent redirect so that existing links, like the one from Welcome
Bot, continue to work.
Pass the HttpRequest explicitly through the two webhooks that log to
the webhook loggers.
get_current_request is now unused, so remove it (in the same commit
for test coverage reasons).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The initial followup_day1 email confirms that the new user account
has been successfully created and should be sent to the user
independently of an organization's setting for send_welcome_emails.
Here we separate out the followup_day1 email into a separate function
from enqueue_welcome_emails and create a helper function for setting
the shared welcome email sender information.
The followup_day1 email is still a scheduled email so that the initial
account creation and log-in process for the user remains unchanged.
Fixes#25268.
The followup_day2 email is scheduled with a delay as a welcome email
and is therefore more likely to exist as a scheduled email in these
deactivation cases.
Updates comment to not include the number of emails generated so
that it doesn't need to be updated every time a new email is added.
The current count in the comment is already out-of-date.
Because the third party might not be expecting a 400 from our
webhooks, we now instead use 200 status code for unknown events,
while sending back the error to Sentry. Because it is no longer an error
response, the response type should now be "success".
Fixes#24721.
This commit removes "@" from name of role-based system groups
since we have added a restricion on having user group names
starting with "@" in the previous commit as they look odd in
mention syntax.
We also add a migration in this commit to update the name of
role-based system groups in existing realms to remove "@"
from the name. This migration also updates the names of
non-system user groups by removing the invalid prefixes
from their names and if there is a group already with that
name, we insted name the group as "group:{group_id}".
Fixes#26148.
We do not allow user group names to start with "@", "role:",
"user:", "stream:" and "channel:".
Group names starting with "@" look odd in mentions and
"role:", "user:" and "stream:" prefixes are reserved for
system groups which will be used in the new groups-based
permission model. We do not allow "channel:" prefix for
now just to be safe in a case where we use it instead of
"stream:" prefix for stream based groups in future.
Fixes part of #26148.
Previously we had database level restriction on length of
user group names. Now we add the same restriction to API
level as well, so we can return a better error response.
We remove the cache functionality for the
get_realm_stream function, and we also change it to
return a thin Stream object (instead of calling
select_related with no arguments).
The main goal here is to remove code complexity, as we
have been prone to at least one caching validation bug
related to how Realm and UserGroup interact. That
particular bug was more theoretical than practical in
terms of its impact, to be clear.
Even if we were to be perfectly disciplined about only
caching thin stream objects and always making sure to
delete cache entries when stream data changed, we would
still be prone to ugly situations like having
transactions get rolled back before we delete the cache
entry. The do_deactivate_stream is a perfect example of
where we have to consider the best time to unset the
cache. If you unset it too early, then you are prone to
races where somebody else churns the cache right before
you update the database. If you set it too late, then
you can have an invalid entry after a rollback or
deadlock situation. If you just eliminate the cache as
a moving part, that whole debate is moot.
As the lack of test changes here indicates, we rarely
fetch streams by name any more in critical sections of
our code.
The one place where we fetch by name is in loading the
home page, but that is **only** when you specify a
stream name. And, of course, that only causes about an
extra millisecond of time.
This changes bulk_get_streams so that it just uses the
database all the time. Also, we avoid calling
select_related(), so that we just get back thin and
tidy Stream objects with simple queries.
About not caching any more:
It's actually pretty rare that we fetch streams by name
in the main application. It's usually API requests that
send in stream names to find more info about streams.
It also turns out that for large queries (>= ~30 rows
for my measurements) it's more efficent to hit the
database than memcached. The database is super fast at
scale; it's just the startup cost of having Django
construct the query, and then having the database do
query planning or whatever, that slows us down. I don't
know the exact bottleneck, but you can clearly measure
that one-row queries are slow (on the order of a full
millisecond or so) but the marginal cost of additional
rows is minimal assuming you have a decent index (20
microseconds per row on my droplet).
All the query-count changes in the tests revolve around
unsubscribing somebody from a stream, and that's a
particularly odd use case for bulk_get_streams, since
you generally unsubscribe from a single stream at a
time. If there are some use cases where you do want to
unsubscribe from multiple streams, we should move
toward passing in stream ids, at least from the
application. And even if we don't do that, our cost for
most queries is a couple milliseconds.
This add audit log entries when any group based setting of a user group
is updated. We store both the old and new values in extra_data, along
with the name of that setting. Entries populated during user group creation
are hardcoded to track "can_mention_group".
Potentially we can adjust "set_defaults_for_group_settings" so that it
populates realm audit logs with it, but that is out of scope for this change.
We use an atomic transaction so that the audit logs are committed
together with the updates.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This add audit log entries when the name or description of a user group
is updated. We store both the old and new values in extra_data. We wrap
the functions inside an atomic transaction so that the audit logs and
the updates are committed together.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This is mostly the same as tracking subgroup changes, except that now
modified_user_group is the subgroup.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
It's worth noting that instead of adding another field to the
RealmAuditLog model, we store the modified subgroup ids in extra_data as
a JSON encoded dict with the key "subgroup_ids". We don't create audit
log entries for supergroup changes at this point.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This also add audit log entries during user creation and role change,
because we modify system group memberships there.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
We also create RealmAuditLog entries for the initial memberships that
get added along with the creation of a UserGroup. System user groups are
not created with members so no audit logs are populated for that.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Various cleanups:
* clean up comments
* improve names for constants and variables
* express first ORM query as a single statement
* use set differences to simplify logic
* avoid all the reversing churn
* avoid early-exit idiom since this function is so small
Note that it's plausible that we should just combine the two
queries and let the database exclude the already-used ids,
but that felt a little risky for now. As I mentioned on
Zulip, I think the one-week window has dubious value, but
I am biased by having wasted time chasing down a test
flake related to the time window.
We extract code from process_new_human_user with
no modifications.
This has all the best outcomes of extracting a function:
* better profile info
* easier to test for query counts (signup gets real noisy)
* simplifies a long, messy function
It has no real drawbacks, since the helper function doesn't need
to pass back any intermediate state to the parent for the rest
of what the parent does.
When you profile test_signup and test_invite, with a decent
sample size, the set_up_streams_for_new_human_user function
does about 20% of the work for process_new_human_user, which
is a lot considering that most tests don't create a ton of
pre-registered or default streams.
At least as measured by test_events.py, which has over 1000
calls to fetch initial data for page loads, this should
be about a 10% improvement in how much time the server
spends fetching data.
We mostly avoid a select_related() query that did this nastiness:
INNER JOIN "zerver_realm" ON ("zerver_stream"."realm_id" = "zerver_realm"."id")
INNER JOIN "zerver_usergroup" ON ("zerver_stream"."can_remove_subscribers_group_id" = "zerver_usergroup"."id")
INNER JOIN "zerver_realm" T4 ON ("zerver_usergroup"."realm_id" = T4."id")
INNER JOIN "zerver_usergroup" T5 ON ("zerver_usergroup"."can_mention_group_id" = T5."id")
INNER JOIN "zerver_realm" T6 ON (T5."realm_id" = T6."id")
INNER JOIN "zerver_usergroup" T7 ON (T5."can_mention_group_id" = T7."id")
INNER JOIN "zerver_realm" T8 ON (T7."realm_id" = T8."id")
INNER JOIN "zerver_usergroup" T9 ON (T7."can_mention_group_id" = T9."id")
INNER JOIN "zerver_realm" T10 ON (T9."realm_id" = T10."id")
INNER JOIN "zerver_usergroup" T11 ON (T9."can_mention_group_id" = T11."id")
WHERE "zerver_stream"."id" IN (SELECT U0."stream_id" FROM "zerver_defaultstream" U0 WHERE U0."realm_id" = 2
Future commits will address the codepath for creating users.
I created zerver/lib/default_streams.py, so that various
views and events.py don't have to awkwardly reach into
an "actions" file.
I copied over two functions verbatim from actions/default_streams.py:
get_default_streams_for_realm
streams_to_dicts_sorted
The latter only remains as an internal detail in the new library.
I also created two new helpers:
get_default_stream_ids_for_realm:
This is both faster and easier to use in all the places
where we only need to get a set of default stream ids.
get_default_streams_for_realm_as_dicts:
This just wraps the prior calls to
streams_to_dicts_sorted(get_default_streams_for_realm(...)),
and it doesn't yet address the slowness of the underlying
code.
All the "real" code should be functionally the same.
In a few tests I now use this wrapper instead of
calling get_default_streams_for_realm, just to get
slightly deeper coverage.
Updates find_proper_insertion_index to check for the inline image
classes as matching at least one of the classes in the element's
attrib["class"] so that cases where an inline preview image has
multiple classes, like YouTube video previews, will have the
correct insertion index.
Fixes#26186.
Added an additional test case to `test_submessages.py` for testing the
message object containing `submessages` meta data.
Previous to this commit we were never validating the `submessage` schema
in the `message` objects.
Fixes#25896.
It replaces the "File not found." text with:
"This file does not exist or has been deleted."
At present when a file is deleted it results in a confusing
experience when looking at the "File not found." message.
In order to clarify the situation is not a bug, the message
has been replaced with a better alternative.
Fixes part of Issue #23739.
This prep commit replaces the 'wildcard' keyword in the codebase
with 'stream_wildcard' at some places for better readability, as
we plan to introduce 'topic_wildcards' as a part of the
'@topic mention' project.
Currently, 'wildcards = ["all", "everyone", "stream"]' which is an
alias to mention everyone in the stream, hence better renamed as
'stream_wildcards'.
Eventually, we will have:
'stream_wildcard' as an alias to mention everyone in the stream.
'topic_wildcard' as an alias to mention everyone in the topic.
'wildcard' refers to 'stream_wildcard' and 'topic_wildcard' as a whole.
The 'get_gcm_alert' and 'get_apns_alert_subtitle' functions
don't include the case when the trigger is
'NotificationTriggers.FOLLOWED_TOPIC_WILDCARD_MENTION'.
This commit updates the functions to include
'NotificationTriggers.FOLLOWED_TOPIC_WILDCARD_MENTION'.
The emails sent for missed messages have a text at the bottom
explaining the reason why the email was sent.
This commit reorders the conditional statements in the email
template to align with the trigger priority order defined
in the 'get_email_notification_trigger'.
It takes about 31ms per page on my box, but 191
help pages adds up quickly. I am not sure how to
optimize this test, but it will be a good litmus
test for a future better markdown processor.
This did not speed up the tests as much as I expected,
but it certainly makes the code easier to read, and
Tim is pretty confident that the zephyr logic is
fairly stable, so it's sufficient to test it on a
subset of representative urls.
dbe930394f changed the
"missing string" from "Log in" to "xyz" for some
unknown reason. The current code makes no sense.
Also, even the original test code here had the common
pitfall of only testing one side of the condition.
Presumably if you are testing that a certain string
is missing in a landing-page scenario, then you also
want to check that it **does** exist in other
scenarios. Otherwise, the flag would have been
named something more generic. Of course, I am mostly
guessing due to lack of comments.
If there is some test logic here that we need to
resurrect, then we should just write a custom test
for the /hello page rather than crufting up
all our helpers.
This removes some confusing default boolean flags, and
it checks both sides of the do-you-want-to-allow-robots
condition, so it's more thorough.
For the two strange exceptions to the normal policy,
I now handle them together in the helper function with
a comment.
I also disentangle the logic to look for og tags from
the robot logic, and this should also lead to more
thorough testing.
The prior name was just strange. This test could really
use a better comment explaining its purpose.
Also, presumably these pages don't always get 404s, so
we should really have the test exercise both conditions.
This makes us correctly run landing page logic where we
didn't before, and, more importantly, lets us skip landing
page logic where we had been erroneously running it.
This speeds up my runs from 35s to 25s.
This commit updates the text on email confirmation page to
make it more clear what's going on and why the user needs
to check their email.
Fixes#25900.
This commit adds backend code to check whether a user is allowed
to mention a user group while editing a message as per
can_mention_group setting of that group.
Fixes a part of #25927.
This commit adds backend code to check whether user has permission
to mention a group while sending message as per the can_mention_group
setting of the group.
Fixes a part of #25927.
We eliminate 220 zephyr-related checks that are all fairly
expensive.
On my machine this test went from 46s to 23s.
Note that we still get coverage of the zephyr codepath
from other tests.
(All the same code gets executed here, but in a slightly
different order.)
There is some code duplication between the two new
helper functions, but I didn't make the situation any
worse, and it's slightly non-trivial to consolidate
the logic. Hopefully the long term strategy is to remove
the zephyr checks or at least isolate a single test for
any specific zephyr quirks that we need to maintain.
This is a first step toward two goals:
* support dictionary-like narrows when registering events
* use readable dataclasses internally
This is gonna be a somewhat complicated exercise due to how
events get serialized, but fortunately this interim step
doesn't require any serious shims, so it improves the codebase
even if the long-term goals may take a while to get sorted
out.
The two places where we have to use a helper to convert narrows
from tuples to dataclasses will eventually rely on their callers
to do the conversion, but I don't want to re-work the entire
codepath yet.
Note that the new NarrowTerm dataclass makes it more explicit
that the internal functions currently either don't care about
negated flags or downright don't support them. This way mypy
protects us from assuming that we can just add negated support
at the outer edges.
OTOH I do make a tiny effort here to slightly restructure
narrow_filter in a way that paves the way for negation support.
The bigger goal by far, though, is to at least support the
dictionary format.
In 2484d870b4 I created tests
using a fixture called narrow.json. I believe my intention
was to eventually use the fixture for similar tests on the
frontend, but that never happened.
Almost seven years later, I think it's time to just use
straightforward code in Python to test build_narrow_filter.
In particular, we want to move to dataclasses, so that would
create an addition nuisance for fixture-based tests. The
fixture was already annoying in terms of being an extra moving
part, being hard to read, and not being type-safe.
In order to avoid typos, I mostly code-generated the new
Python code by instrumenting the old test:
narrow_filter = build_narrow_filter(narrow)
+ print("###\n")
+ print(f"narrow_filter = build_narrow_filter({narrow})\n")
for e in accept_events:
message = e["message"]
flags = e["flags"]
@@ -610,6 +612,8 @@ class NarrowLibraryTest(ZulipTestCase):
if flags is None:
flags = []
self.assertTrue(narrow_filter(message=message, flags=flags))
+ print(f"self.assertTrue(narrow_filter(message={message}, flags={flags},))")
+ print()
for e in reject_events:
message = e["message"]
flags = e["flags"]
@@ -618,6 +622,8 @@ class NarrowLibraryTest(ZulipTestCase):
if flags is None:
flags = []
self.assertFalse(narrow_filter(message=message, flags=flags))
+ print(f"self.assertFalse(narrow_filter(message={message}, flags={flags},))")
+ print()
I then basically pasted the output in and ran black to format it.
We no longer pass in a big opaque event to narrow_filter
(which is inside build_narrow_filter). We instead explicitly
pass in message and flags. This leads to a bit more type
safety, and it's also more flexible. There's no reason to
build an entire event just to see if a message belongs to
a narrow.
The changes to the test work around the fact that the fixtures
are sloppy with types. I plan a subsequent commit to clean
up those tests significantly.
Subsequent commits will add "on_delete=models.RESTRICT"
relationships, which will result in the Attachment
objects being deleted after Realm has been deleted from
the database.
In order to handle this, we update
get_realm_used_upload_space_cache_key function to accept
realm_id as parameter instead of realm object, so that
the code for flushing the cache works even after the
realm is deleted. This change is fine because eventually
only realm_id is used by this function and there is no
need of the complete realm object.
This commit changes the code in test_user_groups.py to use
check_add_user_group function to create user groups instead
of directly using django ORM to make sure that settings
would be set to the correct defaults in further commits.
For tests that use the dev server, like test-api, test-js-with-puppeteer,
we don't have the consumers for the queues. As they eventually timeout,
we get unnecessary error messages. This adds a new flag, disable_timeout,
to disable this behavior for the test cases.
This endpoint was previously marked as `intentionally_undocumented`
but that was mistake.
Removed `intentionally_undocumented` and added proper documentation
with valid `python_example` for this Endpoint.
Fixes: #24084
This verifies that updates of the user group name/description are
correctly done by doing additional queries. This also empathsizes on
checking that the state before and after API calls are indeed different.
We extract the checks needed for user membership changes into a method,
verifying that the members of the user group are matching the expected
values exactly.
Adds testing coverage for validating the documented examples for
each event in the `api/get-events` endpoint documentation.
This will help us catch basic typos / mistakes when adding new
event examples. And if fields / objects are removed or modified
for existing events in the API, then failing to update the
examples for those changes will also be caught by this additional
test coverage.
Adding new fields / objects to existing event schemas without
updating the example will not be caught unless the new field
is marked as required in the documentation.
This is a follow-up to 4c8915c8e4, for
the case when the `team:read` permission is missing, which causes the
`team.info` call itself to fail. The error message supplies
information about the provided and missing permissions -- but it also
still sends the `X-OAuth-Scopes` header which we normall read, so we can
use that as normal.
The `tabbed_instructions` widget used for both language toggles in our
API documentation and app toggles in our Help Center documentation
misleadingly calls the identifier for the tab `language` in local
variables and its interface.
- Renames local variables `language` -> `tab_key`.
- Renames HTML data attributes `data-language` -> `data-tab-key`.
Fixes#24669.
This is primarily to prevent impersonation, such as `zulipteam`. We
only enable these protections for CORPORATE_ENABLED, since `zulip` is
a reasonable test name for self-hosters.
django-stubs 4.2.1 gives transaction.on_commit a more accurate type
annotation, but this exposed that mypy can’t handle the lambda default
parameters that we use to recapture loop variables such as
for stream_id in public_stream_ids:
peer_user_ids = …
event = …
transaction.on_commit(
lambda event=event, peer_user_ids=peer_user_ids: send_event(
realm, event, peer_user_ids
)
)
https://github.com/python/mypy/issues/15459
A workaround that mypy accepts is
transaction.on_commit(
(
lambda event, peer_user_ids: lambda: send_event(
realm, event, peer_user_ids
)
)(event, peer_user_ids)
)
But that’s kind of ugly and potentially error-prone, so let’s make a
helper function for this very common pattern.
send_event_on_commit(realm, event, peer_user_ids)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
9d97af6ebb addressed the one major source of inconsistent data which
would be solved by simply re-attempting the ScheduledEmail row. Every
other instance that we have seen since then has been a corrupt or
modified database in some way, which does not self-resolve. This
results in an endless stream of emails to the administrator, and no
forward progress.
Drop this to a warning, and make it remove the offending row. This
ensures we make forward progress.
This commit makes it possible for users to control the wildcard
mention notifications for messages sent to followed topics
via a global notification setting.
There is no support for configuring this setting
through the UI yet.
This commit makes it possible for users to control
the push notifications for messages sent to followed topics
via a global notification setting.
There is no support for configuring this setting
through the UI yet.
This commit makes it possible for users to control
the email notifications for messages sent to followed topics
via a global notification setting.
Although there is no support for configuring this setting
through the UI yet.
Add five new fields to the UserBaseSettings class for
the "followed topic notifications" feature, similar to
stream notifications. But this commit consists only of
the implementation of email notifications.
THUMBNAIL_IMAGES was previously set to true as there were tests on a new
thumbnail functionality. The feature was never stable enough to remain in
the codebase and the setting was left enabled. This setting also doesn't
reflect how the production deployments are and it has been decided that we
should drop setting from test_extra_settings altogether.
Co-authored-by: Joseph Ho <josephho678@gmail.com>
Failing to remove all of the rules which were added causes action at a
distance with other tests. The two methods were also only used by
test code, making their existence in zerver.lib.rate_limiter clearly
misplaced.
This fixes one instance of a mis-balanced add/remove, which caused
tests to start failing if run non-parallel and one more anonymous
request was added within a rate-limit-enabled block.
The user group depedency graph should always be a DAG.
This commit adds code to make sure we keep the graph DAG
while adding subgroups to a user group.
Fixes#25913.
Note that we use the DjangoJSONEncoder so that we have builtin support
for parsing Decimal and datetime.
During this intermediate state, the migration that creates
extra_data_json field has been run. We prepare for running the backfilling
migration that populates extra_data_json from extra_data.
This change implements double-write, which is important to keep the
state of extra data consistent. For most extra_data usage, this is
handled by the overriden `save` method on `AbstractRealmAuditLog`, where
we either generates extra_data_json using orjson.loads or
ast.literal_eval.
While backfilling ensures that old realm audit log entries have
extra_data_json populated, double-write ensures that any new entries
generated will also have extra_data_json set. So that we can then safely
rename extra_data_json to extra_data while ensuring the non-nullable
invariant.
For completeness, we additionally set RealmAuditLog.NEW_VALUE for
the USER_FULL_NAME_CHANGED event. This cannot be handled with the
overridden `save`.
This addresses: https://github.com/zulip/zulip/pull/23116#discussion_r1040277795
Note that extra_data_json at this point is not used yet. So the test
cases do not need to switch to testing extra_data_json. This is later
done after we rename extra_data_json to extra_data.
Double-write for the remote server audit logs is special, because we only
get the dumped bytes from an external source. Luckily, none of the
payload carries extra_data that is not generated using orjson.dumps for
audit logs of event types in SYNC_BILLING_EVENTS. This can be verified
by looking at:
`git grep -A 6 -E "event_type=.*(USER_CREATED|USER_ACTIVATED|USER_DEACTIVATED|USER_REACTIVATED|USER_ROLE_CHANGED|REALM_DEACTIVATED|REALM_REACTIVATED)"`
Therefore, we just need to populate extra_data_json doing an
orjson.loads call after a None-check.
Co-authored-by: Zixuan James Li <p359101898@gmail.com>
This in-progress feature was started in 2018 and hasn't
been worked on much since. It's already in a broken state,
which makes it hard to iterate on the existing search bar
since it's hard to know how those changes will affect search
pills.
We do still want to add search pills eventually, and when
we work on that, we can refer to this diff to readd the
changes back.
An implicit coercion from an untyped dict to the TypedDict was hiding
a type error: CapturedQuery.sql was really str, not bytes. We should
always prefer dataclass over TypedDict to prevent such errors.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This adds support to accepting extra_data being dict from remote
servers' RealmAuditLog entries. So that it is forward-compatible with
servers that have migrated to use JSONField for RealmAuditLog just in
case. This prepares us for migrating zilencer's audit log models to use
JSONField for extra_data.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This prepares for the audit log migration which requires us to populate
a JSONField from the extra_data field. "data" is not representative of
the actual extra_data field for RealmAuditLog entries of event types
in SYNC_BILLING_EVENTS.
We intentionally leave the test cases unchanged without bothering to
verify if the extra_data arrives as-is to keep this change minimal.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Twitter removed their v1 API. We take care to keep the existing cached
results around for now, and to not poison that cache, since we might
be able replace this with something that can still use the existing
cache.
Part of splitting creating and editing scheduled messages.
Should be merged with final commit in series. Breaks tests.
Splits out editing an existing scheduled message into a new
view function and updated `edit_scheduled_message` function.
Part of splitting creating and editing scheduled messages.
Should be merged with final commit in series. Breaks tests.
Removes `scheduled_message_id` parameter from the create scheduled
message path.
Prep commit for splitting create/edit endpoint for scheduled
messages.
Because of `test-api` runs the tests in alphabetical order based on
the `operationId`, we need two scheduled messages in the test database.
The first for the curl example delete (delete-scheduled-message) and
the second for the curl example update (update-scheduled-message).
This commit removes realm_community_topic_editing_limit_seconds
field from register response since topic edit limit is now
controlled by move_messages_within_streams_limit_seconds
setting.
We also remove DEFAULT_COMMUNITY_TOPIC_EDITING_LIMIT_SECONDS
constant since it is no longer used.
As the relevant comment elaborates - what happens next in the test in
simulating the step that happens in the desktop app. Thus a new session
needs to be used. Otherwise, the old session created normally in the
browser pollutes the state and can give falsely passing tests.
This should be happening for all social auth tests using this, not just
in that one SAML test, thus moving it inside the helper method.
This is a useful improvement in general for making correct
LogoutRequests to Idps and a necessary one to make SP-initiated logout
fully work properly in the desktop application. During desktop auth
flow, the user goes through the browser, where they log in through their
IdP. This gives them a logged in browser session at the IdP. However,
SAML SP-initiated logout is fully conducted within the desktop
application. This means that proper information needs to be given to the
the IdP in the LogoutRequest to let it associate the LogoutRequest with
that logged in session that was established in the browser. SessionIndex
is exactly the tool for that in the SAML spec.
This gives more flexibility on a server with multiple organizations and
SAML IdPs. Such a server can have some organizations handled by IdPs
with SLO set up, and some without it set up. In such a scenario, having
a generic True/False server-wide setting is insufficient and instead
being able to specify the IdPs/orgs for SLO is needed.
Closes#20084
This is the flow that this implements:
1. A logged-in user clicks "Logout".
2. If they didn't auth via SAML, just do normal logout. Otherwise:
3. Form a LogoutRequest and redirect the user to
https://idp.example.com/slo-endpoint?SAMLRequest=<LogoutRequest here>
4. The IdP validates the LogoutRequest, terminates its own user session
and redirects the user to
https://thezuliporg.example.com/complete/saml/?SAMLRequest=<LogoutResponse>
with the appropriate LogoutResponse. In case of failure, the
LogoutResponse is expected to express that.
5. Zulip validates the LogoutResponse and if the response is a success
response, it executes the regular Zulip logout and the full flow is
finished.
This commit updates the API to check the permission to subscribe other
users while inviting. The API will error if the user passes the
"stream_ids" parameter (even when it contains only default streams)
and the calling user does not having permission to subscribe others to
streams.
For users who do not have permission to subscribe others, the
invitee will be subscribed to default streams at the time of
accepting the invite.
There is no change for multiuse invites, since only admins are allowed
to send them, and admins always have the permission to subscribe
others to streams.
Since 74dd21c8fa in Zulip Server 2.1.0, if:
- ZulipLDAPAuthBackend and an external authentication backend (any aside
of ZulipLDAPAuthBackend and EmailAuthBackend) are the only ones
enabled in AUTHENTICATION_BACKENDS in /etc/zulip/settings.py
- The organization permissions don't require invitations to join
...then an attacker can create a new account in the organization with
an arbitrary email address in their control that's not in the
organization's LDAP directory.
The impact is limited to installations which have the specific
combination of authentication backends described above, in addition to
having the "Invitations are required for joining this organization
organization" permission disabled.
Realm exports may OOM on deployments with low memory; to ensure
forward progress, log the start time in the RealmAuditLog entry, and
key off of the existence of that to prevent re-attempting an export
which was already tried once.
This makes it less likely we will accidentally fail to include a class
if the subclassing of QueueProcessingWorker changes, and lets mypy
more accurately understand the typing.
We now allow users to change email address visibility setting
on the "Terms of service" page during first login. This page is
not shown for users creating account using normal registration
process, but is useful for imported users and users created
through API, LDAP, SCIM and management commands.
We now set tos_version to "-1" for imported users and the ones
created using API or using other methods like LDAP, SCIM and
management commands. This value will help us to allow users to
change email address visibility setting during first login.
Earlier when a user who is not allowed to add subscribers to a
stream because of realm level setting "Who can add users to streams"
is subscribing other users while creating a new stream than new stream
was created but no one is subscribed to stream.
To fix this issue this commit makes changes in the API used
for adding subscriptions. Now stream will be created only when user
has permissions to add other users.
With a rewrite of the test by Tim Abbott.
In the case of a user editing a scheduled message that the server
had failed to send at the scheduled time due to an error, we want
to update the `failed` and `failure_message` fields as the intent
is for the server to retry to send the scheduled message based on
the updated information provided by the user.
In the case that there is an error when sending a scheduled message,
we now send a message from the notification bot to the user who
scheduled the message about the failure/error.
The notification message is not sent if the error when sending the
scheduled message was due to the realm or sender being deactivated.
This commit adds a new test to check how the visibility policy updates
when moving messages to a topic that didn't exist previously.
This test also helps us adding coverage for the code which just
skips setting visibility_policy if there is no need to update the
value because both previous and new value of visibility policy
is INHERIT. The "actions/message_edit.py" file has 100% coverage
now and thus is removed from "not_yet_fully_covered" list.
The code for updating visibility policy values on moving messages
had two bugs.
- There was a typo in elif condition where "user_profile" was being
used instead of "user_profile_with_policy".
This commit fixes the typo.
- It was assumed that there would be no UserTopic rows for target
topic if the target topic didn't exist. But there can be such case
where some messages were sent to that topic and the user muted
the topic. But then the messages in that topic was deleted. In
such case there can be UserTopic rows for a stream-topic pair
that does not exist.
This commit fixes the code to handle such case as well and set
the visibility policy of new topic to what was set for the original
topic. This change simplifies the condition to just check whether
new_visibility_policy is equal to target_topic_visibility_policy
and skip if so, and update the visibility policy otherwise.
Due to this change, we now do not try to mute the already muted
topic if the topic is moved to a topic which didn't exist
previously and thus we modify the existing test to not expect
any INFO logs.
This commit adds tests to cover the case of message editing
not allowed due to allow_message_editing set to False and
the case when there is no limit set when moving all messages
in a topic.
The "actions/message_edit.py" file does not have 100% coverage
still and it will be addressed in the next commit.
Adds test coverage for the error sent for editing a scheduled
message that was successfully sent.
`zerver/actions/scheduled_messages.py` now has 100% test coverage
again.
We were missing a few checks for raw_unread_msgs being present before
trying to parse and update it.
The test only covers 2/3 of the cases, but I wasn't convinced it was
worth adding another test just for the corner case of removing a
message flag; this seems fairly unlikely to regress.
We now allow users to invite without specifying any stream to join.
In such cases, the user would join the default streams, if any, during
the process of account creation after accepting the invite.
It is also fine if there are no default streams and user isn't
subscribed to any stream initially.
We do not add user to the default streams if the streams list passed
while sending the invite (both email and multi-use) was empty since
invite explicitly selected to not subscribe the user to default
streams.
For scheduled stream messages, we already limited the `to`
parameter to be the stream ID, but here we return a JsonableError
in the case of a ValueError when the passed value is not an integer.
For scheduled direct messages, we limit the list for the `to`
parameter to be user IDs. Previously, we accepted emails like
we do when sending messages.
Test coverage for `zerver/actions/message_delete.py`.
Both callers of this function would already return if there were
no Messages specified to delete, which is why existing tests did
not cover this.
Fixes#25414.
We add Attachment.scheduled_messages relation to track ScheduledMessages
which reference the attachment.
The import bits can be done after merging this, by updating #25345.
Because education organizations and users have slightly specialized
use cases, we update the Welcome Bot message content sent to new
users and new organization owners for these types of organizations
to link to help center articles/guides geared toward these users
and organizations.
Also, updates the demo organization warning to only go to the new
demo organization owner because the 30 day deletion text is only
definitely accurate when the organization is created.
Fixes#21694.
Prep commit for adding the scheduled-message endpoints to the API
documentation.
Adds a scheduled message for Iago in the test database so that it
can be deleted in the delete cURL example in the api-test suite.
This will help us remove scheduled message and reminder logic
from `/messages` code path.
Removes `deliver_at`/`defer_until` and `tz_guess` parameters. And
adds the `scheduled_delivery_timestamp` instead. Also updates the
scheduled message dicts to return `scheduled_delivery_timestamp`.
Also, revises some text in `/delete-scheduled-message` endpoint
and in the `ScheduledMessage` schema in the API documentation.
Updates the objects in the API for scheduled messages so that those
for stream messages return the `to` property as an integer since it
is always the unique stream ID and so that those for direct messages
do not have a `topic` property since direct messages never have a
topic.
Also makes small update so that web app scheduled messages overlay
has the correct stream ID.
Previously, entering an organization via 'accounts/go' with the
web-public stream enabled took the user to the web-public view
even if the user was not logged in.
Now, a user is always redirected to the 'login_page' with
the next parameter, if present.
The 'login_page' view is updated to redirect an authenticated
user based on the 'next' parameter instead of always redirecting
to 'realm.uri'.
Fixes#23344.
The "Resend" link for realm creation was not working correctly
because it is implemented by basically submiting the registration
form again which results in resending the email but all the
required parameters were not passed to the form after recent
changes in the realm creation flow.
This commit fixes it by passing all the required parameters -
email, realm name, realm type and realm subdomain, when submitting
form again by clicking on the "resend" link.
Fixes#25249.
In #23380 we want to change all occurrences of `uri` with `url`.
This commit changes the occurrences in a context key `api_uri_context`
and a function name `add_api_uri_context`.
Adds CSS formatting for `invalid_email.html`.
Uses the `white-box` style because this page is a redirect when
there is an error with the email the user provided during
registration.
Also, updates the text of this page for some grammar errors and
to clarify the language between an invalid email and an email that
is not allowed by the Zulip organization in question.
Finally, makes any references to the `realm_name` also link to
the Zulip organization with the `realm_uri`.
Adds CSS formatting for `no_spare_licenses.html`.
Uses the `white-box` style because this page is a redirect when
a user tries to register for a Zulip Cloud organization that does
not have any available licenses for new users.
Updates reference to `realm_name` to be a link to the `realm_uri`.
This also removes the error in one of these functions that was using a
different constant instead of
PRESENCE_LEGACY_EVENT_OFFSET_FOR_ACTIVITY_SECONDS.
This implements the core of the rewrite described in:
For the backend data model for UserPresence to one that supports much
more efficient queries and is more correct around handling of multiple
clients. The main loss of functionality is that we no longer track
which Client sent presence data (so we will no longer be able to say
using UserPresence "the user was last online on their desktop 15
minutes ago, but was online with their phone 3 minutes ago"). If we
consider that information important for the occasional investigation
query, we have can construct that answer data via UserActivity
already. It's not worth making Presence much more expensive/complex
to support it.
For slim_presence clients, this sends the same data format we sent
before, albeit with less complexity involved in constructing it. Note
that we at present will always send both last_active_time and
last_connected_time; we may revisit that in the future.
This commit doesn't include the finalizing migration, which drops the
UserPresenceOld table.
The way to deploy is to start the backfill migration with the server
down and then start the server *without* the user_presence queue worker,
to let the migration finish without having new data interfering with it.
Once the migration is done, the queue worker can be started, leading to
the presence data catching up to the current state as the queue worker
goes over the queued up events and updating the UserPresence table.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
The migration to css-inline "fixed" the fact that styles from this
file previously were never applied to the internals of missed-message
emails.
Rewrite much of the CSS to more correctly scope to apply to the
appropriate elements, and document with comments the purpose of most
blocks.
Co-authored-by: Tim Abbott <tabbott@zulip.com>
In a previous commit, the frontend of the web app was updated for
URLs with `#narrow/dm/...` for direct messages (group and 1-on-1).
Updates the URLs generated for email notifications and outgoing
webhook notification messages to use the new `/dm/...` format.
Adds backend support for `dm-including` operator. This will
deprecate the `group-pm-with` operator, but we keep support
for backwards-compatibility.
For testing updates, because the messages returned by these
two operators are different, most of the tests for `group-pm-with`
remain unchanged, but added comments about deprecated state.
Also, cleans up remaining instance of "PM" in `narrow.py` to
be "DM".
The general API changelog and documentation updates will be done
in a final commit in the series of commits that adds support for
the various new direct message narrows.
Adds backend support for `dm` operator. This will deprecate the
`pm-with` operator, but we keep support for backwards-compatibility.
For testing updates, updates the existing tests for `pm-with` to
use `dm`, and adds one basic test for `pm-with` in the `add_term`
tests as the two operators refer to the same `by_*` method.
The general API changelog and documentation updates will be done
in a final commit in the series of commits that adds support for
the various new direct message narrows.
Adds backend support for `is` operator with the `dm` operand. This
will deprecate the `is` operator with the `private` operand, but we
keep support for backwards-compatibility.
Note that there is some clean up of references to private messages
in the updated backend test. In commit 43ec7ed, the documentation
for `build_narrow_filter` wasn't updated for the rename of
`BuildNarrowFilterTest` to `NarrowLibraryTest`, so that's also
corrected in these changes.
The general API changelog and documentation updates will be done
in a final commit in the series of commits that adds support for
the various new direct message narrows.
This swaps out url_format_string from all of our APIs and replaces it
with url_template. Note that the documentation changes in the following
commits will be squashed with this commit.
We change the "url_format" key to "url_template" for the
realm_linkifiers events in event_schema, along with updating
LinkifierDict. "url_template" is the name chosen to normalize
mixed usages of "url_format_string" and "url_format" throughout
the backend.
The markdown processor is updated to stop handling the format string
interpolation and delegate the task template expansion to the uri_template
library instead.
This change affects many test cases. We mostly just replace "%(name)s"
with "{name}", "url_format_string" with "url_template" to make sure that
they still pass. There are some test cases dedicated for testing "%"
escaping, which aren't relevant anymore and are subject to removal.
But for now we keep most of them as-is, and make sure that "%" is always
escaped since we do not use it for variable substitution any more.
Since url_format_string is not populated anymore, a migration is created
to remove this field entirely, and make url_template non-nullable since
we will always populate it. Note that it is possible to have
url_template being null after migration 0422 and before 0424, but
in practice, url_template will not be None after backfilling and the
backend now is always setting url_template.
With the removal of url_format_string, RealmFilter model will now be cleaned
with URL template checks, and the old checks for escapes are removed.
We also modified RealmFilter.clean to skip the validation when the
url_template is invalid. This avoids raising mulitple ValidationError's
when calling full_clean on a linkifier. But we might eventually want to
have a more centric approach to data validation instead of having
the same validation in both the clean method and the validator.
Fixes#23124.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This is implemented by replacing all matches of "%(var_name)s" in a URL
format string with "{var_name}". Since we do want to ensure that the
templates aren't broken after this migration, a RuntimeError is raised
to let the maintainer know that certain linkifier cannot be converted
automatically if it does not pass the uri_template.validate check.
Also, we need to escape "%%", which is used to represent "%" in the old
format string syntax, as well as "{" and "}", which is a part of the
URL template syntax.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
This commit updates the logic for migrating user_topic rows
during the move-messages operation when the target topic
already has messages.
Previously, the target_topic's visibility_policy was simply
set to the original_topic's visibility_policy,
and the original_topic's visibility_policy was set to INHERIT.
This commit updates the move-messages code path to determine
the new visibility_policy depending on the visibility policies
of the original and target topics.
The target_topic's visibility_policy is then updated.
The number of db queries has increased by two:
One query corresponds to determining if 'target_topic_has_messages'.
Another query corresponds to 'get_users_with_user_topic_visibility_policy'
to determine 'target_topic_user_profile_to_visibility_policy'.
Moves jwt_fetch_api_key endpoint to v1_api_mobile_patterns so
that tools/test-api detects it as an API endpoint that is pending
documentation.
Fixes#24982.
For endpoints with a `type` parameter to indicate whether the message
is a stream or direct message, `POST /typing` and `POST /messages`,
adds support for passing "direct" as the preferred value for direct
messages, group and 1-on-1.
Maintains support for "private" as a deprecated value to indicate
direct messages.
Fixes#24960.
Refactors instances of `message_type_name` and `message_type`
that are referring to API message type value ("stream" or
"private") to use `recipient_type_name` instead.
Prep commit for adding "direct" as a value for endpoints with a
`type` parameter to indicate whether the message is a stream or
direct message.
So far, we've used the BitField .authentication_methods on Realm
for tracking which backends are enabled for an organization. This
however made it a pain to add new backends (requiring altering the
column and a migration - particularly troublesome if someone wanted to
create their own custom auth backend for their server).
Instead this will be tracked through the existence of the appropriate
rows in the RealmAuthenticationMethods table.
If the ID of the scheduled message is passed by the client, we
edit the existing scheduled message instead of creating a new one.
However, this will soon be moved into its own API endpoint.
Servers that had upgraded from a Zulip server version that did not yet
support the user_uuid field to one that did could end up with some
mobile devices having two push notifications registrations, one with a
user_id and the other with a user_uuid.
Fix this issue by sending both user_id and user_uuid, and clearing
This commit fixes the comment about number of database queries
when moving message from muted topic to mention clearly about
the number of queries added due to original topic being muted.
We do not include the queries that is executed to check whether
the topic is muted or not, as they will be executed in all cases.
We previously allowed moving messages that have passed the time limit
using "change_all" value for "propagate_mode" parameter. This commit
changes the behavior to not allow moving messages (both stream and
topic edit) that have passed the time limit for non-admin and
non-moderator users.
Previously, editing topic of "(no topic)" messages was allowed
irrespective of time limit or the "edit_topic_policy" setting.
Since we are working in the direction of having "no topic" messages
feel reasonable, this commit changes the code to not consider them
as a special case and topic editing restrictions apply to them as
well now like all other messages.
We still highlight the topic edit icon in recipient bar without
hovering for "no topic" messages, but it is only shown when user
has permission to edit topics.
The previous implementation leaked database connections, as a new
thread (and thus a new thread-local database connection) was made for
each timer execution. While these connections were relatively
lightweight in Python, they also incur memory overhead in the
PostgreSQL server itself. The logic for managing the timer was also
unclear, and the unavoidable deadlock in the stopping logic was rather
unfortunate.
Rewrite with one explicit worker thread which handles the delayed
message sending. The RabbitMQ consumer creates the database rows, and
notifies the worker to start its 5s timeout. Because it is controlled
by a condition variable, it does not hold the lock while waiting, and
can be notified to exit.
Adds a new welcome email, `onboarding_zulip_guide`, to be sent four
days after a new user registers with a Zulip organization if the
organization has specified a particular organization type that has
a guide in the corporate `/for/.../` pages. If there is no guide,
then no email is scheduled or sent.
The current `for/communities/` page is not very useful for users
who are not organization administrators, so these onboarding guide
emails are further restricted for those organization types to
only go to new users who are invited/registered as admins for the
organzation.
Adds two database queries for new user registrations: one to get
the organization's type and one to create the scheduled email.
Adds two email logs because the email is sent both to a new user
who registers with an existing organization and to the organization
owner when they register a new organization.
Co-authored by: Alya Abbott <alya@zulip.com>
Refactors the logic for adjusting the delay for sending an email
to not land on a weekend so that it can be used to schedule any
number of onboarding emails we decide to send.
Consolidates duplicate testing into
`zerver/tests/test_email_notifications.py`. The initial test and
function were introduced in commit 610f2cbacf with the test
located in `zerver/tests/test_signup.py`.
Prep commit for adding new welcome / follow up email.
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.
After this commit a notification message is sent to users if they are
added to user_groups by someone else or they are removed from user_groups
by someone else.
Fixes#23642.
Previously, we had an architecture where CSS inlining for emails was
done at provision time in inline_email_css.py. This was necessary
because the library we were using for this, Premailer, was extremely
slow, and doing the inlining for every outgoing email would have been
prohibitively expensive.
Now that we've migrated to a more modern library that inlines the
small amount of CSS we have into emails nearly instantly, we are able
to remove the complex architecture built to work around Premailer
being slow and just do the CSS inlining as the final step in sending
each individual email.
This has several significant benefits:
* Removes a fiddly provisioning step that made the edit/refresh cycle
for modifying email templates confusing; there's no longer a CSS
inlining step that, if you forget to do it, results in your testing a
stale variant of the email templates.
* Fixes internationalization problems related to translators working
with pre-CSS-inlined emails, and then Django trying to apply the
translators to the post-CSS-inlined version.
* Makes the send_custom_email pipeline simpler and easier to improve.
Signed-off-by: Daniil Fadeev <fadeevd@zulip.com>
Adds the user ID to the return values for the `/fetch_api_key` and
`/dev_fetch_api_key` endpoints. This saves clients like mobile a
round trip to the server to get the user's unique ID as it is now
returned as part of the log in flow.
Fixes#24980.
This commit updates 'test_user_ids_unmuting_topic' to make
an api_post call to '/api/v1/user_topics' instead of
calling the internal function 'do_set_user_topic_visibility_policy'
to verify the logic.
This commit adds a new endpoint, 'POST /user_topics' which
is used to update the personal preferences for a topic.
Currently, it is used to update the visibility policy of
a user-topic row.
Primary goal of library replacement is improving execution speed.
This commit should not affect the functionality of the system
or make any changes to it.
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, tests that exercised code paths that added local
uploads did not always clean up `settings.LOCAL_UPLOADS_DIR`
after the test was complete.
Updates the `ZulipTestCase` class to remove any local uploads
in the unique `settings.LOCAL_UPLOADS_DIR` in `tearDown` for
all tests.
This commit adds code to create a "Nobody" system user group
to realms which will be used in settings to represent "Nobody"
option.
We also add a migration to add this group to existing realms.
This commit adds a new helper submit_realm_creation_form,
similar to existing submit_reg_form_for_user, to avoid
duplicate code for creating realms in tests.
In previous commits, we updated the realm creation flow to show
the realm name, type and subdomain fields in the first form
when asking for the email of the user. This commit updates the
user registration form to show the already filled realm details
as non-editable text and there is also a button to edit the
realm details before registration.
We also update the sub-heading for user registration form as
mentioned in the issue.
Fixes part of #24307.
We now use PreregistrationRealm objects in registration_helper
function when creating new realms instead of PreregistrationUser
objects.
Fixes part of #24307.
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.
"check_add_user_group" is a safer helper function than
"create_user_group" to use when creating user_groups. It does
error handling and notify the client with the appropriate event.
Note that the populate_db command still uses "create_user_group"
because we do not need to enqueue events at that point.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Since this function creates a new user group into the database,
it is more appropriate to have it not as a generic "lib" function
but as an "action".
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Prior to commit a9b3a9c, the server implementation for documented
search operators with dashes, also implicitly supported clients
sending those same operators with underscores. This has been the
case sense the server side support for narrow filtering was
introduced in commit 3af2bf345a.
Updates the stricter version of mapping operator strings to `by*`
functions, to also include the underscore version of any operators
that have dashes. Adds a note that these undocumented versions are
tied to the support for the documented versions.
We previously created RealmAuditLog entries for user notification
settings only. This commit changes the code to create entries for
all user settings. We cannot backfill the entries since we don't
have the data to do that.
When a new realm is created, a notification message is sent to
the realm configured as the settings.SYSTEM_BOT_REALM if there
is a "signups" stream that exists in that realm. This is used
for Zulip Cloud, but is an undocumented feature.
The topic of the message has been the subdomain of the new realm,
and the message content has been "Signups enabled" translated
into the default language of the new realm.
In order to make these messages more explicitly for Zulip Cloud,
the settings.CORPORATE_ENABLED is checked before sending these
messages.
To make these messages more useful, the topic for these
notifications is changed to be "new organizations". The content
of these messages is updated to have the new realm name (with a
link to the admin realm's activity support page for the realm),
subdomain (with a link to the realm), and organization type.
`render_markdown_path` renders Markdown, and also (since baff121115)
runs Jinja2 on the resulting HTML.
The `pure_markdown` flag was added in 0a99fa2fd6, and did two
things: retried the path directly in the filesystem if it wasn't found
by the Jinja2 resolver, and also skipped the subsequent Jinja2
templating step (regardless of where the content was found). In this
context, the name `pure_markdown` made some sense. The only two
callsites were the TOS and privacy policy renders, which might have
had user-supplied arbitrary paths, and we wished to handle absolute
paths in addition to ones inside `templates/`.
Unfortunately, the follow-up of 01bd55bbcb did not refactor the
logic -- it changed it, by making `pure_markdown` only do the former
of the two behaviors. Passing `pure_markdown=True` after that commit
still caused it to always run Jinja2, but allowed it to look elsewhere
in the filesystem.
This set the stage for calls, such as the one introduced in
dedea23745, which passed both a context for Jinja2, as well as
`pure_markdown=True` implying that Jinja2 was not to be used.
Split the two previous behaviors of the `pure_markdown` flag, and use
pre-existing data to control them, rather than an explicit flag. For
handling policy information which is stored at an absolute path
outside of the template root, we switch to using the template search
path if and only if the path is relative. This also closes the
potential inconsistency based on CWD when `pure_markdown=True` was
passed and the path was relative, not absolute.
Decide whether to run Jinja2 based on if a context is passed in at
all. This restores the behavior in the initial 0a99fa2fd6 where a
call to `rendar_markdown_path` could be made to just render markdown,
and not some other unmentioned and unrelated templating language as
well.
This adds tests for more corner cases, in exchange for dropping the
query count tests, which were of dubious utility. It also adds the
time-machine library to mock the current time to test that the limits
do expire.
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.
Currently, there is a checkbox setting for whether to
"Include realm name in subject of message notification emails".
This commit replaces the checkbox setting with a dropdown
having values: Automatic [default], Always, Never.
The Automatic option includes the realm name if, and only if,
there are multiple Zulip realms associated with the user's email.
Tests are added and(or) modified.
Fixes: #19905.
Removes the notification message that was sent if a stream named
"signups" exists in the `settings.SYSTEM_BOT_REALM`. This was a
undocumented feature that would send a notification message when
a new user registered with a Zulip organization that was hosted
by an admin realm like Zulip Cloud.
This removes two database queries when a new user is created: one
to get the system bot realm and the other to get the notification
bot in said realm.
Note that there are still notification messages sent when a new
organization is registered with the admin realm if the "signups"
stream exists.
The Django convention is for __repr__ to include the type and __str__
to omit it. In fact its default __repr__ implementation for models
automatically adds a type prefix to __str__, which has resulted in the
type being duplicated:
>>> UserProfile.objects.first()
<UserProfile: <UserProfile: emailgateway@zulip.com <Realm: zulipinternal 1>>>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
While the function which processes the realm registration and
signup remains the same, we use different urls and functions to
call the process so that we can separately track them. This will
help us know the conversion rate of realm registration after
receiving the confirmation link.
Zulip already has integrations for server-side Sentry integration;
however, it has historically used the Zulip-specific `blueslip`
library for monitoring browser-side errors. However, the latter sends
errors to email, as well optionally to an internal `#errors` stream.
While this is sufficient for low volumes of users, and useful in that
it does not rely on outside services, at higher volumes it is very
difficult to do any analysis or filtering of the errors. Client-side
errors are exceptionally noisy, with many false positives due to
browser extensions or similar, so determining real real errors from a
stream of un-grouped emails or messages in a stream is quite
difficult.
Add a client-side Javascript sentry integration. To provide useful
backtraces, this requires extending the pre-deploy hooks to upload the
source-maps to Sentry. Additional keys are added to the non-public
API of `page_params` to control the DSN, realm identifier, and sample
rates.
b4dd118aa1 changed how the `user_info_str` parsed information out of
the events it received -- but only changed the server errors, not the
browser errors, though both use the same codepath. As a result, all
browser errors since then have been incorrectly marked as being for
anonymous users.
Build and pass in the expected `user` dict into the event.
This commit adds 'visibility_policy' as a
parameter to user_allows_notifications_in_StreamTopic
function.
This adds logic inside the user_allows_notifications_in_StreamTopic
function, to not return False when a stream is muted
but the topic is UNMUTED.
Adds a method `user_id_to_visibility_policy_dict`
to 'StreamTopicTarget' class to fetch
(user_id => visibility_policy) in single db query.
Co-authored-by: Kartik Srivastava <kaushiksri0908@gmail.com>
Co-authored-by: Prakhar Pratyush <prakhar841301@gmail.com>
This commit replaces 'remove_topic_mute' with
'set_user_topic_visibility_policy_in_database' and
updates it to delete UserTopic row with any configured
visibility_policy and not just muting.
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 refactors the existing pattern (real-time usage)
used to assert 'date_muted' in tests.
A fixed value is used at the start of the test to
assert 'date_muted', replacing the timedelta or real-time usage pattern.
Replaces 'do_unmute_topic' with 'do_set_user_topic_visibility_policy'
and associated minor changes.
This change is made to align with the plan to use a single function
'do_set_user_topic_visibility_policy' to manage
user_topic - visibility_policy changes and corresponding event
generation.
This commit is a step in the direction of having a common
function to handle visibility_policy changes and event
generation instead of separate functions for each
visibility policy.
In order to support different types of topic visibility policies,
this renames 'do_topic_mute' to 'do_set_user_topic_visibility_policy'
and refactors it to accept a parameter 'visibility_policy'.
The "add_topic_mute" and "remove_topic_mute" library functions
shouldn't be called directly from tests.
They should instead call "do_mute_topic" and "do_unmute_topic"
The reason being:
Library functions are meant to be internal interfaces
for just changing the database, and shouldn't generally be
called elsewhere.
Creates `MutableJsonResponse` as a subclass of Django's `HttpResponse`
that we can modify for ignored parameters in the response content.
Updates responses to include `ignored_parameters_unsupported` in
the response data through `has_request_variables`. Creates unit
test for this implementation in `test_decorators.py`.
The `method` parameter processed in `rest_dispatch` is not in the
`REQ` framework, so for any tests that pass that parameter, assert
for the ignored parameter with a comment.
Updates OpenAPI documentation for `ignored_parameters_unsupported`
being returned in the JSON success response for all endpoints.
Adds detailed documentation in the error handling article, and
links to that page in relevant locations throughout the API docs.
For the majority of endpoints, the documentation does not include
the array in any examples of return values, and instead links to
the error handling page. The exceptions are the three endpoints
that had previously supported this return value. The changes note
and example for these endpoints is also used in the error
handling page.
In commit 8181ec4b56, we removed the `realm_str` as a parameter
for `send_message_backed`. This removes a missed test that included
this as a parameter for that endpoint/function.
Actions like deleting realms may leave unreferenced uploads in the
attachment storage backend.
Fix these by walking the complete contents of the attachment storage
backend, and removing files which are no longer present in the
database. This may take quite some time, as it is necessarily O(n) in
the number of files uploaded to the system.
This commit renames reset_emails_in_zulip_realm function to
reset_email_visibility_to_everyone_in_zulip_realm which makes
it more clear to understand what the function actually does.
This commit also adds a comment explaining what this function
does.
This already became useless in 6e11754642,
as detailed in the API changelog entry here. At this point, we should
eliminate this param and the weird code around it.
This commit also deletes the associated tests added in
6e11754642, since with realm_str removed,
they make no sense anymore (and actually fail with an OpenAPI error due
to using params not used in the API). Hypothetically they could be
translated to use the subdomain= kwarg, but that also doesn't make
sense, since at that point they'd be just testing the case of a user
making an API request on a different subdomain than their current one
and that's just redundant and already tested generally in
test_decorators.
Updates the text and title used when the password reset done page
to work for situations where the user is resetting a forgotten
password and for situation where the user is setting a password
for the first time (e.g. SSO login, demo organizations).
This is the behaviour inherited from Django[^1]. While setting the
password to empty (`email_password = `) in
`/etc/zulip/zulip-secrets.conf` also would suffice, it's unclear what
the user would have been putting into `EMAIL_HOST_USER` in that
context.
Because we previously did not warn when `email_password` was not
present in `zulip-secrets.conf`, having the error message clarify the
correct configuration for disabling SMTP auth is important.
Fixes: #23938.
[^1]: https://docs.djangoproject.com/en/4.1/ref/settings/#std-setting-EMAIL_HOST_USER
`./manage.py import` does not take a tarball; it takes a directory.
Making a separate tarball is a waste of CPU time and disk, as it is
never used.
This was included in the commit of the initial Slack conversion code
in 5b37c5562b and propagated from there into every conversion tool.
Remove the unnecessary tarball creation.
View that handled `PATCH user_groups/<int:user_group_id>` required
both name and description parameters to be passed. Due to this
clients had to pass values for both these parameters even if
one of them was changed.
To resolve this name description parameters to
`PATCH user_groups/<int:user_group_id>` are made optional.
This commit adds backend code to set email_address_visibility when
registering a new user. The realm-level default and the value of
source profile gets overridden by the value user selected during
signup.
This lets us simplify the long-ish ‘../../static/js’ paths, and will
remove the need for the ‘zrequire’ wrapper.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
In the case where a stream existed but had no subscribers, the error
message used to send to the owner always used `stream_name`, which
may have been None.
Switch to using `stream.name` rather than `stream_name` for this case.
We add stream_permission_group_settings object which is
similar to property_types framework used for realm settings.
This commit also adds GroupPermissionSetting dataclass for
defining settings inside stream_permission_group_settings.
We add "do_change_stream_group_based_setting" function which
is called in loop to update all the group-based stream settings
and it is now used to update 'can_remove_subscribers_group'
setting instead of "do_change_can_remove_subscribers_group".
We also change the variable name for event_type field of
RealmAuditLog objects to STREAM_GROUP_BASED_SETTING_CHANGED
since this will be used for all group-based stream settings.
'property' field is also added to extra_data field to identify
the setting for which RealmAuditLog object was created.
We will add a migration in further commits which will add the
property field to existing RealmAuditLog objects created for
changing can_remove_subscribers_group setting.
This makes use of the new case insensitive UNIQUE index added in the
earlier commit. With that index present, we can now rely solely on the
database to correctly identify duplicates and throw integrity errors as
required.
In 141b0c4, we added code to handle races caused by duplicate muting
requests. That code can also handle the non-race condition, so we don't
require the first check.
Removes the initial check in `_internal_prep_message` of the length
of the message content because the `check_message` in the try block
will call `normalize_body` on the message content string, which
does a more robust check of the message content (empty string, null
bytes, length). If the message content length exceeds the value of
`settings.MAX_MESSAGE_LENGTH`, then it is truncated based on that
value. Updates associated backend test for these changes.
The removed length check would truncate the message content with a
hard coded value instead of using the value for
`settings.MAX_MESSAGE_LENGTH`.
Also, removes an extraneous comment about removing null bytes. If
there are null bytes in the message content, then `normalize_body`
will raise an error.
Note that the previous check had intentionally reduced any message over
the 10000 character limit to 3900 characters, with the code in
question dating to 2012's 100df7e349.
The 3900 character truncating rule was implemented for incoming emails
with the email gateway, and predated other features to help with
overly long messages (better stripping of email footers via Talon,
introduced in f1f48f305e, and
condensing, introduced in c92d664b44).
While we could preserve that logic if desired, it likely is no longer
a necessary or useful variation from our usual truncation rules.
The password parameter being passed in the `_do_test` helper
function for `TestAuthenticatedJsonPostViewDecorator` tests was
being ignored, as the user needs to be logged in. Removes the
parameter from the helper function and updates the success test
to use `assert_json_success` instead of just checking the status
code.
Also adds a test case for when a user is not logged in to confirm
that it returns an UnauthorizedError.
This reverts commit 851d68e0fc.
That commit widened how long the transaction is open, which made it
much more likely that after the user was created in the transaction,
and the memcached caches were flushed, some other request will fill
the `get_realm_user_dicts` cache with data which did not include the
new user (because it had not been committed yet).
If a user creation request lost this race, the user would, upon first
request to `/`, get a blank page and a Javascript error:
Unknown user_id in get_by_user_id: 12345
...where 12345 was their own user-id. This error would persist until
the cache expired (in 7 days) or something else expunged it.
Reverting this does not prevent the race, as the post_save hook's call
to flush_user_profile is still in a transaction (and has been since
168f241ff0), and thus leaves the potential race window open.
However, it much shortens the potential window of opportunity, and is
a reasonable short-term stopgap.
The Client.name field is only 30 characters long, but there is no
limit to the length of parsed User-Agent value which we may attempt to
store in it. This can cause requests with long user-agents to 500
when the creation of the Client row fails.
Truncate the name at 30 characters for the cache key, and passing
`name` to `get_or_create`.
This commits update the code to use user-level email_address_visibility
setting instead of realm-level to set or update the value of UserProfile.email
field and to send the emails to clients.
Major changes are -
- UserProfile.email field is set while creating the user according to
RealmUserDefault.email_address_visbility.
- UserProfile.email field is updated according to change in the setting.
- 'email_address_visibility' is added to person objects in user add event
and in avatar change event.
- client_gravatar can be different for different users when computing
avatar_url for messages and user objects since email available to clients
is dependent on user-level setting.
- For bots, email_address_visibility is set to EVERYONE while creating
them irrespective of realm-default value.
- Test changes are basically setting user-level setting instead of realm
setting and modifying the checks accordingly.
Previously, user objects contained delivery_email field
only when user had access to real email. Also, delivery_email
was not present if visibility setting is set to "everyone"
as email field was itself set to real email.
This commit changes the code to pass "delivery_email" field
always in the user objects with its value being "None" if
user does not have access to real email and real email otherwise.
The "delivery_email" field value is None for logged-out users.
For bots, the "delivery_email" is always set to real email
irrespective of email_address_visibility setting.
Also, since user has access to real email if visibility is set
to "everyone", "delivery_email" field is passed in that case
too.
There is no change in email field and it is same as before.
This commit also adds code to send event to update delivery_email
field when email_address_visibility setting changes to all the
users whose access to emails changes and also changes the code to
send event on changing delivery_email to users who have access
to email.
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.
In Zulip, message topics are case-insensitive but case-preserving.
The `get_context_for_message` function erroneously did a
case-sensitive search, and thus only messages whose topic matched
exactly were pulled in as context.
Make the missed-message pipeline aware that message topics are not
case-sensitive. This means that, when collapsing adjacent messages,
we merge messages with topic headers which are "different"; create a
separate explicit "grouping" to know which to collapse.
Rename 'muting.py' to 'user_mutes.py' because it, now
, contains only user-mute related functions.
Includes minor refactoring needed after renaming the file.
This commit moves topic related stuff i.e. topic muting functions
to a separate file 'views/user_topics.py'.
'views/muting.py' contains functions related to user-mutes only.
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
This adds a new endpoint /jwt/fetch_api_key that accepts a JWT and can
be used to fetch API keys for a certain user. The target realm is
inferred from the request and the user email is part of the JWT.
A JSON containing an user API key, delivery email and (optionally)
raw user profile data is returned in response.
The profile data in the response is optional and can be retrieved by
setting the POST param "include_profile" to "true" (default=false).
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
This will be useful for re-use for implementation of an endpoint for
obtaining the API by submitting a JWT in the next commits.
It's not a pure refactor, as it requires some tweaks to remote_user_jwt
behavior:
1. The expected format of the request is changed a bit. It used to
expect "user" and "realm" keys, from which the intended email was
just generated by joining with @. Now it just expects "email"
straight-up. The prior design was a bt strange to begin with, so this
might be an improvement actually.
2. In the case of the codepath of new user signup, this will no longer
pre-populate the Full Name in the registration form with the value
from the "user" key. This should be a very minor lost of
functionality, because the "user" value was not going to be a proper
Full Name anyway. This functionality can be restored in a future
commit if desired.
This is an API change, but this endpoint is nearly unused as far as
we're aware.
Moves the check for calling the `api-doc-template.md` directly,
so that we don't return a 500 error from the server, to happen
earlier with other checks for returning a 404 / missing page.
Also adds a specific test to `zerver/tests/test_urls` for this
template.
Prep commit for moving API documentation directory to be a top
level directory.
- Clean up the language.
- Add a prominent "Go to organization" button.
- Link to guides for new users and admins.
- Fix duplication bug in text email version.
Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>
Black 23 enforces some slightly more specific rules about empty line
counts and redundant parenthesis removal, but the result is still
compatible with Black 22.
(This does not actually upgrade our Python environment to Black 23
yet.)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Until now, custom emojis with "periods" in their name were allowed, even though
they don't really fit the pattern of how we name them, and in fact the Markdown
processor would not render such custom emoji. Fix this by just disallowing the
character.
Also update the error strings accordingly.
Note that this does not include a migration to eliminate any existing custom emoji with this
character in their name.
Fixes#24066.
This commit refactors the template code for source-realm
select element to have same structure as other inputs
and select element in the page. Thus this change also
makes the styling of source-realm select element consistent
with other select element in the page.
We change the do_create_user function to use transaction.atomic
decorator instead of using with block. Due to this change, all
send_event calls are made inside transaction.on_commit.
Some other changes -
- Remove transaction.atomic decorator from send_inital_realm_messages
since it is now called inside a transaction.
- Made changes in tests which tests message events and notifications
to make sure on_commit callbacks are executed.
This commit changes the do_reactivate_user such that the complete function
is called inside an atomic transaction and events are called after the
transaction is commited using on_commit helper. This is a prep commit
for unsubscribing the bots of unaccessible private streams when reactivating
them.
These files are not Jinja2 templates, so there's no reason that they needed
to be inside `templates/zerver`. Moving them to the top level reflects their
importance and also makes it feel nicer to work on editing the help center content,
without it being unnecessary buried deep in the codebase.
The content of a message is truncated to `MAX_MESSAGE_LENGTH`, which
is 1000 characters. Since the email gateway places attachments at the
very end of the extracted body, that means that they are the first
thing to get truncated off.
That is, if an incoming email message contains 1000 `a`s and an image
attachment, the link that attaches the attachment to the message will
get truncated off, leaving it dangling in the database.
Truncate the message body content separately from the attachment links
which are included at the end of the body.
Since we want to use `accounts/new/send_confirm` to know how many
users actually register after visiting the register page, we
added it to Google Tag Manager, but GTM tracks every user
registration separately due <email> in the URL
making it harder to track.
To solve this, we want to pass <email> as a GET parameter which
can be easily filtered inside GTM using a RegEx and all the
registrations can be tracked as one.
A missed message email notification, where the message is the welcome
message sent by the welcome bot on account creation, get sent when
the user somehow not focuses the browser tab during account creation.
No missed message email or push notifications should be sent for the
messages generated by the welcome bot.
'internal_send_private_message' accepts a parameter
'disable_external_notifications' and is set to 'True' when the sender
is 'welcome bot'.
A check is introduced in `trivially_should_not_notify`, not to notify
if `disable_external_notifications` is true.
TestCases are updated to include the `disable_external_notifications`
check in the early (False) return patterns of `is_push_notifiable` and
`is_email_notifiable`.
One query reduced for both `test_create_user_with_multiple_streams`
and `test_register`.
Reason: When welcome bot sends message after user creation
`do_send_messages` calls `get_active_presence_idle_user_ids`,
`user_ids` in `get_active_presence_idle_user_ids` remains empty if
`disable_external_notifications` is true because `is_notifiable` returns
false.
`get_active_presence_idle_user_ids` calls `filter_presence_idle_user_ids`
and since the `user_ids` is empty, the query inside the function doesn't
get executed.
MissedMessageHookTest updated.
Fixes: #22884
When the email mirror gateway is sending messages "as" a user (as
triggered by having access to the missed-message email address),
attachments were still created as the Email Gateway bot. Since the
sender (the end-user) was not the owner of those attachments (the
gateway bot), nor were they referenced yet anywhere, this resulted in
the attachments being "orphaned" and not allowed to be accessed by
anyone -- despite the attachment links being embedded in the message.
This was accompanied by the error:
```
WARN [] User 12345 tried to share upload 123/3LkSA4OcoG6OpAknS2I0SFAQ/example.jpf in message 123456, but lacks permission
INFO [zerver.lib.email_mirror] Successfully processed email from user 12345 to example-stream
```
We solve this by creating attachment objects as the users the message
will be sent from.
The max inline preview limit was previously increased to 10 by #20789.
However, as issue #23624 shows, it's still causing confusion for users
when they include more than 10 links.
Bump this limit up to 24, which is a multiple of the 4 image preview
per line logic.
Overrides the default context `allow_search_engine_indexing` to
always be `False` for `templates/corporate/attribution.html` so
that it does not appear in Google / search engine indexes.
Updates test of documentation pages in `test_docs.py` to have an
option for corporate pages to set this value in the template and
verifies that the meta tag for robots noindex, nofollow is
always in the response.
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.
When file uploads are stored in S3, this means that Zulip serves as a
302 to S3. Because browsers do not cache redirects, this means that
no image contents can be cached -- and upon every page load or reload,
every recently-posted image must be re-fetched. This incurs extra
load on the Zulip server, as well as potentially excessive bandwidth
usage from S3, and on the client's connection.
Switch to fetching the content from S3 in nginx, and serving the
content from nginx. These have `Cache-control: private, immutable`
headers set on the response, allowing browsers to cache them locally.
Because nginx fetching from S3 can be slow, and requests for uploads
will generally be bunched around when a message containing them are
first posted, we instruct nginx to cache the contents locally. This
is safe because uploaded file contents are immutable; access control
is still mediated by Django. The nginx cache key is the URL without
query parameters, as those parameters include a time-limited signed
authentication parameter which lets nginx fetch the non-public file.
This adds a number of nginx-level configuration parameters to control
the caching which nginx performs, including the amount of in-memory
index for he cache, the maximum storage of the cache on disk, and how
long data is retained in the cache. The currently-chosen figures are
reasonable for small to medium deployments.
The most notable effect of this change is in allowing browsers to
cache uploaded image content; however, while there will be many fewer
requests, it also has an improvement on request latency. The
following tests were done with a non-AWS client in SFO, a server and
S3 storage in us-east-1, and with 100 requests after 10 requests of
warm-up (to fill the nginx cache). The mean and standard deviation
are shown.
| | Redirect to S3 | Caching proxy, hot | Caching proxy, cold |
| ----------------- | ------------------- | ------------------- | ------------------- |
| Time in Django | 263.0 ms ± 28.3 ms | 258.0 ms ± 12.3 ms | 258.0 ms ± 12.3 ms |
| Small file (842b) | 586.1 ms ± 21.1 ms | 266.1 ms ± 67.4 ms | 288.6 ms ± 17.7 ms |
| Large file (660k) | 959.6 ms ± 137.9 ms | 609.5 ms ± 13.0 ms | 648.1 ms ± 43.2 ms |
The hot-cache performance is faster for both large and small files,
since it saves the client the time having to make a second request to
a separate host. This performance improvement remains at least 100ms
even if the client is on the same coast as the server.
Cold nginx caches are only slightly slower than hot caches, because
VPC access to S3 endpoints is extremely fast (assuming it is in the
same region as the host), and nginx can pool connections to S3 and
reuse them.
However, all of the 648ms taken to serve a cold-cache large file is
occupied in nginx, as opposed to the only 263ms which was spent in
nginx when using redirects to S3. This means that to overall spend
less time responding to uploaded-file requests in nginx, clients will
need to find files in their local cache, and skip making an
uploaded-file request, at least 60% of the time. Modeling shows a
reduction in the number of client requests by about 70% - 80%.
The `Content-Disposition` header logic can now also be entirely shared
with the local-file codepath, as can the `url_only` path used by
mobile clients. While we could provide the direct-to-S3 temporary
signed URL to mobile clients, we choose to provide the
served-from-Zulip signed URL, to better control caching headers on it,
and greater consistency. In doing so, we adjust the salt used for the
URL; since these URLs are only valid for 60s, the effect of this salt
change is minimal.
Moving `/user_avatars/` to being served partially through Django
removes the need for the `no_serve_uploads` nginx reconfiguring when
switching between S3 and local backends. This is important because a
subsequent commit will move S3 attachments to being served through
nginx, which would make `no_serve_uploads` entirely nonsensical of a
name.
Serve the files through Django, with an offload for the actual image
response to an internal nginx route. In development, serve the files
directly in Django.
We do _not_ mark the contents as immutable for caching purposes, since
the path for avatar images is hashed only by their user-id and a salt,
and as such are reused when a user's avatar is updated.
The `django-sendfile2` module unfortunately only supports a single
`SENDFILE` root path -- an invariant which subsequent commits need to
break. Especially as Zulip only runs with a single webserver, and
thus sendfile backend, the functionality is simple to inline.
It is worth noting that the following headers from the initial Django
response are _preserved_, if present, and sent unmodified to the
client; all other headers are overridden by those supplied by the
internal redirect[^1]:
- Content-Type
- Content-Disposition
- Accept-Ranges
- Set-Cookie
- Cache-Control
- Expires
As such, we explicitly unset the Content-type header to allow nginx to
set it from the static file, but set Content-Disposition and
Cache-Control as we want them to be.
[^1]: https://www.nginx.com/resources/wiki/start/topics/examples/xsendfile/
Enforcing a consistent `type` helps us double-check that we're not
playing fast-and-loose with any file paths for local files. As noted
in the comment, this is purely for defense-in-depth.
Passing `write_local_file` a consistent `type` requires removing the
"avatars" out of `realm_avatar_and_logo_path` -- which makes it
consistent across upload backends.
This, in turn, requires a compensatory change to zerver.lib.export, to
be explicit that the realm icons are exported from the avatars
directory. This clarity is likely an improvement.
sendfile already applied a Content-Disposition header, but the
algorithm may provide both `filename=` and `filename*=` values (which
is potentially confusing to clients) and incorrectly slash-escapes
quotes in Unicode strings.
Django provides a correct implementation, but it is only accessible to
FileResponse objects. Since the entire point is to offload the
filehandle handling, we cannot use a FileResponse.
Django 4.2 will make the function available outside of FileResponse.
Until then, extract our own Content-Disposition handling, based on
Django's.
We remove the very verbose comment added in d4360e2287, describing
Content-Disposition headers, as it does not add much.
Add more tests analogous to existing ones but for different scenarios.
This is mostly boring text, but is important for completeness, since the
notificability logic underneath is subtle.
Split the one giant `test_end_to_end_missedmessage_hook` into many
smaller tests.
This allows us to not worry about resetting database state after each
test case and also allows extracting a lot of common stuff into setUp
and tearDown.
There is probably even more scope of deduplication here (for example,
the mock and the `assert_maybe_enqueue_notifications_call_args` call are
same for all test cases) but that might not be worth the added
complexity.
We also change a few
```
user_profile.<setting> = <value>
user_profile.save()
```
expressions to instead use the `do_change_user_setting` function.
For alert words, we currently don't send email/push notifications --
only desktop notifications. Thus, we don't need to consider alert words
here, since desktop notifications do not utilize the presence status
calculated at this stage.
Tested manually that alert word desktop notifications work as expected.
When we implement email/push notifications for alert words (issues #5137
and #13127), we can add new fields like
`notifications_data.alert_word_email_notify`, similar to the existing
`notifications_data.wildcard_mention_email_notify`, which will allow us
to keep the alert word notifiability check inside the dataclass, similar
to how the mentions checks are done currently. So, even when that
feature is implemented, the code which this commit removes would be
unnecessary.
Prakhar Pratyush
Alex Vandiver
Sahil Batra
Aman Agrawal
Mateusz Mandera
Anders Kaseorg
Tim Abbott
Alya Abbott
Prakhar Pratyush
Lauryn Menard
roanster007
kimry02
Lalit
Riken Shah
David
Greg Price
Hemant Umre
Sayyed Arib Hussain
Esther Anierobi
lapaz
Simon Michalke
Pedro Madriz
Steve Howell
N-Shar-ma
Adrián Oliva
Zixuan James Li
Ujjawal Modi
Eeshan Garg
Akshat
Danny Su
Samuel
evykassirer
Joelute
Zixuan Li
nimish
abdullahm1
arghyadeep10
evykassirer
rht
AcKindle3
Daniil Fadeev
Ruchir Harbhajanka
Akarsh Jain
Abhijeet Prasad Bodas
Kartik Srivastava
m-e-l-u-h-a-n
Alessandro Toppi
Alya Abbott
Lauryn Menard
Trident Pancake