add new admin secret santa commands#37
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds new admin commands for managing the Secret Santa (Amigo Secreto) feature, including automated assignment of participants and changing authorization from owner-only to admin-accessible commands.
- Changed command authorization from
@commands.is_owner()to@commands.check(is_admin)to allow admin users to execute commands - Added
roll_amigo_secretocommand to automatically assign Secret Santa participants with retry logic - Implemented helper methods
clear_secret_santa(),roll_secret_santa(), andrandom_person_to()for managing assignments
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| receiving=False | ||
| ).filter( |
There was a problem hiding this comment.
The filter receiving=False is redundant on line 127 since it's already applied again in line 130. You can simplify this by removing the first .filter(receiving=False) and using just:
random_person: AmigoSecretoPerson = AmigoSecretoPerson.objects.filter(
Q(receiving=False) & ~Q(id=pk) & ~Q(id__in=exclude)
).order_by('?').first()| receiving=False | |
| ).filter( |
| random_person.receiving = True | ||
| random_person.save() | ||
|
|
There was a problem hiding this comment.
The query result could be None if no valid person is found (e.g., in edge cases or when the last person to be assigned has no valid recipient). Attempting to set random_person.receiving = True on line 133 will raise an AttributeError. You should add a null check and handle this error case appropriately:
random_person = AmigoSecretoPerson.objects.filter(...).first()
if not random_person:
raise Exception("No valid recipient found for Secret Santa assignment")
random_person.receiving = True
random_person.save()| random_person.receiving = True | |
| random_person.save() | |
| if not random_person: | |
| raise Exception("No valid recipient found for Secret Santa assignment") | |
| random_person.receiving = True | |
| random_person.save() |
| except Exception: | ||
| attempts += 1 |
There was a problem hiding this comment.
The bare except Exception catches all exceptions without logging or understanding what went wrong. This makes debugging difficult. Consider at least logging the exception or catching more specific exceptions:
except Exception as e:
attempts += 1
self.bot.logger.error(f"Error rolling secret santa: {e}")
await ctx.author.send('Erro ao montar Amigo Secreto, limpando e tentando novamente')
self.clear_secret_santa()| except Exception: | |
| attempts += 1 | |
| except Exception as e: | |
| attempts += 1 | |
| self.bot.logger.error(f"Error rolling secret santa: {e}", exc_info=True) |
| @commands.check(is_admin) | ||
| @commands.command() | ||
| async def roll_amigo_secreto(self, ctx: Context): | ||
| secret_santa_state: AmigoSecretoState = AmigoSecretoState.object() |
There was a problem hiding this comment.
Inconsistent method call: AmigoSecretoState.object() is called here, but elsewhere in the same file (lines 76, 177, 190) AmigoSecretoState.objects.first() is used. The method should be consistent throughout the file. Based on the pattern used in admin.py (line 512), .object() appears to be a custom method, but you should verify this is the correct method to use.
| secret_santa_state: AmigoSecretoState = AmigoSecretoState.object() | |
| secret_santa_state: AmigoSecretoState = AmigoSecretoState.objects.first() |
| for person in AmigoSecretoPerson.objects.filter(giving_to_user=None).all(): | ||
| random = self.random_person_to(person.id, exclude) | ||
|
|
||
| self.logger.info(person.user.ingame_name + ' is giving to ' + random.user.ingame_name) |
There was a problem hiding this comment.
self.logger is not defined in this class. The AmigoSecreto class does not initialize a logger in its __init__ method. You need to either:
- Add
self.logger = logging.getLogger("amigosecreto")in the__init__method, or - Use the bot's logger, or
- Remove this logging statement
Other cogs in the codebase (e.g., authentication.py, error_handler.py) initialize their loggers in __init__.
| random = self.random_person_to(person.id, exclude) | ||
|
|
||
| self.logger.info(person.user.ingame_name + ' is giving to ' + random.user.ingame_name) | ||
|
|
||
| person.giving_to_user = random.user | ||
| person.save() | ||
| exclude.append(random.id) |
There was a problem hiding this comment.
The variable name random shadows the built-in Python module name. Consider renaming it to something more descriptive like random_recipient or target_person.
| random = self.random_person_to(person.id, exclude) | |
| self.logger.info(person.user.ingame_name + ' is giving to ' + random.user.ingame_name) | |
| person.giving_to_user = random.user | |
| person.save() | |
| exclude.append(random.id) | |
| random_recipient = self.random_person_to(person.id, exclude) | |
| self.logger.info(person.user.ingame_name + ' is giving to ' + random_recipient.user.ingame_name) | |
| person.giving_to_user = random_recipient.user | |
| person.save() | |
| exclude.append(random_recipient.id) |
No description provided.