This change lets us eliminate the need for new authentication backends
to edit get_auth_backends_data, since we're just computing it from the
official registry in zproject/backends.py. Should save a few lines of
work whenever we add a new auth backend, and make that more accessible
to new contributors.
This adds a new realm_logo field, which is a horizontal-format logo to
be displayed in the top-left corner of the webapp, and any other
places where we might want a wide-format branding of the organization.
Tweaked significantly by tabbott to rebase, fix styling, etc.
Fixing the styling of this feature's loading indicator caused me to
notice the loading indicator for the realm_icon feature was also ugly,
so I fixed that too.
Fixes#7995.
Support for extended mention syntax was added as a part of
commit fbe99b812ee8fbca7257a5b7156c57a6cd74195b in the
python-zulip-api repository. The relevant function,
extract_query_without_mention now relies on the client's ID
in order to check for the extended syntax. Since the
EmbeddedBotHandler has no user_id attribute, the latest
python-zulip-api release broke a test in the main repo.
This is a simple change to our validation, to allow multiple copies of
the main linkifier syntax, which lets us support things like generic
GitHub URLs.
Fixes#10914.
Apparently, when we renamed these files to no longer have a .txt
extension, we accidentally removed them from the set of strings for
translation, because `manage.py makemessages` by default only
processes .txt and .html files under the templates/ directory.
Fix this by adding a .txt extension.
It appears that our i18n logic was only using the recipient's language
for logged-in emails, so even properly tagged for translation and
translated emails for functions like "Find my team" and "password
reset" were being always sent in English.
With great work by Vishnu Ks on the tests and the to_emails code path.
The previous version was also doing almost the same thing.
But checking for DEVELOPMENT_LOG_EMAILS would allow us
to control the call of send_email by altering the value
of DEVELOPMENT_LOG_EMAILS in tests.
This form isn't actively used, which is how it ended up broken, but it
basically didn't display its content properly at all.
Convert it to use our standard white-box framework.
This still doesn't look great in various ways, but it's at least not
obviously totally busted now.
The previous logic for soft deactivation ended up doing a giant
transaction in the case that there were thousands of users to
deactivate; this was messy and potentially buggy.
The batched transactions were useful for RealmAuditLog management,
however. So the right solution is to do reasonably sized batches
(e.g. 100 users).
Apparently, our do_batch_update method (used, e.g., in a pgroonga
migration) was using semi-invalid syntax that was removed in postgres
10.
Thanks to Ilya Evseev for the report.
Fixes#11063.
This should make it possible for blueslip error reports to be sent on
our logged-out portico pages, which should in turn make it possible to
debug any such issues as they occur.
This checks if push_notification_enabled() is set to false in
handle_push_notification and adds an early return statement.
This is a significant performance optimization for our unit tests
because the push notifications code path does a number of database
queries, and this migration means we don't end up doing those queries
the hundreds of times we send PMs or mentions in our tests where we're
not trying to test the push notifications functionality.
This should also have a small message sending scalability improvement
for any Zulip servers without push notifications enabled.
Tweaked by tabbott to fix a few small issues.
Fixes#10895.
While reviewing #11012, I discovered a nondeterministic result for
test_signup, which I tracked down to specifically this triple of tests
failing when run in this order:
test-backend GCMSuccessTest \
zerver.tests.test_push_notifications.TestAPNs.test_get_apns_client \
zerver.tests.test_signup.LoginTest.test_register
with a query count mismatch like this:
expected length: 73
actual length: 79
Comparing the list of queries, it's clear that test_register was
seeing `push_notifications_enabled()` returning True in this test order.
It's not clear why GCMSuccessTest was required here (it was!), but
further debugging determined the problem was that
`test_get_apns_client` left the _apns_client initialization system in
a state where get_apns_client would return a non-None value, resulting
in push_notifications_enabled() returning True for future tests.
The immediate fix is to just reset the `_apns_client` and
`_apns_client_initializedstate` state properly after the test runs;
but arguably we should do a larger refactor to make this less
fragile.
The previous migration code path was broken in two ways:
* ScheduledEmail objects generally contain a `None` value for
whichever of `to_user_id` and `to_email` isn't in use; this could
result in us sending a [None] to send_email(), which doesn't make
sense.
* We were calling handle_send_email_format_changes in the wrong order
with respect to the JSON loading process.
Thanks to Tom Daff for the report!
Our list of allowed characters in realm filter patterns has long been
too string; fix this by extending the pattern.
Also, extend the tests to have examples of actual strings one would
use with the patterns, for clarity.
Fixes#10953, fixes#6835.
Technically, we will only need to process deactivated users for the
purpose of reactivating them (and can ignore, e.g., name changes).
But it's simplest to just process them unconditionally.
This should make life a lot more convenient for organizations that use
the LDAP integration and have their avatars in LDAP already.
This hasn't been end-to-end tested against LDAP yet, so there may be
some minor revisions, but fundamentally, it works, has automated
tests, and should be easy to maintain.
Fixes#286.
Fixes a bug in import_realm where secondary attributes like message
visibility weren't being set, and also makes bugs like this less likely in
the future.
Also, putting the plan_type change at the end of import_realm, so that
future restrictions to LIMITED realms don't affect the import process.
Apparently, Django's get_current_site function (used, e.g., in
django-two-factor to look up the domain to use in QR codes) first
tries to use the Sites framework, and if unavailable, does the right
thing (namely, using request.get_host()).
We don't use the Sites framework for anything in Zulip, so the correct
fix is to just remove it.
Fixes#11014.
The Slack import process would incorrectly issue
CustomProfileFieldValue entries with a value of "" for users who
didn't have a given CustomProfileField (especially common for the
"skype" and "phone" fields). This had no user-visible effect, but
certainly added some clutter in the database.
We should rate-limit users when our rate limiter deadlocks trying to
increment its count; we also now log at warning level (so it doesn't
send spammy emails) and include details on the user and route was, so
that we can properly investigate whether the rate-limiting on the
route was in error.
The code paths for accessing user-uploaded files are both (A) highly
optimized so as to not require a ton of work, and (B) a code path
where it's totally reasonable for a client to need to fetch 100+
images all at once (e.g. if it's the first browser open in a setting
with a lot of distinct senders with avatars or a lot of image
previews).
Additionally, we've been seeing exceptions logged in the production
redis configuration caused by this code path (basically, locking
failures trying to update the rate-limit data structures).
So we skip running our current rate limiting algorithm for these views.
Now that we've styled this feature properly, this makes it possible to
copy various user-preferences type profile data in production when
making a new account with the same email address as an existing
account.
This styles the avatar and username that show when the registering
user is importing their settings from an existing Zulip account.
Tweaked by tabbott to fix the test/linter failures, a bit of styling,
and tag strings for translation.
Apparently, while the main code path through
login_or_register_remote_user was correctly calling
remote_user_to_email(username) to get a proper email address for
situations where auth username != email (i.e. when SSO_APPEND_DOMAIN
is set), we neglected to do so in the mobile_flow_otp corner case.
Fixes#11005.
Broaden the type of the AbstractEnum __reduce_ex__ parameter to object; this
matches the parameter type specified in the latest enum.pyi file in typeshed.
Fixes#10996.
Also, add a new notification sound, "ding". It comes from
https://freesound.org, where the original Zulip notification sound comes
from as well. In the future, new sounds can be added by adding audio
files to the `static/audio/notification_sounds` directory.
Tweaked significantly by tabbott:
* Avoided removing static/audio/zulip.ogg, because that file is
checked for by old versions of the desktop app.
* Added a views check for the sound being valid + tests.
* Added additional tests.
* Restructured the test_events test to be cleaner.
* Removed check_bool_or_string.
* Increased max length of notification_sound.
* Provide available_notification_sounds in events data set if global
notifications settings are requested.
Fixes#8051.
A key part of this is the new helper, get_user_by_delivery_email. Its
verbose name is important for clarity; it should help avoid blind
copy-pasting of get_user (which we'll also want to rename).
Unfortunately, it requires detailed understanding of the context to
figure out which one to use; each is used in about half of call sites.
Another important note is that this PR doesn't migrate get_user calls
in the tests except where not doing so would cause the tests to fail.
This probably deserves a follow-up refactor to avoid bugs here.
This is preparatory work for settings controlling who can see user
emails; it includes the API-level support for editing it, but no code
to actually enforce the policy.
We've had a long stream of bugs existed because only one of these two
code paths was tested (usually the local uploads backend). By
deduplicating these functions, we ensure that this category of bugs no
longer happens.
Following my recent refactor, this is just a straightforward merge,
with code for one or the other backend ending up inside an if
statement.
Previously, we were incorrectly importing avatar PNGs to a filename
without the .png extension, resulting in them effectively not being
imported.
This was mitigated by the fact that we imported the originals and ran
the appropriate `ensure_` functions, but still a bug.
Apparently, there was a bug in notify_bot_owner_on_invalid_json, where
we didn't reraise the JsonableError.
We fix this with a refactoring that makes the exception layering
clearer as well.
In a quick scan of today's nginx logs on chat.zulip.org, there
were 20 distinct user-agents that begin with 'ZulipMobile/'.
Here's a representative sampling of them, such that the rest
were all boringly similar to one of these.
First, to make room for these without an excess of copy-paste and
overlong lines, convert this test to a data-oriented style. The
existing, synthetic cases appear in the new data followed by the
seen-in-the-wild cases.
Happily, the code being tested passes all these new cases unchanged.
This release is from 2018-08-22, a little over 100 days ago.
It was the first release with the important fix so that when the
server advises it to stop displaying a notification because the user
has read the message (as the SEND_REMOVE_PUSH_NOTIFICATIONS server
setting enables), the app doesn't instead replace the notification
with a broken one reading "null". We have that setting running now
on chat.zulip.org, and intend to roll it out more broadly soon.
The `# take 0` thing is a slightly absurd workaround for the fact
that our funky out-of-line way of marking lines to ignore doesn't
work right if there are multiple such lines in a given file that
are equal modulo leading and trailing whitespace.
This makes it more convenient for developers to set very short values
for this (e.g. 1 minute) for the purposes of testing/debugging; there
aren't obvious problems with letting users set short values for this.
This works by yielding messages sorted based on timestamp. Because
the Slack exports are broken into files by date, it's convenient to do
a 2-layer sorting process, where we open all the files for a given
day, and then sort their messages by timestamp before yielding them.
Fixes#10930.
If a user deletes message between when it triggered a potential push
notification for another user, and when that notification was actually
sent, we'd end up with a situation where the `Message` table didn't
have an entry for the requested message ID.
The clean fix for this is to still throw an exception in the event
that the message doesn't exist at all, but if it exists in
ArchivedMessage, don't throw a user-facing exception.
This seems to happen when Apple is having a partial outage on some of
their APNS shards; it should be treated like other networking errors
connecting to APNS (with an automatic retry).
Apparently, we have a second code path where we might try to call
send_email library functions on old data, namely in the
queue_processors codebase. So we apply the same migration logic here.
This adds a function that sends provided email to all administrators
of a realm, but in a single email. As a result, send_email now takes
arguments to_user_ids and to_emails instead of to_user_id and
to_email.
We adjust other APIs to match, but note that send_future_email does
not yet support the multiple recipients model for good reasons.
Tweaked by tabbott to modify `manage.py deliver_email` to handle
backwards-compatibily for any ScheduledEmail objects already in the
database.
Fixes#10896.
While this would never happen for a real article, this prevents a 500
in this case for a situation which is definitely user error and should
be a 40x (in this case, 404).
As part of this, we refactor the main view code to do validation in a
single code path, since the semi-duplicated-in-3-places logic was
getting pretty buggy.
This prevents the warning about push notifications not being
registered for from being printed in development environment startup
by default. In development, that's the expected state, and we don't
need to spam up the output with that notice.
Previously, we frequently accessed user_profile.realm from outside the
loops that interact with UserProfile objects. This variable reuse
outside the loop could be confusing and should be a style/lint
violation.
While in this case, the behavior was correct (in that all users in the
loops were within the same realm), extracting a separate `realm`
variable significantly clarifies what's going on here.
The previous implementation used run_parallel incorrectly, passing it
a set of very small jobs (each was to download a single file), which
meant that we'd end up forking once for every file to download.
This correct implementation sends each of N threads 1/N of the files
to download, which is more consistent with the goal of distributing
the download work between N threads.
There are (at least) two types of objects that could be sent with a
charge.succeeded event, a Charge (e.g. for credit cards) or a Payment (if
they pay by ACH). We were handling the first but not the second.
This commit also updates the fixture for the existing charge.succeeded event
to the latest API version.
This reverts commit 2fa77d9d54.
Further investigation has determined that this did not fix the
password-reset problem described in the previous commit message;
meanwhile, it causes other problems. We still need to track down the
root cause of the original password-reset bug.
This provides a nice user experience for folks where we do know what
their LDAP credentials are.
Though we need to fix#10917 before the content in the email with be
correct.
This is the first step of letting users use Zulip markdown in their
SHORT_TEXT and LONG_TEXT custom profile fields, so that they can
include emphasis, links, etc.
This doesn't include any frontend logic yet, however.
Recent changes merged in #10877 didn't handle these events
correctly. The linkified_id function breaks for the `discount`
object in the JSON payload. A cursory glance at Stripe's docs
tells me that since a discount is associated with a customer
or a coupon, it makes sense for a `discount` object to not have
an ID that can necessarily be linked to. So, we can just link
to the associated coupon instead.
This commit changes the return type of get_possible_mentions_info to a
list instead of a dict, thus disposing off the hacky logic of storing
users with duplicate full names with name|id keys that made the code
obfuscated.
The other functions continue to use the dicts as before, however, there
are minor variable changes where needed in accordance with the updated
definition of get_possible_mentions_info.
This function is equivalent to recipient_for_emails, but fetches
user_profiles by IDs, not by emails.
This commit is a part of our efforts surrounding #9474, but is
more primarily geared towards adding support for sending typing
notifications by user IDs.
Previously, get_user_profiles() was split into two functions:
* user_profiles_from_unvalidated_emails, which raised a
ValidationError upon encountering a non-existent user email.
* get_user_profiles, which caught the ValidationError raised
by user_profiles_from_unvalidated_emails and raised a
JsonableError instead.
According to Steve Howell, this complexity is partly a relic
of past refactoring and is unnecessarily heavy. It is better to
just raise JsonableError directly.
recipient_for_emails is used by our typing notifications code.
user_profiles_from_unvalidated_emails is used by our typing
notifications code *and* for sending messages.
user_profiles_from_unvalidated_emails is a part of a larger
framework used by Addressee to validate recipient emails when sending
messages and will eventually need to be removed as we move forward
with #9474. So it makes sense to just inline this function within
recipient_for_emails so that we don't break our typing notifications
code in the future.
This commit is a part of our efforts surrounding #9474.
This library was absolutely essential as part of our Python 2->3
migration process, but all of its calls should be either no-ops or
encode/decode operations.
Note also that the library has been wrong since the incorrect
refactoring in 1f9244e060.
Fixes#10807.
This adds a web flow and management command for reactivating a Zulip
organization, with confirmation from one of the organization
administrators.
Further work is needed to make the emails nicer (ideally, we'd send
one email with all the admins on the `To` line, but the `send_email`
library doesn't support that).
Fixes#10783.
With significant tweaks to the email text by tabbott.
The previous content made it sound like we were actually sending a
push notification, which could be confusing/alarming in some cases
(see e.g. 9c224ccdd3). Instead, we make
clear that we're sending it to all clients (which one might correctly
suspect is vacuous in the development environment).
While it could make sense to print these logging statements at WARN
level on server startup, it doesn't make sense to do so on every
message (though it perhaps did make sense to do so before more recent
changes added good ways to discover you forgot to configure push
notifications).
Instead, we now just do a WARN log on queue processor startup, and
then at DEBUG level for individual messages.
Fixes#10894.
For messages with strange senders, we don't import
messages. Basically, we only import a message if
it has sender with an id that maps to a non-deleted
user.
We now account for streams having users that
may be deleted. We do a couple things:
- use a loop instead of map
- only pass in users to hipchat_subscriber
- early-exit if there are not users
- skip owner/members logic for public streams
Change the truncation marker from `...` to `\n[message truncated]`
when receiving messages from the API or through e-mail. Also, update
tests to account for the new change.
Fix#10871.
There are only a handful of non-JSON webhooks that wouldn't
benefit from the notify_bot_owner_on_invalid_json feature.
Specifically, these are the webhooks where the third-party product
uses another format, whether it be HTML form-encoded, XML, or
something else.
Tweaked by tabbott to correc the list of excluded webhooks.
Previously, the Stripe webhook code was riddled with implicit
assertions that there were exactly N event types within a given
category, and we handled the last one in a final `else` clause in the
block. This was likely to cause confusing problems in the event that
we're missing an event type (or Stripe adds a new one).
We fix this by just adding a few more conditionals and raising the
standard "unexpected event type" exception for the others.
Our recent change in 2fa77d9d54 to
disable the cached_db cache backend broke upgrade-zulip-from-git with
an attributeerror; we fix that by checking the session engine before
trying to access its cache-related attributes.
Removes email_not_verified option. That option was used to assign
email_data a different set of emails for a test. Instead of that,
this refactor allows to specify the email_data itself in the function
which calls github_oauth2_test. Flags like email_not_verified are
generally used in one test. This is a preparatory refactor for
choose email screen which may have introduced multiple flags otherwise.
The email_list returned has the primary email as the first element.
Testing: The order of the emails in the test was changed to put a
verified email before the primary one. The tests would fail without
this commit's change after the changes in the order of test emails.
This is initial work, which will help us establish habits of using a
well-tested approach for renaming a Zulip organization (since as part
of https://github.com/zulip/zulip-mobile/issues/3142, we'll likely
need to make this function do more).
This seems like kind of a silly function to extract
to topic.py, but it will theoretically help us sweep
"subject" if we change the DB.
It had test coverage.
We use the message a lot for the query modified
here, so I think it's worth taking the up-front
hit of getting bulkier objects to avoid O(N)
hops back to the database.
Normal hipchat exports use integer ids for their
users and "rooms," which we just borrowed during
conversion.
Atlassian Stride uses stride UUIDs for these instead, but otherwise
has the same export format.
We now introduce IdMapper to handle external ids
that aren't integer. The IdMapper will map UUID
ids to ints and remember them. For ints it just
leaves them alone.
Fixes#10805.
Our webhook-errors.log file is riddled with exceptions that are
logged when a webhook is incorrectly configured to send data in
a non-JSON format. To avoid this, api_key_only_webhook_view
now supports an additional argument, notify_bot_owner_on_invalid_json.
This argument, when True, will send a PM notification to the bot's
owner notifying them of the configuration issue.
Until we resolve https://github.com/zulip/zulip/issues/10832, we will
need to maintain our own forked copy of Django's SessionMiddleware.
We apparently let this get out of date.
This fixes a few subtle bugs involving the user logout experience that
were throwing occasional exceptions (e.g. the UpdateError fix you can
see).
We (lexically) remove "subject" from the conversion code. The
`build_message` helper calls `set_topic_name` under the hood,
so things still have "subject" in the JSON.
There was good code coverage on `build_message`.
We're trying to sweep "subject" out of the codebase,
even when it has nothing to do our legacy "subject"
field. The rewording here will prevent some linter
noise.
This was a pretty nasty error, where we were accidentally accessing
the parent list in this inner loop function.
This appears to have been introduced as a refactoring bug in
7822ef38c2.
The various vars here that had recipient_subject
in the name now have either bucket or bucket_tup
there.
The shorter names are a bit easier to read, and the
original names were misleading for the PM case.
This was basically two search/replaces, and we have
good test coverage here, so it's pretty low risk
despite the messy diff.
We now attach zulip_db_data to the markdown engines
for classes that need it. This was the last remaining
global we had, so we remove `arguments.py` here.
The Markdown processor makes it fairly simple for
the helper classes to access the `md` engine. We
now write `_md_engine.zulip_message` to avoid having
the current message in the global namespace.
Note that we do reuse engines for multiple messages,
but each engine is specific to a realm. And we therefore
avoid even the theoretical possibility of leaking message
data between realms.
This makes us consistent with how we import codehilite.
Using Python's normal import mechanism avoids some overhead
with Markdown having to parse dotted notation.
These modules are tiny, so they shouldn't impact startup
too much. Also, by explicitly importing them, we avoid
the pitfall of having a sucessful startup and a broken
renderer.
We were building the same link regex every time
we build a Markdown engine, which happens twice
per realm. It's an expensive operation due to
the complexity of the regex and us reading a file.
Nested classes are kind of expensive in Python,
particularly when you throw in mypy annotations.
Also, flatter is arguably better, although it is
kind of a pain here not to have closures.
This change avoids hitting the Django ORM when
we don't find any possible group mentions in
the message content.
Django doesn't necessarily actually hit the database,
but it's still slow and shows up in profiles.
This commit speeds up the import by avoiding
sender lookups and instead using the data
for users that we already have in memory.
This avoids a few DB hops, many hops to memcached,
plus some object construction.
We now call do_render_markdown() directly. This
also makes it more explicit that the import has
never rendered alert words.
For the import-data codepath, we will call
the extracted function directly in a
subsequent commit.
The do_render_markdown() function has more
required parameters, which allows for more
explicit code and also allows us to flatten
out some logic related to alert words. (We
just pass in empty sets/dicts as needed).
We can rely on `message_realm` being the same
as `message.sender.realm`, which allows us to
skip two queries to the database for the rare
Zephyr mirroring case.
This function requires a message object, whereas
we want to work with JSON data to avoid necessary
queries when we import data. Inlining the function
sets us up for a subsequent refactoring.
We change the way we deal with theoretical return
values of `None` to use an assertion; otherwise,
we would have to loosen up a bunch of mypy types
from `str` to `Optional[str]`. It's not clear `None`
is even possible--we've moved toward throwing exceptions
there instead of silently failing.
This is somewhat hairy logic, so it's nice
to extract it and not worry about variable leaks.
Also, this moves some legacy "subject" references out
of actions.py.
We start by including functions that do custom
queries for topic history.
The goal of this library is partly to quarantine
the legacy "subject" column on Message.
A recent change to check_send_webhook_message allows webhooks to
unescape stream names before sending a message. This commit adds
a test for the edge case where the webhook URL is escaped twice by
a third-party.
Recently, one of our users reported that a JIRA webhook was not
able to send messages to a stream with a space character in its
name. Turns out that JIRA does something weird with webhook URLs,
such that escaped space characters (%20) are escaped again, so
that when the request gets to Zulip, the double escaped %20 is
evaluated as the literal characters `%20`, and not as a space.
We fix this by unescaping the stream name on our end before
sending the message forward!
The previous logic was incorrect, in that if `content_type` was set to
None (which happens with Slack/HipChat export, among other things),
then we wouldn't run the `guess_type` logic to auto-detect the
Content-Type to send to S3.
The last_modified field is intended to support setting the
orig-last-modified field in the S3 backend when importing, basically
to keep track of this bit of pre-export data for debugging. In the
event that it isn't available, the correct thing to do is not write
out an invalid `last_modified` field; we should just not write it out
at all.
This fixes the fact that these emoji were sometimes not displaying
properly (because of changes in the emoji names used in the codebase),
while also making this integration more standard (since it was the
only one with such an aggressive use of emoji).
The UserMessage table can be huge, so creating a
bunch of entries in `ID_MAP` can overflow memory.
We don't have any tables that depend on `UserMessage`,
and we don't send the 'id' fields from `zerver_usermessage`
to the database, so re-mapping them was just busy-work.
seresheim
Tim Abbott
Eeshan Garg
Joshua Pan
Vishnu Ks
ishanrai05
Rishi Gupta
Roger Souza
Roman Godov
Max Nussenbaum
Jack Zhang
neiljp (Neil Pilgrim)
Marco Burstein
rht
Greg Price
Anders Kaseorg
Raymond Akornor
Rohitt Vashishtha
Hemanth V. Alluri
Steve Howell
Marco Burstein
Cynthia Lin
Tim Abbott
Shubham Padia
sameerchoubey