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

Implement admin only commands and make some commands admin-able #18

Merged
merged 9 commits into from
Jun 5, 2024

Conversation

someyuck
Copy link

@someyuck someyuck commented Jun 4, 2024

I've completed tasks 1, 2, 3 and 5 in this PR. I added functions to check for bot admins, which are to be added in the .env file. I've made the backend_info command admin-only, and query and roll can now be run by admins outside the academic server constraint.

Comment on lines +80 to +88
class CheckFailedException(commands.CommandError):
"""Custom exception to help identify which check function failed."""

def __init__(self, check_name: str):
"""Initialise with the name of the check."""
super().__init__()
self.check_name = check_name


Copy link
Member

Choose a reason for hiding this comment

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

Instead of storing the "exception type as a string" I would suggest a more OOP-like approach

Suggested change
class CheckFailedException(commands.CommandError):
"""Custom exception to help identify which check function failed."""
def __init__(self, check_name: str):
"""Initialise with the name of the check."""
super().__init__()
self.check_name = check_name
class CheckAdminOrAcademic(commands.CheckFailure):
pass
class CheckAdmin(CheckAdminOrAcademic):
pass
class CheckAcademic(CheckAdminOrAcademic):
pass

This way isinstance(error, CheckAdmin) can detect admin command failure, isinstance(error, CheckAcademic) can detect academic server error, and isinstance(error, CheckAdminOrAcademic) can be used to detect either of above

is a bot admin or not."""
author = ctx.message.author
if not is_bot_admin(author.id):
raise CheckFailedException("is_author_bot_admin")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise CheckFailedException("is_author_bot_admin")
raise CheckAdmin()

Comment on lines +270 to +273
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_author_bot_admin"
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_author_bot_admin"
):
if isinstance(error, CheckAdmin):

Comment on lines +286 to +292
author = ctx.message.author
is_admin = is_bot_admin(author.id)
if not server_configs[ctx.guild.id]["is_academic"] and not is_admin:
raise CheckFailedException("is_academic_or_bot_admin")
return True
except KeyError as err:
raise CheckFailedException("server_config") from err
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to leave this function as it is, as is_academic, and instead define a new function

def is_academic_or_bot_admin(ctx: commands.Context):
    """Checks if the server is an academic server or if the author is a bot admin."""
    return is_author_bot_admin(ctx) or is_academic(ctx)

Of course, you would need to modify is_academic to raise CheckAcademic errors instead of returning False

Copy link
Member

Choose a reason for hiding this comment

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

I just realised that @commands.check_any exists. We can make the implementation much simpler with this!

Comment on lines +333 to +336
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_academic_or_bot_admin"
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_academic_or_bot_admin"
):
if isinstance(error, CheckAdminOrAcademic):

Comment on lines +389 to +392
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_academic_or_bot_admin"
):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (
isinstance(error, CheckFailedException)
and error.check_name == "is_academic_or_bot_admin"
):
if isinstance(error, CheckAdminOrAcademic):

Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

I have done some experimenting and have simplified the implementation a bit.

Thanks for contributing, I will merge your work onto a new branch, and push my changes on top of that!

@ankith26 ankith26 changed the base branch from master to admin-command June 5, 2024 14:18
@ankith26 ankith26 merged commit cf3adf4 into OSDG-IIITH:admin-command Jun 5, 2024
1 check passed
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