Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement `create_streams_policy`. #12276

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Contributor

commented May 6, 2019

This pull request migrates from create_stream_by_admins_only to create_streams_policy as per #12236.

cc @timabbott

Testing Plan:
I've updated all of the unit tests and tested manually.

GIFs or Screenshots:
This change does not impact the user interface (only renaming a identifier).

davidtwco added some commits May 6, 2019

Add database migrations and tests.
This commit adds database migrations and the accompanying tests to
migrate from `create_stream_by_admins_only` to `create_stream_policy`
which mirrors the existing `invite_to_stream_policy`.
Implement `create_stream_policy`.
This commit modifies the rest of Zulip (+ tests) so that it uses
`create_stream_policy` instead of `create_stream_by_admins_only`.

Fixes #12236.
Rename `create_stream_permission`.
This commit renames the `create_stream_permission` field in the
templates to `create_stream_policy`, matching the field used in the
database model. This matches what `invite_to_stream_policy` does and
will be clearer when the `waiting_period_threshold` is split into its
own field.
@zulipbot

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

@davidtwco
Copy link
Contributor Author

left a comment

I think this is most of the way there, left a few comments with the one or two things I was unsure about.

Show resolved Hide resolved frontend_tests/node_tests/dispatch.js
Show resolved Hide resolved static/js/server_events_dispatch.js
@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

I'm also unsure about what I did to cause the coverage regression that we're seeing on CI, everything passed locally so hopefully all the tests will pass after that is fixed.

@timabbott

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@davidtwco yeah, that error is definitely not your fault. It should be fixed in master soon.

@timabbott

This comment has been minimized.

Copy link
Member

commented May 6, 2019

Nice work @davidtwco! I fixed the typo above, squashed the first two commits since they're conceptually not separable (and redid the commit messages to be spelled settings: ... so that it's easy when skimming the commit history to notice which commits touch which parts of the project) .
And then merged as the series of commits ending with f53a8f8.

A few notes:

  • There's definitely some follow-up work to do withtest_realm_boolean and friends; I opened #12284 for that work. I had meant to open that when opened #12236, but forgot to.
  • We should open an issue for the UI-level work of separating out the waiting period as a separate field visually, now that is is indeed independent on the database level.

@rishig do you have thoughts on how the "waiting period" field should be described in the UI / where it should sit? Possibly you could just open the issue and link it here.

Huge thanks for doing this @davidtwco!

@timabbott timabbott closed this May 6, 2019

@davidtwco davidtwco deleted the davidtwco:issue-12236 branch May 7, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

Thanks @timabbott. I'm happy to work on separating the waiting period out as its own field once there are some clearer plans on how you'd want that to look.

@rishig

This comment has been minimized.

Copy link
Collaborator

commented May 7, 2019

One option for how this could look is

Prevent new users from
[ ] Creating streams
[ ] Adding users to streams
[ ] Adding custom emoji
[ ] Initiating private messages with non-admins
[ ] etc
New users are members and guests less than [3] days old.

The first part is unfortunately not great from a i18n perspective.
We could also change the first line to "Prevent new users from doing the following:"

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

One option for how this could look is

Prevent new users from
[ ] Creating streams
[ ] Adding users to streams
[ ] Adding custom emoji
[ ] Initiating private messages with non-admins
[ ] etc
New users are members and guests less than [3] days old.

The first part is unfortunately not great from a i18n perspective.
We could also change the first line to "Prevent new users from doing the following:"

I've submitted #12291.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.