Skip to content

Manage nomination threads in bot#1928

Merged
Akarys42 merged 10 commits into
mainfrom
kill-sir-threadevere
Dec 1, 2021
Merged

Manage nomination threads in bot#1928
Akarys42 merged 10 commits into
mainfrom
kill-sir-threadevere

Conversation

@ChrisLovering
Copy link
Copy Markdown
Member

These changes moves the management of nomination threads from Sir Threadevere to the bot. Once this PR is merged, Sir Threadevere can be shutdown.

These changes include creating a thread while posting the nomination, pinging staff inside the thread, and then archiving it once the nomination is concluded.

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.

It would be nice if we could also add a listener on this cog automatically unarchiving threads if they get archived through a timeout and not manually

Comment thread bot/exts/recruitment/talentpool/_review.py Outdated
Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

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

This looks good. I would leave an approval, but I have a few comments which may be nice to address or not.

Comment thread bot/exts/recruitment/talentpool/_review.py Outdated
Comment thread bot/exts/recruitment/talentpool/_review.py
Comment thread bot/exts/recruitment/talentpool/_review.py Outdated
@Xithrius Xithrius added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: recruitment Related to recruitment: (talentpool) p: 2 - normal Normal Priority s: needs review Author is waiting for someone to review and approve t: feature New feature or request labels Nov 7, 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.

I ran into an error when trying to archive

pythondiscord-bot-1        | 2021-11-20 17:07:28 | bot | ERROR | Unhandled exception in on_raw_reaction_add.
pythondiscord-bot-1        | Traceback (most recent call last):
pythondiscord-bot-1        |   File "/usr/local/lib/python3.9/site-packages/discord/client.py", line 351, in _run_event
pythondiscord-bot-1        |     await coro(*args, **kwargs)
pythondiscord-bot-1        |   File "/bot/bot/exts/recruitment/talentpool/_cog.py", line 526, in on_raw_reaction_add
pythondiscord-bot-1        |     await self.reviewer.archive_vote(message, emoji == Emojis.incident_actioned)
pythondiscord-bot-1        |   File "/bot/bot/exts/recruitment/talentpool/_review.py", line 171, in archive_vote
pythondiscord-bot-1        |     user_id = int(NOMINATION_MESSAGE_REGEX.search(content).group(1))
pythondiscord-bot-1        | ValueError: invalid literal for int() with base 10: 'Chrisjl'

The regex doesn't seem right. I'd also remove the **Nominated by:** literal, to try to not make manual reviews fail. Also maybe we should properly handle that error and send an error message somewhere?

@ChrisLovering ChrisLovering force-pushed the kill-sir-threadevere branch 2 times, most recently from 9807bc7 to 73acf5f Compare November 20, 2021 17:12
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.

Excellent :shipit:

@Akarys42 Akarys42 enabled auto-merge November 20, 2021 17:15
@Akarys42 Akarys42 requested a review from jchristgit November 20, 2021 17:23
Copy link
Copy Markdown
Contributor

@janine9vn janine9vn left a comment

Choose a reason for hiding this comment

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

This works well! (After I lost a year of my life debugging emojis).

Approving this since you've already fixed my comments.

Goodbye Sir Threadevere!

@@ -95,6 +94,12 @@ async def post_review(self, user_id: int, update_database: bool) -> None:
for reaction in (reviewed_emoji, "\N{THUMBS UP SIGN}", "\N{THUMBS DOWN SIGN}"):
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.

This is a general comment, but the reviewed_emoji will almost always return a valid ducky. But if it doesn't find a ducky for whatever reason, it will return ":eyes:" and that will fail when it tries to add it as a reaction.

Doesn't need to be fixed in this PR since it's out of scope, but that was Not Fun debugging.

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 has been fixed in 4bde791

Comment on lines +80 to +87
review, reviewed_emoji, nominee = await self.make_review(user_id)
if not review:
return

guild = self.bot.get_guild(Guild.id)
channel = guild.get_channel(Channels.nomination_voting)

log.trace(f"Posting the review of {user_id}")
log.trace(f"Posting the review of {nominee} ({nominee.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.

So! This is fun. The addition of nominee.id will no longer make this fail silently if a user leaves the guild after they were nominated but before they're up for a vote.

self.make_review() returns (string, None, None). review is therefore a non-empty string and that short-circuit logic there won't work anymore. nominee.id now tries to access the id attribute which a NoneType doesn't have.

So, the if not review should explicitly check for the nominee, not the review.

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.

Fixed in 769660b

This change creates a thread while posting the nomination, and then archives it once the nomination is concluded.
This supresses both the mesage deleteions and the thread archive, so that if they are removed before the code can get to them, it does not raise an error.
This is most relevant in local dev testing where the Emojis.check_mark could be the same as the Emojis.incident_actioned or Emojis.incident_unactioned, which would cause the bot to attempt to archive the post_review invocation if it was posted in the nomination voting channel.
Using a 👀 style emoji string in a ctx.add_reaciton call will error. Discord expects either a unicode emoji, or a custom emoji.
@Akarys42 Akarys42 merged commit 762dca1 into main Dec 1, 2021
@Akarys42 Akarys42 deleted the kill-sir-threadevere branch December 1, 2021 01:13
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label Dec 15, 2021
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: recruitment Related to recruitment: (talentpool) p: 2 - normal Normal Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants