This method is a bit complex, but I think it's
worthwhile to force PM autocompletes and mention
autocompletes through the same code path.
We also kill off this method:
typeahead_helper.sort_people_and_user_groups
This fixes some regressions from a recent
commit that might not have been deployed.
9f7be51ce8
Even if that change had been deployed, it
should not have been user-facing, but it
would have spammed us with blueslip errors
every time somebody used the stream/topic
popovers in the left sidebar.
There's no reason any more to have a single function
filter both persons and groups. Instead, we just
filter each cohort with a more direct function.
This is a minor performance speedup (avoiding the
conditional in the loop), but I mostly wanted to
simplify the code.
The sort_people_and_user_groups function's only
value-add over sort_recipients is to split out
groups and users, but the caller already had
them split out, so it was kinda silly to concatenate
them back together.
I doubt this was a dumb decision at the time; I think
it was probably a consequence of how bootstrap's
normal approach is kinda inflexible when you're
using typeahead to pull data from multiple sources.
I wanted to kill off sort_people_and_user_groups
completely, but the mention/silent_mention autocompletes
are still a bit awkward to refactor.
For historical reasons pm_list was handling just
one possible edge case of where is:private was
combined with other search terms, namely the
pm-with operator.
The code was correct in realizing the is:private
was redundant there, but now we handle that
upstream in Filter.fix_operators (see previous
commit).
Now we just look for any is:private term.
This change makes these two functions more alike:
- get_search_result
- get_search_result_legacy
To test the UI modify zerver/views/home.py by
replacing `settings.SEARCH_PILLS_ENABLED` with
`True`. I only did a quick sanity check, since
any bugs with the new system are more likely due
to bitrot than any changes I have made here.
The history is this:
Tim cloned the code (before the smaller
helpers were extracted):
db4f6e278f
In 8b153f6452
Shubham removed get_operator_subset_suggestions but
accidentally left a `concat` statement in that got
misapplied to the previous suggestions:
- suggestions = get_operator_subset_suggestions(operators);
result = result.concat(suggestions);
The error there was carried over in some recent changes,
but this commit fixes that strangeness.
In 73e4f3b3fa
Shubham made this change, which makes sense only for
pills, and this code remains intact.
- if (operators.length > 0) {
- last = operators.slice(-1)[0];
+ if (query_operators.length > 0) {
+ last = query_operators.slice(-1)[0];
+ } else {
+ // If query_operators = [] then last will remain
+ // {operator: '', operand: '', negated: false}; from above.
+ // `last` has not yet been added to operators/query_operators.
+ // The code below adds last to operators/query_operators
+ operators.push(last);
+ query_operators.push(last);
}
Mohit made a couple changes to both old and new.
Anders made a couple non-substantive changes related to
the ES6 migration.
Steve (me) made several structural changes to the code. For
some of them I only changed the legacy code, not the pills
code. I didn't fix Shubham's mistake until this change.
Now the two functions should look similar except in the places
where they are intentionally different. I also added a comment
explaining the get_operator_subset_suggestions difference.
Fixes#13609
There may be a deeper issue that various JavaScript logic expects
every message to have a `.message_content` element, but we definitely
should have the `.rendered_markdown` class on any markdown content.
Fixes#13634.
The reason we use functions here will be clear
in the next change.
This is a "prefactor" commit that doesn't change
any user-facing behavior nor significantly change
performance.
A few reasons to extract it:
- we can shorten lines (and not repeat query
every time)
- we can scope the big block comment explaining
why util.prefix_sort is a strange name
- the name is better (it's an O(N) operation that
mostly partitions)
- we may want to swap it out with a true partition
function that's truly a partition (since the
case checks done by prefix_sort are possibly
either a non-feature or mostly overridden by
the other sorts)
The recip.id || recip.user_id idiom has only been
needed for some old unit tests.
It was previously required as a bad workaround for the
local echo issue fixed in dd1a6a97bd
where we would get `display_recipient` values added in an invalid format.
I want to be able to easily test this without
having to simulate all the jQuery side effects.
This simply preserves the old logic, which seems
to handle one edge case without handling every
possible edge case. The edge cases aren't super
important here, though, since the only thing it affects
is bolding "Private Messages", and when to do that
is somewhat up to personal tastes.
Having said that, we could definitely improve
this code and possibly should move some of this
logic to either narrow_state.js or filter.js.
Instead of doing various ad-hoc calculations of
which PM is "active" and plumbing it through various
functions and then updating it via jQuery instead of
just the template, we now just calculate `is_active`
in `_build_private_messages_list` with a little
helper function.
In 3cfc3ca24b I removed
the feature that limited PM conversations to five or
less (including the active conversation), but I
didn't clean up this parameter. I think lint was
confused by the fact that we did mutate it.
I am wondering if this started out as an experiment
and was never fully polished before the push? Or
maybe I was just careless. Anyway, I don't
think were any symptoms here--it was just dead code
that we didn't need.
This fixes a rebase issue between the int_dict introduction and use
for people.js with the introduce of filter_values on dict.js and use
inside people.js.
Note that we haven't fully swept this for Dict,
since some dicts are keyed by strings. For
example PM counts can have a huddle like
"101,102,103" as a key.
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.
This looks like simple code cleanup, but it's more
than that.
The code cleanup here is that we don't have three
callbacks to get a list of typeaheads for bootstrap.
Instead, we just have one function that does all the
main work.
And then the speedup comes from the fact we no longer
need to remove diacritics from the query for every
time through our loop of seeing if a person matches
the query.
It's a bit subtle to see in the diff, but these are
the relevant lines:
const matcher = exports.get_person_or_user_group_matcher(query);
const filtered_results = _.filter(people_and_groups, matcher);
Before this, bootstrap was doing $.grep, and we'd have
to reinitialize the matcher for every person.
If you profile this before and after, you'll see that
remove_diacritics gets called fewer times.
To profile this, you want to loads lots of users into
your DB and try to autocomplete "Extra", as in "Extra1 User".
If you try to autocomplete something else, then my patch
won't really help, and `remove_diacritics` will still
show up as expensive. Because it is that expensive a function.
These had to be done in tandem, since they were
both kinda coupled to the function that is now
called query_matches_name_description.
(This commit slightly negatively impacts PM
lookups, but this is addressed in the subsequent
commit, which makes PMs much faster. The impact
is super minimal--it's just an extra function
dispatch.)
This may seem silly now, since we are returning a function
that still dispatches over all flavors of search for
every item, but subsequent commits will make it obvious
why I'm doing this.
We want to do our own matching of items, rather than
just giving a callback to bootstrap, which does $.grep
on all the items.
Doing our own matching gives us flexibility for future
improvements like custom data structures for searching
through big amounts of data. Even in the short term
we can speed up searches by pulling expensive operations
outside the grep/filter call.
This architecture has been in place for our search
bar since ~2014.
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 had a potentially nasty bug where we
weren't guaranteeing that all/stream/everyone
collated in consistent ways inside of
`compare_people_for_relevance`, which can
send certain types of sort algorithms into
an infinite loop. I doubt this ever happened
in practice, but it's obviously worth fixing.
Now we also have a clear tiebreaker between
any two all/everyone/stream mentions, which
is the idx field.
Finally, this should be a bit more efficient.
This name was misleading, since this code is used
in sort_recipients, which happens when you, for
example, autocomplete persons in the "To:" box
when composing (and has nothing to do with
mentioning).
This makes it a bit easier to find common patterns,
plus it sets us up to pull the calls even further
up the stack.
The first rule of dealing with user data is sanitize
at the edges, not deep down in some function that
has many callers. Putting this code so deep down
in the stack means it's more likely to be called in
a loop.
This moves clean_query into all the callers
of query_matches_source_attrs.
This doesn't change anything performance-wise,
but it sets up future commits.
This change is easy--we only had one caller.
This change means any query going against a
target with multiple `match_attrs`, such as
user names (first name, last) only has to
clean the query once per person.
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 two user_ids in a recent huddle have ids
that sort lexically differently than numerically,
such as 7 and 66, then we were creating two
different buckets in pm_conversations.
This regression was introduced in
263ac0eb45 on
November 21, 2019.
Instead of having our callers pass in a possibly
non-canonical version of a user_ids_string, just
have them pass in a list.
The next commit will canonicalize the sort.
The only thing get_color() does is look
up a sub:
exports.get_color = function (stream_name) {
const sub = exports.get_sub(stream_name);
if (sub === undefined) {
return stream_color.default_color;
}
return sub.color;
};
So if we have a sub already, there's no point
calling the helper.
Obviously, this isn't a huge deal, but it happens
N times during page load.
This should make any operation on subscribed
streams faster (we won't need to filter out
unsubscribed streams every time).
I started writing this before I realized we
had a bug where we call `subscribed_streams`
in a nested loop.
After fixing the bugs, this is not as much of
a bottleneck, but it's still a speedup in many
important places:
* build left sidebar
* every keystroke in search bar
* first keystroke in making #stream_links
* every keystroke in compose stream box
The streams settings code is kinda complicated.
It does a non-deterministic sort of the "others"
bucket when you add elements to the left panel.
They get hidden, anyway. Our values() call now
puts subscribed streams first. It never guaranteed
order, but putting subscribed streams first is
probably a good behavior for most situations.
This defers O(N*S) operations, where
N = number of streams
S = number of subscribers per stream
In many cases we never do an O(N) operation on
a stream. Exceptions include:
- checking stream links from the compose box
- editing a stream
- adding members to a newly added stream
An operation that used to be O(N)--computing
the number of subscribers--is now O(1), and we
don't even pay O(N) on a one-time basis to
compute it (not counting the cost to build the
array from JSON, but we have to do that).
Calling `set_filter_out_inactives` is expensive, since we
count up the number of subscribed streams, which iterates
through all your streams, creates a new list of subscribed
streams, then counts them.
In my dev setup, I created 700 streams, and this shaved
about 700ms off of the initial call to `build_stream_list`.
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.
It appears c28c3015 caused a regression where we
set `email` to undefined if a user does not have
`delivery_email` set, and this causes filtering
of users to fail for admins doing user settings.
This fixes only one of the issues reported in
issue #13554.
There's probably no easy fix to scrolling taking
long, but I think fixing search will mostly
address that complaint.
The Rust folks seem to agree with me that the
search results are too noisy. If I search for
"s" I get:
* names like Steve (good)
* names like Jesse (noisy)
* anybody with s in their email (super noisy)
Here is the relevant code:
return (
item.full_name.toLowerCase().indexOf(value) >= 0 ||
email.toLowerCase().indexOf(value) >= 0
);
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.)
Once we have max_items results, stop trying
to get more items.
This should really help large realms when
you do a search on streams that turns up
more than N streams (where N is about 12).
We won't even bother to find people.
This class gives us more control over attaching
suggestions to our eventual result. The main
thing we do now is remove duplicates as they're
encountered.
This will make sense in the follow up commit,
where we can short circuit actions as soon as
we get enough results.
This has a few benefits:
- we remove some duplicate code
- we can see finalize_results in profiles
It turns out finalize_results is expensive
for some searches. If the search itself doesn't
do a ton of work but returns a lot of results,
we see it in finalize_results. It brings to
attention that we should be truncating items
earlier instead of doing lots of unnecessary
work.
This isn't a huge speedup, but it's an easy
code change.
We remove the two-liner highlight_with_escaping,
which was only called in one place, and when
we inline it into the caller, we can pull the
first line, which builds the regex, out of the
loop.
The code we removed in highlight_with_escaping
is exactly the same code as in
highlight_with_escaping_and_regex.
I actually copy/pasted this code five years
ago and am now removing the duplication. :)
When we're highlighting all the people that show
up in a search from the search bar, we need
to fairly expensively build a regex from the
query:
query = query.toLowerCase();
query = query.replace(/[\-\[\]{}()*+?.,\\\^$|#\s]/g, '\\$&');
const regex = new RegExp('(^' + query + ')', 'ig');
Even though the final regex is presumably cached, we
still needed to do that `query.replace` for every person.
Even for relatively small numbers of persons, this would
show up in profiles as expensive.
Now we just build the query once by using a pattern
where you call a function outside the loop to build
an inner function that's used in the loop that closes
on the `query` above. The diff probably shows this
better than I explained it here.
This fixes a cross-site scripting vulnerability in the upcoming Inline
URL Previews feature found by Graham Bleaney and Ibrahim Mohamed using
Pysa.
This commit doesn't get a CVE because the bug was present in a code
path introduced in the 2.1.x development branch, so it doesn't impact
any Zulip release.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The streams:all adveritsement notice in search should only appear
after we've already received the response from the server, to avoid a
mix of problems ranging from misplaced loading indicator to scrolling
issues to the notice just being distracting while you're waiting for
the server to return results.
We need to add a pre_scroll_cont parameter to the message_fetch API,
since adding this notice would otherwise potentially throw off the
scroll positioning logic for which message to select.
Fixes#13441.
In 452e226ea2 and
648a60baf6, we changed how `search:`
narrows work to:
(1) Never mark messages as read inside searches (search:)
(2) Take you to the bottom, not the first unread, if a `near:` or
similar wasn't specified.
This is far better behavior for these use cases, because in these
narrows, you can't actually see all the context around the target
messages, so marking them as read is counterproductive. This is
especially important in `has:mention` where you goal is likely
specifically to keep track of which threads mentioning you haven't
been read. But in many other narrows, the current behavior is
effectively (1) setting the read bit on random messages and (2) if the
search term matches many messages in a muted stream with 1000s of
unreads, making it hard or impossible to find recent search matches.
The new behavior is that any narrow that is structurally a search of
history (including everything that that isn't a stream, topic,
pm-with, "all messages" or "private messages") gets that new behavior
of being unable to mark messages as read and narrows taking you to the
latest matching messages.
A few corner cases of interest:
* `is:private` is keeping the old behavior, because users on
chat.zulip.org found it confusing for `is:private` to not mark
messages as read when one could see them all. Possibly a more
complex answer is required here.
* `near:` narrows are getting the new behavior, even if it's a stream:
+ topic: narrow. This is debatable, but is probably better than
what was happening before.
Modified significantly by tabbott for cleanliness of implementation,
this commit message, and unit tests.
Fixes#9893. Follow-up to #12556.
In 1fe4f795af, we added the
wildcard_mentions_notify setting, which controls whether wildcard
mentions should be treated as mentions for the purposes of
notifications. The original implementation focused on the more
important area of email/push notifications, and neglected to address
desktop notifications for wildcard mentions.
This change makes the wildcard_mentions_notify flag behave correctly
for desktop/sound notifications, including unit tests.
Fixes#13073.
We register ZulipRemoteUserBackend as an external_authentication_method
to make it show up in the corresponding field in the /server_settings
endpoint.
This also allows rendering its login button together with
Google/Github/etc. leading to us being able to get rid of some of the
code that was handling it as a special case - the js code for plumbing
the "next" value and the special {% if only_sso %} block in login.html.
An additional consequence of the login.html change is that now the
backend will have it button rendered even if it isn't the only backend
enabled on the server.
Adds required API and front-end changes to modify and read the
wildcard_mentions_notify field in the Subscription model.
It includes front-end code to add the setting to the user's "manage
streams" page. This setting will be greyed out when a stream is muted.
The PR also includes back-end code to add the setting the initial state of
a subscription.
New automated tests were added for the API, events system and front-end.
In manual testing, we checked that modifying the setting in the front end
persisted the change in the Subscription model. We noticed the notifications
were not behaving exactly as expected in manual testing; see
https://github.com/zulip/zulip/issues/13073#issuecomment-560263081 .
Tweaked by tabbott to fix real-time synchronization issues.
Fixes: #13429.
If a message begins with /me, we do not have any cases where the
rendered content would not begin with `<p>/me`. Thus, we can safely
remove the redundant checks both on the backend and frontend.
The "Stream settings" UI was always intended to be initialized in the
"Subscribed" tab when opened not through navigation that explicitly
aims to via "All streams". We had implemented that through how the UI
is rendered as well as the internal state tracking variable
`subscribed_only`, which was initialized to `true`.
The bug was that we didn't reset that to `true` when re-opening
"Stream settings" via a code path that calls `setup_page` (e.g. via
the menus on the left sidebar).
Ths fixes a bug where the stream-list in the stream settings would
list all streams but would show the 'Subscribed' label after
navigating to "All streams", closing "Manage streams", and then
reopening it.
Fixes#13297.
In e42c3f7418, we made the assumption
that compose_pm_pill.get_recipient() would return no users for stream
messages. It turns out, due to the confusing name of
compose_state.recipient (which we just renamed to
compose_state.private_message_recipient), this assumption was wrong.
As a result, when composing a stream message using the reply hotkeys,
we'd end up sending typing notiifcations to the person who sent the
message we're replying to as though a PM was being composed.
We fix this by avoiding passing an (expected to be unused) value for
private_message_recipient to compose_state.start.
The compose_state.recipient field was only actually the recipient for
the message if it was a private_message_recipient (in the sense of
other code); we store the stream in compose_state.stream instead.
As a result, the name was quite confusing, resulting in the
possibility of problematic correctness bugs where code assumes this
field has a valid value for stream messages. Fix this by changing it
to compose_state.private_message_recipient for clarity.
Fixes commit id 648a60baf6. When
allow_use_first_unread_when_narrowing() is false last message of
narrow is shown in view.
Comments rewritten by tabbott to explain in detail what's happening.
This simple change switches us to take advantage of the
server-maintained data for the pm_conversations system we implemented
originally for mobile use.
This should make it a lot more convenient to find historical private
message conversations, since one can effectively scroll infinitely
into the history.
We'll need to do some profiling of the backend after this is deployed
in production; it's possible we'll need to add some database indexes,
denormalization, or other optimizations to avoid making loading the
Zulip app significantly slower.
Fixes#12502.
message_id, rather than timestamps, is our standard way to sort by
time. And this refactor is important because we're about to start
using data from the server to populate this data structure.
This avoids a stream having potentially near-infinite height when
opened in a stream with a large number of unread topics; the benefit
is that you can easily access the next stream.
We show an unread count next to "more topics" to make it hard to miss
that there might be more, older topics with unread messages.
With CSS work by Anders Kaseorg.
Fixes#13087.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit was automatically generated by `tools/lint --only=eslint
--fix`, except for the `.eslintrc.json` change itself.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Updates the message editing process to do a local 'echo'.
On slow connections, now there is visual confirmation of the edit,
similar to when sending messages. The contains_backend_only_syntax
logic and check are the same as there.
We showing "(SAVING)" until the edit is completed, and on successful
edit, the word "(EDITED)" appears. There's likely useful future work
to do on making the animation experience nicer.
Substantially rewritten by tabbott to better handle corner cases and
communicate more clearly about what's happening.
Fixes: #3530.
This change makes it possible for users to control the notification
settings for wildcard mentions as a separate control from PMs and
direct @-mentions.
This commit was automatically generated by `tools/lint --only=eslint
--fix`, after an `.eslintrc.json` change.
A half dozen files were removed from the changes by tabbott pending
further work to ensure we avoid breaking valuable PRs with merge
conflicts.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Hovering over user names (and user circles for PM List) now displays
Name, Status Message and Last online time in a js tooltip.
Hovering over group names displays the names of all group members.
Unavailable users are shown as "Last active: Today".
Hovering on a user circle in the Buddy List results in a js tooltip
with Active/Idle/Offline/Unavailable for
green/orange/white/white-with-line.
Resolves#11607.
When strings are tagged for translation using `tr this`, the strings
were passed into the frontend i18n as-is (including new line and tab
characters that are not functional in the text, existing just to
format the HTML files reasonably).
This did not match the algorithm used in `manage.py makemessages` for
extracting strings for translation, which (correctly) removed that
whitespace to provide a good experience for translators. The fix is
for the `tr this` implementation to use that same whitespace-stripping
algorithm.
Tested manually by checking if those strings that were not translated
earlier were translated, and also fixed an automated test that had the
wrong result, which should help prevent regressions.
Fixes#13389.
Previously, we had a "Return to login" button on the previous page of
the password reset flow, but none on the final page.
Note that this button is only shown in the Zulip Electron app.
Fixes#13378.
These should work consistently with how the individual user setting
works; see the last commit.
With changes from tabbott to fix real-time sync.
Fixes#12553.
This fixes two regressions in 1946692f9a.
The first bug was actually introduced much earlier, namely that we
were not sending a `bot_owner_id` field at all for bot users without
an owner. The correct behavior would have been send `None` for the
owner field.
The second bug was simply that we needed to update the webapp to look
for the `bot_owner_id` field, rather than an old email-address format
`bot_owner` field.
Thanks to Vinit Singh for reporting this bug.
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>
This commit modifies the `#add-stream-link` element to be a `div`
containing the previous `a` element. The margin that was added to
`#stream-filters-container .simplebar-content` is then moved to that new
`div`.
This preserves the intended behaviour of the commit which introduced
the margin, to fix#12519 while removing an unnecessary scrollbar
which could hide the top-most stream in the stream list.
Fixes#13050
Signed-off-by: David Wood <david@davidtw.co>
The js_typings directory is not set up correctly for us to add new
type declarations for untyped external modules. The correct
configuration would be something like
{
"compilerOptions": {
"baseUrl": ".",
"paths": {
"*": ["js_typings/*"],
},
"typeRoots": ["js_typings"],
},
"exclude": [
"js_typings",
],
}
but that configuration is incompatible with using the same directory
for _internal_ modules like the ones declared here.
Also, correct some mistakes the generation of this list.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Set `--esModuleInterop` and `--isolatedModules` for consistency with
Babel. `tsc --init` adds `--esModuleInterop` by default.
Set `--moduleResolution node` so we can find type definitions in
modules that provide them.
Set `--forceConsistentCasingInFileNames`, which seems like a good
idea, and which `tsc --init` will add by default in TypeScript 3.7.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
`--jsx preserve` and `--removeComments false` are already the default.
`--strict` already implies `--noImplicitAny`, `--noImplicitThis`,
`--alwaysStrict`.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Commit d17b577d0c (#13321) incorrectly
transformed this line, even though I thought my script had a specific
guard against this.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Webpack code splitting will make the inclusion order of CSS files less
obvious, and we need to guarantee that these rules follow the rules
they override.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
login_context now gets the social_backends list through
get_social_backend_dicts and we move display_logo customization
to backend class definition.
This prepares for easily adding multiple IdP support in SAML
authentication - there will be a social_backend dict for each configured
IdP, also allowing display_name and icon customization per IdP.
ESLint won’t convert these automatically because it can’t rule out a
behavior difference arising from an access to a self-referential var
before it’s initialized:
> var x = (f => f())(() => x);
undefined
> let y = (f => f())(() => y);
Thrown:
ReferenceError: Cannot access 'y' before initialization
at repl:1:26
at repl:1:15
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Because of the separate declarations, ESLint would convert them to
`let` and then trigger the `prefer-const` error.
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>
This feels a bit more semantically appropriate: it more clearly says
"here's some information: there is no (relevant) recipient", rather
than "no information available". (Both `null` and `undefined` in JS
can have either meaning, but `undefined` especially commonly means
the latter.)
Concretely, it ensures a bit more explicitness where the value
originates: a bare `return;` becomes `return null;`, reflecting the
fact that it is returning a quite informative value.
Also make the implementation more explicit about what's expected here,
replacing truthiness tests with `!== null`. (A bit more idiomatic
would be `!= null`, which is equivalent when the value is well-typed
and a bit more robust to ill-typing bugs. But lint complains about
that version.)
It'd already been the case for some while that calling `stop` had the
same effect as calling `update` (previously `handle_text_input`) with
a falsy recipient. With the API changes in the previous few commits,
this becomes quite natural to make explicit in the API.
This was named after when it gets called from the UI, rather than
after what it can be expected to do.
Naming it after what it's meant to do -- and giving a summary line to
expand on that -- provides a more helpful semantic idea for reasoning
about the function. Doubly so for using the function in a different
client with its own UI, like the mobile app.
The main motivation for this change is to simplify this interface
and make it easier to reason about.
The case where it affects the behavior is when
is_valid_conversation() returns false, while current_recipient
and get_recipient() agree on some truthy value.
This means the message-content textarea is empty -- in fact the
user just cleared it, because we got here from an input event on
it -- but the compose box is still open to some PM thread that we
have a typing notification still outstanding for.
The old behavior is that in this situation we would ignore the
fact that the content was empty, and go ahead and prolong the
typing notification, by updating our timer and possibly sending a
"still typing" notice.
This contrasts with the behavior (both old and new) in the case
where the content is empty and we *don't* already have an
outstanding typing notification, or we have one to some other
thread. In that case, we cancel any existing notification and
don't start a new one, exactly as if `stop` were called
(e.g. because the user closed the compose box.)
The new behavior is that we always treat clearing the input as
"stopped typing": not only in those cases where we already did,
but also in the case where we still have the same recipients.
(Which seems like probably the common case.)
That seems like the preferable behavior; indeed it's hard to see
the point of the "compose_empty" logic if restricted to the other
cases. It also makes the interface simpler.
Those two properties don't seem like a coincidence, either: the
complicated interface made it difficult to unpack exactly what
logic we actually had, which made it easy for surprising wrinkles
to hang out indefinitely.
Returning true from this function means we go on to send, or extend
the lifetime of, a typing notification; returning false means we don't.
It's hard to see why having a partially-entered name in the recipient
box should mean we're *more* inclined to send a typing notification to
the set of recipients that are already entered; if anything, it seems
like it should make us *less* inclined to do so. So we're better off
without this conditional.
The conditional was introduced in commit 72295e94b, as part of a
conversion from user emails to user IDs; there, it seems to replace a
condition that went in the opposite direction, returning *false* if
there were any invalid emails in the recipient box. So perhaps it's
just inverted.
Moreover, the (re-)inverted version would also be wrong: if the user
is typing a PM addressed to some users, and they hit send, the message
will go to those users whether or not they have any unconverted text
in the recipients box. So the typing notifications should too.
The real purpose these two callbacks serve is exactly what an ordinary
parameter is perfect for:
* Each has just one call site, at the top of the function.
* They're not done for side effects; the point is what they return.
* The function doesn't pass them any arguments of its own, or
otherwise express any internal knowledge that doesn't just as
properly belong to its caller.
So, push the calls to these callbacks up into the function's caller,
and pass in the data they return instead.
This greatly simplifies the interface of `handle_text_input` and of
`typing_status` in general.
This is intended as a pure refactor, making the data flow clearer in
preparation for further changes. In particular, this makes it
manifest that the calls to `get_recipient` and `is_valid_conversation`
don't depend on anything else that has happened during the call to
`handle_text_input`.
This is indeed a pure refactor because
* is_valid_conversation itself has no side effects, either in the
implementation in typing.js or in any reasonable implementation,
so calling it sooner doesn't affect anything else;
* if we do reach it, the only potentially-side-effecting code it's
moving before is a call to `stop_last_notification`, and that in
turn (with the existing, or any reasonable, implementation of
`notify_server_stop`) has no effect on the data consulted by
the implementation of `is_valid_conversation`.
Users generally don't expect wildcard mentions in muted streams and
topics to be treated as a mention, either for the purposes of desktop
notifications or the unread mention counts.
This fixes the unread mention counts part of the issue.
Fixes part of #13073.
When email address visibility is set to everyone, there is no change in
behavior, but when it is set to "admins-only", we don't show any email
in user profile modal (just like popovers) for everyone but admins.
When email address visibility is set to everyone, there is no change in
behavior, but when it is set to "admins-only", we don't show any email
in popovers for everyone but admins.
It should be azuread-oauth2-wrapper, as the name of the corresponding
backend is 'azuread-oauth2'. Without the correct name, the icon isn't
showing on the "Log in with AzureAD" button.
This adds the general machinery required, and sets it up for the file
`typing_status.js` as a first use case.
Co-authored-by: Anders Kaseorg <anders@zulipchat.com>
These indeed used to be strings, but were converted to arrays in
b8250fc61, and these names didn't get updated to match.
A classic example of why type-checking is a great job to get
machines to do. :-)
There are a few outstanding issues that we expect to resolve beforce
including this in a release, but this is good checkpoint to merge.
This PR is a collaboration with Tim Abbott.
Fixes#716.
When a user performs a search that might contain historical public
streams messages that the user has access to (but doesn't because
we're searching the user's own personal history), we add a notice
above the first search result to let the user know that not all
messages may have been searched.
Fixes#12036.
A somewhat recent refactoring of the left sidebar had introduced a gap
between the hover areas that looked off; this fixes this with a slight
rearrangement with where the 1px of space between elements lives.
Fixes#12508.
In 50545a3 we made an incomplete revert of some style changes from
7b8da9b, this commit reverts the "x" to "fa fa-times" and also fixes an
alignment issue for the "Discard" box in chrome.
Fixes#13233.
This fixes a glitch where the keyboard shortcuts icon, which is meant
to be a feature of the right sidebar, appears overlapping the "Reply"
button.
Fixes#13122.
When typing_status adds 10000 to this value, it would previously
obtain wacky strings like
"Fri Oct 04 2019 16:45:59 GMT-0700 (Pacific Daylight Time)10000"
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The previous code for ensuring the sort order of emoji choices was
correct relied on an OrderedDict structure, which isn't guaranteed to
be preserved when passed to the frontend via JSON (in fact, it isn't,
since we converted the way page_params is passed to use
sort_keys=True). Switch it to a list of dictionaries to correct this.
Fixes#13220.
The avatar source was misspositioned when avatar changes were disabled.
This also repositions the "X" for when avatar changes are allowed.
Fixes#12524.
The historical behavior of having `Enter` exit was optimized for the
"View source" use case; but `Esc` now handles that reasoanbly, and we
really should make it convenient to type in the user-editable text
box here.
Fixes part 1 of #11834.
This ensures that typing '```java' and pressing enter would result in
getting dropped into a java codeblock instead of javascript codeblock.
We implement this by pushing the exact match of a query to be pushed to
the top of the returned matches in `sort_languages`.
With some comments added by tabbott in the tests explaining the
current reasoning.
Fixes#13109.
Apparently, the changes in fe2adeeee1 to
fix a Firefox focus bug accidentally had the side effect of removing
the topic text box from the area being considered, resulting in the
escape key no longer working to end the message edit from within that
text box.
This is a simple and small commit which will alphabetically order the
entries of the fixtures dropdown menu in the "integrations developer
panel" devtool.
Apparently, the Zulip notifications (and resulting emails) were
correct, but the download links inside the Zulip UI were incorrectly
not including S3 prefix on the URL, making them not work.
While we're at this, we rewrite the somewhat convoluted previous
system for formatting the data export output.
This has two purposes:
1. Prevent stupid stacks of diacritical marks from overflowing into
other messages. Fixes#7843.
2. Prevent Chrome from collapsing the inside bottom margin with the
.messagebox outside (in a way that Firefox doesn’t).
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Bootstrap v2.2.0^2~40^2~6 changes this default to false, so this is a
prerequisite to upgrading Bootstrap, and it’s also safer.
This closes an HTML injection path via user full names in the emoji
reaction tooltip. It doesn’t appear to be exploitable for cross-site
scripting because we disallow `>` in full names, and the code happens
to be written such that the next `>` is in a different parser
invocation.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In a gigantic realm where we send several MB of `page_params`, it’s
slightly better to have the rest of the `<body>` available to the
browser earlier, so it can show the “Loading…” spinner and start
fetching subresources.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
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.
This changes the availability icon for bot users to user_circle_green;
previously it was accidentally defaulting to user_circle_empty, making
it appear that bots were never available.
Fixes#13149.
As it turns out, our rerender_the_whole_thing function (used whenever
we were adding messages and discovered that the resulting message list
would be out-of-order) was just broken and scrolled the browser to a
random location.
This caused two user-facing bugs:
* On very fast networks, if two users sent messages at very close to
the same time, we could end up with out-of-order message deliveries,
triggering this code path, which was intended to silently correct
the situation, but failed.
* In some narrows to streams with muted topics in the history but some
recent traffic, the user's browser-cached history might have some
gaps that mean the server fetch we do after narrowing discovers the
history is out-of-order, again triggering the
rerender_the_whole_thing code path.
The fix is to just remove that function, adding a new option to the
well-tested rerender_preserving_scrolltop (which has explicit logic to
preserve the scroll position) instead.
Fixes#12067. Likely also fixes#12498.
This sidesteps tricky escaping issues, and will make it easier to
build a strict Content-Security-Policy.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This sidesteps tricky escaping issues, and will make it easier to
build a strict Content-Security-Policy.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
I changed the element to be a `p` instead of `div` because the styling
for `a`s inside paragraphs is already there and the element should
anyway be a paragraph.
Fixes part of #12853.
Commit ba66dfe977 incorrectly inflated
the specificity level of these rules by moving them inside
.rendered_markdown “entirely for readability”. KaTeX has its own
rules that work better, so just delete ours.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This brings us in line, and also allows us to style these more like
unordered lists, which is visually more appealing.
On the backend, we now use the default list blockprocessor + sane list
extension of python-markdown to get proper list markup; on the
frontend, we mostly return to upstream's code as they have followed
CommonMark on this issue.
Using <ol> here necessarily removes the behaviour of not renumbering
on lists written like 3, 4, 7; hopefully users will be OK with the
change.
Fixes#12822.
This caused weird behavior in the relevant band of window widths, and
removing it works considerably better.
There's still bad behavior in handling situations where the stream
name is too long and thus this wraps, but we should address that
as a follow-up.
Fixes#13134 as the last commit in the series for this issue.
Solves the "The (?) should just be a target=_blank link to
/help/message-a-stream-by-email." part of the issue.
As a result, a bunch code managing the email hint popup can be deleted,
together with a node test for that.
cssnano reduces this to a constant in a production build. (We could
add postcss-calc if we wanted this reduced in development.)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Many of them are now automatically generated by autoprefixer, while
others are unnecessary based on .browserslistrc, and some were just
wrong (the linear-gradient based checkerboard pattern in lightbox has
been broken in Firefox for a while).
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
It’s about as fast as node-sass (faster, according to their
benchmarks) and more flexible. Autoprefixer is neat: we can now go
delete all our -moz-, -webkit-, etc. lines and have them autogenerated
as necessary based on .browserslistrc.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
As other data of field, such as field name, hint etc. are
relative to field type, this commit moves the field type
input to the first order in create field form in org settings.
There was a bug where the success banner stuck
around even after the export completed. We now
nicely fade and remove the banner upon a successful
population of the export in the table.
Fixes: #13045
02cfb47 removed a couple HTML tags that were
being used to sort the table. We fix this,
but disable filtering exports by marking the
input type as `hidden`. We use this approach as
it seems `list_render` doesn't like an
undefined `opts.filter.element`, which is
what happens if we simply remove the `filter`
key.
Follow up of commit 2a1305d. Replace all local variables named 'msgid'
with 'message_id' in all JS and HTML files, and adds a linter rule for
it as well.
Resolves#12952.
Since these rules are overwritten we can remove them. For
message_header_colorblock we can remove `!important` from
box-shadow since it was present due to the removed rules.
Add ability to search entire message history of all public streams at
once. It includes all subscibed, non subscribed public streams messages
and even historical public stream messages sent before user had joined
an organization or stream.
Fixes#8859.
This commit fixes an issue where when you click on the sort button of
a table twice, reversing stops.
The problem is we are checking the truthness of meta.sorting_function
instead of just the function argument sorting_function. This commit
extract the reverse operation out of sort() to unclutter the logic.
When a user toggles a setting back to its original value without
saving, we automatically hide the save/discard widget, since
effectively the user has discarded their changes.
The logic has previously incorrectly configured this as returning to
the "saved" state, not the "discarded" state, which caused an
unintentional delay before the widget disappeared (by accidentally
running code that was designed for the save -> saved transition).
While doing this I have fixed a very minor bug that we haven't sent
fadeout_delay argument as 0, but having its value as undefined still
defaults to 0 so there will no impact of this change.
Fixes: #12258.
This is in series of refactoring of code for realm logo settings.
Further, we will remove ids from the template as well and simply use
general classes (.day-settings and .night-settings) to identify to which
theme-mode particular element belongs i.e. day or night as we did in this
change.
- These ids will further be used to represent each section concisely and
deduplicating code.
- Also, removed `realm-night-logo-section` class as it was redundant.
If we call `popovers.hide_all` with a smaller browser
window, this breaks the functionality that the
conditional is attempting to handle. We instead use
`hide_all_except_sidebars` to prevent the user list
from being closed.
If the display setting to show the user list in the
left sidebar is enabled, the behavior is even worse.
We add a conditional to maintain the streamlist
sidebar when clicking the chevron to show and hide
the popover here as well.
It seems `presence.presence_info[item.user_id]` works fine for the current
user as well and there is no need to hardcode extra condition for the
current user.
For organization settings page there are few sections' panels which are not
visible (unless you click on 'show more') but when we use up-down arrows to
navigate between sections, sections of hidden panels also get visible which
leads to confusion.
Fixes: #13008.
While refactoring, I tested all the rules and removed the CSS that was
not needed or duplicated.
I removed the `$("#integration-list-link").css('display', 'block');` and
moved it to css because there is no case in which the back link is
hidden.
I rearranged the elements of the left sidebar in HTML in order to appear
in the order they are displayed and removed the absolute positioning,
because it was not needed if the elements are arranged correctly. I used
`flex` display to arrange them on column.
I removed the styling that positioned the elements absolutely.
Then I tweaked the margins in order to make the elements look good.
Fixes: #12929
I added the `white-box` as it was in the other similar pages
(`/accounts/go`).
In order to be able to style it better, I removed the buttons and added
`div`s instead, then added click handler for submitting the form.
If the email is associated to a Zulip account, the avatar of the account
is displayed and the text `Log in`, otherwize a `+` sign is
displayed and the text `Create new account`.
I changed the class of the title in order to use the same styling as the
other similar pages (like `/accounts/go` or `/login`).
Changed the related test.
Fixes: #2734.
`local_id` was being transmitted to the server as a string by the AJAX
transmission path, and as a number by by the WebSocket transmission
path. Then, one of the two racing success callback paths would use
the original number, while the other would use the type returned by
the server. Depending on which transmission path was used and which
callback path won the race, `reify_message_id` would sometimes be
passed a string that would fail to compare equal to the numerical
selection id. If the locally echoed message was selected, this would
cause the selection to disappear.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
If a user was active within the last 90 days,
show number of days (23 Days ago).
If the user was active more than 90 days ago and in the same year,
then show MMM DD (Mar 15).
In any other case show MMM DD YYYY (Nov 10 2018),
Change timerender.js test to accomodate changes.
Previous cleanups (mostly the removals of Python __future__ imports)
were done in a way that introduced leading newlines. Delete leading
newlines from all files, except static/assets/zulip-emoji/NOTICE,
which is a verbatim copy of the Apache 2.0 license.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In the emails-hidden case, for non-admins, we should remove the email
field from "Users" list in the organization settings page.
Tweaked by tabbott to correctly handle the bots and deactivated users pages.
Our implementation requires at least 1 space after the
'#' not not break existing linkifiers like '#123', etc.
that generally follow the convention we show in linkifier
examples.
- [valid] : # Hello
- [valid] : # Hello
- [invalid]: #Hello
For the frontend, we have taken the code from v0.7.0 of
upstream marked and made minor changes to avoid having
to refactor a significant part of our marked code.
For the backend, we merely have to change the regex to
force require spaces after #, and add hashheader to our
list of blockparsers.
Fixes#11418.
This fixes an issue where we were accidentally double-escaping the
compose placeholder text if it contained HTML entities; once in
`i18n.t` and again when inserting it into the `placeholder` DOM via
`.attr`.
We can provide a function that returns an HTML string: `this.header()` to
display a header text above the typeahead. This can be used to provide
contextual information such as hinting about the silent mentions syntax
or the topic mentions syntax.
At the end of this commit, the HTML structure is:
$container <div>
$header <p>
info-icon
header-text
$menu <ul>
list-items
This change allows us to add custom changes to the HTML generated
by the typeahead without interfering with the core functions that
are provided by the library.
At the end of this commit, the HTML structure is:
$container <div>
$menu <ul>
list-items
This moves our main CSS for rendered Zulip message content into an
external file, which may be reusable but in any case should make it
easier to find this content.
This reverts commit 76e50af78e.
Empirically, this caused weird issues with the cursor jumping around,
so more investigation is required into the right way to fix it.
This commit adds click handler on date type custom profile
fields on field initialization itself.
This commit also fixes the bug in date type fields in user
profile in org settings.
This commit adds a click handler on datepicker custom profile
fields, which hides the `remove_date` button if the field value
is not set.
Fixes part of #11453
On change value click handlers on user profile fields in user settings
were also initialized on profile fields in org settings -> users
section. In org settings -> users, we do not need on change value
click handlers.
This commit fixes above issue by setting up handlers only on
user settings page.
Although SimpleBar automatically sets itself up on elements with a
`data-simplebar` attribute, sometimes we try to set event listeners
before that happens. Create the SimpleBar early in that case.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Apparently, the 30px width we allocated to the bullets was
insufficient with the larger font size there.
Edit by tabbott: better to just increase it to 32px everywhere.
`code-section` is a feature of the markdown system, therefore the
associated CSS should be in the `markdown.scss` file. I also refactored
to use SCSS nesting.
I added the `@media (max-width: 500px)` because the text from the inner
content was gong outside the white background on mobile because of the
height of the `.markdown` class for this viewport.
I moved this `.integration-instructions .help-content h3 { margin: 20px
0 ; }` from the `portico.scss` because it should be in `integrations
.scss`.
I removed the `#hubot-integrations` because I didn't find that id
anywhere.
I removed `.portico-landing.integrations ol ul` because `.markdown`
takes care of that left spacing.
When you press enter on a typeahead and start typing, your cursor is
placed at the end of the textbox, whereas we want it to be placed at
the end of the typeahead immediately. This causes some characters to
appear at the end of the message before you again get to typing from
where you left off.
To fix, we use the change event triggered on typeahead completion to
reposition the cursor instead of using a setTimeout().
Fixes#12621.
Steve Howell
Tim Abbott
Anders Kaseorg
Mohit Gupta
Mateusz Mandera
Rohitt Vashishtha
Nat1405
Gaurav Thapar
joaomcarvalho
Jack Tiggleman
Vinit Singh
Dinesh
Ivan Mitev
Jan Koscisz
Sophie
Matheus Melo
Thomas Ip
David Wood
Greg Price
Vishnu KS
chgl
Pragati Agrawal
Ryan Rehman
YashRE42
Dinesh
YashRE42
bartek
Hemanth V. Alluri
clarammdantas
Rishi Gupta
Alexandra Ciobica
Yashashvi Dave
Wyatt Hoodes
Kanishk Kakar
Vaibhav
Priyank Patel
Akash Nimare