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

Allow admins to kick members from a team #16

Merged
merged 12 commits into from
Mar 10, 2024
Merged

Allow admins to kick members from a team #16

merged 12 commits into from
Mar 10, 2024

Conversation

eluric
Copy link
Contributor

@eluric eluric commented Mar 5, 2024

This is an important ability so that team members that are causing grievances can be removed from a team that they may otherwise be unwilling to leave.

EDIT:
Upon abstracting the code for removing players from a team (since the code for kicking a player is largely the same as a player leaving by themself), we encountered CommandInvokeErrors.
Specifically
discord.app_commands.errors.CommandInvokeError: Command 'kick_member' raised an exception: NotFound: 404 Not Found (error code: 10008): Unknown Message

This happens when the code attempts to respond to an interaction in a channel that has been deleted. This did not occur before the abstraction of code because the interaction received a response before the channels were deleted. However, post-abstraction the folllowup response is only sent after the deletion of channels. This is a little hard to get around. We can possibly leave the actual deletion of the channels to the individual cog commands but that is not an ideal solution. We can also pass in the interaction to the context command and allow it to respond to the interaction.

However, we've chosen to simply catch the error when it happens and ignore it.

cogs/admin.py Outdated Show resolved Hide resolved
@eluric
Copy link
Contributor Author

eluric commented Mar 6, 2024

Since leave_team and kick_member had essentially the same code, it has been abstracted out of both methods and placed in a separate file.
Typecasting has also been removed to fit with Pydantic implemented in #15

@eluric eluric requested a review from SkellyBG March 6, 2024 06:10
@SkellyBG
Copy link
Contributor

SkellyBG commented Mar 6, 2024

hey nice work! I feel like nitpicking ;;-; but I think this is too DRY. Generally when you DRY up a piece of code, you want its to be a logical unit that's testable - which this context code isn't quite. A better option would be to say, have a function that handles the deletion of a player, and another function that handles the deletion of a team + all the channels - then they would be much more testable. If that doesn't quite make sense, let me know!

@SkellyBG
Copy link
Contributor

SkellyBG commented Mar 6, 2024

mhmm I think, at least for me, that cogs are the "front-end" of discord bots and should handle the messages instead of context - passing in a boolean to control how the message is sent feels like a code smell to me

@eluric
Copy link
Contributor Author

eluric commented Mar 7, 2024

mhmm I think, at least for me, that cogs are the "front-end" of discord bots and should handle the messages instead of context - passing in a boolean to control how the message is sent feels like a code smell to me

I'm not sure what you mean by code smell but I've fixed it.

Copy link
Contributor

@SkellyBG SkellyBG left a comment

Choose a reason for hiding this comment

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

LGTM - just some small nitpicks!

src/context/team.py Outdated Show resolved Hide resolved
src/context/team.py Outdated Show resolved Hide resolved
@eluric eluric merged commit c6d7fc8 into main Mar 10, 2024
1 check passed
@eluric eluric deleted the kick-members branch March 10, 2024 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants