Skip to content

Additional code jam management utilities#1677

Merged
mbaruh merged 12 commits into
mainfrom
mbaruh/jam-management
Aug 15, 2021
Merged

Additional code jam management utilities#1677
mbaruh merged 12 commits into
mainfrom
mbaruh/jam-management

Conversation

@mbaruh
Copy link
Copy Markdown
Member

@mbaruh mbaruh commented Jul 8, 2021

The purpose of this PR is for us to avoid manually searching the permissions of team channels, and dealing with user-based overwrites. This also allows us to quickly tell what team a participant belongs to.

Added:

  • An info embed with the team the member is in. The team is decided by finding in which channel the member has overwrites.
  • A Command to move a member from one team to another by changing the permissions of the appropriate team channels.
  • A command to end the code jam and delete all the team channels and categories.
  • A command to remove a participant from their team.

How to review

This PR isn't as big as it may seem. Most of the diff is from moving the cog to another extension, and moving all the channel creation functions to another module. What I actually added are the few commands in the last couple of commits.

mbaruh and others added 4 commits July 7, 2021 22:24
The channel creations are static and clutter the cog class. We want to add more commands to the cog, so we move the static functions away to a separate file first.
- An info embed with team the member is in. The team is decided by finding in which channel the member has overwrites.
- Command to move a member from one team to another by changing the permissions of the appropriate team channels.
- A command to end the code jam and delete all the team channels and categories.
@mbaruh mbaruh requested review from Akarys42, jb3 and ks129 as code owners July 8, 2021 20:17
@mbaruh mbaruh changed the title Mbaruh/jam management Additional code jam management utilities Jul 8, 2021
@mbaruh mbaruh requested a review from janine9vn July 8, 2021 20:19
@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: moderation Related to community moderation functionality: (moderation, defcon, verification) t: feature New feature or request p: 1 - high High Priority a: events Related to management of core events and removed a: moderation Related to community moderation functionality: (moderation, defcon, verification) labels Jul 8, 2021
@SebastiaanZ SebastiaanZ self-requested a review July 11, 2021 18:54
Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite like the approach chosen to get additional confirmation for the command that removes the team channels. but it's not a blocker to me.


def setup(bot: Bot) -> None:
"""Load the CodeJams cog."""
from bot.exts.events.code_jams._cog import CodeJams
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know what kind of side effects we're trying to prevent here? I noticed a similar tactic in the help_channels package, but I'm not sure what we're specifically trying to prevent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circular imports presumably

Comment thread bot/exts/events/code_jams/_cog.py Outdated
Comment thread bot/exts/events/code_jams/_cog.py Outdated
@mbaruh
Copy link
Copy Markdown
Member Author

mbaruh commented Jul 12, 2021

Concerns were raised about blindly deleting a mass of channels. I'll improve the confirmation message before merging this PR.

@mbaruh mbaruh marked this pull request as draft July 12, 2021 13:32
The command now sends the details of all channels about to be deleted to the pasting service, and confirmation is done through a reaction by the invoker of the command within a limited time (10 seconds).
@mbaruh mbaruh marked this pull request as ready for review July 24, 2021 21:44
@mbaruh mbaruh requested a review from SebastiaanZ July 24, 2021 21:44
@mbaruh mbaruh added p: 2 - normal Normal Priority and removed p: 1 - high High Priority labels Jul 27, 2021
@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Aug 8, 2021

Setting this to low as the last code jam that needed these utilities occurred not to long ago, and the next code jam probably won't happen for a while.

@Xithrius Xithrius added p: 3 - low Low Priority and removed p: 2 - normal Normal Priority labels Aug 8, 2021
@janine9vn janine9vn added p: 2 - normal Normal Priority and removed p: 3 - low Low Priority labels Aug 15, 2021
Copy link
Copy Markdown
Contributor

@Akarys42 Akarys42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like I reviewed this but never actually approved, lgtm!


def setup(bot: Bot) -> None:
"""Load the CodeJams cog."""
from bot.exts.events.code_jams._cog import CodeJams
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circular imports presumably

Comment thread bot/exts/events/code_jams/_channels.py
Comment thread bot/exts/events/code_jams/_channels.py Outdated
Comment thread bot/exts/events/code_jams/_cog.py
Comment thread bot/exts/events/code_jams/_cog.py
Comment thread bot/exts/events/code_jams/_cog.py Outdated
Comment thread bot/exts/events/code_jams/_cog.py
Comment thread bot/exts/events/code_jams/_cog.py Outdated
Comment thread bot/exts/events/code_jams/_cog.py Outdated
Comment thread bot/exts/events/code_jams/_cog.py Outdated
mbaruh and others added 5 commits August 15, 2021 23:10
Co-authored-by: Bluenix <bluenixdev@gmail.com>
Co-authored-by: Bluenix <bluenixdev@gmail.com>
Interestingly enough, the reason doesn't seem to be displayed for channel permission overrides.
Co-authored-by: Bluenix <bluenixdev@gmail.com>
@mbaruh mbaruh requested a review from Bluenix2 August 15, 2021 21:01
Copy link
Copy Markdown
Contributor

@Bluenix2 Bluenix2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found two nit-picks, feel free to disregard those unless you're changing other things.

I have not tested or ran these changes myself, at the earliest I can do that tomorrow if necessary

member = ctx.guild.get_member(int(row["Team Member Discord ID"]))

if member is None:
log.trace(f"Got an invalid member ID: {row['Team Member Discord ID']}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we want to keep track of this, or maybe just a bool. So that you know when running the command that some members were omitted?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was raised in the PR which added this command, but out of scope for this one.

await ctx.send("Command timed out.", reference=message)
return

else:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You return in the except, no need to have this in an else.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, but I personally like the explicitness of the flow.

@mbaruh mbaruh merged commit 4c9405b into main Aug 15, 2021
@mbaruh mbaruh deleted the mbaruh/jam-management branch August 15, 2021 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: events Related to management of core events p: 2 - normal Normal Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants