A user can subscribe to a stream and sometimes (depending
on stream permissions) see messages from the stream
that were sent before they subscribed, and that user
won't have a UserMessage row for that message.
In order to do things like star a message, we need
to create UserMessage records on the fly.
In the past we wisely constrained this logic to the
specific use cases. But I think we can generalize
the logic now. For example, we are now building a
feature to mark messages as unread, and it motivates
the same need to auto-create UserMessage rows.
So now we handle this in a more generalized fashion.
Refactors and cleans up the shared `MessagesBase` schema in
the OpenAPI so that it accurately reflects the general base
for message objects for endpoints that use it as a reference.
A follow-up to adding `edit_history` as a property of message
objects. And a prepartory commit for `GET /messages/{msg_id}`
to return not only the raw content of the message but also
the message object.
When removing notifications, we skip the access control on if the user
still can read them -- they should not have a notification of them,
both because they currently cannot read the message, as well as
because they have already done so.
Translators found it confusing, since it's not at all obvious that the
word "quote" should not be translated.
I'm not altogether happy with the code formatting for this.
While we're changing this, also standardize on the "```` quote" style
of quote blocks to ensure code/quote blocks in stream descriptions are
unlikely to conflict with this syntax.
In steady-state, requests to FCM take about a second; however, in
cases where the remote FCM server is unstable, the request has been
observed to block for more than a minute.
As noted in the previous commit, pushes must complete within 30s;
fail fast, and let the retries and exponential backoff handle errors.
The worst-case total time taken with timeouts and errors for an FCM
notification is 19.5s. Unfortunately, `aioapns` does not appear to
have any timeouts, and thus this commit cannot guarantee a total of
fewer than 30s.
This reverts bc15085098 (which provided
not justification for its change) and moves further, down to 2 retries
from the default of 5.
10 retries, with exponential backoff, is equivalent to sleeping 2^11
seconds, or just about 34 minutes (though the code uses a jitter which
may make this up to 51 minutes). This is an unreasonable amount of
time to spend in this codepath -- as only one worker is used, and it
is single-threaded, this could effectively block all missed message
notifications for half an hour or longer.
This is also necessary because messages sent through the push bouncer
are sent synchronously; the sending server uses a 30-second timeout,
set in PushBouncerSession. Having retries which linger longer than
this can cause duplicate messages; the sending server will time out
and re-queue the message in RabbitMQ, while the push bouncer's request
will continue, and may succeed.
Limit to 2 retries (APNS currently uses 3), and results expected max
of 4 seconds of sleep, potentially up to 6. If this fails, there
exists another retry loop above it, at the RabbitMQ layer (either
locally, or via the remote server's queue), which will result in up to
3 additional retries -- all told, the request will me made to FCM up
to 12 times.
This model is by designed intended to exist on a 1:1 relationship with
Realms, and we attempt to ensure that with application code, but we
should have a unique constraint too, since a database with duplicate
such entries would be corrupted.
We do this via the standard Django OneToOneField.
It was there to work around https://bugs.python.org/issue17519. This
workaround with del seems like a partial improvement.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We want TypedDicts that have actual teeth.
In order to make type checks meaningful, we want
to avoid Any, object, or crazy Union types when
we aggregate each type of message, so we replaced
a generic function with three concrete functions.
Provide stream privacy and description in stream notification events
when stream is created.
In function "send_messages_for_new_subscribers" for when stream is
created, put policy name and description of the stream.
Fixes#21004
Removes `LEGACY_PREV_TOPIC` which is no longer needed due to the
message edit history migration.
Also remove additions to the linter exclude list that were added
earlier in this commit series.
Since we've changed the database to contain these new fields, we just
need to stop dropping them in the API code.
This also changes the public API to match the database format again
by removing `prev_subject` from edit history API.
Adds an API changelog feature update for the renamed `prev_subject`
field (to `prev_topic`) and new fields (`topic` and `stream`)
in the message `edit_history`.
Also, documents said `edit_history` in the `MessagesBase` schema
in the api documentation, which is used by the `/get-messages`,
`/get-events` and `/zulip-outgoing-webhooks` endpoints.
Fixes#21076.
Co-authored-by: Lauryn Menard <lauryn.menard@gmail.com>
Now that we have code to support reading prev_topic, this is no longer
necessary.
We'll want to deploy this change to production before running the
migration to remove prev_subject from edit history entries, so that
prev_subject can be fully purged from the database.
We modify the message_edit_history marshalling code so that this
commit does not change the API, since we haven't backfilled the data
yet.
FormattedEditHistoryEvent, introduced in the previous commit, doesn't
directly inherit fields from EditHistoryEvent, so no changes are
required there.
We fix the mutation of caller and other bad patterns, as well as
adding explicit typing to make the code readable.
We also update the OpenAPI documentation for previously
undocumented `prev_strem` field in the `/get-message-history`
endpoint for API validation testing.
Co-authored-by: Lauryn Menard <lauryn.menard@gmail.com>
These types will help make iteration on this code easier.
Note that `user_id` can be null due to the fact that
edit history entries before March 2017 did not log
the user that made the edit, which was years after
supporting topic edits (discovered in test deployment
of migration on chat.zulip.org).
Co-authored-by: Lauryn Menard <lauryn.menard@gmail.com>
Moves the encodeHashComponent and decodeHashComponent functions out of
hash_util and into internal_url which belongs to shared. This is to
accommodate sharing of this code with mobile or any other codebases that
do not wish to duplicate logic.
TestMaybeSendToRegistration needs tweaking here, because it wasn't
setting the subdomain for the dummy request, so
maybe_send_to_registration was actually running with realm=None, which
is not right for these tests.
Also, test_sso_only_when_preregistration_user_exists was creating
PreregistrationUser without setting the realm, which was also incorrect.
create_preregistration_user is a footgun, because it takes the realm
from the request. The calling code is supposed to validate that
registration for the realm is allowed
first, but can sometimes do that on "realm" taken from something else
than the request - and later on calls create_preregistration_user, thus
leading to prereg user creation on unvalidated request.realm.
It's safer, and makes more sense, for this function to take the intended
realm as argument, instead of taking the entire request. It follows that
the same should be done for prepare_activation_url.
In these tests, the code ends up with a logged in session when it's
undesired - later on these tests make requests to a different subdomain
- where a logged in session is not supposed to exist. This leads to an
unintended, strange situation where request.user is a user from the old
subdomain but the request itself is to a *different* subdomain. This
throws off get_realm_from_request, which will return the realm from
request.user.realm - which is not what these tests want and can lead to
these tests failing when some of the production code being tested
switches to using get_realm_from_request instead of
get_realm(get_subdomain).
The codepaths for joining an organization via a multi-use invitation
(accounts_home_from_multiuse_invite and maybe_send_to_registration)
weren't validating whether
the organization the invite was generated for matches the organization
the user attempts to join - potentially allowing an attacker with access
to organization A to generate a multi-use invite and use it to join
organization B within the same deployment, that they shouldn't have
access to.
The database value for expiry_date is None for the invite
that will never expire and the clients send -1 as value
in the API similar to the message retention setting.
Also, when passing invite_expire_in_days as an argument
in various functions, invite_expire_in_days is passed as
-1 for "Never expires" option since invite_expire_in_days
is an optional argument in some functions and thus we cannot
pass "None" value.
A migration should not import zerver.lib.streams at all, but this
solves the immediate problem with check-database-compatibility.py.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Removes `client` parameter from backend tests using the
`POST /messages` endpoint when the test can use the default
`User-Agent` as the client, which is set to `ZulipMobile` for API
requests and a browser user agent string for web app requests.
Various tests use the `PATCH /stream/{stream_id} endpoint in
`test_subs.py`. Because the stream id is in the URL path, it
does not also need to be passed as a query parameter.
Removes instances of `stream_name` being passed as a query
parameter to tests.
Removes `topic_name` parameter in `test_message_flags.py`
where is being passed to a test for marking a stream as
read because it is an ignored parameter for that endpoint.
Steve Howell
Lauryn Menard
Alex Vandiver
Tim Abbott
byshen-dev
Anders Kaseorg
Mateusz Mandera
Dinesh
NerdyLucifer
Lauryn Menard
Austin Riba
Sahil Batra