FEATURE: Add new group visibility option for "logged on users" (PR #7814)

In some cases it is useful to only allow logged on users to be able to view group membership. That’s what this PR does. It also sets all automatic groups (excepteveryone) to be visible to “logged on users”, previously they were marked as public but suppressed in the group page for non-staff.

Admin UI screenshot: image

GitHub

You’ve signed the CLA, pmusaraj. Thank you! This pull request is ready for review.

This looks good to me, go ahead and merge… a tiny bit mixed about id (4) belonging to this thing, cause the numbers are kind of expanding in scope… but re-numbering this thing is going to be complicated.

Can you first perhaps see how complex it will be to re-number?

It shouldn’t be too painful to renumber. Is this below the preferred numbering?

public: 0, logged_on_users: 1, members: 2, staff: 3, owners: 4

Yeah I think that numbering is right … technically “staff” kind of means “moderators” cause “admins” unconditionally get to see groups. I think “owners” can be 4 as long as “mods” can not see it when set… otherwise it needs to be 3.

I have added a migration for the renumbering, and made two more changes.

First, I set the default visibility for the “everyone” group to be “staff” in db/fixtures/002_groups.rb. This way it’s more consistent with the Group.refresh_automatic_group! method that gets called by the ensure_consistency job, see https://github.com/discourse/discourse/blob/master/app/models/group.rb#L320

Second change: I modified the language in the UI, adding a field description with “Admins can see all groups.”:

image

and removed the “admins” from the label of each option in the dropdown:

image

Super minor but this could have been SET visibility_level = visibility_level + 1 right? And below the opposite? (-1). No need to change it just an observation :slight_smile:

Looks good to me, merge when you’re comfortable!

Oh my this typo is in so many places. Just goes to show how much code is copied and pasted :slight_smile:

Yeah, that would have been better (I would need to modify it so that 0-level groups shouldn’t be bumped to level 1 on up).