The series of commits to consolidate CSS classes
for the various unread-count spans across our app
created a bug where the stream_list.js code's selector
starting capturing the unread spans in topic list items.
Suppose you had a stream with these topics:
Foo 10
a 3
b 3
c 4
If another unread came in, you would briefly see:
Foo 11
a 11
b 11
c 11
Now we just use subscription_block to find the
element that we want to tweak.
I remove a convoluted node test here. Part of the
reason the node test was convoluted was that the
original implementation was overly complex. I will
try to re-introduce a simpler test soon, but this
is a bit of an emergency fix.
This reduces our dependency on message_list code (via
message_util), and it makes moving streams/topics and
deleting messages more performant.
For every single message that was being updated or
deleted, the previous code was basically re-computing
lots of things, including having to iterate through
every message in memory to find the messages matching
your topic.
Now everything basically happens in O(1) time.
The only O(N) computation is that we now lazily
re-compute the max message id every time you need it
for typeahead logic, and then we cache it for
subsequent use. The N here is the number of messages
that the particular sender has sent to the particular
stream/topic combination, so it should always be quite
small, except for certain spammy bots.
Once the max has been calculated, the common operation
of adding a message doesn't invalidate our cached
value. We only invalidate the cache on deletes.
The main change that we make here from a data
standpoint is that we just keep track of all
message_ids for all senders. The storage overhead here
should be negligible. By keeping track of our own
messages, we don't have to punt to other code for
update/delete situations.
There is similar code in recent_topics that I think can
be improved in similar ways, and it would allow us to
eliminate functions like this one:
export function get_messages_in_topic(stream_id, topic) {
return message_list.all
.all_messages()
.filter(
(x) =>
x.type === "stream" &&
x.stream_id === stream_id &&
x.topic.toLowerCase() === topic.toLowerCase(),
);
}
In an effort to use a common class to display unread counts across
the app, we simplify the elements used to show unreads and use a
single `span` with `unread_count` class to do so.
This reverts commit f81cc16a0f (#17999).
The {{#tr}} helper now includes the functionality that we wanted from
this.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
* This introduces a new event type `realm_linkifiers` and
a new key for the initial data fetch of the same name.
Newer clients will be expected to use these.
* Backwards compatibility is ensured by changing neither
the current event nor the /register key. The data which
these hold is the same as before, but internally, it is
generated by processing the `realm_linkifiers` data.
We send both the old and the new event types to clients
whenever the linkifiers are changed.
Older clients will simply ignore the new event type, and
vice versa.
* The `realm/filters:GET` endpoint (which returns tuples)
is currently used by none of the official Zulip clients.
This commit replaces it with `realm/linkifiers:GET` which
returns data in the new dictionary format.
TODO: Update the `get_realm_filters` method in the API
bindings, to hit this new URL instead of the old one.
* This also updates the webapp frontend to use the newer
events and keys.
Changed the name of the test-user cordelia from `Cordelia Lear` to
`Cordelia, Lear's daughter`.
This change will enable us to test users with escape characters in
their names.
I also updated the Node, Puppeteer, Backend tests and Fixtures to
support this change.
* Don't require strings to be unnecessarily JSON-encoded.
* Use check_capped_string rather than custom code for length checks.
* Update frontend to pass the right parameters.
With a much simplified populate_data_for_request design suggested by
Anders; we only support a handful of data types, all of which are
correctly encoded automatically by jQuery.
Fixes part of #18035.
We no longer need to add_tooltip_to_left_panel_row since
tippyjs.delegate will automatically do that for us.
Tippyjs automatically places it to left on small widths and bottom
for large widhts.
When the user's full name is long, the full name + `(you)`
in the buddy list starts to truncate, but when hover, the
tooltip displays the full name but not `(you)`.
This commit fixes this by adding `(you)` in the tooltip.
There was a bug where invite users link is shown in the Invitations
section of settings overlay, irrespective if the user is allowed to
invite or not based on the realm settings. So here we just use the
'can_invite_others_to_realm' field of page_params, that was added in
previous commit itself to hide the link accordingly.
Also added the code in server_events_dispatch.js for this field.
Though it doesn't do live update when the overlay is opened similar
to other policies like create_stream_policy, but it ensures that the
visibility of link is correct if the overlay is opened after the
settings has changed.
We should probably do the complete live update in future of the
elements related to such realm settings.
Since the base hash for org settings and user settings are
different (organization and settings), the hashchange module
gets confused that we are going from one overlay to other.
A reproducer for this flow is to visit the organization "Bots" page,
click on your own profile as the owner of a bot, and then click "Edit
profile" from there.
So, we fix this by making an exception for this particular case
in the module.
Fixes part of #18011.
Muted users are stored in a map with key as user ID and
the value as the timestamp of muting.
Names can be easily fetched from existing functions
in `people.js` and hence not stored.
This change should make live-update code less brittle,
or at least less cumbersome.
Instead of having to re-compute calculated fields for
every change to a stream message, we now just compute
the fields right before we render stream settings UI.
This is mostly a pure code move.
In passing I remove an unneeded call to
update_calculated_fields in the dispatch code,
plus some tests that don't need them.
For filter values which don't exist or are invalid in some
way, we return false to show user that there are no messages
in the filter user is trying to render. Our previous behaviour
was to show all the messages and ignore the filter which
isn't good.
Previously we were liable to have false positives in our tests here
because we did not reset the visible state for these selectors, this
commit adds a helper and relevant calls to it in order to prevent such
false positives.
We now consistently set the PM counts for the right
sidebar toggle in unread_ui, similar to what we
do for the overall counts in the left sidebar toggle.
(Use a thin window to see the code in action.)
This breaks a dependency cycle.
In passing I improve the test coverage for the
actual job that pm_list still does (updating its
own total count in the "Private Messages" section).
This data structure has never been one that we actually render into
the DOM; instead, its role is to support clicking into view that
contain muted streams and topics quickly.
This downgrade makes that situation much more explicit, and is also
useful refactoring to help simpify the upcoming changes in #16746.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
I lift this function out of message_store to
break some dependencies, and it's also more
consistent with the rest of the codebase:
alert_words.process_message
pm_conversations.process_message
recent_topics.process_messages
recent_senders.process_message_for_senders
We can do further cleanup to make these names
consistent (and possibly have them all work in
bulk), but that's out of the scope of the current PR.
We move the message_store.add_message_metadata function
(and all its dependencies) into a new module called
message_helper and rename the function to process_new_message.
(It does a bit more than adding message metadata, such
as updating our message store.)
We also have a "protected" interface now in message_store,
so that message_helper can access the message store:
update_message_cache
get_cached_message
Because update_message_cache is identical to
the former create_mock_message, we just renamed it
in the tests.
Most callers should use these functions:
message_helper.process_new_message (setting)
message_store.get (getting)
It's slightly annoying that the setter interface
is in a different module than the getter interface,
but that's how you break a bunch of dependencies.
We also extract the tiny message_user_ids class:
user_ids()
add_user_ids()
All the code moves here are pretty trivial, and
the code that was moved maintains 100% line
coverage.
The module name `message_helper` is not ideal, but it's a single
function and it'll save time to just do the topology change now and
leave thinking through the right name to later.
Before this we did not have remove event in server_events_dispatch.js
for the user group delete event even though server had. This was
leading to blueslip errors. Extracted the logic which was used in
success() of channel.del for user_groups into the remove case in
server_events_dispatch. Also removed the redundant reload call as
we already do that in server events.
This is a mostly verbatim extraction.
I re-phrased one line of code to work around a lint
false alarm. (Look for `preamble` in the diff.)
There are about 8 lines missing coverage here, so
the new module might be a good candidate to get
100% line coverage on.
Before this change, you would need to remove 74
edges from our dependency graph to make it
acyclic. Now it's 72.
This change does not impact the overall complexity
of our dependency graph (at least in terms of the
number of edges that we would need to remove to get
a tree), but it does clarify the picture a bit.
* `op` (operation) field, added in f6fb88549f, was never intended for
`custom_profile_fields` event. This commit removes the `op` as it doesn't
have any use in the code.
* As a part of cleanup, this also eliminates the schema check warnings
for `custom_profile_fields` event, mentioned in #17568.
When a user entered an invalid character (whitespace or characters not
present in a name), the cleaned-up array, and hence the query,
would be empty which resulted in an error.
Fixes#17542
Extend our markdown system to support mentioning of users
by id also. Following these changes, it would be possible
to mention users with @**|user_id** and silently mention
using @_**|user_id**.
Main intention for extending the mention syntax is to make
it convenient for bots to mention a users using their ids. It
is to be noted that previous syntax are also supported.
Documentation tweaked by tabbott for better readability.
The changes were tested manually in development server, and also
by adding some new backend and frontend tests.
Fixes: #17487.
These were introduced in ff9a929d7a
with no explanation of why they were necessary.
Generally you only render a few things, and it's
important that they're up to date.
We weren't doing a good job of invalidating the cache.
Eliminating the cache will fix bugs (like presence circles
being out of date) and break some dependencies.
I removed some very fragile test code that was relying
on invalid values taken out of the cache. (We now have
less line coverage, but if we want to test our rendering,
there are much cleaner ways to do it.)
As part of testing this, I renamed Hamlet to "aaron", so
that there are two aarons, and then I logged on as Iago
to see the "secondary" code in action that shows their
emails to distinguish them.
This add the schema checker, openapi schema, and also a test for
realm/deactivated event.
With several block comments by tabbott explaining the logic behind our
behavior here.
Part of #17568.
This adds support for unstarring all (starred)
messages from a particular topic, from the topic
popover.
The earlier implementation of this in #16898
was reverted in bc55aa6a01 (#17429)
because it had two problems-
1. The crash reported in bc55aa6a01
was due to message_store returning undefined. This happens
when the message itself hasn't been fetched from the server
yet, but we know that the message is starred from the ids
in `page_params` in `starred_messages.js`.
This commit handles this case explicitly.
Note that, we simply ignore those messages which
we haven't fetched, and because of this, it may
happen that we don't unstar some messages from that
topic. The correct implementation for this would
be to ask the backend for starred IDs in a topic.
2. The earlier implementation actually unstarred **all**
messages. This was because it grabbed the topic and stream_id
from the topic popover `data` attributes, after the topic
popover had been closed. This passed `undefined`, which
the function then interpreted as an action to unstar all
messages.
With this commit, we use the confirm_dialog widget,
which eliminates the need to store this data in the DOM.
This is mostly a refactoring to break the unnecessary
dependency of bot_data on settings_bots.
This is a bit more than a refactoring, as I remove all
the debounced calls to render bots during the
initialization of bot_data. (The debouncing probably
meant we only rendered once, but it was still needless
work.)
We don't need to explicitly render bots during
bot_data.initialize(), which you can verify by loading
"#settings/your-bots" as the home page. It was just an
artifact of how add() was implemented.
Note that for the **admin** screen, we did not and
still do not do live updates for add/remove; we only do
it for updates. Fixing that is out of the scope of this
change. The code that was moved here affects
**personal** bot settings.
Note that the debounce code is quite fragile. See my
code comment that explains it. I don't have time to go
down the rabbit hole of a deep fix here. The puppeteer
tests would fail without the debounce, even though I
was able to eliminate the debounce in an earlier
version of this fix and see good results during manual
testing. (My testing may have just been on the "lucky"
side of the race.) I created #17743 to address this
problem.
This mainly extracts a new module called
browser_history. It has much fewer dependencies
than hashchange.js, so any modules that just
need the smaller API from browser_history now
have fewer transitive dependencies.
Here are some details:
* Move is_overlay_hash to hash_util.
* Rename hashchange.update_browser_history to
brower_history.update
* Move go_to_location verbatim.
* Remove unused argument for exit_overlay.
* Introduce helper functions:
* old_hash()
* set_hash_before_overlay()
* save_old_hash()
We now have 100% line coverage on the extracted
code.
The only caller for this function was settings_config,
so we put it there.
For the stream_edit test we no longer mock the function.
(The reason we mocked the function was more about avoiding
the heavy settings_notifications import than the function
itself.) This gives some incidental coverage, but then I
also add some more real coverage on it.
We extract compose_fade_users and compose_fade_helper.
This is a pretty verbatim extraction of code, apart from adding a few
exports and changing the callers.
This change makes the buddy_data module no longer sit "above" these
files in the dependency graph (at least not via compose_fade):
* jquery
* lodash (not a big deal)
* compose_state
* floating_recipient_bar
* message_viewport
* rows
The new moules have dependencies that buddy_data already
had directly for other reasons:
* people
* util
And then buddy_data still depends on stream_data indirectly through
the compose-fade logic for stream_data. Even without compose-fade, it
would depend indirectly on stream_data via hash_util.
Note that we could have lifted the calls to compose_fade out of
buddy_data to move some dependencies around, but it's useful to have
buddy_data fully encapsulate what goes into the buddy list without
spreading responsibilities to things like activity.js and
buddy_list.js. We can now unit-test the logic at the level of
buddy_data, which is a lot easier than trying to do it via modules
that delegate drawing or do drawing (such as activity.js and
buddy_list.js).
Note that we still don't have 100% line coverage on the
compose_fade.js module, but all the code that we extracted now is
covered, mostly via buddy_data tests.
Previously, it was tedious to create actual message
objects in message_store for use in node tests.
This was mainly because, `add_message_metadata`
in message_store has many dependencies and
validation checks. Since it was difficult to create
actual message objects, many tests just mocked
the `message_store.get()` method to return the desired
message.
This commit adds a new helper method (`create_mock_message`)
to message_store, for use in node tests. This just stores
the object passed to it in the `stores_messages` map,
without any validation. We do not add any
default fields to the message object before saving
it from this helper, because doing so would decrease
the utility of this helper, and, if a test
depends on some field having a particular value,
then it would be better to just pass the field: value
pair from the test itself, for readability, rather
than relying on the helper to add the field for us.
This helper allows us to write deeper tests.
This commit also replaces some instances of mocking
`message_store.get()` to use this new helper method.
Fixes#17466
This commit will change encoding logic. Initial logic
was not encoding parenthesis, and this creates conflicts
with the markdown link format. To resolve this while encoding,
we're now replacing parenthesis with ".28" and ".29."
There is no need to change decoding logic because before
decoding any URL, we first convert all the “.” to “%.”
optimization: No need to replace parenthesis in popovers.js.
A few years ago I introduced the anti-pattern
of automatically calling success functions within
channel.get stubs. It's better to just capture
the success function and call it explicitly.
Also, we now have override(), so it's easy to
inline these types of things in a safe way.
These sigils will help make it easier to see that this is a special
Zulip syntax feature, not just something the other user typed, which
might help set the expectation that we're showing the time in the
user's timezone.
Tweaked by tabbott to improve variable/template naming.
We have generally gone away from using $(...)
initialization in modules that we test with
zjsunit, but there are a few remaining special
cases related to our billing and portico
codebases.
I remove an obsolete comment--we use get_streams()
for the `n` key now.
I also remove a guard statement from sort_groups()
that returned `undefined` for empty lists.
That guard statement would break this code:
const stream_groups = stream_sort.sort_groups(streams, get_search_term());
if (stream_groups.same_as_before && ...
The calling code prevents the situation anyway:
const streams = stream_data.subscribed_stream_ids();
if (streams.length === 0) {
return;
}
I modify the "no_subscribed_streams" test to test
the new behavior. (Even though stream_list currently
short-circuits the call here, that may change in the future.)
I also introduce the test() wrapper to explicitly clear
our data.
Use fully resolvable request paths because we need to be able to refer
to third party modules, and to increase uniformity and explicitness.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This introduces the make_stream_message()
helper to avoid all the strange
`messages[0] === message1` confusion.
We also clear data explicitly at the beginning
of the test.
It's still a messy test.
A bunch of tests were mutating the same message.
Now they just make their own copy.
I introduce the name "sample_message" for the common
message, but I remove the two bogus reactions, and
the "warning" test now just creates its own
test-specific data.
I considered using set_info({}, 0) to clear data,
but it has a lot of machinery that could lead
to accidental line coverage and/or extra test
complexity.
This commit adds a new class for typeahead items that do not need
a presence circle. It is changed to support addition of new items
that do not require presence circle.
There should not be any visual change due to this.
* move one test below setup
* add a test() wrapper to clear mutes
* split up a test
* use two different dates for java vs. gossip
* deterministically sort by date
All these changes make it so that the order of
the tests is a lot less fragile.
TextField is used to allow users to set long stream + topic narrow
names in the urls.
We currently restrict users to only set "all_messages" and
"recent_topics" as narrows.
This commit achieves 3 things:
* Removes recent topics as the default view which loads when
hash is empty.
* Loads default_view when hash is empty.
* Loads default_view on pressing escape key when it is unhandled by
other present UI elements.
NOTE: After this commit loading zulip with an empty hash will
automatically set hash to default_view. Ideally, we'd just display
the default view without a hash, but that involves extra complexity.
One exception is when user is trying to load an overlay directly,
i.e. zulip is loaded with an overlay hash. In this case,
we render recent topics is background irrespective of default_view.
We consider this last detail to be a bug not important enough to block
adding this setting.
We want to be careful about MIT code, as it leads
to more lax checks in the filter code.
And then we just use with_field in a couple places
and clear subscriptions.
For most functions that we were using __Rewire__ for,
it's better to just use the override helper, which
use __Rewire__ under the hood, but also resets
the reference at the end of run_tests.
Another nice thing about override() is that it reports
when you never actually needed the mock, and this
commit fixes the instances found here.
I didn't replace every call to __Rewire__. The
remaining ones fall under these categories:
* I looked for ") =>" in my code sweep,
so I missed stuff like "noop" helpers.
* Sometimes we directly update something
in a module that's not a function. This
is generally evil, and we should use setters.
* Some tests have setup() helpers or similar
that complicated this code sweep, so I
simply punted.
* Somes modules rely on intra-test leaks. We
should fix those, but I just punted for the
main code sweep.
This is a deceptively ugly diff. It makes
the actual code way more tidy.
I basically inlined some calls to mock_module
and put some statements in lexical order.
We now just use a module._load hook to inject
stubs into our code.
For conversion purposes I temporarily maintain
the API of rewiremock, apart from the enable/disable
pieces, but I will make a better wrapper in an
upcoming commit.
We can detect when rewiremock is called after
zrequire now, and I fix all the violations in
this commit, mostly by using override.
We can also detect when a mock is needlessly
created, and I fix all the violations in this
commit.
The one minor nuisance that this commit introduces
is that you can only stub out modules in the Zulip
source tree, which is now static/js. This should
not really be a problem--there are usually better
techniques to deal with third party depenencies.
In the prior commit I show a typical workaround,
which is to create a one-line wrapper in your
test code. It's often the case that you can simply
use override(), as well.
In passing I kill off `reset_modules`, and I
eliminated the second argument to zrequire,
which dates back to pre-es6 days.
I created this test in Jan 2019 with
6b7a4f8611.
At the time I created this we were still migrating modules
over from using the $(() => {...}) style of initialization,
so the test served a purpose as an easy smoke test that we
weren't doing something really stupid in
ui_init.initialize().
Now that that migration is complete, the test's warts start
to outweigh its benefits. Because it's just a smoke test,
we get credit for artificial line coverage where we're
exercising code without actually verifying anything other
than that it runs without crashing.
There are still possibilities for bugs within
ui_init.initialize() where we initialize modules in the
wrong sequence, but I am not confident that the node test
would catch them with any more likelihood (or clarity) than
either the puppeteer tests or czo bug reports.
And then on top of that, the zjquery setup in the node test
is hard to maintain. It's also hard to verify that some
of the setup isn't cruft.
If you want to see how much of a maintenance headache this
has been in recent times, just look at how many of the
recent commits have been related to things like es6
migrations or code cleanup sweeps.
We should just kill it.
"Alert Words" is one of Zulip's oldest settings UI elements, and as a
result is buggy. This commit converts it to use our standard
progressive-table-wrapper system used for settings tables, which has
the side effect of fixing a bug that mad ethe tables look pretty bad
if one adds a very long word.
Fixes#17172.
This commit addresses the problem of user's status visibility to
some extent. It adds presence circles, like we have in buddy_list to the
typeahead suggestions that are given for mentioning users in messages.
Tweaked by tabbott to adjust vertical alignment of group mentions as well.
Testing for the changes is done manually in the developement server,
and also by updating frontend tests to address these changes.
Fixes: #17138
Found by running the tests after
sed -i 's/\.with(/.toBeUsed().with(/g' frontend_tests/node_tests/*.js
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Steve Howell
Aman Agrawal
Anders Kaseorg
Abhijeet Prasad Bodas
Arun Sankar
Tim Abbott
Riken Shah
sahil839
Mateusz Mandera
YashRE42
Sumanth V Rao
tushar912
shanukun
Vishnu KS
Ganesh Pawar
Nishant Mittal
m-e-l-u-h-a-n
100RABHpy
Nikhil Maske
Josh Gilley