-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
Create an admin task when votes are 2 weeks old #2323
Create an admin task when votes are 2 weeks old #2323
Conversation
…old-votes-in-github
This will allow us to avoid looking for the message again since we'll need it to add a reaction
@ChrisLovering Here's a draft PR for #2226. The only missing part is the Github issue part since i'm not quite aware of the repo/project structure for admins. Note |
Fwiw, you can target one PR to another branch (i.e it would look something like What that would do is basically try applying the changes from the final branch (3) in this chain to the middle branch (2), so when you check out the 3rd branch, it would have all the changes from branch 2 as well, but the PR itself will only have the changes of branch 3. For the admin repo, there's an issue template which Chris can probably fetch for ya. The template body is completely unmodified at issue creation, we just set the nominated user as the issue title. You can use this API to create the issue (as far as I can tell the API doesn't expose a way to create an issue from a template), which you'll need a token with access for. Simply add that as an environment variable in the default-config file (example), and the devops team will worry about getting it set, just ping us to do so when the PR is ready. Also please have a sensible default behavior for development environments where that token is unlikely to be set/exist. |
@HassanAbouelela Thanks for the tip, I didn't know that was feasible. I see that we already have a keys section in our config, and we have a GitHub key as well but I think we'll need a new one because of permission levels, so I'll add a new key for it. As for the default behavior, I have just thought of a warning log and an early return when that value of that key is null. |
A couple of points to discuss when I publish this PR:
The issue template/content/title is still to be discussed |
threads = [await get_or_fetch_channel(nomination.thread_id) for nomination in history] | ||
|
||
nomination_vote_threads = ", ".join( | ||
[ | ||
f"[Thread-{thread_number}]({thread.jump_url})" if thread else '' | ||
for thread_number, thread in enumerate(threads, start=1) | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem related to the issue, so might be worth splitting to another PR, since there's some more logic you'll need here.
get_or_fetch_channel
will raise an error if the channel isn't found, rather than returning None that you are assuming here.
In your nomination_vote_threads
list comp if the thread is non, you're adding ''
, which will result in odd output since, it will still add the comma separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is due to the fact that my other PR that hasn't been merged is needed, so I merged it here.
When it does get merged, I'll clean up the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think commit e68d756 should hold the answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now that my previously mentioned PR has been merged, these changes no longer exist
Merge happened here 59319b4
thread = None | ||
|
||
if nomination.thread_id: | ||
thread = await get_or_fetch_channel(nomination.thread_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handle the case where this raises an error if the thread doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually handled in the original PR that adds the thread id to the nominations table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be visible in e68d756
if issue_created: | ||
await nomination_vote_message.add_reaction(FLAG_EMOJI) | ||
|
||
async def _get_forgotten_nominations(self) -> List[Nomination]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def _get_forgotten_nominations(self) -> List[Nomination]: | |
async def _get_forgotten_nominations(self) -> list[Nomination]: |
This can be done since Python 3.9. There are more examples in this PR where typing
is used where built-ins could be used instead, please replace them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 68c625c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't resolved.
self, | ||
nominations: List[Nomination] | ||
) -> List[Tuple[Nomination, discord.Message]]: | ||
"""Filter the forgotten nominations that are still untracked in GitHub.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't just a filter, since it returns more data that it was given. This should be described in the docstring, and maybe change the function name to make it clear it isn't just a filter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alrighty, fixed in 1c8059a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't resolved.
Returns True when the issue has been created, False otherwise. | ||
""" | ||
if not GithubAdminRepo.token: | ||
log.warn(f"No token for the {GithubAdminRepo.name} repository was provided, skipping issue creation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.warn(f"No token for the {GithubAdminRepo.name} repository was provided, skipping issue creation.") | |
log.warning(f"No token for the {GithubAdminRepo.name} repository was provided, skipping issue creation.") |
warn is a deprecated alias to warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3ca37a0
|
…ng tracked nominations
# Conflicts: # bot/exts/recruitment/talentpool/_cog.py # bot/exts/recruitment/talentpool/_review.py
@@ -43,6 +49,7 @@ def __init__(self, bot: Bot) -> None: | |||
|
|||
async def cog_load(self) -> None: | |||
"""Start autoreview loop if enabled.""" | |||
self.track_forgotten_nominations.start() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this configurable to run just like we do for autoreview ?
threads = [await get_or_fetch_channel(nomination.thread_id) for nomination in history] | ||
|
||
nomination_vote_threads = ", ".join( | ||
[ | ||
f"[Thread-{thread_number}]({thread.jump_url})" if thread else '' | ||
for thread_number, thread in enumerate(threads, start=1) | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is due to the fact that my other PR that hasn't been merged is needed, so I merged it here.
When it does get merged, I'll clean up the history.
starter_message = thread.starter_message | ||
if not starter_message: | ||
# Starter message will be null if it's not cached | ||
starter_message = await self.bot.get_channel(Channels.nomination_voting).fetch_message(thread.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5a916ac
Returns True when the issue has been created, False otherwise. | ||
""" | ||
if not GithubAdminRepo.token: | ||
log.warn(f"No token for the {GithubAdminRepo.name} repository was provided, skipping issue creation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3ca37a0
thread = None | ||
|
||
if nomination.thread_id: | ||
thread = await get_or_fetch_channel(nomination.thread_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be visible in e68d756
threads = [await get_or_fetch_channel(nomination.thread_id) for nomination in history] | ||
|
||
nomination_vote_threads = ", ".join( | ||
[ | ||
f"[Thread-{thread_number}]({thread.jump_url})" if thread else '' | ||
for thread_number, thread in enumerate(threads, start=1) | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think commit e68d756 should hold the answer
threads = [await get_or_fetch_channel(nomination.thread_id) for nomination in history] | ||
|
||
nomination_vote_threads = ", ".join( | ||
[ | ||
f"[Thread-{thread_number}]({thread.jump_url})" if thread else '' | ||
for thread_number, thread in enumerate(threads, start=1) | ||
] | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now that my previously mentioned PR has been merged, these changes no longer exist
Merge happened here 59319b4
This relies on turning the thread snowflake id into a date time We then use that for comparison, since an existing active thread id means an existing review
Closing in favor of #2553 |
closes #2226
The idea is to fetch active nominations that are more than 2 weeks old, and create admin issues for them in Github so that staff will do the follow up.
As soon as a vote is tracked, we add a "🎫" reaction (which can change) to it to make sure we don't create duplicate issues for it, instead of keeping track in another way