Since production testing of `message_retention_days` is finished, we can
enable this feature in the organization settings page. We already had this
setting in frontend but it was bit rotten and not rendered in templates.
Here we replaced our past text-input based setting with a
dropdown-with-text-input setting approach which is more consistent with our
existing UI.
Along with frontend changes, we also incorporated a backend change to
handle making retention period forever. This change introduces a new
convertor `to_positive_or_allowed_int` which only allows positive integers
and an allowed value for settings like `message_retention_days` which can
be a positive integer or has the value `Realm.RETAIN_MESSAGE_FOREVER` when
we change the setting to retain message forever.
This change made `to_not_negative_int_or_none` redundant so removed it as
well.
Fixes: #14854
This adds a way to keep track of max_message_id of a
stream and fetch it using the method get_max_message_id().
This will be useful for sorting streams by most recent
activity which will be implemented in the upcoming commit.
Essentially rewritten by tabbott to have a coherent tracking system,
and provide documentation.
Part of #10794.
When switching from Private Messages narrow to
All messages narrow, stream list max-height was not
correctly updated. Stream list max-height was calculated
before new height were updated by browser for
All message narrow.
Inshort:
Stream list max-height was being updated before the browser could
render height for `#global_filters`. Calling resize after narrow
completes removes this issue.
This exists in all versions of the desktop app that we still support,
and will eventually let us delete a bit of annoying compatibility code
from the desktop app’s injected JavaScript.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Previously, the message and event APIs represented the user differently
for the same reaction data. To make this more consistent, I added a
user_id field to the reaction dict for both messages and events. I
updated the front end to use the user_id field rather than the user
dict. Lastly, I updated front end and back end tests that used user
info.
I primarily tested this by running my local Zulip build and
adding/removing reactions from messages.
Fixes#12049.
In passing, we also get coverage for
list_render.get.
This test also implicitly demonstrates that
you can call `widget.sort` directly from some
external event handler; in other words, you
are not locked into the widget's conventions
for setting up <th> tags.
The check here was too late, and it should
have given a blueslip error. We obviously
don't expect these errors at runtime; this
is a convenience for developers creating
new widgets.
This covers how we wire up the filter, and it
covers using `filterer` instead of `predicate`.
I also go away from the strange length-based
predicate that I had in the original test.
The intention behind the original test was
to show that filters could be more than simple
string-matching, but that was just a strange
way to demonstrate it.
This commit:
- Switches margin for padding on the search closed icon, to ensure we
cover the region to the right of icon as clickable area.
- Applies the click handler that initiates the search to the second
last element of the navbar:
- This will most commonly be the narrow_description element, but may
also be the entire navbar eg in the case of "ALL" or "starred".
Applying this change to user names in "group-pm-with: ..." based
narrows is a little questionable, but there are no other triggers
on these names so this change makes sense for now.
- The narrow_description may also contain links, which need to be
handled correctly so that the behave like links should. We work
around the onClick on the narrow_description, by applying a
handler to <a> tags and invoking stopPropagation.
- We also add CSS to change the cursor to a pointer to make the
search icon change color on hover over the clickable area to
indicate that the search box can be opened with a single click.
- However, since <a> tags are handled differently, we add a hover
listener which makes sure it behaves appropriately. We also increase
the vertical padding of the <a> tags so they cover the entire
vertical navbar region.
We shouldn't add redundant data to page_params. Since we already have
page_params.realm_notifications_stream_id, we can use that value instead
of creating page_params.notifications_stream.
We, however, still need the name of the notifications stream to render
it in templates. Thus we create stream_data.get_notifications_stream().
This commit removes most of the duplicate logic for the stream selection
dropdowns for the settings: `realm_signup_notifications_stream_id` and
`realm_notifications_stream_id`.
We also make minot changes to DropdownListWidget to accomodate the stream
rendering of the format: `#stream_name`.
We finally switch to using stream_ids instead of stream_name everywhere
which makes reading data from page_params simpler.
'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 makes it so that inline (recipient bar) topic edits follow
a different path from full message row edits in `message_edit.js`.
This commit:
- deletes `.save()` endpoint and replaces all calls to it with
`.save_message_row_edit()` and `.save_inline_topic_edit()`
- deletes `.end()` endpoint and replaces all calls to it with calls to
either ".end_message_row_edit()" and ".end_inline_topic_edit()".
Some extraneous zrequires were added in
3bc818b9f7
This is not a huge deal, but it makes it
appear as if data modules are dependent
on things that they don't really care
about. The tests should provide a bit
of signal on how "deep" an object's
dependencies go.
The set_up_muted_topics_ui and templates have been
refactored to use list_render.
This is done to support filtering and sorting of
the muted stream topics.
This also includes the addition of a new Date muted header.
The div containing options for filtering streams was placed in the
centre. Aligned it towards the right. Had to pass a special check
variable in subs.js:540 to add the specific class for this purpose.
This was a specific scenario where this sort of CSS was to be added,
hence had to make a specific case.
Also, fixed the bottom border color of the search streams bar for night
mode.
Previously, we would always pick up the stream and topic name from
compose_state. This would work for message edits as well when the
composebox was open.
Now, if we are in a message edit, we get the stream and topic of the
message being edited before falling back on trying to populate using
the composebox state.
Fixes#14545.
This commit changes the code to show user according to emails based
on email_address_visibilty_values and the type of user.
1. email_address_visibility = admins,members and guests
Typeaheads are shown according to original emails.
2. email_address_visibility = admins only
Typeaheads are shown according to original email to admins which
were previously shown according to system-generated email of
form "user10@zulipdev.com".
For non-admins, typeaheads are not shown according to emails as
they are not visible in the typeahead itself to non-admins.
3. email_address_visibility = nobody
Typeaheads are not shown according to emails for all type of users.
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.
We change the user facing interface to allow specifying expected
number of error messages (default=1). Now an average test can look
like:
```
// We expect 3 error messages;
blueslip.expect('error', 'an error message', 3);
throwError();
throwError();
throwError();
blueslip.reset();
```
This change adds a toggle widget to the "add streams" page that
lets the user change the sort order of the streams list. So far,
this supports sorting by stream name, by number of subscribers,
or by estimated weekly traffic.
Some uploads can be rejected in the frontend, like when the file
size is too big, without sending the file to the server. Remove the
'Uploading file...' message from the compose box in such cases.
Now, the system uses word='' and an editing=True for rendering an
form for addition of alert words. This is a very vulnerable
way to implement said feature and this commit fixes that.
The addition form has been moved to alert_word_settings.hbs
thereby rendering it only once but always. Now, we do not have
to manually add an empty word and editing for the form to be
rendered.
As part of refactoring, the editing parameter has also been
removed as there is no purpose left.
This updates the logged-in top navbar to display the stream/message
name, number of users, and description. It also replaces the search
bar with a search icon that expands into a full-width search bar.
Co-authored-by: Max Nussenbaum <max@maxnuss.com>
Fixes: #164.
Fixes: #5198.
In ee0d4541b4, we renamed the topic_date
-> stream_topic_history, and in the process renamed some local object
properties from .name => .topic_name, and accidentally change the
type for the data from the server as well.
The test fixtures were incorrectly migrated in the same way, so we fix
that as well.
`stream_topic_history` is a more appropriate name as this
module will contain information about last message of a
stream in upcoming commits. Function and variable names
are changed accordingly like:
* topic_history() -> per_stream_history()
* get_recent_names() -> get_recent_topic_names()
* name -> topic_name
If a file cannot be added for upload because of restrictions in frontend
we call cancelAll immediately in 'info-visible' callback. This would
prevent files that are already added to be cancelled but does not cancel
files that are yet to be added. So we use break to prevent any more files
from being added.
Calling uppy.cancelAll() when a batch of uploads is completed
result in the cancelation of any other batch of uploads that is
in progress. This case happens when a user uploads some files
and then tries to upload another bunch of files before the existing
upload is completed.
The form for entering alert words has been moved above the list
of words.
The list of words will be presented alphabetically rather than
time of addition.
We already know which list widget a `<th>`
tag is associated with when we set up the
event handler, so it's silly to read data
from the DOM to find that widget again
when the handler runs.
This commit eliminates a whole class of possible
errors and busy work.
For some widgets we now avoid duplicate redraw
events from this old pattern:
widget = list_render.create(..., {
}).init();
widget.sort(...);
The above code was wasteful and possibly
flicker-y due to the fact that `init` and
`sort` both render.
Now we do this:
widget = list_render.create(..., {
init_sort: [...],
});
For other widgets we just clean up the need
to call `init()` right after `create()`.
We also allow widgets to pass in `sort_fields`
during initialization (since you may want to
have `init_sort` use a custom sort before the
first render.)
Finally, we make the second and third calls
eliminate the prior updates from the previous
widget. This can prevent strange bugs with
double-reversing columns (although that's
been prevented in a better way with a recent
commit), as well as avoiding double work
with sorting.
This code has always been kind of convoluted
and buggy, starting with the first
sorting-related commit, which put filtering
before sorting for some reason:
3706e2c6ba
This should fix bugs like the fact that
changing filter text would not respect
reversed sorts.
Now the scheme is simple:
- external UI actions set `meta` values like
filter_value, reverse_mode, and
sorting_function, as needed, through
simple setters
- use `hard_redraw` to do a redraw and
trigger external actions
- all filtering/sorting/reverse logic on
the *data* happens in a single, simple
function called `filter_and_sort`
We put this in `scroll_util` to make it more likely
we will eventually unify this with other scrolling
logic. (A big piece to move is ui.get_scroll_element,
but that's for another PR.)
And then the other tactical advantage is that we get
100% line coverage on it.
I changed the warning to an error, since I don't
think we ever expect scrolling at the `body` level,
and I don't bother with the preview node.
I pushed this risk commit to the end of
a PR that had a bunch of harmless prep
commits at the front, and I didn't make
it clear enough that the last commit (this
one) hadn't been tested thoroughly.
For the list_render widget, we can simplify
the intialization pretty easily (avoid
extra sorts, for example), but the cache aspects
are still tricky on subsequent calls.
For some widgets we now avoid duplicate redraw
events from this old pattern:
widget = list_render.create(..., {
}).init();
widget.sort(...);
The above code was wasteful and possibly
flicker-y due to the fact that `init` and
`sort` both render.
Now we do this:
widget = list_render.create(..., {
init_sort: [...],
});
For other widgets we just clean up the need
to call `init()` right after `create()`.
We also allow widgets to pass in `sort_fields`
during initialization (since you may want to
have `init_sort` use a custom sort before the
first render.)
We had a bug where if your peer mentioned you in
message, but then edited the message not to mention
you, the latter wouldn't reset your unread counts
for "Mentions". And the same problem would happen
vice versa.
The fix basically extracts `update_message_for_mention`
and makes sure it handles all combinations of
unread/mentioned flags, instead of assuming
any invariants about which directions of change
are possible.
And then we call that new function from
`message_events.js` whenever we get message
edit events.
Fixes#14544
We use a somewhat more realistic message, mostly
to prep for testing some mention/unread stuff in
a subsequent commit.
We also set message booleans.
Unfortunately, `recent_senders` is kind of awkward
for checking a single message, since its only
public API is for sorting. I don't bother with it.
But I do check the `topic_data` interaction.
The function message_send_error was messing up
on calls to message.get when we were passing in
string versions of `local_id`. Now we pass in
float ids.
This fixes a traceback where we tried to set
`.failed_request` on to an `undefined` value
that we had instead expected to be a locally
echoed message from our message store.
We stop using `local_id_counter`, which was just noise,
and instead we just make the test more realistic:
- Use 123.04 for our local id on the message that
we're simulating sending.
- Use 127 as the message id that the server gives
us back in the success payload.
We still stub echo functions, but for
one of our stubs (`try_deliver_locally`)
we now exercise one its actual callees
in the stub (`echo.insert_local_message`).
And we're still stubbing some callees
of `echo.insert_local_message`, since
that has all kinds of unwanted side
effects, too.
The main piece we want from
`insert_local_message`, for now,
is somewhat realistic handling of
our local message ids.
We also add a little sanity check
that our timestamp does get plumbed
through to `local_message.insert_message`.
Option is added to video_chat_provider settings for disabling
video calls.
Video call icon is hidden in two cases-
1. video_chat_provider is set to disabled.
2. video_chat_provider is set to Jitsi and settings.JITSI_SERVER_URL
is none.
Relevant tests are added and modified.
Fixes#14483
This adds a new realm setting: default_code_block_language.
This PR also adds a new widget to specify a language, which
behaves somewhat differently from other widgets of the same
kind; instead of exposing methods to the whole module, we
just create a single IIFE that handles all the interactions
with the DOM for the widget.
We also move the code for remapping languages to format_code
function since we want to preserve the original language to
decide if we override it using default_code_clock_language.
Fixes#14404.
This is a prep commit for changes to the top navbar, it adds helpers
to filter.js which will help control the behavior of some aspects of
the redesigned navbar.
Modified by tabbott to add comments, internationalization tags on the
strings, support streams:public, and change various title strings.
We fix this by adding a more expressive data function, with tests, for
whether a filter is on UserMessage data, which would mean that
streams:public could never add additional matches.
We now use `assert.throws()` to test that we're
properly calling `blueslip.fatal`.
In order to not break line coverage here, we have
to remove an unreachable `return` in `stream_data.js`.
Usually we test `fatal` for line coverage reasons.
Most places where we use `blueslip.fatal` fall in
these categories:
* the code is theoretically unreachable, but
we have `blueslip.fatal` for defensive reasons
* we have some upstream bug that we should just
fix
* the code should recover gracefully and just
use blueslip.errors()
It's possible that we should eliminate `blueslip.fatal`
from our API and just throw errors when really important
invariants get broken. This will make it more obvious
to somebody reading the code that we're not going to
continue after the call, and `blueslip` already knows
how to catch exceptions and report them.
When we redraw the left sidebar, we need to tell the
topic list to clear its data structures (and do other
stuff like hiding its popover), since we are clearing
its parent container.
The commit f0e18b3b3e
introduced this regression in late January 2020.
That commit made topic_list use a vdom to avoid
unnecessary updates. Before that, topic_list did
a lot of brute-force redraws, which covered up the
fact that we weren't having stream_list telling it
when the rug was being pulled out from under it.
The boundary between stream_list and topic_list
has always been kind of complicated code, since
topic lists get embedded into the stream list.
The main interactions, though, are basically:
* topic_zoom.clear_topics() - you're leaving
a narrow that may or may not be zoomed
* topic_list.clear() - you're about to redraw
stream items in the unzoomed stream list
* topic_list.rebuild(stream_li, stream_id) -
you're building or updating a topic list
for the newly active stream
Fixes#14465
In case of video embeds, the previous logic used
`data-src-fullsize` or `src` as a key to look
for the metadata of video in `lightbox.open()`,
but while parsing, the key used while storing
the metadata was the video ID.
This doesn't make any sense because video's data
could never be accessed from `asset_map` and we
always needed to lookup the DOM for this.
This commit fixes this by using $img.attr('src')
as a key for `asset_map` for both, images and
videos. Since `src` is the link of preview image
in case of video embeds, it will always uniquely
determine the video ID and we won't loose
anything with the change in how videos handle
things.
Part of #14152.
This changes the payload that is used
to populate `page_params` for the webapp,
as well as responses to the once-every-50-seconds
presence pings.
Now our dictionary of users only has these
two fields in the value:
- activity_timestamp
- idle_timestamp
Example data:
{
6: Object { idle_timestamp: 1585746028 },
7: Object { active_timestamp: 1585745774 },
8: Object { active_timestamp: 1585745578,
idle_timestamp: 1585745400}
}
We only send the slimmer type of payload
to clients that have set `slim_presence`
to True.
Note that this commit does not change the format
of the event data, which still looks like this:
{
website: {
client: 'website',
pushable: false,
status: 'active',
timestamp: 1585745225
}
}
This adds a new test_realm_integer, replacing test_realm_boolean for
testing integer fields like realm_create_stream_policy,
realm_invite_to_stream_policy, and realm_invite_required in dispatch.js.
Fixes#12284
Fields like realm_email_address_visibility and realm_bot_creation_policy
were strings instead of integers in page_params obeject in
settings_org.js tests.
Also use values struct defined in settings_config.js and setting_bots.js
instead of direct values for improving readability.
If folks use an overly broad selector for message rows,
they will accidentally include drafts from the drafts
dialog, which won't have zids. More specific selectors
will be more efficient and possibly prevent strange
behaviors.
For testing convenience, we extract the message.
The UI in the `#settings/notifications` page is updated similarly
to what is done in the `update_global_notifications` path present
in the `server_events_dispatch` file.
This setting is being overridden by the frontend since the last
commit, and the security model is clearer and more robust if we don't
make it appear as though the markdown processor is handling this
issue.
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
While we could fix this issue by changing the markdown processor,
doing so is not a robust solution, because even a momentary bug in the
markdown processor could allow cached messages that do not follow our
security policy.
This change ensures that even if our markdown processor has bugs that
result in rendered content that does not properly follow our policy of
using rel="noopener noreferrer" on links, we'll still do something
reasonable.
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This function returns a list of objects to create a
list_render object, and each item contains the streams
whose atleast one notification setting differs from the
default set by the user.
This is done by comparing the global settings in the
`#settings/notifications` page with those settings
present in the subscribed streams.
Work towards #9228.
This flag was used to delay unread count updates while the bankruptcy
modal was visible. Now that bankrupcty is no longer a modal, we don't
need this flag at all.
Starred messages from muted topics were not shown in the starred
messages view. Condition for muting_enabled is modified accordingly
such that the starred messages from muted topics is shown in the
starred messages narrowed view.
Node tests are updated accordingly.
Fixes#13548
We've noticed that many production organizations don't set either an
organization description or profile picture, even large open source
organizations that could definitely take advantage of this feature.
This adds a top-of-page banner that bugs organization administrators
to add an organization description and profile picture, generally
starting on the second login (as we only do it on page load after
notifications are configured).
Significantly tweaked by tabbott to get the right user experience.
Fixes#14019.
I think we could write this test better, but it's not a big deal for
this to break in the rare even that we change/remove one of the 2
strings it interacts with.
We now only use `page_params.realm_default_streams` during
initialization, and then after that we use `stream_data`
APIs to get default stream ids and related info. (And
for the event that replace the data, we just update our
internal data structures as well.)
Long term we should have the server just send us ids here,
since we are now hydrating info from stream data in all places.
We only used get_default_stream_names() in a
test, so now it's being replaced with a function
that just gets ids.
We'll have use for get_default_streams_ids()
in an upcoming commit.
Now if a default stream gets deleted, we just
redraw the table. We always have a small number
of default streams, and the way that we were removing
rows without the actual consent of `list_render` was
really janky (and just a vestige of pre-list-render
code that never got fully ported).
This also makes us consistent with how we handle
added streams (i.e. just call
`update_default_streams_table`).
ASIDE:
Ideally we will update `list_render` at some point to
have an API for adding and removing elements. It does
allow you now to call `data()` to reset its data, but
for now we just build a new `list_render` object every
time.
We stopped needing this with
0329b67048
(Dec 2016).
The function sets `bot.can_admin`,
which was only used in `bot_data.get_editable`.
We removed two tests (and then put back
some test setup that needed to leak down
to the last test).
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.
Instead of having logical expressions in templates, it's always preferred
to calculating them in javascript and pass the results as a context. It
also enhances the readability of templates and testing of such logic is
easier in js over templates.
The use case for this are small or fixed tables, which do not need
filtering support. Thus we are able to not include the unnecessary
search input inside the html parent container.
It is not used at present, but will be required when we refactor
the settings pages.
We also split out exports.validate_filter function for
unit testing the above condition.
Before this commit, the reactions code would
take the `message.reactions` structure from
the server and try to "collapse" all the reactions
for the same users into the same reactions,
but with each reaction having a list of user_ids.
It was a strangely denormalized structure that
was awkward to work with, and it made it really
hard to reason about whether the data was in
the original structure that the server sent or
the modified structure.
Now we use a cleaner, normalized Map to keep
each reaction (i.e. one per emoji), and we
write that to `message.clean_reactions`.
The `clean_reactions` structure is now the
authoritatize source for all reaction-related
operations. As soon as you try to do anything
with reactions, we build the `clean_reactions`
data on the fly from the server data.
In particular, when we process events, we just
directly manipulate the `clean_reactions` data,
which is much easier to work with, since it's
a Map and doesn't duplicate any data.
This rewrite should avoid some obscure bugs.
I use `r` as shorthand for the clean reaction
structures, so as not to confuse it with
data from the server's message.reactions.
It also avoids some confusion where we use
`reaction` as a var name for the reaction
elements.
Fixes#14254
You can test this on dev:
* do "-stream:Verona" in the search bar (the minus
sign negates the search here)
* reload the browser
You should see the same search (all streams besides Verona).
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
Given that can_mark_messages_read is called whenever the blue box
cursor stops on a message and that it is calculated purely on the
basis of sorted_term_types, it makes sense to cache the result.
If you were in the "Starred messages" narrow and
your pointer was on a message with the stream/topic
of "social/lunch", we wouldn't move you to the unread
messages for that topic.
I fixed this by removing the code that looked at
the current message's topic. Instead, we only look
at the active narrow to figure out the "next" topic
to go to.
Fixes#14120.
Original email address is shown to admin users in subscriber list when
email_address_visibilty is set to "Admins only" by passing delivery_email
at required places. Email address are not shown to non-admin users when
visibility is set to "Admins only".
Tweaked by tabbott to fix a few bugs and dead code.
Fixes a part of #13541.
This intent is that we'll be able to reuse this when editing streams
as well.
* Rename method: filter_with_new_topic to filter_with_new_param.
* Fix tests and method calls.
This extends our email address visibility settings to deny access to
user email addresses even to organization administrators.
At the moment, they can of course change the setting (which leaves an
audit trail), but in the future only organization owners will be able
to change that setting.
While we're at this, we rewrite the settings_data.js test to cover all
the cases in a more consistent way.
Fixes#14111.
The file populates `windows.i18n`, so now
the file name matches our convention.
Note that the module really just initializes
`i18next` and then does this:
window.i18n = i18next;
It doesn't really add any functionality to
third party library.
Before this test, we were validating the behavior
of `i18next`, but we weren't validating our light
layer that sits on top of `i18next`, which currently
resides in the slightly misnamed `translations.js`
file.
The translations module is now so small that I'll
just quote it verbatim here:
import i18next from 'i18next';
i18next.init({
lng: 'lang',
resources: {
lang: {
translation: page_params.translation_data,
},
},
nsSeparator: false,
keySeparator: false,
interpolation: {
prefix: "__",
suffix: "__",
},
returnEmptyString: false, // Empty string is not a valid translation.
});
window.i18n = i18next;
We now just do `zrequire('translations')` to initialize
the `i18next` library, which allows us to have simpler
test setup and to actually exercise the above call to
`i18next.init`.
This change now gives us 100% line coverage of `translations.js`,
which of course isn't that hard to acheive (see above).
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 follows the convention of other code calling into
add_sub_to_table of checking whether the stream settings overlay is
open (and thus in the DOM) before trying to rerender it.
We add these two functions to the API,
so that we no longer have `alert_words_ui`
using private data from `alert_word`:
alert_words.has_alert_word()
alert_words.get_word_list()
And to initialize the data, we have a proper
`initialize` method that is passed in only
the parameters that it needs from `ui_init`.
(We also move the step of deleting `alert_words`
from `page_params` to the `ui_init` module.)
Because it's a bit less cumbersome to initialize
`alert_words`, we now just it directly in the
node tests for `alert_words_ui`.
This is follow up to da79fd206a
I accidentally skipped over pm_conversations. Same
ideas as the bigger previous commit--we pass in params
to the initialize function and do the delete cleanup
within ui_init.
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 gives them cache-compatible URLs, and also avoids some extra
copies of the sprite sheet images.
Comments on the Octopus emoji added by tabbott.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: _.defaults
mutates its first argument. However, the code does not always appear
to have been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: _.extend mutates
its first argument. However, the code does not always appear to have
been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This moves some code from settings_display.js
into the new module settings_config.js.
Extracting this module breaks some dependencies
on settings_display.js (which has some annoying
transitive dependencies, including jQuery).
In particular this isolates stream_data from
from settings_display.js.
Two of the three structures that we moved here
weren't even directly used by settings_display.js,
since we do a lot of rendering in the modules
admin.js and setting.js.
We make get_all_display_settings() a function
to avoid a require-time dependency on page_params.
Breaking the dependencies simplifies a few
node tests.
Most of the node test complexity came from the
following commit in March 2019:
5a130097bf
The commit itself seems harmless enough, but
dependencies can have a somewhat "viral" nature,
where making stream_data depend on settings_display
caused us to modify four different node tests.
This refactoring is the first step toward sharing
our markdown code with mobile. This focuses on
the Zulip layer, not the underlying third party `marked`
library.
In this commit we do a one-time initialization to
wire up the markdown functions, but after further
discussions with Greg, it might make more sense
to just pass in helpers on every use of markdown
(which is generally only once per sent message).
I'll address that in follow-up commits.
Even though it looks like a pretty invasive change,
you will note that we barely needed to modify the
node tests to make this pass. And we have pretty
decent test coverage here.
All of the places where we used to depend on
other Zulip modules now use helper functions that
any client (e.g. mobile) can configure themselves.
Or course, in the webapp, we configure these from
modules like people/stream_data/hash_util/etc.
Even in places where markdown used to deal directly with
data structures from other modules, we now use functions.
We may revisit this in a future commit, and we might
just pass data directly for certain things.
I decided to keep the helpers data structure completely flat,
so we don't have ugly nested names like
`helpers.emoji.get_emoji_codepoint`. Because of this,
some of the names aren't 1:1, which I think is fine.
For example, we map `user_groups.is_member_of` to
`is_member_of_user_group`.
It's likely that mobile already has different names
for their versions of these functions, so trying for
fake consistency would only help the webapp. In some
cases, I think the webapp functions have names that
could be improved, but we can clean that up in future
commits, and since the names aren't coupled to markdown
itself (i.e. only the config), we will be less
constrained.
It's worth noting that `marked` has an `options`
data structure that it uses for configuration, but
I didn't piggyback onto it, since the `marked`
options are more at the lexing/parsing layer vs.
the app-data layer stuff that our helpers mostly
help with.
Hopefully it's obvious why I just put helpers in
the top-level namespace for the module rather than
passing it around through multiple layers of the
parser.
There were a couple places in markdown where we
were doing awkward `hasOwnProperty` checks for
emoji-related stuff. Now we use the Python
principle of ask-forgiveness-not-permission and
just handle the getters returning falsy data. (It
should be `undefined`, but any falsy value is
unworkable in the places I changed, so I use
the simpler, less brittle form.)
We also break our direct dependency on
`emoji_codes.json` (with some help from the
prior commit).
In one place I rename streamName to stream_name,
fixing up an ancient naming violation that goes
way back to before this code was even extracted
away from echo.js. I didn't bother to split this
out into a separate commit, since 2 of the 4
lines would be immediately re-modified in the
subsequent commit.
Note that we still depend on `fenced_code`
via the global namespace, instead of simply
requiring it directly or injecting it. The
reason I'm postponing any action there is that
we'll have to change things once we move
markdown into a shared library. (The most
likely outcome is that we'll rename/move both files
at the same time and fix the namespace/require
details as part of that commit.)
Also the markdown code still relies on `_` being
available in the global namespace. We aren't
quite ready to share code with mobile yet, but the
underscore dependency should not be problematic,
since mobile already uses underscore to use the
webapp's shared typing_status module.
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 add this API to emoji.js, so that markdown
doesn't need to look at internal data structures
(or even need to understand any kind of record
format for results).
Here are the functions:
get_realm_emoji_url()
get_emoji_name()
get_emoji_codepoint()
We use the API now in markdown, which eliminates
the need for the markdown parser to require
the emoji JSON file.
Each function has a simple docstring:
get_emoji_name('1f384') === 'holiday_tree'
get_emoji_codepoint('avocado') === '1f951'
get_realm_emoji_url('shrug') === '/user_avatars/2/emoji/images/31.png'
Also we have simple test coverage for the API
(including tests that verify the docstrings).
This name was misleading, because we weren't
actually setting realm_filters (that's what
`page_params.realm_filters = realm_filters`
is for); we were instead updating our
realm filter rules.
These functions were just shims that were
used in the somewhat painful migration from
subject_* to topic_*.
The commit 4572be8c27
fixed it so that the client never needs to
deal with "subject_links".
So now we just go back to simpler code:
message.topic_links = links
links = message.topic_links
I am not quite ready to declare victory on
the subject/topic migration, but we are super
close. In this commit I bump a blueslip
warning to a blueslip error, so that we'll
be notified of any codepath that is still
using the janky fall-back-to-subject defensive
code here.
If we go a couple days without any errors, then
we can remove the blueslip warning and the
defensive code immediately and then inline
the callers at our leisure. I wouldn't be
wildly against keeping these wrappers in some
parts of the code, but that debate is out of
the scope of this immediate fix, and I haven't
thought hard about it yet.
We can basically sweep set_message_topic() now,
if we wanted to, since it's truly just a one-liner.
(At one point it was encapsulating something
like `message.subject = foo`).
This required a tiny change to compose_fade
test setup.
Most of this logic is specific to markdown
message processing, so we move the code to
markdown.js.
The only responsibility that we leave with
`emoji.js` is to provide us with a list
of translations (regex and replacement text).
But now `markdown.js` actually (directly) executes
those translations against Zulip messages
as part of its preprocessing.
This should simplify the upcoming mobile conversion.
Instead of mobile needing to duplicate this fairly
complex function, they will just need to pass
us in a list similar to `emoji_translations` inside
of `emoji.js`. That code has a comment that shows
what the data structure looks like.
I am 99% sure we can rely on trimRight() and
trim() being available in all browsers that
we support. I verified in FF.
This removes the util dependency from both
modules touched here.
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 used to have a block of code doing this just in the presence
endpoint because that's where we'd had error-handling problems with it
not being present, but it seems more correct for it to run
unconditionally on all HTTP requests.
This requires adding a dependency of channel on reload_state, which we
record in the webpack configuration for now.
webpack optimizes JSON modules using JSON.parse("{…}"), which is
faster than the normal JavaScript parser.
Update the backend to use emoji_codes.json too instead of the three
separate JSON files.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
I believe we can remove these and rely on
other parts of our testing/code-review
to ensure template quality.
These tests never really exercised our
app code, as evidenced by us not regressing
any of the 100%-line-coverage files.
We have a couple other ways that we verify
the correct format of the templates:
- webpack (can they compile?)
- check-templates (are they nicely indented?)
For deep testing, we have Casper, which
exercises most of our most important templates
in some meaningful way.
I think it's pretty rare that we get bugs
now that are directly caused by bad templates,
and an even smaller subset of them would
have been caught by the node tests.
If that trend changes in the future, I would prefer to
just do something "greenfield" to address
any common problems rather than resurrect
this code, but we could always resurrect it
from git.
The template node tests did check a little bit of
detail about which fields are there, but not
in an integrated way, so that aspect of the tests
wasn't very useful either.
This effectively reverts the following
commit from May 2019:
be527905ca
The implementation of closest() was a bit
buggy and complex. It's easy enough
to just stub the method yourself. We may
want to eventually re-implement it, but we
should follow the template of parent/set_parent.
If you fail to stub `closest` zjquery gives
a fairly helpful error message:
Error: You must create a stub for $("link-stub").closest
We stub out jquery elements rather than giving
the illusion of having real DOM.
Also, we make it so that the message_store
interaction has an assertion attached to it.
In the next commit we're going to change what the
server sends for the following:
- page_params
- server responses to /json/users/me/presence
We will **not** yet be changing the format of the data
that we get in events when users update their presence.
It's also just a bit in flux what our final formats
will be for various presence payloads, and different
optimizations may lead us to use different data
structures in different payloads.
So for now we decouple these two things:
raw_info: this is intended to represent a
snapshot of the latest data from the
server, including some data like
timestamps that are only used
in downstream calculations and not
user-facing
exports.presence_info: this is calculated
info for modules like buddy_data that
just need to know active vs. idle and
last_active_date
Another change that happens here is we rename
set_info_for_user to update_info_for_event,
which just makes it clear that the function
expects data in the "event" format (as opposed
to the format for page_params or server
responses).
As of now keeping the intermediate raw_info data
around feels slightly awkward, because we just
immediately calculate presence_info for any kind
of update. This may be sorta surprising if you
just skim the code and see the various timeout
constants. You would think we might be automatically
expiring "active" statuses in the client due to
the simple passage of time, but in fact the precise
places we do this are all triggered by new data
from the server and we re-calculate statuses
immediately.
(There are indirect ways that clients
have timing logic, since they ask the
server for new data at various intervals, but a
smarter client could simply expire users on its
own, or at least with a more efficient transfer
of info between it and the server. One of
the thing that complicates client-side logic
is that server and client clocks may be out
of sync. Also, it's not inherently super expensive
to get updates from the server.)
The important details for the test setup here
are just the number of users who are active.
We don't need to simulate the currently awkward
way of populating this data.
The _.each calls with an inline function expression have already been
converted to for…of loops. We could do that here, but using .forEach
when we’re just reusing an existing function seems like a good
guideline.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
predicate is expected to return a function, not a boolean. The
boolean true was causing _.filter to match items with a property named
"true", which is definitely not what was intended. Matching no items
is probably also not intended, but matching every item causes the test
to fail.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We just get the stream_name from the sub struct now.
This mostly affects node tests.
The only place in real code where we called add_sub()
was when we initialized data from the server.
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 saves a tiny bit of bandwidth, but more
importantly, it protects us against races for
stream name changes. There's some argument that
if the user is thinking they're sending to
old_stream_name, and unbeknownst to them, the
stream has changed to new_stream_name, then we
should fail. But I think 99% of the time the
user just wants the message to go that stream
despite any renames.
In order to verify the blueslip error, we
had to turn on error checking, which required
a tiny fix to a place where we left out
a stream_id for add_sub.
We avoid complicated code to update unread counts
by just using vdom.js.
One small change here is that if click on "more
topics", we replace it with the spinner instead
of putting the spinner after it. This saves us
a redraw under the new scheme.
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>
We had a plan at some point to use this to display a phone icon or
something for users who would receive push notifications if you
messaged them. IT's not clear that feature was a good idea in any
case, but it certainly shouldn't be synced as presence data; it would
change >100x less often than the rest of presence and so should likely
be synced differently, maybe as a property on user. So it's best to
delete this prototype.
The “Smileys & People” category has been split into “Smilys & Emotion”
and “People & Body”.
Also, fix generate_sha1sum_emoji to read the emoji-datasource-google
version from yarn.lock, since package.json only gives a version range.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
When quoting a message with fenced code blocks without a language,
we used to have ambiguity in which '```' fence terminates the quote.
This commit adds explicitly non-interfering fences, which fixes the
above issue as well as makes the raw message easier to quickly read.
Fixes#12446.
This commit includes a new `stream_post_policy` setting,
by replacing the `is_announcement_only` field from the Stream model,
which is done by mirroring the structure of the existing
`create_stream_policy`.
It includes the necessary schema and database migrations to migrate
the is_announcement_only boolean field to stream_post_policy,
a smallPositiveInteger field similar to many other settings.
This change is done to allow organization administrators to restrict
new members from creating and posting to a stream. However, this does
not affect admins who are new members.
With many tweaks by tabbott to documentation under /help, etc.
Fixes#13616.
This flag affects page_params and the
payload you get back from POSTs to this
url:
users/me/presence
The flag does not yet affect the
presence events that get sent to a
client.
This is defensive code for the scenario that we
have a user_id in presence but not people. This is
unlikely to occur by the time that we actually render
the buddy list, which is the context for this code.
We have previously been reporting an error here via
the people code, but we add an additional warning.
Also, we filter the user_id from the result.
This reverts commit d84646f091 (which
incorrectly assumed in unread_topic_counter that the messages were
present in the message store), while fixing the type confusion problem
by using IntDict for stream_id keys.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
stream_count and topic_count in the actual code have been IntDict
since commit 9ba1829243 (#13569).
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Fixes type confusion in unread_topic_counter, which uses stream IDs as
keys.
Since unread_topic_counter calls message_store.get now, update the
mocks so that message_store.get knows about our mocked messages.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Previously the sender was not included in display_recipient when
a private message was locally echoed. This broke the copy conversation
link functionality, if the user try to copy the link immedeatly after
sending the message. This issue is present only during local echo.
This was fixed by including the recipient of the user during
local echo.
Fixes#13547.
Edited the warning to clearly state that most members/most stream members
will be notified on using wildcard mentions, along with the specific
mention (e.g. @ALL, @everyone and @stream).
Did a separate check for all wildcard mentions in util.js and stored the
corresponding mention in wildcard_mention inside compose.js.
Fixes: #13636
For few settings like `waiting_period_threshold` it makes sense to have the
"value" attribute of option to have a value other than the actual setting
value because multiple settings are depending upon this dropdown, so
handling them in JS code makes more sense. But for many settings (which has
integer values), we have followed a wrong trend over the time of
representing every new dropdown with human-readable values and manually
handling them in JS Code, where it makes more sense to use actual setting
value. The result of which is code has become less concise, sensible and
less likely to be mistaken.
We now use vdom-ish techniques to track the
list items for the pm list. When we go to update
the list, we only re-render nodes whose data
has changed, with two exceptions:
- Obviously, the first time we do a full render.
- If the keys for the items have changed (i.e.
a new node has come in or the order has changed),
we just re-render the whole list.
If the keys are the same since the last re-render, we
only re-render individual items if their data has
changed.
Most of the new code is in these two modules:
- pm_list_dom.js
- vdom.js
We remove all of the code in pm_list.js that is
related to updating DOM with unread counts.
For presence updates, we are now *never*
re-rendering the whole list, since presence
updates only change individual line items and
don't affect the keys. Instead, we just update
any changed elements in place.
The main thing that makes this all work is the
`update` method in `vdom`, which is totally generic
and essentially does a few simple jobs:
- detect if keys are different
- just render the whole ul as needed
- for items that change, do the appropriate
jQuery to update the item in place
Note that this code seems to play nice with simplebar.
Also, this code continues to use templates to render
the individual list items.
FWIW this code isn't radically different than list_render,
but it's got some key differences:
- There are fewer bells and whistles in this code.
Some of the stuff that list_render does is overkill
for the PM list.
- This code detects data changes.
Note that the vdom scheme is agnostic about templates;
it simply requires the child nodes to provide a render
method. (This is similar to list_render, which is also
technically agnostic about rendering, but which also
does use templates in most cases.)
These fixes are somewhat related to #13605, but we
haven't gotten a solid repro on that issue, and
the scrolling issues there may be orthogonal to the
redraws. But having fewer moving parts here should
help, and we won't get the rug pulled out from under
us on every presence update.
There are two possible extensions to this that are
somewhat overlapping in nature, but can be done
one a time.
* We can do a deeper vdom approach here that
gets us away from templates, and just have
nodes write to an AST. I have this on another
branch, but it might be overkill.
* We can avoid some redraws by detecting where
keys are moving up and down. I'm not completely
sure we need it for the PM list.
If this gets merged, we may want to try similar
things for the stream list, which also does a fairly
complicated mixture of big-hammer re-renders and
surgical updates-in-place (with custom code).
BTW we have 100% line coverage for vdom.js.
Now that we have the type situation of having anchor support passing a
string, this is a much more natural way to implement
use_first_unread_anchor.
We still support the old interface to avoid breaking compatibility
with legacy versions of the mobile apps.
This makes the code more readable, by just passing the anchor through
without changing its field name back and forth.
There's no reason for this parameter to involve parsing and integer --
it should be a number in all incoming code paths.
The feature is used for editing stream descriptions as well, and in
any case, what's important is that it's a content-editable widget (aka
a form of input box).
We now give "slight smile" precedence over
"small airplane" if you type "sm".
More generally, we favor popularity over prefix
matches for emoji matches, as long as the popular
emoji matches on any of its pieces.
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 now only go the server if both of these
conditions are true:
- our message data seems incomplete for
the stream
- we haven't already fetched history
This function will make more sense when we start
tracking api calls that retrieve topic history.
The unit tests here are kinda duplicating what we
have in the stream_data tests. If we move the
function out of stream_data, we can kill off the
tests there, but for now I think a bit of duplicate
testing is fine here.
This is mostly for tactical reasons. It's hard to
get 100% test coverage on topic_list.js, but it
should be easy to get 100% test coverage on this
very important function.
I considered just moving this code into topic_data.js,
but it just didn't feel quite right. I feel like
this is a pretty core piece of code that's nice
to be by itself and not be near other complicated
code that does stuff like build widgets or talk
to servers. (And, again, it's not just the actual
code here, which is pretty small, it's the unit
tests, which are inherently verbose to exercise
all the edge cases.)
This test mostly tests how we glue everything
together, but I want to change that in an upcoming
commit.
Also, the data stuff that it tests is now better
covered by the test recent tests I added.
The only place we ever set active-sub-filter is
right after we build the template, so there is
no reason to have it be a separate step.
(I made a similar fix to pm_list recently, and
this helps set the stage for doing vdom-like
stuff.)
We may revisit this in the future, but similar to is:private, the
current Zulip user experience makes users expect that in the
is:mentioned view, they should really be able to mark messages as
read.
Further, the practice use case for not marking them as read is very
low, since it's rare for someone to have so many mentions that
revisiting the mentions view isn't sufficient to see everything that
needs their attention.
Previously, is_exactly() had already been repalced with can_bucket_by().
This commit removes is_exactly() and replaces its usage in our tests
with can_bucket_by().
We now incorporate people.get_message_people() in our
logic for compose/PM typeaheads. This not only gives
users better results in some cases, but it will also
improve performance for large realms in some cases.
A recent commit removed test coverage for the
actual filtering/sorting of mention typeaheads
when you did a non-silent method. This commit
now tests that important step again.
Note that we also had (and still have) tests
that make sure the is_silent flag is set
correctly by get_candidates.
We don't have a true full-stack test, but those
can be quite tricky to set up and maintain.
This is relatively unobtrusive, and we don't send
anything to the server.
But any user can now enter blueslip.timings in the
console to see a map of how long things take in
milliseconds. We only record one timing per
event label (i.e. the most recent).
It's pretty easy to test this by just clicking
around. For 300 users/streams most things are
fast except for:
- initialize_everything
- manage streams (render_subscriptions)
Both do lots of nontrivial work, although
"manage streams" is a bit surprising, since
we're only measuring how long to build the
HTML from the templates (whereas the real
time is probably browser rendering costs).
This change sets us up to optimize how we
filter users in the admin user settings.
See #13554 for more context on the user
facing issues.
This fix is basically three related things:
- Add filterer options to list_render.
- Add helper method to people.js.
- Use filterer in settings_users.js.
The filter "callback" was only a "callback" in the
most general sense of the word.
It's just a filter predicate that returns a bool.
This is to prepare for another filtering option,
where the caller can filter the whole list
themselves. I haven't figured out what I will name
the new option yet, but I know I want to make the
two options have specific names.
We are already providing callbacks everywhere, so
it would be nice to eliminate some dead code.
This also speeds things up ever so slightly (no
longer type-checking the option every time through
the loop).
We also split out exports.filter to make unit testing
easier. The function seems kinda silly now, being so
small, but I hope to add another filtering option soon.
Zulip has had a small use of WebSockets (specifically, for the code
path of sending messages, via the webapp only) since ~2013. We
originally added this use of WebSockets in the hope that the latency
benefits of doing so would allow us to avoid implementing a markdown
local echo; they were not. Further, HTTP/2 may have eliminated the
latency difference we hoped to exploit by using WebSockets in any
case.
While we’d originally imagined using WebSockets for other endpoints,
there was never a good justification for moving more components to the
WebSockets system.
This WebSockets code path had a lot of downsides/complexity,
including:
* The messy hack involving constructing an emulated request object to
hook into doing Django requests.
* The `message_senders` queue processor system, which increases RAM
needs and must be provisioned independently from the rest of the
server).
* A duplicate check_send_receive_time Nagios test specific to
WebSockets.
* The requirement for users to have their firewalls/NATs allow
WebSocket connections, and a setting to disable them for networks
where WebSockets don’t work.
* Dependencies on the SockJS family of libraries, which has at times
been poorly maintained, and periodically throws random JavaScript
exceptions in our production environments without a deep enough
traceback to effectively investigate.
* A total of about 1600 lines of our code related to the feature.
* Increased load on the Tornado system, especially around a Zulip
server restart, and especially for large installations like
zulipchat.com, resulting in extra delay before messages can be sent
again.
As detailed in
https://github.com/zulip/zulip/pull/12862#issuecomment-536152397, it
appears that removing WebSockets moderately increases the time it
takes for the `send_message` API query to return from the server, but
does not significantly change the time between when a message is sent
and when it is received by clients. We don’t understand the reason
for that change (suggesting the possibility of a measurement error),
and even if it is a real change, we consider that potential small
latency regression to be acceptable.
If we later want WebSockets, we’ll likely want to just use Django
Channels.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
As the part of making notification stream settings to change using
"save/discard" widget instead of immediate saving, we need to access the
stream id which is being selected at the moment.
For "New stream notifications" and "New user notifications" it is more
intuitive to just use the new system for showing success/saving status
feedback.
Extracting the function makes it a bit easier to
test and use in a generic way.
Also, I wanted this to live in stream_data, so that
it's easier to find if we change how we model
subscriber data.
Finally, I use _.every to do the subset check
instead of `_.difference`, since _.difference
is actually N-squared:
_.difference = restArguments(function(array, rest) {
rest = flatten(rest, true, true);
return _.filter(array, function(value){
return !_.contains(rest, value);
});
});
And we don't actually want to build a list only
to check that it's zero vs. nonzero length.
We now do this, which short circuits as soon
as it finds any key that is only in sub1:
return _.every(sub1.subscribers.keys(), (key) => {
return sub2_set.has(key);
});
First, there are no more convoluted signals.
We also simplify the parameter to just the "mentioned"
object corresponding to either a user or a broadcast
mention.
For the user group scenario, this has always been dead
code, which you only realized when you got to the comment
at the bottom. Now we actually do nothing.
And I moved the relevant commment to the
the typeahead code (with new wording).
I also moved the is_silent check to the caller. I don't
feel too strongly about that either way. It's kind of silly
to call a function only to give that function an additional
responsibility to worry about. On the other hand, I see
the logic of that function enforcing everything. I went
with the former for now.
Arguably we should have a warning for silent mentions,
since doing a silent mention of somebody not on a stream
is a good indication of a typo. I do understand the use
case, but the user can always ignore the warning. Anyway,
we have decent test coverage on this.
This isn't really an extraction; it's more giving
a name to an anonymous function and moving it to
higher module scope.
We convert this to an ordinary function call, which
allows us to move it out of intialize().
Since there's just one simple parameter now (linked_stream),
we can avoid some error checking.
We also avoid the comment that describes the function,
since it now has a name.
And then one minor tweak is to do the inexpensive
`invite_only` higher in the function. This will be
a nice speedup when you link to really large public
streams.
The unit tests are also a bit easier to read now--less
setup and more explicit names.
Previously, if you tried to invite a user whose account had been
deactivated, we didn't provide a clear path forward for reactivating
the users, which was confusing.
We fix this by plumbing through to the frontend the information that
there is an existing user account with that email address in this
organization, but that it's deactivated. For administrators, we
provide a link for how to reactivate the user.
Fixes#8144.
The sort_recipients helper is used for many different
typeaheads, such as compose PMs, compose mentions,
and some settings-related code.
We now avoid unnecessary sorting steps in cases
where we have plenty of results in the top buckets
(such as users who match on prefix).
This change should not have any user-facing
implications.
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
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
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.
This test mostly tests logic that I'm about
to remove in subsequent commits, and it's a bit
messy.
This commit removes 100% line coverage, but I
will restore that a few commits later.
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.
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 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.
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.
The benchmark is commented out. It takes only a few
milliseconds to run, so there may be no reason not
to always run it. It doesn't test correctness, so
it would arguably inflate line coverage, but set/get
are obviously covered elsewhere.
We now require the actual tests to explicitly
to zrequire Dict, rather than magically adding this.
In one case, the use of Dict was clearly just for
the test (not the app), so I converted that an ordinary
JS object (see timerender.js).
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.
We don't have people named "all". Instead, we
create pseudo person objects with email/full_name
of "all" (along with some other fields). The tests
now reflect this.
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 server may send us ids in the order
[11, 2], instead of [2, 11]. We don't want
to rely on server behavior, regardless, for
the sort.
Our tests now show we process that data.
The current code is is still buggy and causes
us to show the same huddle two different times
for situations where the lexical sort doesn't
match the numerical sort.
This happens on czo often, where Tim is user
7, and his id sorts lexically after ids like
58, 622, 4444, etc.
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.
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 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 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.
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.
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.
Steve Howell
Aman Agrawal
Pragati Agrawal
Hashir Sarwar
sahil839
Anders Kaseorg
rebtung
Steve Howell
YashRE42
Rohitt Vashishtha
Ryan Rehman
vaibhavrajsingh2001
Roland Crosby
Divyanshu Agrawal
Abhishek-Balaji
Tim Abbott
Vishnu KS
YashRE42
Stefan Weil
Jagan
Wbert Adrian Castro Vera
shubhamgupta2956
Chris Heald
Tlazypanda
Mohit Gupta
Nat1405