The first bug fixed here has been around for a long
time--we were redundantly updating unread counts
indirectly via muting_ui.initialize(). The
unread counts also get updated in
unread_ui.initialize(), when we have more valid
state. (And it's worth noting here that the unread
counts get updated yet again once message fetches
complete.)
The second bug was a very recent regression from
my recent stream name -> stream id cleanup in the
muting system. We now depend on stream_data to
initialize muting data, so we need to initialize
muting.js slightly later in the process.
These fixes are intertwined, because they were both
somewhat caused by the anti-pattern of having
muting_ui.js initialize unread_ui.js and muting.js,
instead of doing more direct, fine-grained initialization
from ui_init.js.
Essentially we replace this code:
exports.update_muted_topics = function (muted_topics) {
muting.set_muted_topics(muted_topics);
unread_ui.update_unread_counts();
};
with this:
exports.initialize = function () {
exports.set_muted_topics(page_params.muted_topics);
};
And the modules load like this:
stream_data
...
muting
...
unread_ui
And we don't need any page-load initialization for muting_ui,
which is mostly used for Settings/Muted topics.
This function used to be called initialize_from_page_params(),
and we called it indirectly through `subs.js`.
Now we call it directly from `ui_init.js`, which gives us a
bit more control over how things are initialized. In fact,
this sets us up for the next commit, where I fix a recent
regression I introduced.
This is mostly about cleaning up the naming convention
for streams and topics, but it also adds a test that
specifically tests the muted-topic case (without any
other factors that would prevent a notification).
Before this commit, it was possible to change the
API for muting topics and get false positives, even
when the test setup was clearly broken.
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 data attribute here has some value if you're
inspecting the HTML in the browser, but it's not
worth the extra code.
All the list items have data-stream-id, so there's
no need for the parent to have it.
The stream_list test that was fixed here was sort of
broken. It accomplished the main goal of verifying
what gets rendered, but now the data setup part is
more like the actual app code (and simpler, too).
This fixes the most core data structures inside of
muting.js. We still use stream names for incoming
data to set_muted_topics and outgoing data from
get_muted_topics.
This will make us more resilient to stream name changes.
Before, if you were logged on when a stream rename
occured, topics that were muted under that stream would
appear to be unmuted. (You could fix it with a reload,
but it can be jarring to have a bunch of unread messages
appear in your feed suddenly.)
Fixes#11033
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.
The fixture changes are because self.upgrade formerly used to cause a page load
of /billing, which in turn calls Customer.retrieve.
If we ran the full test suite with GENERATE_STRIPE_FIXTURES=True, we would
likely see several more Customer.retrieve.N.json's being deleted. But
keeping them there for now to keep the diff small.
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.
This makes it possible to add --skip-purge-old-deployments in the
deploy_options section of /etc/zulip/zulip.conf, and control whether
old deployments are purged automatically on a system.
We still need to do https://github.com/zulip/zulip/issues/10534 and
probably also to add these arguments to be directly passed into
upgrade-zulip, but that can wait for future work.
Fixes#10946.
Like the other similar commits, we were doing the same work in all
code paths, just with a much more error-prone approach.
We can also now remove the now-unused finish_initial_narrow function.
Like the other commits in this series, we were already doing this in
all of the callers of load_messages; this centralizes that logic in a
less ad-hoc feeling way.
We no longer use or need the start_initial_narrow function.
Previously, each individual caller of load_messages that passed
num_before > 0 would do its own manual management of fetch_status;
now, we just do it inside load_messages.
Apparently, the older side of the FetchStatus object for home_msg_list
was incorrectly not being maintained. We got away with this, because
the do_backfill code path (which runs after we're done with the
load_more cycle) will correct the error for found_oldest. But we
didn't have proper handling for history_limited here.
When we're doing the load_more frontfill, we were not correctly
declaring that we were in the process of doing a fetch. Because the
next load_more call clears this state anyway, this was generally a
short race, off-screen, but it is still a data flow bug.
See the upcoming commits for a refactor that will eliminate the
possibility of this sort of bug.
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.
Steve Howell
ishanrai05
Tim Abbott
Rishi Gupta
Roger Souza
rht
Vishnu Ks
Cynthia Lin
Sumanth V Rao