aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat (limited to '')
-rw-r--r--bot/exts/backend/error_handler.py4
-rw-r--r--bot/exts/recruitment/talentpool/_cog.py68
-rw-r--r--bot/exts/recruitment/talentpool/_review.py138
-rw-r--r--tests/bot/exts/backend/test_error_handler.py3
-rw-r--r--tests/bot/exts/moderation/recruitment/__init__.py0
-rw-r--r--tests/bot/exts/moderation/recruitment/talentpool/test_review.py153
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])
generated by cgit v1.2.3 (git 2.39.1) at 2025-10-31 20:04:46 +0000