Tag groups can belong to groups (PR #10854)

Allows tag groups to be assigned to groups other than the ‘staff’ group.

https://meta.discourse.org/t/allow-restriction-of-tags-to-non-staff-groups/161322

GitHub

    return permissions.everyone !== PermissionType.READONLY;

It’s preferred not to use the array syntax for javascript objects unless you have to.

This looks great overall to me. I’ve suggested a bunch of minor changes. I assume you’ve tested it fairly comprehensively locally and everything is working well? If so feel free to merge when you’re around for a bit in case it needs updating/changes.

    allGroups.forEach((group) => {

We prefer the fat arrow syntax unless you need a new this context.

    return permissions.everyone === PermissionType.FULL;
      this.allGroups.forEach((group) => {
    assert.ok(find(".tag-group-content .btn.btn-default:disabled").length);

This is a huge nitpick but the query selector can find the disabled element which is slightly safer code than calling [0] which might not exist. I know it’s a test, but still.

The indentation here looks a bit off.

    if guardian.authenticated?

The way the various chunks of hand-written SQL is formatted in this file is extremely inconsistent. Do we have a set of rules/style guide on how it should be done?

I agree it’s inconsistent, but maybe this is displaying differently for you. Check this out:

image

The INNER JOINs are to the left of the (SELECT, that they belong to. I feel like any parts of a subquery should be to the right of where it starts, or at least aligned in a column. The FROM and WHERE seem good.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/allow-restriction-of-tags-to-non-staff-groups/161322/10