Incidents: link `proxy_url` if attachment fails to download

Suggested by Mark during review. If the download fails, we fallback
on showing an informative message, which will link the attachment
cdn link.

The attachment-handling logic was moved from the `archive` coroutine
into `make_embed`, which now also returns the file, if available.
In the end, this appears to be the smoothest approach.

Co-authored-by: MarkKoz <[email protected]>

2 files changed, 59 insertions, 50 deletions

diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py
index 65b0e458e..018538040 100644
--- a/bot/cogs/moderation/incidents.py
+++ b/bot/cogs/moderation/incidents.py
@@ -41,6 +41,10 @@ ALLOWED_ROLES: t.Set[int] = set(Guild.moderation_roles)
 # Message must have all of these emoji to pass the `has_signals` check
 ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal}
 
+# An embed coupled with an optional file to be dispatched
+# If the file is not None, the embed attempts to show it in its body
+FileEmbed = t.Tuple[discord.Embed, t.Optional[discord.File]]
+
 
 async def download_file(attachment: discord.Attachment) -> t.Optional[discord.File]:
     """
@@ -55,7 +59,7 @@ async def download_file(attachment: discord.Attachment) -> t.Optional[discord.Fi
         log.exception("Failed to download attachment")
 
 
-def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> discord.Embed:
+async def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> FileEmbed:
     """
     Create an embed representation of `incident` for the #incidents-archive channel.
 
@@ -66,6 +70,11 @@ def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.
     of information will be relayed in other ways, e.g. webhook username.
 
     As mentions in embeds do not ping, we do not need to use `incident.clean_content`.
+
+    If `incident` contains attachments, the first attachment will be downloaded and
+    returned alongside the embed. The embed attempts to display the attachment.
+    Should the download fail, we fallback on linking the `proxy_url`, which should
+    remain functional for some time after the original message is deleted.
     """
     log.trace(f"Creating embed for {incident.id=}")
 
@@ -83,7 +92,18 @@ def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.
     )
     embed.set_footer(text=footer, icon_url=actioned_by.avatar_url)
 
-    return embed
+    if incident.attachments:
+        attachment = incident.attachments[0]  # User-sent messages can only contain one attachment
+        file = await download_file(attachment)
+
+        if file is not None:
+            embed.set_image(url=f"attachment://{attachment.filename}")  # Embed displays the attached file
+        else:
+            embed.set_author(name="[Failed to relay attachment]", url=attachment.proxy_url)  # Embed links the file
+    else:
+        file = None
+
+    return embed, file
 
 
 def is_incident(message: discord.Message) -> bool:
@@ -215,17 +235,7 @@ class Incidents(Cog):
         message is not safe to be deleted, as we will lose some information.
         """
         log.debug(f"Archiving incident: {incident.id} (outcome: {outcome}, actioned by: {actioned_by})")
-        embed = make_embed(incident, outcome, actioned_by)
-
-        # If the incident had an attachment, we will try to relay it
-        if incident.attachments:
-            attachment = incident.attachments[0]  # User-sent messages can only contain one attachment
-            log.debug(f"Attempting to archive incident attachment: {attachment.filename}")
-
-            attachment_file = await attachment.to_file()  # The file will be sent with the webhook
-            embed.set_image(url=f"attachment://{attachment.filename}")  # Embed displays the attached file
-        else:
-            attachment_file = None
+        embed, attachment_file = await make_embed(incident, outcome, actioned_by)
 
         try:
             webhook = await self.bot.fetch_webhook(Webhooks.incidents_archive)
diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py
index 273916199..9b6054f55 100644
--- a/tests/bot/cogs/moderation/test_incidents.py
+++ b/tests/bot/cogs/moderation/test_incidents.py
@@ -89,30 +89,58 @@ class TestDownloadFile(unittest.IsolatedAsyncioTestCase):
         self.assertIsNone(acquired_file)
 
 
-class TestMakeEmbed(unittest.TestCase):
+class TestMakeEmbed(unittest.IsolatedAsyncioTestCase):
     """Collection of tests for the `make_embed` helper function."""
 
-    def test_make_embed_actioned(self):
+    async def test_make_embed_actioned(self):
         """Embed is coloured green and footer contains 'Actioned' when `outcome=Signal.ACTIONED`."""
-        embed = incidents.make_embed(MockMessage(), incidents.Signal.ACTIONED, MockMember())
+        embed, file = await incidents.make_embed(MockMessage(), incidents.Signal.ACTIONED, MockMember())
 
         self.assertEqual(embed.colour.value, Colours.soft_green)
         self.assertIn("Actioned", embed.footer.text)
 
-    def test_make_embed_not_actioned(self):
+    async def test_make_embed_not_actioned(self):
         """Embed is coloured red and footer contains 'Rejected' when `outcome=Signal.NOT_ACTIONED`."""
-        embed = incidents.make_embed(MockMessage(), incidents.Signal.NOT_ACTIONED, MockMember())
+        embed, file = await incidents.make_embed(MockMessage(), incidents.Signal.NOT_ACTIONED, MockMember())
 
         self.assertEqual(embed.colour.value, Colours.soft_red)
         self.assertIn("Rejected", embed.footer.text)
 
-    def test_make_embed_content(self):
+    async def test_make_embed_content(self):
         """Incident content appears as embed description."""
         incident = MockMessage(content="this is an incident")
-        embed = incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember())
+        embed, file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember())
 
         self.assertEqual(incident.content, embed.description)
 
+    async def test_make_embed_with_attachment_succeeds(self):
+        """Incident's attachment is downloaded and displayed in the embed's image field."""
+        file = MagicMock(discord.File, filename="bigbadjoe.jpg")
+        attachment = MockAttachment(filename="bigbadjoe.jpg")
+        incident = MockMessage(content="this is an incident", attachments=[attachment])
+
+        # Patch `download_file` to return our `file`
+        with patch("bot.cogs.moderation.incidents.download_file", AsyncMock(return_value=file)):
+            embed, returned_file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember())
+
+        self.assertIs(file, returned_file)
+        self.assertEqual("attachment://bigbadjoe.jpg", embed.image.url)
+
+    async def test_make_embed_with_attachment_fails(self):
+        """Incident's attachment fails to download, proxy url is linked instead."""
+        attachment = MockAttachment(proxy_url="discord.com/bigbadjoe.jpg")
+        incident = MockMessage(content="this is an incident", attachments=[attachment])
+
+        # Patch `download_file` to return None as if the download failed
+        with patch("bot.cogs.moderation.incidents.download_file", AsyncMock(return_value=None)):
+            embed, returned_file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember())
+
+        self.assertIsNone(returned_file)
+
+        # The author name field is simply expected to have something in it, we do not assert the message
+        self.assertGreater(len(embed.author.name), 0)
+        self.assertEqual(embed.author.url, "discord.com/bigbadjoe.jpg")  # However, it should link the exact url
+
 
 @patch("bot.constants.Channels.incidents", 123)
 class TestIsIncident(unittest.TestCase):
@@ -343,11 +371,10 @@ class TestArchive(TestIncidents):
             content="this is an incident",
             author=MockUser(name="author_name", avatar_url="author_avatar"),
             id=123,
-            attachments=[],  # This incident has no attachments
         )
         built_embed = MagicMock(discord.Embed, id=123)  # We patch `make_embed` to return this
 
-        with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)):
+        with patch("bot.cogs.moderation.incidents.make_embed", AsyncMock(return_value=(built_embed, None))):
             archive_return = await self.cog_instance.archive(incident, MagicMock(value="A"), MockMember())
 
         # Now we check that the webhook was given the correct args, and that `archive` returned True
@@ -359,34 +386,6 @@ class TestArchive(TestIncidents):
         )
         self.assertTrue(archive_return)
 
-    async def test_archive_relays_incident_with_attachments(self):
-        """
-        Incident attachments are relayed and displayed in the embed.
-
-        This test asserts the two things that need to happen in order to relay the attachment.
-        The embed returned by `make_embed` must have the `set_image` method called with the
-        attachment's filename, and the file must be passed to the webhook's send method.
-        """
-        attachment_file = MagicMock(discord.File)
-        attachment = MagicMock(
-            discord.Attachment,
-            filename="abc.png",
-            to_file=AsyncMock(return_value=attachment_file),
-        )
-        incident = MockMessage(
-            attachments=[attachment],
-        )
-        built_embed = MagicMock(discord.Embed)
-
-        with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)):
-            await self.cog_instance.archive(incident, incidents.Signal.ACTIONED, actioned_by=MockMember())
-
-        built_embed.set_image.assert_called_once_with(url="attachment://abc.png")
-
-        send_kwargs = self.cog_instance.bot.fetch_webhook.return_value.send.call_args.kwargs
-        self.assertIn("file", send_kwargs)
-        self.assertIs(send_kwargs["file"], attachment_file)
-
     async def test_archive_clyde_username(self):
         """
         The archive webhook username is cleansed using `sub_clyde`.