We add a function subscribed_stream_ids which returns an array
of stream ids of all subscribed streams.
This is a prep commit for changing the logic for sorting streams
to store stream ids instead of names.
Prettier would do this anyway, but it’s separated out for a more
reviewable diff. Generated by ESLint.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit changes stream_data.is_user_subscribed to use stream id
instead of stream name.
We are using stream ids so that we can avoid bugs related to live
update after stream rename.
We change validate_stream_message to check the existence of stream from
the stream name in compose box early and we then pass stream_id or the
obtained sub objects accordingly to other validate functions.
Passing stream_id or sub objects to these functions, enables us to use
stream_id instead of stream name in stream_data.get_subscriber_count.
stream_data.get_stream_post_policy is also removed as we only used it in
validate_stream_message_policy, but we do not need it now as we can get
stream_post_policy directly from sub object obtained by early check of
valid stream name.
This commit changes stream_data.create_sub_from_server_data to use
stream id, instead of stream name, for checking whether subscription
already exists or not. We are using stream ids so that we can avoid
bugs related to live update after stream rename.
This commit changes stream_data.remove_subscriber to use stream id
instead of stream name. We are using stream ids so that we can
avoid bugs related to live update after stream rename.
Thsi commit changes stream_data.add_subscriber to use stream_id
instead of stream name. We are using stream ids so that we can
avoid bugs related to live update after stream rename.
This commit changes receives_notifications function to use
stream_ids instead of stream names. We are using stream ids so
that we can avoid bugs related to live update after stream rename.
This commit adds frontend support for setting and updating message
retention days of a stream from stream settings.
Message retention days can be changed from stream privacy modal of the
stream and can be set from stream_creation_form while creating streams.
Only admins can create streams with message_retention_days value other
than realm_default.
This commit also contains relevant changes to docs.
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.
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().
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();
```
`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
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.
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.
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.
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.
Explicitly stubbing i18n in 48 different files
is mostly busy work at this point, and it doesn't
provide much signal, since often it's invoked
only to satisfy transitive dependencies.
This cleans up the handoff of page_params
data between ui_init and modules that
take over ownership of page_params-derived
data.
Read the long comment in ui_init for a bit
more context.
Most of this diff is actually test cleanup.
And a lot of the diff to "real" code is
just glorified `s/page_params/params/`
in the `initialize` functions.
One little oddity is that we don't actually
surrender ownership of `page_params.user_id`
to `people.js`. We could plausibly sweep
the rest of the codebase to just use
`people.my_user_id()` consistently, but it's
not a super high priority thing to fix,
since the value never changes.
The stream_data situation is a bit messy,
since we consume `page_params` data in the
initialize() function in addition to the
`params` data we "own". I added a comment
there and intend to follow up. I tried
to mostly avoid the "word soup" by extracting
three locals at the top.
Finally, I don't touch `alert_words` yet,
despite it also doing the delete-page-params-data
dance. The problem is that `alert_words`
doesn't have a proper `initialize()`. We
should clean that up and have it use a
`Map` internally, too.
This 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.
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 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 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.
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);
});
sahil839
Anders Kaseorg
Steve Howell
Ryan Rehman
clarammdantas
Rohitt Vashishtha
Hashir Sarwar
Steve Howell
Anders Kaseorg