Add staff counts to !server#970
Conversation
Cleaning up a particularly dirty line by turning it into like 10 lines, and also adding the number of channels that are hidden to the `@everyone` role - which we're classifying as "Staff channels".
lemonsaurus
requested review from
GhostofGoes and
eivl
and removed request for
a team
lemonsaurus
added
a: frontend
We now check: - Does the @everyone role have explicit read deny permissions? - Do staff roles have explicit read allow permissions? If the answer to both of these are yes, it's a staff channel. By 'staff roles', I mean Helpers, Moderators or Admins.
I wish this test didn't exist.
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
I don't understand how this change makes it more precise. Under what circumstances would a channel hidden from @everyone not have at least one staff role with read permissions?
This was mostly in reference to what @SebastiaanZ said about us potentially cherry picking which channels are visible in lurk mode. If we did that, we would make them read deny to This also resolves the Code Jam channels @SebastiaanZ asked about, because these do not have any explicit staff read permits, only admins can see them because admins can see everything. At least, I think so. I skimmed the code in CodeJams to come to that conclusion. |
I think I've triggered quite a mess of complexity with that comment. I'm not quite sure if it is really needed; the only downside of not caring about it is that our staff channel count is going to look massive, especially during code jams. Still, the thought of people thinking "What are those people doing in all those channels?" is quite funny. For the Code Jam, the code is not entirely story there, I think. I know we had a discussion in the mods team about allowing moderators to see the code jam channels around the time the previous jam kicked off, but I don't quite recall the conclusion to that discussion. I think we settled on keeping it admins-only, which would make the solution fine. (The new events team will get access, but that role isn't counted as a staff role in our constants.) I'm glad we're not talking about "send" overwrites, because that would be a mess of a totally different level. |
- We're using a set comprehension and flipping the order for counting the number of channels that are both staff allow and @everyone deny. - We're breaking the staff channel count stuff into a separate helper function so it doesn't crowd the server_info() scope. These fixes are both to address the code review from @MarkKoz, thanks Mark.
Pretty sure we decided against this. It's been fine to handle this in admins only. And yes, this is a silly bit of complexity for such an absolutely inessential feature, but I do think the code has been improved by this, overall. edit: nevermind, that last commit broke everything. |
We're using the set comprehension to prevent duplicates anyway, so flipping these back makes more sense. Also added a missing ctx and tested ok.
Simplification comes from being able to access permissions as attributes on the overwrite object. This removes the need to iterate all permissions. Efficiency comes from checking all roles within a single iteration of all channels. This also removes the need to flatten and filter the channels afterwards, which required additional iterations.
This de-clutters the main `server_info` function and improves its readability.
MarkKoz
left a comment
MarkKoz
left a comment
There was a problem hiding this comment.
If you're up for it, this could use some tests.
I hate to say this but I'm not really up for this, at least not in the very near future. If someone else wants to commit some tests for this, you're welcome to. Otherwise, since it's not at all a critical feature and it seems to work, we could probably merge this without tests. If you insist on tests, I can probably write some in like a week or two when I've freed up some time. Sorry! |
|
No, I don't insist. I approved, after all. |
There was a problem hiding this comment.
This looks good and I'm happy to approve. The new information will be nice to have. I agree that tests aren't critical for this feature, and we can add them later on, if we want to.
I've done a code review & tested but I haven't yet had the time to go through all the conversation here to make sure everything has been resolved, and I don't feel comfortable merging without doing that first. Feel free to merge though - I'll get around to it later tonight if no one does by then.
4 participants
A small change to add number of staff channels and number of staff members to !server. These are nice numbers to track, and may be interesting for everyone.
Closes #969