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`.
Explicitly stubbing i18n in 48 different files
is mostly busy work at this point, and it doesn't
provide much signal, since often it's invoked
only to satisfy transitive dependencies.
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.
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.
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.
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.
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().
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.
ES6 and TS modules don’t insert themselves into `window`, so our tests
shouldn’t insert them either. Since the test `window` behaves like
`global` now, we can rely on legacy modules that do insert themselves
to do it themselves.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
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>
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 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.
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.
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.
This run_test helper sets up a convention that allows
us to give really short tracebacks for errors, and
eventually we can have more control over running
individual tests. (The latter goal has some
complications, since we often intentionally leak
setup in tests.)
Also adds a custom rule to eslint. Since the recommended way of extending
eslint is to create plugins as standalone npm packages, the separate rule
is published as 'eslint-plugins-empty-returns'.
Fixes#8669.
@brockwhittaker wrote the original prototype for having
pills in the recipient box when users compose PMs (either
1:1 or huddle). The prototype was test deloyed on our
main realm for several weeks.
This commit includes all the original CSS and HTML from
the prototype.
After some things changed with the codebase after the initial
test deployment, I made the following changes:
* In prior commits I refactored out a module called
`user_pill.js` that implemented some common functions
against a more streamlined version of `input_pill.js`,
and this commit largely integrates with that.
* I made changes in a prior commit to handle Zephyr
semantics (emails don't get validated) and tested
this commit with zephyr.
* I fixed a reload bug by extracting code out to
`compose_pm_pill.js` and re-ordering some
calls to `initialize`.
There are still two flaws related to un-pill-ified text in the
input:
* We could be more aggressive about trying to pill-ify
emails when you blur or tab away.
* We only look at the pills when you send the message,
instead of complaining about the un-pill-ified text.
(Some folks may consider that a feature, but it's
probably surprising to others.)
This change prepares us to have the server send avatar_url
of None when somebody wants a gravatar avatar (as opposed
to a user-uploaded one).
Subsequent commits will change behavior on both the server
and client to have this happen. So this commit has no-op
code for now, but it will soon use the fallback-to-gravatar
logic.
This logic used to be in extract_people_from_message(), but
we are deprecating extract_people_from_message(), whereas
the maybe_incr_recipient_count() function has logic that we
want to keep.
This change is the first step in making it so that we load
non-active users at page load time in the webapp.
Before this change, we would reactively handle deactivated
users when we saw them as senders in messages. Subsequent
changes will make it a warning if we see unknown senders
in messages.
This is mostly a pure extraction, but we needed to duplicate
a bit of setup from people.js, and I added an assertion
for warning about "No user_id provided".
We were incorrectly reporting active bots as non-active in
popovers, and we had no test coverage for cross-realm bots.
We also rename the function to is_active_user_for_popover,
since the old name, realm_user_is_active_human_or_bot, suggested
the wrong semantics for cross-realm bots.
Last but not least, we only do a blueslip warning if a user id
is not found. When lookups fail, we are pretty confident that
the user is not active, so an error is overkill. We can change
that as part of issue #7120.
Fixes#7153
It's easier to unit test logic inside of people.js than compose.js.
We allow users to compose emails to any of our cross-realm bots.
Someday we may tighten up which cross-realm bots are valid targets,
since it's not necessarily the case that those bots do anything
useful when you send them messages.
The function name `get_realm_human_user_ids` was a lie--it
includes active bots as well.
The only user of this function is `activity.js`, which wasn't
impacted by the misleading name, because we eventually filter
out bots in the `info_for` function.
It's possible that we actually want to include bots in the right
sidebar, since they can be difficult to discover in other parts
of the UI. Or, if we want to keep the right sidebar as all
human users, we may eventually want to make the logic to exclude
bots happen higher in the stack (but for real, this time).
Apparently this is a bug that slipped in when we started showing
normal users as deactivated in the user popovers: all bot users were
treated that way as well.
We'll want to do #7153 as a follow-up to get things fully working how
we want them.
This covers all blueslip errors and warnings
in people.js. These do not need to be tested
to rigorously and just need to be covered to
get people.js to 100% coverage.
We have added people.pm_with_url(message), which computes a
PM url from a private message using user ids rather than emails.
We call this in add_message_metadata(), since the slugs will
be valid even if emails change, so we don't need to compute
them on the fly during message rendering.
The function people.update_email() is not yet connected
to anything, but it sets the stage for upcoming changes.
When emails get updated, fundamentally we just update
the appropriate person object and add a new key to
people_dict. We sort of get a shim for free--old email
lookups will continue to work--but we add blueslip warnings
for stale lookups.
The function people.filter_by_search_terms() used
to return a JS object with emails as keys to represent
a set of users. Now we return a Zulip Dict() object
with user_ids as keys.
If we get a realm_user update for a user that is **not**
changing their full name, we no longer call
admin.update_user_full_name().
This was probably a fairly minor bug.
Earlier commits removed all uses of page_params.email outside
of people.js, and it turns out we have page_params.user_id, so
we don't even need page_params.email for seeding the data.
This commit doesn't change any behavior yet, but it starts us
down the road of deprecating page_params.email and allowing
people.js to control all access to the current user's email,
which will be important for email changes.
The slugs for PM-with narrows now have user ids in them, so they
are more resilient to email changes, and they have less escaping
characters and are generally prettier.
Examples:
narrow/pm-with/3-cordelia
narrow/pm-with/3,5-group
The part of the URL that is actionable is the comma-delimited
list of one or more userids.
When we decode the slugs, we only use the part before the dash; the
stuff after the dash is just for humans. If we don't see a number
before the dash, we fall back to the old decoding (which should only
matter during a transition period where folks may have old links).
For group PMS, we always say "group" after the dash. For single PMs,
we use the person's email userid, since it's usually fairly concise
and not noisy for a URL. We may tinker with this later.
Basically, the heart of this change is these two new methods:
people.emails_to_slug
people.slug_to_emails
And then we unify the encode codepath as follows:
narrow.pm_with_uri ->
hashchange.operators_to_hash ->
hashchange.encode_operand ->
people.emails_to_slug
The decode path didn't really require much modication in this commit,
other than to have hashchange.decode_operand call people.slug_to_emails
for the pm-with case.
When somebody changes their name, we will now update
the buddy list right away. The old code was trying
to do this through a code path that was designed for
true presence updates, but it was also passing in an
empty array, instead of undefined, which caused it to
fail to invoke the intended part of the codepath to
redraw the buddy list.
Now we just call the new activity.redraw() function,
which does the right thing for the buddy list.
The group PM list was live-updating before this change,
and it continues to live-update as part of the new
activity.redraw() function.
This commit changes people.remove() to be people.deactivate(),
and it fixes a bug where deactivating users was causing tracebacks
in the PM list if somebody had PM'ed the deactivated user
recently.
We need this for node tests, so that you don't have to explicitly
remove every user between tests. (Also, people.remove() is about
to have different semantics.)
* In most cases, eslint --fix with the right comma-dangle settings was
able to update the code correctly.
* The exceptions were cases where the parser incorrectly treated the
arguments to functions like `assert_equal` as arguments; we fixed
these manually. Since this is test code, we can be reasonably
confident that just fixing the failures suffices to correct any bugs
introduced by making changes automatically.
We now sort lists of users ids deterministically, and we also
sort list of emails deterministically and without regard to case.
This probably fixes the bug #2343, although I never got a great
repro on that.
Steve Howell
Anders Kaseorg
Priyank Patel
Hemanth V. Alluri
Rohitt Vashishtha
Yashashvi Dave
Tim Abbott
Cynthia Lin
Shubham Dhama
Shubham Padia
Brock Whittaker
Cory Lynch
Steve Howell
Joshua Pan
Rafid Aslam