This legacy cross-realm bot hasn't been used in several years, as far
as I know. If we wanted to re-introduce it, I'd want to implement it
as an embedded bot using those common APIs, rather than the totally
custom hacky code used for it that involves unnecessary queue workers
and similar details.
Fixes#13533.
When a user clicked the current emoji format in "display settings",
we'd show an infinite loading spinner (basically as a side effect of
trying to tell the server to change the emoji format to what it
already was).
Fix this by aborting early if the emoji format is already the option
that the user clicked.
Fixes#13684.
We now only go the server if both of these
conditions are true:
- our message data seems incomplete for
the stream
- we haven't already fetched history
This function will make more sense when we start
tracking api calls that retrieve topic history.
The unit tests here are kinda duplicating what we
have in the stream_data tests. If we move the
function out of stream_data, we can kill off the
tests there, but for now I think a bit of duplicate
testing is fine here.
All the callers seem to have integer stream_ids
already, either from the message object or
some sub object.
We also use clear() inside the test-only reset()
method.
Previously, links to deleted streams would be incorrectly rendered as
stream's name).
Fixes an issue that was reported where after deleting the "general"
stream, the welcome turtle messages might appear as links to
This is mostly for tactical reasons. It's hard to
get 100% test coverage on topic_list.js, but it
should be easy to get 100% test coverage on this
very important function.
I considered just moving this code into topic_data.js,
but it just didn't feel quite right. I feel like
this is a pretty core piece of code that's nice
to be by itself and not be near other complicated
code that does stuff like build widgets or talk
to servers. (And, again, it's not just the actual
code here, which is pretty small, it's the unit
tests, which are inherently verbose to exercise
all the edge cases.)
There was an edge case with the old
code when you had exactly between 6 and 8
topics and all in cache, with a couple of
the topics being unread.
We would show "more topics" when you were
actually seeing all your possible topics.
To test this:
- create 7 topics on Venice
- as Iago, narrow to any of the Venice
topics
- as Aaron, send unreads to 3 or 4
of the other topics
Eventually Iago will have all possible
topics in the sidebar. On master we'll
show "more topics", whereas after this commit
we correctly avoid that.
It's a pretty harmless bug, since it just
leads to a useless zoom-in.
I have always felt we should zoom-in
regardless of how many topics you have,
just for consistency sake, but I also
understand the rationale behind our
current intentions.
This is basically trying to confine the
rendering logic to a smaller function,
since I want to work toward a better
approach for redrawing the topic list.
Also, since the new function is now
purely data-oriented, it will be a
bit easier to test various edge cases.
If you clicked for no more topics and then the server didn't find any,
we once had code that would say "No more topics" in light gray at the
bottom of the topic list.
The feature appears to have been broken by some detail in the
`self.dom` refactoring. More importantly, it's not clear it's useful
as opposed to clutter.
Since we added the `stream.first_message_id` feature, it's now very
rare for the `more topics` option to appear when there aren't in fact
older topics that could be fetched. In cases where there are not, the
UI is still clear about what's happening -- it shows a loading
indicator and then displays a list of topics that doesn't have
anything new.
So we're removing this feature; we can re-add it without too much
difficulty if user feedback in the future suggests it would be useful
after all.
The only place we ever set active-sub-filter is
right after we build the template, so there is
no reason to have it be a separate step.
(I made a similar fix to pm_list recently, and
this helps set the stage for doing vdom-like
stuff.)
The previous logic was a bit byzantine, making a lot of inferences
based on which conditionals had already been processed that made it
hard to read. This simple function approach promises to be more
readable.
This is for consistency with how we show unreads in muted topics at
the stream level, avoiding distracting users with the appearance of
unread messages in muted topics that they've made clear they are not
interested in.
Arguably, we should show a faded count if there are unreads on muted
topics (but none on unmuted topics), but that seems somewhat complex
to maintain, and we'd benefit from user feedback to make an effective
decision on whether it'd be an improvement.
Fixes#13676.
I think this probably matches users' expected behavior that muted
streams shouldn't get in their way unless the user is actively looking
for them. If a user has a lot of muted topics with active traffic
(e.g. because topics corresponding to channels in a mirrored Slack
instance), they would previously find their 5 slots cluttered with
those muted topics even if there were unmuted topics with unread
messages.
Fixes#13677.
We may revisit this in the future, but similar to is:private, the
current Zulip user experience makes users expect that in the
is:mentioned view, they should really be able to mark messages as
read.
Further, the practice use case for not marking them as read is very
low, since it's rare for someone to have so many mentions that
revisiting the mentions view isn't sufficient to see everything that
needs their attention.
Previously, is_exactly() had already been repalced with can_bucket_by().
This commit removes is_exactly() and replaces its usage in our tests
with can_bucket_by().
For Manage Streams, when we render the subscriptions
template, a significant amount of time is taken
by the "t" helper.
Obviously for the first call, we expect "t" to be
somewhat expensive, but subsuquent calls should be
fast, but i18next seems to have some overhead.
Also, we can save a tiny bit of overhead (marking it
as a safe string) that comes from our helper.
As an aside, are we sure it's ok to mark translations
as safe strings?
To test before and after, use blueslip.timings before
and after this commit. When I tested with about 300
streams, the difference is pretty striking:
without cache: 100ms
with cache: 20ms
This is particularly interesting, since the subscriptions
templates have long strings for things like the SVG-based
checkmarks, but they're not really the bottleneck.
Unfortunately, this doesn't seem to be a huge win
elsewhere. In some places we don't call "t", but of
course those might change in the future and benefit from
the cache. And in other places we have smart widgets
that avoid rendering all N objects at one (e.g. buddy
list and list_render).
So this might be too big a hammer to speed up one
screen (albeit a really slow one). It's possible
that we should simply move the i18n.t step **outside**
of certain templates to avoid doing them in a loop.
We now incorporate people.get_message_people() in our
logic for compose/PM typeaheads. This not only gives
users better results in some cases, but it will also
improve performance for large realms in some cases.
We'll use this in two places coming up, so it's
worth extracting, plus I wanted to add the
fairly lengthy comment here. (Tim, feel free
to edit down the comment as you see fit).
This is relatively unobtrusive, and we don't send
anything to the server.
But any user can now enter blueslip.timings in the
console to see a map of how long things take in
milliseconds. We only record one timing per
event label (i.e. the most recent).
It's pretty easy to test this by just clicking
around. For 300 users/streams most things are
fast except for:
- initialize_everything
- manage streams (render_subscriptions)
Both do lots of nontrivial work, although
"manage streams" is a bit surprising, since
we're only measuring how long to build the
HTML from the templates (whereas the real
time is probably browser rendering costs).
This change sets us up to optimize how we
filter users in the admin user settings.
See #13554 for more context on the user
facing issues.
This fix is basically three related things:
- Add filterer options to list_render.
- Add helper method to people.js.
- Use filterer in settings_users.js.
The filter "callback" was only a "callback" in the
most general sense of the word.
It's just a filter predicate that returns a bool.
This is to prepare for another filtering option,
where the caller can filter the whole list
themselves. I haven't figured out what I will name
the new option yet, but I know I want to make the
two options have specific names.
We are already providing callbacks everywhere, so
it would be nice to eliminate some dead code.
This also speeds things up ever so slightly (no
longer type-checking the option every time through
the loop).
We also split out exports.filter to make unit testing
easier. The function seems kinda silly now, being so
small, but I hope to add another filtering option soon.
It's a bit confusing when you read this code to know
where the original list was created. I'm not a huge
fan of the cache scheme here, but it does seem to
work for live updates.
Zulip has had a small use of WebSockets (specifically, for the code
path of sending messages, via the webapp only) since ~2013. We
originally added this use of WebSockets in the hope that the latency
benefits of doing so would allow us to avoid implementing a markdown
local echo; they were not. Further, HTTP/2 may have eliminated the
latency difference we hoped to exploit by using WebSockets in any
case.
While we’d originally imagined using WebSockets for other endpoints,
there was never a good justification for moving more components to the
WebSockets system.
This WebSockets code path had a lot of downsides/complexity,
including:
* The messy hack involving constructing an emulated request object to
hook into doing Django requests.
* The `message_senders` queue processor system, which increases RAM
needs and must be provisioned independently from the rest of the
server).
* A duplicate check_send_receive_time Nagios test specific to
WebSockets.
* The requirement for users to have their firewalls/NATs allow
WebSocket connections, and a setting to disable them for networks
where WebSockets don’t work.
* Dependencies on the SockJS family of libraries, which has at times
been poorly maintained, and periodically throws random JavaScript
exceptions in our production environments without a deep enough
traceback to effectively investigate.
* A total of about 1600 lines of our code related to the feature.
* Increased load on the Tornado system, especially around a Zulip
server restart, and especially for large installations like
zulipchat.com, resulting in extra delay before messages can be sent
again.
As detailed in
https://github.com/zulip/zulip/pull/12862#issuecomment-536152397, it
appears that removing WebSockets moderately increases the time it
takes for the `send_message` API query to return from the server, but
does not significantly change the time between when a message is sent
and when it is received by clients. We don’t understand the reason
for that change (suggesting the possibility of a measurement error),
and even if it is a real change, we consider that potential small
latency regression to be acceptable.
If we later want WebSockets, we’ll likely want to just use Django
Channels.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Currently, if we change stream we see the immediate saving of stream, but
it is more convenient to have "Save" and "Discard" buttons as we use
everywhere else in the organization setting subsystem.
This is a preliminary commit for further commits where we will be using the
newly created function `save_discard_widget_status_handler` in click
handler for changing the notification stream.
This refactors `discard_property_element_changes` and
`check_property_changed` function to move conditional statements of
properties that need to be handled separately. It's a preliminary commit in
the series of using save/discard widget for notification stream setting.
As the part of making notification stream settings to change using
"save/discard" widget instead of immediate saving, we need to access the
stream id which is being selected at the moment.
(This is another preliminary commit in the direction of having
"save/discard" widget show up rather than saving immediately.)
The code for selecting and processing the stream for both types of
notifications is almost the same, so de-duplicated.
Tim Abbott
orientor
Steve Howell
Jonathan Cobb
Anders Kaseorg
Rohitt Vashishtha
Pragati Agrawal