Reminders: remove _delete_reminder function

Only one call was benefiting from that function also cancelling the
task. Therefore, the function was redundant and has been replaced with
a direct request to delete.

This change has the consequence of also fixing reminder tasks cancelling
themselves. That issue was potentially suppressing errors (such as the
duplicate DELETE request which was fixed earlier). Under normal
circumstances, the scheduler will automatically removed finished tasks
so tasks won't need to cancel/remove themselves.

1 files changed, 11 insertions, 21 deletions

diff --git a/bot/cogs/reminders.py b/bot/cogs/reminders.py
index cbc7d6920..14fe43efb 100644
--- a/bot/cogs/reminders.py
+++ b/bot/cogs/reminders.py
@@ -48,7 +48,7 @@ class Reminders(Cog):
         now = datetime.utcnow()
 
         for reminder in response:
-            is_valid, *_ = self.ensure_valid_reminder(reminder, cancel_task=False)
+            is_valid, *_ = self.ensure_valid_reminder(reminder)
             if not is_valid:
                 continue
 
@@ -61,11 +61,7 @@ class Reminders(Cog):
             else:
                 self.schedule_reminder(reminder)
 
-    def ensure_valid_reminder(
-        self,
-        reminder: dict,
-        cancel_task: bool = True
-    ) -> t.Tuple[bool, discord.User, discord.TextChannel]:
+    def ensure_valid_reminder(self, reminder: dict) -> t.Tuple[bool, discord.User, discord.TextChannel]:
         """Ensure reminder author and channel can be fetched otherwise delete the reminder."""
         user = self.bot.get_user(reminder['author'])
         channel = self.bot.get_channel(reminder['channel_id'])
@@ -76,7 +72,7 @@ class Reminders(Cog):
                 f"Reminder {reminder['id']} invalid: "
                 f"User {reminder['author']}={user}, Channel {reminder['channel_id']}={channel}."
             )
-            asyncio.create_task(self._delete_reminder(reminder['id'], cancel_task))
+            asyncio.create_task(self.bot.api_client.delete(f"bot/reminders/{reminder['id']}"))
 
         return is_valid, user, channel
 
@@ -147,14 +143,6 @@ class Reminders(Cog):
         reminder_datetime = isoparse(reminder['expiration']).replace(tzinfo=None)
         self.scheduler.schedule_at(reminder_datetime, reminder["id"], self.send_reminder(reminder))
 
-    async def _delete_reminder(self, reminder_id: str, cancel_task: bool = True) -> None:
-        """Delete a reminder from the database, given its ID, and cancel the running task."""
-        await self.bot.api_client.delete('bot/reminders/' + str(reminder_id))
-
-        if cancel_task:
-            # Now we can remove it from the schedule list
-            self.scheduler.cancel(reminder_id)
-
     async def _edit_reminder(self, reminder_id: int, payload: dict) -> dict:
         """
         Edits a reminder in the database given the ID and payload.
@@ -180,6 +168,7 @@ class Reminders(Cog):
         """Send the reminder."""
         is_valid, user, channel = self.ensure_valid_reminder(reminder)
         if not is_valid:
+            # No need to cancel the task too; it'll simply be done once this coroutine returns.
             return
 
         embed = discord.Embed()
@@ -205,11 +194,10 @@ class Reminders(Cog):
             mentionable.mention for mentionable in self.get_mentionables(reminder["mentions"])
         )
 
-        await channel.send(
-            content=f"{user.mention} {additional_mentions}",
-            embed=embed
-        )
-        await self._delete_reminder(reminder["id"])
+        await channel.send(content=f"{user.mention} {additional_mentions}", embed=embed)
+
+        log.debug(f"Deleting reminder #{reminder['id']} (the user has been reminded).")
+        await self.bot.api_client.delete(f"bot/reminders/{reminder['id']}")
 
     @group(name="remind", aliases=("reminder", "reminders", "remindme"), invoke_without_command=True)
     async def remind_group(
@@ -401,7 +389,9 @@ class Reminders(Cog):
     @remind_group.command("delete", aliases=("remove", "cancel"))
     async def delete_reminder(self, ctx: Context, id_: int) -> None:
         """Delete one of your active reminders."""
-        await self._delete_reminder(id_)
+        await self.bot.api_client.delete(f"bot/reminders/{id_}")
+        self.scheduler.cancel(id_)
+
         await self._send_confirmation(
             ctx,
             on_success="That reminder has been deleted successfully!",