diff options
| -rw-r--r-- | bot/exts/backend/error_handler.py | 4 | ||||
| -rw-r--r-- | bot/exts/recruitment/talentpool/_cog.py | 68 | ||||
| -rw-r--r-- | bot/exts/recruitment/talentpool/_review.py | 138 | ||||
| -rw-r--r-- | tests/bot/exts/backend/test_error_handler.py | 3 | ||||
| -rw-r--r-- | tests/bot/exts/moderation/recruitment/__init__.py | 0 | ||||
| -rw-r--r-- | tests/bot/exts/moderation/recruitment/talentpool/test_review.py | 153 |
6 files changed, 284 insertions, 82 deletions
diff --git a/bot/exts/backend/error_handler.py b/bot/exts/backend/error_handler.py index 761991488..5c4aaf29f 100644 --- a/bot/exts/backend/error_handler.py +++ b/bot/exts/backend/error_handler.py @@ -75,7 +75,7 @@ class ErrorHandler(Cog): elif isinstance(e, errors.CheckFailure): log.debug(debug_message) await self.handle_check_failure(ctx, e) - elif isinstance(e, errors.CommandOnCooldown): + elif isinstance(e, (errors.CommandOnCooldown, errors.MaxConcurrencyReached)): log.debug(debug_message) await ctx.send(e) elif isinstance(e, errors.CommandInvokeError): @@ -95,7 +95,7 @@ class ErrorHandler(Cog): elif isinstance(e, errors.DisabledCommand): log.debug(debug_message) else: - # MaxConcurrencyReached, ExtensionError + # ExtensionError await self.handle_unexpected_error(ctx, e) async def send_command_help(self, ctx: Context) -> None: diff --git a/bot/exts/recruitment/talentpool/_cog.py b/bot/exts/recruitment/talentpool/_cog.py index 9819152b0..f69e5a647 100644 --- a/bot/exts/recruitment/talentpool/_cog.py +++ b/bot/exts/recruitment/talentpool/_cog.py @@ -1,3 +1,4 @@ +import asyncio import textwrap from collections import ChainMap, defaultdict from io import StringIO @@ -6,8 +7,8 @@ from typing import Optional, Union import discord from async_rediscache import RedisCache from botcore.site_api import ResponseCodeError -from botcore.utils import scheduling from discord import Color, Embed, Member, PartialMessage, RawReactionActionEvent, User +from discord.ext import commands, tasks from discord.ext.commands import BadArgument, Cog, Context, group, has_any_role from bot.bot import Bot @@ -34,21 +35,19 @@ class TalentPool(Cog, name="Talentpool"): def __init__(self, bot: Bot) -> None: self.bot = bot - self.reviewer = Reviewer(self.__class__.__name__, bot, self) + self.reviewer = Reviewer(bot, self) self.cache: Optional[defaultdict[dict]] = None self.api_default_params = {'active': 'true', 'ordering': '-inserted_at'} - self.initial_refresh_task = scheduling.create_task(self.refresh_cache(), event_loop=self.bot.loop) - scheduling.create_task(self.schedule_autoreviews(), event_loop=self.bot.loop) + # This lock lets us avoid cancelling the reviewer loop while the review code is running. + self.autoreview_lock = asyncio.Lock() + + async def cog_load(self) -> None: + """Load user cache and maybe start autoreview loop.""" + await self.refresh_cache() - async def schedule_autoreviews(self) -> None: - """Reschedule reviews for active nominations if autoreview is enabled.""" if await self.autoreview_enabled(): - # Wait for a populated cache first - await self.initial_refresh_task - await self.reviewer.reschedule_reviews() - else: - log.trace("Not scheduling reviews as autoreview is disabled.") + self.autoreview_loop.start() async def autoreview_enabled(self) -> bool: """Return whether automatic posting of nomination reviews is enabled.""" @@ -89,31 +88,42 @@ class TalentPool(Cog, name="Talentpool"): @nomination_autoreview_group.command(name="enable", aliases=("on",)) @has_any_role(Roles.admins) + @commands.max_concurrency(1) async def autoreview_enable(self, ctx: Context) -> None: """ Enable automatic posting of reviews. - This will post reviews up to one day overdue. Older nominations can be - manually reviewed with the `tp post_review <user_id>` command. + A review will be posted when the current number of active reviews is below the limit + and long enough has passed since the last review. + + Users will be considered for review if they have been in the talent pool past a + threshold time. + + The next user to review is chosen based on the number of nominations a user has, + using the age of the first nomination as a tie-breaker (oldest first). """ if await self.autoreview_enabled(): await ctx.send(":x: Autoreview is already enabled.") return + self.autoreview_loop.start() await self.talentpool_settings.set(AUTOREVIEW_ENABLED_KEY, True) - await self.reviewer.reschedule_reviews() await ctx.send(":white_check_mark: Autoreview enabled.") @nomination_autoreview_group.command(name="disable", aliases=("off",)) @has_any_role(Roles.admins) + @commands.max_concurrency(1) async def autoreview_disable(self, ctx: Context) -> None: """Disable automatic posting of reviews.""" if not await self.autoreview_enabled(): await ctx.send(":x: Autoreview is already disabled.") return + # Only cancel the loop task when the autoreview code is not running + async with self.autoreview_lock: + self.autoreview_loop.cancel() + await self.talentpool_settings.set(AUTOREVIEW_ENABLED_KEY, False) - self.reviewer.cancel_all() await ctx.send(":white_check_mark: Autoreview disabled.") @nomination_autoreview_group.command(name="status") @@ -125,6 +135,16 @@ class TalentPool(Cog, name="Talentpool"): else: await ctx.send("Autoreview is currently disabled.") + @tasks.loop(hours=1) + async def autoreview_loop(self) -> None: + """Send request to `reviewer` to send a nomination if ready.""" + if not await self.autoreview_enabled(): + return + + async with self.autoreview_lock: + log.info("Running check for users to nominate.") + await self.reviewer.maybe_review_user() + @nomination_group.command( name="nominees", aliases=("nominated", "all", "list", "watched"), @@ -156,8 +176,8 @@ class TalentPool(Cog, name="Talentpool"): """ Gives an overview of the nominated users list. - It specifies the users' mention, name, how long ago they were nominated, and whether their - review was scheduled or already posted. + It specifies the user's mention, name, how long ago they were nominated, and whether their + review was posted. The optional kwarg `oldest_first` orders the list by oldest entry. @@ -186,8 +206,6 @@ class TalentPool(Cog, name="Talentpool"): line = f"~~{line}~~" if user_data['reviewed']: line += " *(reviewed)*" - elif user_id in self.reviewer: - line += " *(scheduled)*" lines.append(line) if not lines: @@ -289,9 +307,6 @@ class TalentPool(Cog, name="Talentpool"): self.cache[user.id] = response_data - if await self.autoreview_enabled() and user.id not in self.reviewer: - self.reviewer.schedule_review(user.id) - await ctx.send(f"✅ The nomination for {user.mention} has been added to the talent pool.") @nomination_group.command(name='history', aliases=('info', 'search')) @@ -548,9 +563,6 @@ class TalentPool(Cog, name="Talentpool"): ) self.cache.pop(user_id) - if await self.autoreview_enabled(): - self.reviewer.cancel(user_id) - return True async def _nomination_to_string(self, nomination_object: dict) -> str: @@ -604,5 +616,7 @@ class TalentPool(Cog, name="Talentpool"): return lines.strip() async def cog_unload(self) -> None: - """Cancels all review tasks on cog unload.""" - self.reviewer.cancel_all() + """Cancels the autoreview loop on cog unload.""" + # Only cancel the loop task when the autoreview code is not running + async with self.autoreview_lock: + self.autoreview_loop_lock.cancel() diff --git a/bot/exts/recruitment/talentpool/_review.py b/bot/exts/recruitment/talentpool/_review.py index b6abdd24f..f045d6d9c 100644 --- a/bot/exts/recruitment/talentpool/_review.py +++ b/bot/exts/recruitment/talentpool/_review.py @@ -5,12 +5,10 @@ import re import textwrap import typing from collections import Counter -from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone from typing import List, Optional, Union -import arrow from botcore.site_api import ResponseCodeError -from botcore.utils.scheduling import Scheduler from dateutil.parser import isoparse from discord import Embed, Emoji, Member, Message, NotFound, PartialMessage, TextChannel from discord.ext.commands import Context @@ -28,14 +26,18 @@ if typing.TYPE_CHECKING: log = get_logger(__name__) -# Maximum amount of days before an automatic review is posted. -MAX_DAYS_IN_POOL = 30 - # Maximum amount of characters allowed in a message MAX_MESSAGE_SIZE = 2000 # Maximum amount of characters allowed in an embed MAX_EMBED_SIZE = 4000 +# Maximum number of active reviews +MAX_ONGOING_REVIEWS = 4 +# Minimum time between reviews +MIN_REVIEW_INTERVAL = timedelta(days=1) +# Minimum time between nomination and sending a review +MIN_NOMINATION_TIME = timedelta(days=7) + # Regex for finding the first message of a nomination, and extracting the nominee. NOMINATION_MESSAGE_REGEX = re.compile( r"<@!?(\d+)> \(.+#\d{4}\) for Helper!\n\n", @@ -44,37 +46,93 @@ NOMINATION_MESSAGE_REGEX = re.compile( class Reviewer: - """Schedules, formats, and publishes reviews of helper nominees.""" + """Manages, formats, and publishes reviews of helper nominees.""" - def __init__(self, name: str, bot: Bot, pool: 'TalentPool'): + def __init__(self, bot: Bot, pool: 'TalentPool'): self.bot = bot self._pool = pool - self._review_scheduler = Scheduler(name) - def __contains__(self, user_id: int) -> bool: - """Return True if the user with ID user_id is scheduled for review, False otherwise.""" - return user_id in self._review_scheduler + async def maybe_review_user(self) -> bool: + """ + Checks if a new vote should be triggered, and triggers one if ready. + + Returns a boolean representing whether a new vote was sent or not. + """ + if not await self.is_ready_for_review(): + return False + + user = await self.get_user_for_review() + if not user: + return False + + await self.post_review(user, True) + return True + + async def is_ready_for_review(self) -> bool: + """ + Returns a boolean representing whether a new vote should be triggered. + + The criteria for this are: + - The current number of reviews is lower than `MAX_ONGOING_REVIEWS`. + - The most recent review was sent less than `MIN_REVIEW_INTERVAL` ago. + """ + voting_channel = self.bot.get_channel(Channels.nomination_voting) + + review_count = 0 + is_first_message = True + async for msg in voting_channel.history(): + # Try and filter out any non-review messages. We also only want to count + # one message from reviews split over multiple messages. We use fixed text + # from the start as any later text could be split over messages. + if not msg.author.bot or "for Helper!" not in msg.content: + continue + + if is_first_message: + if msg.created_at > datetime.now(timezone.utc) - MIN_REVIEW_INTERVAL: + log.debug("Most recent review was less than %s ago, cancelling check", MIN_REVIEW_INTERVAL) + return False + + is_first_message = False - async def reschedule_reviews(self) -> None: - """Reschedule all active nominations to be reviewed at the appropriate time.""" - log.trace("Rescheduling reviews") - await self.bot.wait_until_guild_available() + review_count += 1 + if review_count >= MAX_ONGOING_REVIEWS: + log.debug("There are already at least %s ongoing reviews, cancelling check.", MAX_ONGOING_REVIEWS) + return False + + return True + + async def get_user_for_review(self) -> Optional[int]: + """ + Returns the user ID of the next user to review, or None if there are no users ready. + + Users will only be selected for review if: + - They have not already been reviewed. + - They have been nominated for longer than `MIN_NOMINATION_TIME`. + + The priority of the review is determined by how many nominations the user has + (more nominations = higher priority). + For users with equal priority the oldest nomination will be reviewed first. + """ + possible = [] for user_id, user_data in self._pool.cache.items(): - if not user_data["reviewed"]: - self.schedule_review(user_id) + if ( + not user_data["reviewed"] + and isoparse(user_data["inserted_at"]) < datetime.now(timezone.utc) - MIN_NOMINATION_TIME + ): + possible.append((user_id, user_data)) + + if not possible: + log.debug("No users ready to review.") + return None - def schedule_review(self, user_id: int) -> None: - """Schedules a single user for review.""" - log.trace(f"Scheduling review of user with ID {user_id}") + # Secondary sort key: creation of first entries on the nomination. + possible.sort(key=lambda x: isoparse(x[1]["inserted_at"])) - user_data = self._pool.cache.get(user_id) - inserted_at = isoparse(user_data['inserted_at']) - review_at = inserted_at + timedelta(days=MAX_DAYS_IN_POOL) + # Primary sort key: number of entries on the nomination. + user = max(possible, key=lambda x: len(x[1]["entries"])) - # If it's over a day overdue, it's probably an old nomination and shouldn't be automatically reviewed. - if arrow.utcnow() - review_at < timedelta(days=1): - self._review_scheduler.schedule_at(review_at, user_id, self.post_review(user_id, update_database=True)) + return user[0] # user id async def post_review(self, user_id: int, update_database: bool) -> None: """Format the review of a user and post it to the nomination voting channel.""" @@ -85,7 +143,7 @@ class Reviewer: guild = self.bot.get_guild(Guild.id) channel = guild.get_channel(Channels.nomination_voting) - log.trace(f"Posting the review of {nominee} ({nominee.id})") + log.info(f"Posting the review of {nominee} ({nominee.id})") messages = await self._bulk_send(channel, review) await pin_no_system_message(messages[0]) @@ -142,7 +200,7 @@ class Reviewer: reviewed_emoji = self._random_ducky(guild) vote_request = ( - "*Refer to their nomination and infraction histories for further details*.\n" + "*Refer to their nomination and infraction histories for further details.*\n" f"*Please react {reviewed_emoji} once you have reviewed this user," " and react :+1: for approval, or :-1: for disapproval*." ) @@ -424,27 +482,5 @@ class Reviewer: return False await self.bot.api_client.patch(f"bot/nominations/{nomination['id']}", json={"reviewed": True}) - if user_id in self._review_scheduler: - self._review_scheduler.cancel(user_id) return True - - def cancel(self, user_id: int) -> None: - """ - Cancels the review of the nominee with ID `user_id`. - - It's important to note that this applies only until reschedule_reviews is called again. - To permanently cancel someone's review, either remove them from the pool, or use mark_reviewed. - """ - log.trace(f"Canceling the review of user {user_id}.") - self._review_scheduler.cancel(user_id) - - def cancel_all(self) -> None: - """ - Cancels all reviews. - - It's important to note that this applies only until reschedule_reviews is called again. - To permanently cancel someone's review, either remove them from the pool, or use mark_reviewed. - """ - log.trace("Canceling all reviews.") - self._review_scheduler.cancel_all() diff --git a/tests/bot/exts/backend/test_error_handler.py b/tests/bot/exts/backend/test_error_handler.py index 7562f6aa8..6b38c43fc 100644 --- a/tests/bot/exts/backend/test_error_handler.py +++ b/tests/bot/exts/backend/test_error_handler.py @@ -167,8 +167,7 @@ class ErrorHandlerTests(unittest.IsolatedAsyncioTestCase): """Should call `handle_unexpected_error` if error is `MaxConcurrencyReached` or `ExtensionError`.""" self.cog.handle_unexpected_error = AsyncMock() errs = ( - errors.MaxConcurrencyReached(1, MagicMock()), - errors.ExtensionError(name="foo") + errors.ExtensionError(name="foo"), ) for err in errs: diff --git a/tests/bot/exts/moderation/recruitment/__init__.py b/tests/bot/exts/moderation/recruitment/__init__.py new file mode 100644 index 000000000..e69de29bb --- /dev/null +++ b/tests/bot/exts/moderation/recruitment/__init__.py diff --git a/tests/bot/exts/moderation/recruitment/talentpool/test_review.py b/tests/bot/exts/moderation/recruitment/talentpool/test_review.py new file mode 100644 index 000000000..444724b9d --- /dev/null +++ b/tests/bot/exts/moderation/recruitment/talentpool/test_review.py @@ -0,0 +1,153 @@ +import unittest +from datetime import datetime, timedelta, timezone +from unittest.mock import Mock, patch + +from bot.exts.recruitment.talentpool import _review +from tests.helpers import MockBot, MockMember, MockMessage, MockTextChannel + + +class AsyncIterator: + """Normal->Async iterator helper.""" + + def __init__(self, seq): + self.iter = iter(seq) + + def __aiter__(self): + return self + + # Allows it to be used to mock the discord TextChannel.history function + def __call__(self): + return self + + async def __anext__(self): + try: + return next(self.iter) + except StopIteration: + raise StopAsyncIteration + + +def nomination(inserted_at, num_entries, reviewed=False): + return ( + MockMember().id, + {"inserted_at": inserted_at.isoformat(), "entries": [Mock() for _ in range(num_entries)], "reviewed": reviewed}, + ) + + +class ReviewerTests(unittest.IsolatedAsyncioTestCase): + """Tests for the talentpool reviewer.""" + + def setUp(self): + self.bot_user = MockMember(bot=True) + self.bot = MockBot(user=self.bot_user) + + self.voting_channel = MockTextChannel() + self.bot.get_channel = Mock(return_value=self.voting_channel) + + self.pool = Mock(name="MockTalentPool") + self.reviewer = _review.Reviewer(self.bot, self.pool) + + @patch("bot.exts.recruitment.talentpool._review.MAX_ONGOING_REVIEWS", 3) + @patch("bot.exts.recruitment.talentpool._review.MIN_REVIEW_INTERVAL", timedelta(days=1)) + async def test_is_ready_for_review(self): + """Tests for the `is_ready_for_review` function.""" + too_recent = datetime.now(timezone.utc) - timedelta(hours=1) + not_too_recent = datetime.now(timezone.utc) - timedelta(days=7) + cases = ( + # Only one review, and not too recent, so ready. + ( + [ + MockMessage(author=self.bot_user, content="wookie for Helper!", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="Not a review", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="Not a review", created_at=not_too_recent), + ], + True, + ), + + # Three reviews, so not ready. + ( + [ + MockMessage(author=self.bot_user, content="Chrisjl for Helper!", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="Zig for Helper!", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="Scaleios for Helper!", created_at=not_too_recent), + ], + False, + ), + + # Only one review, but too recent, so not ready. + ( + [ + MockMessage(author=self.bot_user, content="Chrisjl for Helper!", created_at=too_recent), + ], + False, + ), + + # Only two reviews, and not too recent, so ready. + ( + [ + MockMessage(author=self.bot_user, content="Not a review", created_at=too_recent), + MockMessage(author=self.bot_user, content="wookie for Helper!", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="wookie for Helper!", created_at=not_too_recent), + MockMessage(author=self.bot_user, content="Not a review", created_at=not_too_recent), + ], + True, + ), + + # No messages, so ready. + ([], True), + ) + + for messages, expected in cases: + with self.subTest(messages=messages, expected=expected): + self.voting_channel.history = AsyncIterator(messages) + res = await self.reviewer.is_ready_for_review() + self.assertIs(res, expected) + + @patch("bot.exts.recruitment.talentpool._review.MIN_NOMINATION_TIME", timedelta(days=7)) + async def test_get_user_for_review(self): + """Test get_user_for_review function.""" + now = datetime.now(timezone.utc) + + # Each case contains a list of nominations, followed by the index in that list + # of the one that should be selected, or None if None should be returned + cases = [ + # One nomination, too recent so don't send. + ( + [ + nomination(now - timedelta(days=1), 5), + ], + None, + ), + + # First one has most entries so should be returned. + ( + [ + nomination(now - timedelta(days=10), 6), + nomination(now - timedelta(days=10), 5), + nomination(now - timedelta(days=9), 5), + nomination(now - timedelta(days=11), 5), + ], + 0, + ), + + # Same number of entries so oldest (second) should be returned. + ( + [ + nomination(now - timedelta(days=1), 2), + nomination(now - timedelta(days=80), 2), + nomination(now - timedelta(days=79), 2), + ], + 1, + ), + ] + + for nominations, expected in cases: + nomination_dict = dict(nominations) + + with self.subTest(nominations=nominations, expected=expected): + self.pool.cache = nomination_dict + res = await self.reviewer.get_user_for_review() + + if expected is None: + self.assertIsNone(res) + else: + self.assertEqual(res, nominations[expected][0]) |