Repository: python-discord/bot
Issue: Nominations: Fix logic for when to send a new vote

Currently if a vote is archived within a day of being sent, a new vote can be sent immediately, even though a day hasn't passed since that vote was archived.

We should fix this by using redis to cache the date of when the most recent nomination was sent. Then we'll check if it's been a day since *this* date, even if the nomination was archived.

Changed files:
bot/exts/recruitment/talentpool/_review.py, tests/bot/exts/recruitment/talentpool/test_review.py

diff --git a/bot/exts/recruitment/talentpool/_review.py b/bot/exts/recruitment/talentpool/_review.py
index f41e08fe..b9e76b60 100644
--- a/bot/exts/recruitment/talentpool/_review.py
+++ b/bot/exts/recruitment/talentpool/_review.py
@@ -9,6 +9,7 @@ from datetime import datetime, timedelta, timezone
 from typing import List, Optional, Union
 
 import discord
+from async_rediscache import RedisCache
 from discord import Embed, Emoji, Member, Message, NotFound, PartialMessage, TextChannel
 from pydis_core.site_api import ResponseCodeError
 
@@ -52,6 +53,11 @@ NOMINATION_MESSAGE_REGEX = re.compile(
 class Reviewer:
     """Manages, formats, and publishes reviews of helper nominees."""
 
+    # RedisCache[
+    #    "last_vote_date": float   | POSIX UTC timestamp.
+    # ]
+    status_cache = RedisCache()
+
     def __init__(self, bot: Bot, nomination_api: NominationAPI):
         self.bot = bot
         self.api = nomination_api
@@ -82,8 +88,18 @@ class Reviewer:
         """
         voting_channel = self.bot.get_channel(Channels.nomination_voting)
 
+        last_vote_timestamp = await self.status_cache.get("last_vote_date")
+        if last_vote_timestamp:
+            last_vote_date = datetime.fromtimestamp(last_vote_timestamp, tz=timezone.utc)
+            time_since_last_vote = datetime.now(timezone.utc) - last_vote_date
+
+            if time_since_last_vote < MIN_REVIEW_INTERVAL:
+                log.debug("Most recent review was less than %s ago, cancelling check", MIN_REVIEW_INTERVAL)
+                return False
+        else:
+            log.info("Date of last vote not found in cache, a vote may be sent early")
+
         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
@@ -91,14 +107,6 @@ class Reviewer:
             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:
@@ -182,6 +190,9 @@ class Reviewer:
         )
         message = await thread.send(f"<@&{Roles.mod_team}> <@&{Roles.admins}>")
 
+        now = datetime.now(tz=timezone.utc)
+        await self.status_cache.set("last_vote_date", now.timestamp())
+
         await self.api.edit_nomination(nomination.id, reviewed=True, thread_id=thread.id)
 
         bump_cog: ThreadBumper = self.bot.get_cog("ThreadBumper")
diff --git a/tests/bot/exts/recruitment/talentpool/test_review.py b/tests/bot/exts/recruitment/talentpool/test_review.py
index 03c78ab0..1ddb73ab 100644
--- a/tests/bot/exts/recruitment/talentpool/test_review.py
+++ b/tests/bot/exts/recruitment/talentpool/test_review.py
@@ -67,6 +67,7 @@ class ReviewerTests(unittest.IsolatedAsyncioTestCase):
                     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),
                 ],
+                not_too_recent.timestamp(),
                 True,
             ),
 
@@ -77,6 +78,7 @@ class ReviewerTests(unittest.IsolatedAsyncioTestCase):
                     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),
                 ],
+                not_too_recent.timestamp(),
                 False,
             ),
 
@@ -85,6 +87,7 @@ class ReviewerTests(unittest.IsolatedAsyncioTestCase):
                 [
                     MockMessage(author=self.bot_user, content="Chrisjl for Helper!", created_at=too_recent),
                 ],
+                too_recent.timestamp(),
                 False,
             ),
 
@@ -96,18 +99,25 @@ class ReviewerTests(unittest.IsolatedAsyncioTestCase):
                     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),
                 ],
+                not_too_recent.timestamp(),
                 True,
             ),
 
             # No messages, so ready.
-            ([], True),
+            ([], None, True),
         )
 
-        for messages, expected in cases:
+        for messages, last_review_timestamp, expected in cases:
             with self.subTest(messages=messages, expected=expected):
                 self.voting_channel.history = AsyncIterator(messages)
+
+                cache_get_mock = AsyncMock(return_value=last_review_timestamp)
+                self.reviewer.status_cache.get = cache_get_mock
+
                 res = await self.reviewer.is_ready_for_review()
+
                 self.assertIs(res, expected)
+                cache_get_mock.assert_called_with("last_vote_date")
 
     @patch("bot.exts.recruitment.talentpool._review.MIN_NOMINATION_TIME", timedelta(days=7))
     async def test_get_nomination_to_review(self):