This commit changes the would_receive_message to use user_id
instead of emails.
This change is done because user_ids are immutable and using
user_ids is the correct way of uniquely identifying user.
The change in 'would_receive_message' also leads to change
in util.is_pm_recipient to use a string of user_ids instead
of emails.
We also know that user_ids passed to 'would_receive_message'
are active user_ids, since we get them from buddy_list.
So we don't need to check whether the user is active, which
was previously being checked by get_active_user_for_email.
This commit changes the needs_subscribe_warning function to
use user_id instead of emails.
This change is done because user_ids are immutable and using
user_ids is the correct way to uniquely identify a user.
We already know that user_ids being passed in this function are
active user_ids, since they come from typeaheads.
So, we only need to call 'people.get_by_user_id', to get the user
object from user_id and do not need to check the active status of
user, which was done previously using 'get_active_user_for_email'.
This commit changes the compose_invite_users template to use
data-user-id as property intead of data-useremail.
This is changed to maintain consistency with other parts of the
code where user_ids are used for referring to users.
This also helps in removing some of the checks for the case of
undefined emails.
We now send user_ids to the backend API for subscribing/unsubscribing
users to a stream instead of emails.
This change is done now because we have just migrated the backend API to
support sending user_ids in 2187c84, so it wasn't possible before.
This change is helpful because sending user_ids is more robust, as those
are an immutable reference to a user, rather than something that can
change with time.
The get_active_humans and get_non_active_humans functions used
to return a list of user objects. The get_active_humans is used
on settings_users.js and settings_bots.js, and in both places the
only attributes needed of the person object are the user_id and
full_name.
To make the function return smaller, instead of a list of active
humans, we are returning a list of active human ids, saving memory.
With the ids we can call the people API to get the full_name attribute.
This is done to avoid flakes where our code for focusing first input
interferes with puppeteer's typing. More details in comments.
Thanks Dinesh for testing this out earlier as part finding a solution
to this flake.
Co-authored-by: Dinesh <chdinesh1089@gmail.com>
This reimplements our Zoom video call integration to use an OAuth
application. In addition to providing a cleaner setup experience,
especially on zulipchat.com where the server administrators can have
done the app registration already, it also fixes the limitation of the
previous integration that it could only have one call active at a time
when set up with typical Zoom API keys.
Fixes#11672.
Co-authored-by: Marco Burstein <marco@marco.how>
Co-authored-by: Tim Abbott <tabbott@zulipchat.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
This commit fixes the bug for subscribing the user from mention
warning which was introduced in e52b544.
This is fixed by changing email to be passed as list to
'invite_user_to_stream'.
The reason for this change is that, this is where `Filter` and
actual tracking of what messages are contiguous lives. This
will be beneficial when we will to move to a model where we
cache `MessageListData` objects for a large number of views.
This commit changes the person dict in event sent by do_change_user_role
to send role instead of is_admin or is_guest.
This makes things much more straightforward for our upcoming primary
owners feature.
Restored old behavior accidentally removed in
1ae07b93d8 (diff-e353fab8bea58b8746ec68c83aa39b36L48)
The server only remembers the most recent presence status update per
device. Meaning that, for instance, if the user only uses one client and
that client's last status update was IDLE, then the server only knows
that, doesn't know anything about the user's last ACTIVE time. Thus the
"active_timestamp" the server will serve about this user to the webapp
will be "undefined".
The old behavior was that for the sake of the "Last active: x ago"
status in buddy list popover, the latest status timestamp was used,
whether IDLE or ACTIVE.
The change linked about changed that to only pay attention to
ACTIVE. Thus, if the server doesn't remember any ACTIVE statuses, webapp
would show "Last active: More than 2 weeks ago", which was incorrect.
We restore the old behavior and further improvements can be made on top
of this.
Previously, we had to fiddle with the generated HTML to update
individual values. Now, we can simply ask the widget to rerender
the row that we updated.
This is done by passing an html_selector function that returns
a selector for the rendered item.
If:
- we do not provide html_selector function
- item is not currently rendered
- new html is not a string.
then the render_item() call is a noop.
We remove the "GROUP PMs" section that used
to be in the lower right sidebar.
Most of this is straightforward code removal.
A couple quick notes:
- The message fetching code now just
calls `huddle_data.process_loaded_messages`,
which we still need for search suggestions.
We removed `activity.process_loaded_messages`.
- The `huddle_data.process_loaded_messages`
function no longer needs to return `need_resize`.
- In `resize.js` we now just calculate
`res.buddy_list_wrapper_max_height` directly
from `usable_height`.
We had a bunch of places where we
were calling `resize.resize_bottom_whitespace`
with no arguments, which has been a no-op
since the below commit that removed support
for our `autoscroll_forever` option:
fa44d2ea69
With the `autoscroll_forever` options things
like opening/closing the compose box could
alter how much bottom whitespace you'd want,
but we stopped supporting that feature in
2017.
Since then bottom_whitespace has just always
been 40% of the viewport size. So we only need
to change it on actual resize events.
It's worth noting that we still call
`resize_bottom_whitespace` indirectly in many
places, via `resize_page_components`, and
the latter actually causes
`resize_bottom_whitespace` to do real work,
but that work is redundant for most of those
codepaths, since they're not triggered by
changes to the viewport. So there are other
opportunities for cleanup.
Trigger realm icon upload by clicking on realm icon element itself
rather than having a big upload button and to match our user avatar UI.
Added new spinner over the icon element itself to show while
uploading realm icon.
Since we are doing a assert call checking a url without no page.waitForSelector
or similar calls, we add this to avoid flakes where the assert is called
when we are in process of navigation to the page. Also, we use Promise.all
here, and call page.waitForNavigation first, to avoid race conditions where
form is submitted quickly and navigation occurs and then we are stuck waiting
for a navigation. There were no flakes in the CI related to this issue, but
I expect someone with a slow hardware would probably stumble upto this in future.
We were missing the step for clicking the checkbox to show the subdomain
input field. There was a flake because of this issue in CI where the
"testsubdomain" input was getting typed into the name field. This fixes
that flake.
There were two reason there were flakes in login tests. One problem was
that the assert call for checking that the login page url was happening
too early in rare cases. To fix this issue we wait for email input field
element in the login page. The second issue was the sometimes the url was
/login/ instead of /accounts/login/. The time between the redirect were
super close here too (around ~10ms) so I am suprised this didn't occur flake
locally more often.
Server logs that show the redirect:
2020-05-26 18:55:27.065 INFO [zr] 127.0.0.1 GET 200 36ms (db: 5ms/2q) (+start: 14ms) /accounts/login/ (unauth@zulip via ?)
2020-05-26 18:55:27.074 INFO [zr] 127.0.0.1 GET 200 28ms (db: 3ms/2q) (+start: 23ms) /login/ (unauth@zulip via ?)
To take a screenshot on failure where we have our common error
handling code in common.run_test method we need to have access to
the Page instance. Currently, we create a new Page in the test method
we pass into run_test, so we pass in a new Page instance to that method
so the test and the run_test method have access to the same Page. And,
then we take a screenshot on failure. It will be saved as `failure-${num}`
in var/puppeteer directory.
This is mostly just moving code from the `activity.js`
tests, but I also now explicitly cover the "100%"
use case (i.e. all four folks in the huddle are present).
Computed indexes into these raw objects should be guarded with
Object.prototype.hasOwnProperty; make our accessors do this
automatically and use them consistently.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This is helpful because if the user pastes multiple queries in the
searchbox and there are invalid search operators, then it is visible
through the typeahead.
The main reasoning for this change is as follows:
* When the search bar contains multiple search queries
but no search results, the last search operand does
not get displayed.
This happens due to the fact that filter object
contained 2 terms having the operator key value as
"search" instead of a single term where operator is
"search" and operand is a single string containing
the space seperated search queries. This condition
occurs for search_pills_enabled case only because
we used to Filter.parse the query twice
(once for the `base_operators` and once for the
`suggestion_operator instead of doing both at once).
Thus the `search_query` value inside the
`narrow.show_search_query` function which only
selected the operands of the first term displayed
an incomplete result.
* Another benefit of this commit is to display the narrow
operators in the URL fragment the same way as when
search_pills_enabled = False.
For example, On entering the queries in the mentioned
order -> 'is: starred', 'abc', 'def', 'is: private',
'ghi'. This is the URL:
Previously:
/#narrow/is/starred/is/private/search/abc.20def/search/ghi
Now (same as pills disabled case):
/#narrow/is/starred/is/private/search/abc.20def.20ghi
* We are also able to de-duplicate the non-typeahead search
query code path.
Commit 9b78a73e36 (#15005) made some of
our poorly written Casper tests fail. Now they’re just as poorly
written but passing again.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The people.js tests were using _add_user function to add
cross realm bots. The problem is that _add_user function
doesn't properly simulates the adding process as it doesn't
add the user in cross_realm_dict as well.
To solve this and eliminate the need of calling
people.initialize(), which means the params obj needs to be
defined, we extracted the whole logic of adding a cross realm
user into a separete function, add_cross_realm_user.
This makes it so that search_suggestion.js
does not depend on activity.js.
That dependency hasn't really been "elegant"
for quite some time, but it will become particularly
unnecessary when we go to remove the "Group PMs"
section from the right sidebar.
This commit introduces a temporary wart
where we have these two functions with the
same name in a sort of unnecessarily
complicated code stack:
activity.process_loaded_messages
huddle_data.process_loaded_messages
But we will eliminate the former function
very soon, and our message-related codepaths
will just call the `huddle_data` version
directly.
TESTING NOTES:
Now that `huddle_data` is a tiny leaf
module, it's super easy to just use the
real implementation of what was formerly
called `activity.get_huddles()` (and is
now in `huddle_data`).
When I first wrote this commit, introducing
the real implementation of `get_huddles` exposed
some bugs that I fixed in the immediately
prior commits to this.
When the tests were originally written,
I believe `activity.js` had some annoying
`jQuery` dependencies that made it hard
to unit test against. We've slimmed it over
time to be mostly just a "controller" module.
But even in its current state it would have
been a bit of a bloated dependency.
The other friction for using the actual
version of `get_huddles` was setting up
the message data, but that's pretty minor.
If you have a group PM where some users have
three-digit user_ids and some with four-digit
user_ids (or similar), a huddle could effectively
be ignored when determining the order of
search search suggestions.
Basically, we need a way to canonically sort
user_ids in "huddle" strings, and it's somewhat
arbitrary whether you sort lexically or sort
numerically, but you do need to be consistent
about it.
And JS is not exactly helpful here:
> [99, 101].sort()
[ 101, 99 ]
This is a pretty obscure bug with pretty low
user-facing consequences, and it was never
reported to us as far as I know, but the fix
here is pretty straightforward.
We have had similar bugs of slightly more consequence
in the past. The reason this bug has shown
up multiple times in our codebase is that every
component that deals with huddles has slightly
different forces that determine how it wants
to serialize the huddle. It's just one of those
annoying things. Plus, bugs with group PMs
do tend to escape detection, since most people
spend most of their time either on streams
or in 1:1 PMs.
This is a pure code extraction. The current
code is buggy with respect to user_ids with
different lengths of digits, i.e. it does
a naive lexical sort instead of a numerical
sort. We'll fix that in the next commit.
We already have a loading indicator for fetching older
messages. Thus it makes sense to implement the same
for displaying newer messages.
We set the display of `bottom-messages-logo` to none,
to prevent displaying two loading indicators during
the initial message load.
Fixes#15060.
I consolidate most of our users toward the top
of the file, so that we don't have to clutter
up individual tests. This also avoids some
confusion where charles/maria got repeated
in different tests with different ids.
I also introduce a couple four-digit ids to
try to expose more bugs related to sorting.
Note that it's still easy to keep tests
isolated here, as we have always been able
to cheaply re-initialize `people.js` and then
add individual users back.
There are still some tests where it makes
sense to just declare users locally, especially
if we are mutating their data.
There are a few minor incidental cleanups here,
mostly involving replacing hard coded ids
with things like `maria.user_id`.
This creates a little bit of noise in some
tests where we don't care about users, but
it's worth avoiding confusion about which
users exist at which time. Also the noisy
aspects here may actually catch regressions.
Finally, if the noise gets annoying, we can
do things like rename "Ted" not to collide
with the "Test" stream.
