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

Separate "waiting period threshold" into own setting. #12291

Closed

Conversation

Projects
None yet
4 participants
@davidtwco
Copy link
Contributor

commented May 7, 2019

This PR follows-up on #12276 to separate the "waiting period threshold" based on discussion in Zulip.

Quoting from the first commit:

This commit separates the waiting_period_threshold setting from
the create_stream_policy setting, adding a new setting that the user
can use to select a waiting period threshold.

Both the invite to stream policy and create stream policy now have three
options: admins only, members and admins, or members after waiting
period/admins.

cc @rishig @timabbott

Testing Plan:
I've updated the frontend tests (both node and casper) and tested manually.

GIFs or Screenshots:
image

@timabbott

This comment has been minimized.

Copy link
Member

commented May 7, 2019

I assume for "new users" we want to have it just be "All members with ..." -- admins shouldn't ever have that tag, I think?

Show resolved Hide resolved static/js/admin.js Outdated

@davidtwco davidtwco force-pushed the davidtwco:waiting-period-threshold-setting branch from fda19c5 to c7a9023 May 9, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I've updated this PR after discussion in Zulip with @rishig.

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

I've added a new commit that changes the terminology used based on this conversation.

@davidtwco davidtwco force-pushed the davidtwco:waiting-period-threshold-setting branch from 885af63 to edca257 May 10, 2019

@timabbott

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@rishig can you review the UX on this?

@rishig

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

Thanks David for your work on this! The commits had a number of changes mixed together, so I separated them out a bit and made a few wording and variable name changes as well.

The three commits ending in c3af126 have the changes to the code. Would you be able to take a look and also update the tests?

Once this is merged I can deal with updating and merging the documentation commits.
Thanks again for your work on this!

davidtwco and others added some commits May 14, 2019

org settings: Fix dropdown value inconsistency.
The value in the handlebars template for `invite_to_stream_policy`
is inconsistent with the value in the js file. Changing all three
occurances to a third value, since that's the one we'll want moving
forward.

Co-Authored-By: Rishi Gupta <rishig@zulipchat.com>
org settings: Extract setting for new user waiting period.
This commit separates the `waiting_period_threshold` setting from
the `create_stream_policy` setting, adding a new setting that the user
can use to select a waiting period threshold.

Both the invite to stream policy and create stream policy now have
three options: admins only, members and admins, or members after
waiting period/admins.
org settings: Remove hanging references.
`realm_invite_to_stream_by_admins_only` doesn't appear elsewhere in our
codebase. Introduced in 272ed90, so I'm guessing this is the intended
value.

Co-Authored-By: Rishi Gupta <rishig@zulipchat.com>
docs: Update documentation for new user settings.
This commit updates the documentation to reflect changes to the new user
settings.
docs/org settings: Use "full member" terminology.
This commit modifies the help documentation and templates to use a "full
member"/"new member" terminology throughout.

@davidtwco davidtwco force-pushed the davidtwco:waiting-period-threshold-setting branch from edca257 to c2d84bd May 15, 2019

@davidtwco

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@rishig I've updated this PR with the commits from your branch (GitHub orders them strangely, but they look fine locally so I assume it is just a rendering issue).

I've fixed the tests so they pass after your changes.

@rishig

This comment has been minimized.

Copy link
Collaborator

commented May 21, 2019

Tim, just making sure this isn't blocking on me?

@timabbott

This comment has been minimized.

Copy link
Member

commented May 22, 2019

Oh, I definitely thought this waiting on you, sorry about that! Looking now.

@timabbott
Copy link
Member

left a comment

This is great; I made a few changes (listed here) and merged this, thanks @davidtwco! A few notes for follow-ups @rishig:

  • I added a brief reference to this in /help/moderating-open-organizations, but possibly that could use some further editing.
  • A reminder of whatever further tweaks you had in mind for the documentation (I fixed the most important things, linking to the "full member" definition).
  • We should switch the ? marker to our standard styling for documentation hints; currently it's just an ?.
* **Organization administrators, and members with accounts older than the new user waiting period**
* **Organization administrators**
* **Organization administrators and all members**
* **Organization administrators and full members**

This comment has been minimized.

Copy link
@timabbott

timabbott May 22, 2019

Member

"full members" here should be a link.

for any `N`.
* **Organization administrators**
* **Organization administrators and all members**
* **Organization administrators and full members**

This comment has been minimized.

Copy link
@timabbott

timabbott May 22, 2019

Member

Same here.

@@ -1545,6 +1545,7 @@ body:not(.night-mode) #account-settings .custom_user_field .datepicker {
pointer-events: all;
}

#id_realm_waiting_period_setting,
#id_realm_create_stream_policy,
#id_realm_invite_to_stream_policy,
#id_realm_org_join_restrictions,

This comment has been minimized.

Copy link
@timabbott

timabbott May 22, 2019

Member

We should probably restructure these to use a common CSS class; but probably not a blocker for merging :)

@pragatiagrawal31 @shubhamdhama FYI.

@timabbott timabbott closed this May 22, 2019

@davidtwco davidtwco deleted the davidtwco:waiting-period-threshold-setting branch May 22, 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.