Merge pull request #2245 from python-discord/update-autoreview-system

Update autoreview system

8 files changed, 385 insertions, 85 deletions

diff --git a/bot/exts/backend/error_handler.py b/bot/exts/backend/error_handler.py
index a23d7c9fc..f9ded79f0 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..e3ac1086d 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,22 @@ 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 = 3
+# Minimum time between reviews
+MIN_REVIEW_INTERVAL = timedelta(days=1)
+# Minimum time between nomination and sending a review
+MIN_NOMINATION_TIME = timedelta(days=7)
+
+# A constant for weighting number of nomination entries against nomination age when selecting a user to review.
+# The higher this is, the lower the effect of review age. At 1, age and number of entries are weighted equally.
+REVIEW_SCORE_WEIGHT = 1.5
+
 # 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 +50,112 @@ 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:
+                time_since_message_created = datetime.now(timezone.utc) - msg.created_at
+                if time_since_message_created < MIN_REVIEW_INTERVAL:
+                    log.debug("Most recent review was less than %s ago, cancelling check", MIN_REVIEW_INTERVAL)
+                    return False
+
+                is_first_message = False
+
+            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.
 
-    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()
+        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.
+        """
+        now = datetime.now(timezone.utc)
+
+        possible = []
         for user_id, user_data in self._pool.cache.items():
-            if not user_data["reviewed"]:
-                self.schedule_review(user_id)
+            time_since_nomination = now - isoparse(user_data["inserted_at"])
+            if (
+                not user_data["reviewed"]
+                and time_since_nomination > 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}")
+        oldest_date = min(isoparse(x[1]["inserted_at"]) for x in possible)
+        max_entries = max(len(x[1]["entries"]) for x in possible)
 
-        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)
+        def sort_key(nomination: dict) -> float:
+            return self.score_nomination(nomination[1], oldest_date, now, max_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 max(possible, key=sort_key)[0]
+
+    @staticmethod
+    def score_nomination(nomination: dict, oldest_date: datetime, now: datetime, max_entries: int) -> float:
+        """
+        Scores a nomination based on age and number of nomination entries.
+
+        The higher the score, the higher the priority for being put up for review should be.
+        """
+        num_entries = len(nomination["entries"])
+        entries_score = num_entries / max_entries
+
+        nomination_date = isoparse(nomination["inserted_at"])
+        age_score = (nomination_date - now) / (oldest_date - now)
+
+        return entries_score * REVIEW_SCORE_WEIGHT + age_score
 
     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 +166,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 +223,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 +505,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/poetry.lock b/poetry.lock
index c144c80d4..9d9431370 100644
--- a/poetry.lock
+++ b/poetry.lock
@@ -123,6 +123,7 @@ async-rediscache = ["async-rediscache[fakeredis] (==1.0.0rc2)"]
 [package.source]
 type = "url"
 url = "https://github.com/python-discord/bot-core/archive/refs/tags/v8.2.1.zip"
+
 [[package]]
 name = "certifi"
 version = "2022.9.24"
@@ -782,6 +783,17 @@ py = "*"
 pytest = ">=3.10"
 
 [[package]]
+name = "pytest-subtests"
+version = "0.8.0"
+description = "unittest subTest() support and subtests fixture"
+category = "dev"
+optional = false
+python-versions = ">=3.6"
+
+[package.dependencies]
+pytest = ">=7.0"
+
+[[package]]
 name = "pytest-xdist"
 version = "2.5.0"
 description = "pytest xdist plugin for distributed testing and loop-on-failing modes"
@@ -1100,7 +1112,7 @@ multidict = ">=4.0"
 [metadata]
 lock-version = "1.1"
 python-versions = "3.10.*"
-content-hash = "badc19794463d1558d1851990bdf2bf809e25cc038d6206f5c1be51f266ac3a5"
+content-hash = "e2377eb90e01d4f055532baa60c6157ed01eba4bce94330c8616419792c5c69e"
 
 [metadata.files]
 aiodns = [
@@ -1883,6 +1895,10 @@ pytest-forked = [
     {file = "pytest-forked-1.4.0.tar.gz", hash = "sha256:8b67587c8f98cbbadfdd804539ed5455b6ed03802203485dd2f53c1422d7440e"},
     {file = "pytest_forked-1.4.0-py3-none-any.whl", hash = "sha256:bbbb6717efc886b9d64537b41fb1497cfaf3c9601276be8da2cccfea5a3c8ad8"},
 ]
+pytest-subtests = [
+    {file = "pytest-subtests-0.8.0.tar.gz", hash = "sha256:46eb376022e926950816ccc23502de3277adcc1396652ddb3328ce0289052c4d"},
+    {file = "pytest_subtests-0.8.0-py3-none-any.whl", hash = "sha256:4e28ca52cf7a46645c1ded7933745b69334cdc97a412ed4431f7be7cef9a0994"},
+]
 pytest-xdist = [
     {file = "pytest-xdist-2.5.0.tar.gz", hash = "sha256:4580deca3ff04ddb2ac53eba39d76cb5dd5edeac050cb6fbc768b0dd712b4edf"},
     {file = "pytest_xdist-2.5.0-py3-none-any.whl", hash = "sha256:6fe5c74fec98906deb8f2d2b616b5c782022744978e7bd4695d39c8f42d0ce65"},
@@ -1907,6 +1923,13 @@ PyYAML = [
     {file = "PyYAML-6.0-cp310-cp310-manylinux_2_5_x86_64.manylinux1_x86_64.manylinux_2_12_x86_64.manylinux2010_x86_64.whl", hash = "sha256:f84fbc98b019fef2ee9a1cb3ce93e3187a6df0b2538a651bfb890254ba9f90b5"},
     {file = "PyYAML-6.0-cp310-cp310-win32.whl", hash = "sha256:2cd5df3de48857ed0544b34e2d40e9fac445930039f3cfe4bcc592a1f836d513"},
     {file = "PyYAML-6.0-cp310-cp310-win_amd64.whl", hash = "sha256:daf496c58a8c52083df09b80c860005194014c3698698d1a57cbcfa182142a3a"},
+    {file = "PyYAML-6.0-cp311-cp311-macosx_10_9_x86_64.whl", hash = "sha256:d4b0ba9512519522b118090257be113b9468d804b19d63c71dbcf4a48fa32358"},
+    {file = "PyYAML-6.0-cp311-cp311-macosx_11_0_arm64.whl", hash = "sha256:81957921f441d50af23654aa6c5e5eaf9b06aba7f0a19c18a538dc7ef291c5a1"},
+    {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:afa17f5bc4d1b10afd4466fd3a44dc0e245382deca5b3c353d8b757f9e3ecb8d"},
+    {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:dbad0e9d368bb989f4515da330b88a057617d16b6a8245084f1b05400f24609f"},
+    {file = "PyYAML-6.0-cp311-cp311-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:432557aa2c09802be39460360ddffd48156e30721f5e8d917f01d31694216782"},
+    {file = "PyYAML-6.0-cp311-cp311-win32.whl", hash = "sha256:bfaef573a63ba8923503d27530362590ff4f576c626d86a9fed95822a8255fd7"},
+    {file = "PyYAML-6.0-cp311-cp311-win_amd64.whl", hash = "sha256:01b45c0191e6d66c470b6cf1b9531a771a83c1c4208272ead47a3ae4f2f603bf"},
     {file = "PyYAML-6.0-cp36-cp36m-macosx_10_9_x86_64.whl", hash = "sha256:897b80890765f037df3403d22bab41627ca8811ae55e9a722fd0392850ec4d86"},
     {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:50602afada6d6cbfad699b0c7bb50d5ccffa7e46a3d738092afddc1f9758427f"},
     {file = "PyYAML-6.0-cp36-cp36m-manylinux_2_17_s390x.manylinux2014_s390x.whl", hash = "sha256:48c346915c114f5fdb3ead70312bd042a953a8ce5c7106d5bfb1a5254e47da92"},
diff --git a/pyproject.toml b/pyproject.toml
index d03177249..b5050ef63 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -54,8 +54,10 @@ pytest = "7.1.3"
 pytest-cov = "4.0.0"
 python-dotenv = "0.21.0"
 pytest-xdist = "2.5.0"
+pytest-subtests = "0.8.0"
 taskipy = "1.10.3"
 
+
 [build-system]
 requires = ["poetry-core>=1.0.0"]
 build-backend = "poetry.core.masonry.api"
diff --git a/tests/bot/exts/backend/test_error_handler.py b/tests/bot/exts/backend/test_error_handler.py
index eae1bae20..562c827b9 100644
--- a/tests/bot/exts/backend/test_error_handler.py
+++ b/tests/bot/exts/backend/test_error_handler.py
@@ -163,12 +163,11 @@ class ErrorHandlerTests(unittest.IsolatedAsyncioTestCase):
                 self.assertIsNone(await self.cog.on_command_error(self.ctx, case["error"]))
                 case["mock_function_to_call"].assert_awaited_once_with(self.ctx, case["error"].original)
 
-    async def test_error_handler_two_other_errors(self):
-        """Should call `handle_unexpected_error` if error is `MaxConcurrencyReached` or `ExtensionError`."""
+    async def test_error_handler_unexpected_errors(self):
+        """Should call `handle_unexpected_error` if error is `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/recruitment/__init__.py b/tests/bot/exts/recruitment/__init__.py
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/tests/bot/exts/recruitment/__init__.py
diff --git a/tests/bot/exts/recruitment/talentpool/test_review.py b/tests/bot/exts/recruitment/talentpool/test_review.py
new file mode 100644
index 000000000..ed9b66e12
--- /dev/null
+++ b/tests/bot/exts/recruitment/talentpool/test_review.py
@@ -0,0 +1,203 @@
+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: datetime,
+    num_entries: int,
+    reviewed: bool = False,
+    id: int | None = None
+) -> tuple[int, dict]:
+    return (
+        id or 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 (case_num, (nominations, expected)) in enumerate(cases, 1):
+            nomination_dict = dict(nominations)
+
+            with self.subTest(case_num=case_num):
+                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])
+
+    @patch("bot.exts.recruitment.talentpool._review.MIN_NOMINATION_TIME", timedelta(days=0))
+    async def test_get_user_for_review_order(self):
+        now = datetime.now(timezone.utc)
+
+        # Each case in cases is a list of nominations in the order they should be chosen from first to last
+        cases = [
+            [
+                nomination(now - timedelta(days=10), 3, id=1),
+                nomination(now - timedelta(days=50), 2, id=2),
+                nomination(now - timedelta(days=100), 1, id=3),
+            ],
+            [
+                nomination(now - timedelta(days=100), 2, id=1),
+                nomination(now - timedelta(days=10), 3, id=2),
+                nomination(now - timedelta(days=80), 1, id=3),
+                nomination(now - timedelta(days=79), 1, id=4),
+                nomination(now - timedelta(days=10), 2, id=5),
+            ],
+            [
+                nomination(now - timedelta(days=200), 8, id=1),
+                nomination(now - timedelta(days=359), 4, id=2),
+                nomination(now - timedelta(days=230), 5, id=3),
+                nomination(now - timedelta(days=331), 3, id=4),
+                nomination(now - timedelta(days=113), 5, id=5),
+                nomination(now - timedelta(days=186), 3, id=6),
+                nomination(now - timedelta(days=272), 2, id=7),
+                nomination(now - timedelta(days=30), 4, id=8),
+                nomination(now - timedelta(days=198), 2, id=9),
+                nomination(now - timedelta(days=270), 1, id=10),
+                nomination(now - timedelta(days=140), 1, id=11),
+                nomination(now - timedelta(days=19), 2, id=12),
+                nomination(now - timedelta(days=30), 1, id=13),
+            ]
+        ]
+
+        for case_num, case in enumerate(cases, 1):
+            with self.subTest(case_num=case_num):
+                for i in range(len(case)):
+                    with self.subTest(nomination_num=i+1):
+                        sub_case = dict(case[i:])
+                        self.pool.cache = sub_case
+
+                        res = await self.reviewer.get_user_for_review()
+                        self.assertEqual(res, case[i][0])