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

Add "invite to stream" setting. #12085

Closed

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Contributor

commented Apr 8, 2019

Fixes #12042. This PR decouples the threshold used to determine when a user can invite another user to a stream from the threshold for when a user can create a stream.

Testing Plan:
I've ran all of the tests - adding and updating some existing tests for the new behaviour. I have also tested manually.

GIFs or Screenshots:
image

@zulipbot zulipbot added the size: XL label Apr 8, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

I saw this error locally and wasn’t sure how to resolve, would appreciate any pointers:

Apr 08 20:08:38 TypeError: $(...).change is not a function
Apr 08 20:08:38     at Object.exports.build_page (/home/circleci/zulip/static/js/settings_org.js:13:8394)
Apr 08 20:08:38     at Object.exports.set_up (/home/circleci/zulip/static/js/settings_org.js:8:10586)
Apr 08 20:08:38     at run_test (/home/circleci/zulip/frontend_tests/node_tests/settings_org.js:6:102)
@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@davidtwco thanks for the pull request! That error is likely caused by the zjsquery setup of return values for DOM manipulation being missing in that unit test.

Show resolved Hide resolved zerver/views/streams.py Outdated
Show resolved Hide resolved zerver/models.py Outdated
@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

This generally looks good as an implementation of this concept; thanks for doing this! I posted a bunch of comments on details. A few requests for feedback from others:

  • @rishig are we OK with merging this with a different time threshhold from the other waiting period? It's definitely the simplest way to write the code, and one can imagine a use case for having the waiting periods be different, so I think it's probably a reasonable product expedient to just merge this concept.
  • @showell would you be able to help @davidtwco with figuring out the node test failure issue, since @davidtwco is new to the codebase?
@rishig

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

It'll be a hassle to migrate back to a unified time period though, right? Long term I'm not sure it makes sense to have this level of configuration (different time periods for different confused behaviors).

If not for the migration hassle I think it would be fine as a temporary thing. (Another temporary solution though would just be to remove the "who can add streams" setting from affecting "invite to streams" at all.)

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Something of a hassle. It is the case that creating a stream is for some orgs like 20x more spammy than inviting someone to a stream, and I could see someone who wanted like a 1 day limit for one and a 1 month limit for the other. And if we end up adding several more features with waiting periods, we may eventually have a situation where an org wants different waiting periods for different settings. Based on my experience with settings in the past, I think it's probably only like 2x more likely that we find ourselves eventually wanting to migrate to a single "new user" feature as eventually wanting to do the opposite migration. So probably the single-value approach is better because I think it is more likely to be what we want to keep, but I'm really not sure -- there's a bunch of UX complexity that comes from deciding where to put the "new user waiting period" definition.

@rishig

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

The current UX proposal is adding a single setting like:

Prevent new members and guests from 
[ ] Creating streams
[ ] Inviting others to streams
[ ] Adding custom emoji
[ ] ...
New users are users with accounts less than [3] days old.

Where 3 is either a text box or a dropdown with values like 1, 3, 7, 15, 30.

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Thanks for the comments, I’ll work on addressing these shortly.

I agree that there is a lot of value in having a single unified setting for what new users can and cannot do (and what qualifies as a new user) - it is definitely simpler from a UX perspective. However, I think there will always be a need from some users for more fine grained customisation.

Perhaps, long-term, Zulip could adopt a setting like @rishig is suggesting, which achieves the desired simpler UX. However, when one of the checkboxes is unchecked, then that would reveal a setting (much like the “create stream” and “invite to stream” settings that exist currently) that would allow the user to fine-tune the threshold for that behaviour.

This would enable users to customise each setting to their liking while still hiding the complexity of those additional settings from the majority of users who don’t need as much customisation. Further, should this PR be accepted, the setting that it adds would just become one of the settings that is hidden initially until a checkbox is unchecked.

At the very least, I would suggest updating the documentation to explain the current behaviour, that when the “create stream” setting is set to a threshold then that setting also applies to invitations, but not when it is set to any non-threshold value - as this is quite confusing when you run into it.

@davidtwco davidtwco force-pushed the davidtwco:invite-to-stream-permission branch from f8a3443 to 0503faf Apr 9, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

I've pushed commits that should resolve all of the previously raised concerns, fixed that failing test and includes the issue number in the commit.

@davidtwco davidtwco force-pushed the davidtwco:invite-to-stream-permission branch from 0503faf to 2dccf81 Apr 9, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2019

@timabbott @rishig Is there anything I can do to help see this PR land?

@davidtwco davidtwco force-pushed the davidtwco:invite-to-stream-permission branch from 2dccf81 to 827de1f Apr 27, 2019

@zulipbot zulipbot removed the has conflicts label Apr 27, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2019

I've rebased and resolved the conflicts.

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@davidtwco I thought I'd sent this reply, but I guess I hadn't. After some discussion, I think the right path forward is to go with the model Rishi described where there's just a single time period.

I think we can do this incrementally without implementing the full thing; I'm going to do a quick pass of trying to update this PR to the desired model and see if I can get it mergable.

@timabbott timabbott force-pushed the davidtwco:invite-to-stream-permission branch from 827de1f to 1055697 Apr 29, 2019

@zulipbot zulipbot removed the has conflicts label Apr 29, 2019

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

OK I'm out of time for tonight but I've pushed changes back to this PR which implements in the direction of the settings model we want. The new model is we have a Realm.invite_to_stream_policy field that works similarly to bot_creation_policy (effectively an enum). A few notes:

  • The valid values on the right now backend are members/admins/members with waiting period, and that should be fully unit tested (I rewrote some of the backend test suites to not use hacky methods to set fields on the Realm).
  • The frontend doesn't yet allow setting the "waiting period" option because we need to figure out how to word it (probably for the moment we can copy the N text from stream creation and just plan to fix it soon after this merges), but possibly we will want to fix that before merging this since that should probably be the default for an existing organizations with a waiting_period_threshhold set to avoid settings changing out from under existing installations.
  • We'll need to add a database migration to set that default value correctly; I can likely do this very quickly.
  • I removed the casper tests since we're generally not aiming to test every setting.
  • The node tests are failing; I just didn't have time to finish converting them. Should not be hard.
  • We should update the user docs to reflect the new model.

Thanks for all your work on this so far @davidtwco! If you happen to have time to e.g. fix the node tests or /help/ docs in a new commit that we'll plan to squash, that would be much appreciated.

Once we've merged this, there are a few follow-ups I'd like to note:

  • We should open a release-blocker issue for moving create_stream_by_admins_only to be a create_stream_policy field with a similar model.
@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

@timabbott I've pushed commits that resolve the node test failures and update the help documentation.

Update: I didn't see the failure on CI locally, it looks unrelated to the changes in this PR but I don't have time to dig into it right now.

Update 2: It appears that failure is spurious and a known issue.

settings: Create an explicit invite_to_stream_policy setting.
This commit creates a new organization setting that determines whether
a user can invite other users to streams. Previously this was linked
to the waiting period threshold, but this was both not documented and
overly limiting.

With significant tweaks by tabbott to change the database model to not
involve two threshhold fields, edit the tests, etc.

This requires follow-up work to make the create stream policy setting
work how this code implies it should.

Fixes #12042.

@timabbott timabbott force-pushed the davidtwco:invite-to-stream-permission branch from 79d1e3c to f66a682 Apr 29, 2019

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

OK, I addressed the remaining open items in the checklist above and pushed back to this PR. @rishig it's probably worth your taking a look at this before we merge. I don't think it's super critical that we get everything right, in that I think we should be able to migrate the model for the stream creation policy in the next week or two, but probably worth a look. As a reminder, the planned follow-up projects are these this:

  • We should open a release-blocker issue for moving create_stream_by_admins_only to be a create_stream_policy enum type field with a similar model to invite_to_stream_policy.
  • Once that's done, change the frontend UI to make waiting_period_threshhold its own independent thing.
@rishig

This comment has been minimized.

Copy link
Collaborator

commented Apr 29, 2019

cool. Some notes:

  • Let's make "Who can invite other other users to streams" -> "Who can add users to streams"
  • The comment about the N is a bit confusing -- maybe let's remove the comment and change the dropdown options to the following?
Members and admins
Admins only
All admins, and members with accounts old enough to create streams

It's still not great, but hopefully this will be an interim state.

  • If "Who can create streams" is not on the custom option, changing "Who can add users to streams" to the custom option doesn't show the Save changes widget.
@zulipbot

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Heads up @davidtwco, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

@timabbott Any idea why this isn’t marked as merged despite the commit being on master?

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

OK. I went with "All admins and members who can create streams" because very long text in these dropdowns is somewhat problematic.

Merged, thanks @davidtwco!

@timabbott timabbott closed this Apr 30, 2019

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

@davidtwco because I'd gotten interrupted in between merging it and finishing writing the text above indicating it was merged, so it was still sitting open in my tab waiting for me to get back to it this morning. I'll now open the follow-up issue.

@timabbott

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Opened the follow-up as #12236.

Thanks again for doing this @davidtwco!

@davidtwco davidtwco deleted the davidtwco:invite-to-stream-permission branch May 2, 2019

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.