The people.js tests were using _add_user function to add
cross realm bots. The problem is that _add_user function
doesn't properly simulates the adding process as it doesn't
add the user in cross_realm_dict as well.
To solve this and eliminate the need of calling
people.initialize(), which means the params obj needs to be
defined, we extracted the whole logic of adding a cross realm
user into a separete function, add_cross_realm_user.
If you have a group PM where some users have
three-digit user_ids and some with four-digit
user_ids (or similar), a huddle could effectively
be ignored when determining the order of
search search suggestions.
Basically, we need a way to canonically sort
user_ids in "huddle" strings, and it's somewhat
arbitrary whether you sort lexically or sort
numerically, but you do need to be consistent
about it.
And JS is not exactly helpful here:
> [99, 101].sort()
[ 101, 99 ]
This is a pretty obscure bug with pretty low
user-facing consequences, and it was never
reported to us as far as I know, but the fix
here is pretty straightforward.
We have had similar bugs of slightly more consequence
in the past. The reason this bug has shown
up multiple times in our codebase is that every
component that deals with huddles has slightly
different forces that determine how it wants
to serialize the huddle. It's just one of those
annoying things. Plus, bugs with group PMs
do tend to escape detection, since most people
spend most of their time either on streams
or in 1:1 PMs.
This is a pure code extraction. The current
code is buggy with respect to user_ids with
different lengths of digits, i.e. it does
a naive lexical sort instead of a numerical
sort. We'll fix that in the next commit.
'get_active_message_people` function is added which returns active
users who have sent the messages that are currently showing up in
the feed.
typeahead fetches the users from 'get_active_message_people` instead
of `get_message_people` and thus shows only active users in the
mention typeahead and excludes deactivated users.
Fixes#14310
This commit moves the get_visible_email function to people.js
as this function will be used in other places and people.js seems
relevant file for this.
Tests are added to get full coverage.
This is code simplification motivated
by a recent bug that we fixed with some
server changes, but which was really
caused in some sense by our client code
using an overly finicky
condition to check falsiness.
For cross-realm bots, the value of
`user.bot_owner_id` may be `null`, or it
may simply be `undefined`, depending
on whether the server passes `None`
or simply omits the field.
We don't want out client code to be
coupled to that rather arbitrary
decision.
We were doing a `!== null` check instead
of checking for falsiness, which led to
blueslip errors in the past. Because a
bot owner id could be plausibly 0, a falsiness
check would be brittle in a different way.
Now we avoid that ugliness by calling
`get_bot_owner_user`, which either returns
an object or `undefined`.
And then the caller can just do a concise
check for whether `bot_owner` exists.
And we also fix up the crufty code that
was putting `bot_owner_full_name` on to
the object instead of using a local.
We have a bug report for this again, although
it might be on an old branch.
Fixes#13621.
We had this API:
people.add_in_realm = full-fledged user
people.add = not necessarily in realm
Now the API is this:
people.add = full-fledged user
people._add_user = internal API for cross-realm bots
and deactivated users
I think in most of our tests the distinction between
people.add() and people.add_in_realm() was just an
accident of history and didn't reflect any real intention.
And if I had to guess the intention in 99% of the cases,
folks probably thought they were just creating ordinary,
active users in the current realm.
In places where the distinction was obviously important
(because a test failed), I deactivated the user via
`people.deactivate`.
For the 'basics' test in the people test suite, I clean
up the test setup for Isaac. Before this commit I was
adding him first as a non-realm user then as a full-fledged
user, but this was contrived and confusing, and we
didn't really need it for test coverage purposes.
We want to move more logic to stream_data to facilitate
testing.
Both before and after this commit, we essentially build a
new list of users for typeahead, but now the new list
excludes subscribed users. We can do even better than
this in a follow-up commit.
Before this commit, presence used get_realm_count()
to determine whether a realm was "small" (and thus
should show all human users in the buddy list, even
humans that had not been active in a while).
The `get_realm_count` function--despite a very wrong,
misleading comment--was including bots in its count.
The new function truly counts only active humans
(and no bots).
Because we were overcounting users before this change,
we should technically adjust `BIG_REALM_COUNT` down
by some amount to reflect our original intention there
on the parameter. I'm leaving it alone for now, though,
since we've improved the performance of the buddy list
over time, and it's probably fine if a few "big" realms
get re-classified as small realms (and show more users)
by virtue of this change.
(Also note that this cutoff value only affects the
"normal" view of the buddy list; both small realms
and large realms will show long-inactive users if you
do searches.)
Fixes#14215
This extracts a new module with three
functions, which we will test with 100%
line coverage:
- show_email
- email_for_user_settings
- get_time_preferences
The first two break several dependencies
in the codebase on `settings_org.js`. The
`get_time_preferences` breaks an annoying
dependency on `page_params` within people.
The module is pretty cohesive, in terms that
all three functions are just light wrappers
around `page_params` and/or `settings_config`.
Now all the modules that want to call show_email()
only have to require `settings_data`, instead of
having a dependency on the much heavier
`settings_org.js` module.
I also make some of the unit tests here be more
full-stack, where instead of stubbing show_email,
I basically just toggle `page_params.is_admin`.
This cleans up the handoff of page_params
data between ui_init and modules that
take over ownership of page_params-derived
data.
Read the long comment in ui_init for a bit
more context.
Most of this diff is actually test cleanup.
And a lot of the diff to "real" code is
just glorified `s/page_params/params/`
in the `initialize` functions.
One little oddity is that we don't actually
surrender ownership of `page_params.user_id`
to `people.js`. We could plausibly sweep
the rest of the codebase to just use
`people.my_user_id()` consistently, but it's
not a super high priority thing to fix,
since the value never changes.
The stream_data situation is a bit messy,
since we consume `page_params` data in the
initialize() function in addition to the
`params` data we "own". I added a comment
there and intend to follow up. I tried
to mostly avoid the "word soup" by extracting
three locals at the top.
Finally, I don't touch `alert_words` yet,
despite it also doing the delete-page-params-data
dance. The problem is that `alert_words`
doesn't have a proper `initialize()`. We
should clean that up and have it use a
`Map` internally, too.
This mostly moves logic into people.js.
The people functions added here are glorified
two-liners.
One thing that changes here is that we
are a bit more rigorous about duplicate
names.
The code is slightly awkward, because this
commit preserves the strange behavior
that if 'alice|42' doesn't match on
the user with the name "alice" and user_id
"42", we instead look for a user whose
name is "alice|42". That seems like a
misfeature to me, but there's a test for
it, so I want to check with Tim that it's not
intentional behavior before I simplify
the code.
We now treat util like a leaf module and
use "require" to import it everywhere it's used.
An earlier version of this commit moved
util into our "shared" library, but we
decided to wait on that. Once we're ready
to do that, we should only need to do a
simple search/replace on various
require/zrequire statements plus a small
tweak to one of the custom linter checks.
It turns out we don't really need util.js
for our most immediate code-sharing goal,
which is to reuse our markdown code on
mobile. There's a little bit of cleanup
still remaining to break the dependency,
but it's minor.
The util module still calls the global
blueslip module in one place, but that
code is about to be removed in the next
few commits.
I am pretty confident that once we start
sharing things like the typeahead code
more aggressively, we'll start having
dependencies on util. The module is barely
more than 300 lines long, so we'll probably
just move the whole thing into shared
rather than break it apart. Also, we
can continue to nibble away at the
cruftier parts of the module.
This should return us to a situation where we won't get blueslip
browser error reporting for users created while a device was offline
just before it reloads.
We now require all of our unit tests to handle
blueslip errors for warn/error/fatal. This
simplifies the zblueslip code to not have any
options passed in.
Most of the places changed here fell into two
categories:
- We were just missing a random piece of
setup data in a happy path test.
- We were testing error handling in just
a lazy way to ensure 100% coverage. Often
these error codepaths were fairly
contrived.
The one place where we especially lazy was
the stream_data tests, and those are now
more thorough.
Babel strict generates more code for [...x] than you’d like, while
Babel loose mode assumes x is an array.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Using startsWith is faster than indexOf, especially for long strings
and short prefixes. It's also a lot more readable. The only reason
we weren't using it was when a lot of the code was originally written,
it wasn't available.
This extracts get_emoji_matcher and all the
functions it depended on, most of which were
in composebox_typeahead.js.
We also move remove_diacritics out of the people
module.
This is the first major step for #13728.
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 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.
This should be slightly more performant, and we
often call this function N times, such as when
rendering the buddy list.
There's a minor change to pm_list to avoid
an unnecessary computation on huddles that would
otherwise trigger a blueslip warning for the
huddles case.
Once we get past the special check for fake
person objects already having `pm_recipient_count`,
we can rely on the object being a `person`
object with `user_id` set.
When we are pulling data from message.display_recipient
for private messages, the user_id field is always
called 'id', not 'user_id', so we can simplify
some defensive code.
This required lots of manual testing:
- search/navigate user presence
- send PM and mention user
- pay attention to compose fade
- send stream msg and mention user
- open Private Messages in top-left and click
- test unread counts
- invite user who already has account
- search for users in search bar
- check user settings
- User Groups
- Users
- Deactivated Users
- Bots
- create a bot
- mention user groups
- send group PM then click on lower right
- view/edit/create streams
If there are still pieces of code that don't convert
ids to ints, the code should still work but report
blueslip errors.
I try to mostly convert user_ids to ints in the callers,
since often the callers are dealing with small amounts
of data, like user ids from huddles.
We only ever show 3 or 4 people in search suggestions
(possibly w/a couple variations, like pm-with/sender/etc.),
so we can try to search a smaller subset of people
before going through the entire realm.
We use message_store.user_ids() for this, since you
typically want to search messages for people that
have sent messages recently, and we already sort
based on PM conversations.
This should avoid some memory allocations.
We also use build_person_matcher to avoid
repeating the same logic over and over
again to process the query into termlets.
We also remove people.get_all_persons() and
people.person_matches_query().
This may actually be a slowdown for the worst case
scenario, but it sets us up to be able to easily
short circuit the removal of diacritic characters
for users that have pure ascii names.
For example, czo has lots of names like this:
- Tim Abbott
- Steve Howell
Since they're pure ascii, we can do a one-time
check. A subsequent commit will show how we use
this.
We have ~5 years of proof that we'll probably never
extend Dict with more options.
Breaking the classes into makes both a little faster
(no options to check), and we remove some options
in FoldDict that are never used (from/from_array).
A possible next step is to fine-tune the Dict to use
Map internally.
Note that the TypeScript types for FoldDict are now
more specific (requiring string keys). Of course,
this isn't really enforced until we convert other
modules to TS.
We want to mostly deprecate this function (see
the comment I added), so I gave it a more specific
name.
Ideally I'd just fix `stream_create`, but it does
use this function in a couple places, and it's helpful
to reuse the same sort here. In one place stream_create
actually unshifts the "me" user back to the top of the
list, which makes sense for its use case.
If we aren't showing users emails, then we don't
want to use emails in the search.
And if we are showing users emails, we want to
search on the email that's displayed to them.
For admins this will be delivery_email.
For regular users we arguably shouldn't search
on emails either, since it mostly causes confusion,
but this commit just preserves the current
behavior for those users (unless `show_email` is
false).
We want to be able to unit test this value,
since it's conditional on several factors:
- am I an admin?
- can non-admins view emails?
- do we have delivery_email for the user?
I'm mocking show_email in the tests, since the
show_email code is in `settings_org` and
kind of hard to unit test. It's not impossible,
but it's too much for this commit. (Either
we need to extract it out to a nice file or
deal with mocking jQuery. That module is
mostly data-oriented, so it would be nice
to have something like `settings_config` that
is actually pure data.)
This was duplicate code. I'm moving it to people
for pragmatic reasons--it's hard to unit test stuff
in settings_users.js due to all the jQuery.
It's also nice to have all people-related search
code in one place, just for auditing purposes.
We now can call is_ascii only once per search termlet
when we are filtering multiple persons on the same
query. (This requires the caller to use
`build_person_matcher` outside a loop or before
a `_.filter` call.)
This is not a major speedup, but we do a couple
simple things here:
- trim the query outside the function we
build (that might be called multiple times)
- don't split names before we possibly
early-exit with an email match
This will allow use to change some O(N) behavior
to O(1) where we are performing the same query
on a bunch of people. (Subsequent commits will
actually take advantage of this prefactoring.)
This commit was originally automatically generated using `tools/lint
--only=eslint --fix`. It was then modified by tabbott to contain only
changes to a set of files that are unlikely to result in significant
merge conflicts with any open pull request, excluding about 20 files.
His plan is to merge the remaining changes with more precise care,
potentially involving merging parts of conflicting pull requests
before running the `eslint --fix` operation.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
With webpack, variables declared in each file are already file-local
(Global variables need to be explicitly exported), so these IIFEs are
no longer needed.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Precompiling regexes gives a performance increase of around 10-15%
based on tests. See https://jsperf.com/typeahead-regex. This stacks
up when we have a lot of users in an organisation.
The approach taken here is basically use user IDs in operator that
support it when sending the request for fetching the messages
(see comments in code for more details).
This is a small patch to fix the error message an admin would receive if
they tried to change bot info and owner from the "bots" setting of the
organization settings panel.
This makes it possible to mention a user with a name like Gaël that
contains diacritics by typing e.g. "Gael", significantly reducing the
need to use a special keyboard to mention other users.
Fixes#11183.
This makes it possible it include our standard markdown formatting in
one's custom profile fields, allowing for links, emphasis, emoji, etc.
Fixes#10131.
While we're at it, we remove the JSON parsing that was part of the
user field code path, since this function isn't responsible for
rendering user fields.
We want to avoid `blueslip.error` in cases where
the root cause could just be bad data that is
human-entered.
There are a few callers here who **should** be
sending good data all the time, but hopefully
they either have good test coverage, other
obvious failure symptoms, or, ideally, just
do what the user would mostly expect in the
face of bad data.
Internally we generally omit our own id and email
in data structures related to PMs, except when we
are the sender, but if we receive "perma links"
we will need to filter out our id.
This commit exposes the function is_duplicate_full_name()
that can be used to discern if we cannot identify a user
just by their full name in the interface and have to use
his user id as well to distinguish them from other users.
This is part of work to break some of our
nastier circular dependencies in preparation
for our es6 migration.
This commit should facilitate loading leaf-like
modules such as people.js before all of the things
that reload.js depends on.
This was supposed to be suppressed when a reload is in progress,
however, the logic was accidentally checking that
reload.is_in_progress was a defined function, not whether a reload was
actually in progress.
This commit prepares the frontend code to be consumed by webpack.
It is a hack: In theory, modules should be declaring and importing the
modules they depend on and the globals they expose directly.
However, that requires significant per-module work, which we don't
really want to block moving our toolchain to webpack on.
So we expose the modules by setting window.varName = varName; as
needed in the js files.
Fixes#3380.
The blueslip warning mentioned in #3380 were from paths ending at
people.email_list_to_user_ids_string. Some additional blueslip warnings
were raised after using that function.
Although we can put a validation check somewhere in the call stack of
people.email_list_to_user_ids_string, this function itself is used to
validate the operand by the higher order functions, so it wouldn't make
sense to put a validation check before that. Instead, removing the
blueslip warning altogether was chosen.
people.email_list_to_user_ids_string was replaced by
people.reply_to_to_user_ids_string which is a blueslip-free version
of the same. Other blueslip warnings were removed.
If the browser is in the progress of reloading when it finishes
fetching some messages, it's not really a bug, and we shouldn't report
it as such.
This should help make Zulip's browser error reporting less spammy.
clarammdantas
Steve Howell
sahil839
Steve Howell
Anders Kaseorg
Tim Abbott
Rohitt Vashishtha
Priyank Patel
Hemanth V. Alluri
Thomas Ip
ss62171
Yashashvi Dave
Cynthia Lin
Shubham Dhama
Armaan Ahluwalia
Shubham Padia
Vishnu Ks