Fix stale attachment IDs causing 404 errors in Emails section

pgcommitfest/commitfest/ajax.py

@@ -195,7 +195,7 @@ def parse_and_add_attachments(threadinfo, mailthread):
             # One or more attachments. For now, we're only actually going
             # to store and process the first one, even though the API gets
             # us all of them.
-            MailThreadAttachment.objects.get_or_create(
+            MailThreadAttachment.objects.update_or_create(
                 mailthread=mailthread,
                 messageid=t["msgid"],
                 defaults={

pgcommitfest/commitfest/forms.py

@@ -107,10 +107,13 @@ def __init__(self, *args, **kwargs):
         super(PatchForm, self).__init__(*args, **kwargs)
         self.fields["authors"].help_text = "Enter part of name to see list"
         self.fields["reviewers"].help_text = "Enter part of name to see list"
-        self.fields["committer"].label_from_instance = lambda x: "%s %s (%s)" % (
-            x.user.first_name,
-            x.user.last_name,
-            x.user.username,
+        self.fields["committer"].label_from_instance = lambda x: (
+            "%s %s (%s)"
+            % (
+                x.user.first_name,
+                x.user.last_name,
+                x.user.username,
+            )
         )
 
         self.fields["authors"].widget.attrs["class"] = "add-user-picker"
@@ -144,8 +147,8 @@ def __init__(self, *args, **kwargs):
             self.fields[field].queryset = self.fields[field].queryset.filter(
                 pk__in=set(vals)
             )
-            self.fields[field].label_from_instance = (
-                lambda u: f"{u.get_full_name()} ({u.username})"
+            self.fields[field].label_from_instance = lambda u: (
+                f"{u.get_full_name()} ({u.username})"
             )
 
         # Only allow modifying reviewers and committers if the patch is closed.

pgcommitfest/commitfest/tests/test_attachment_update.py

@@ -0,0 +1,131 @@
+from datetime import datetime
+
+import pytest
+
+from pgcommitfest.commitfest.ajax import parse_and_add_attachments
+from pgcommitfest.commitfest.models import MailThread, MailThreadAttachment
+
+pytestmark = pytest.mark.django_db
+
+
+def _make_thread():
+    return MailThread.objects.create(
+        messageid="test@example.com",
+        subject="Test",
+        firstmessage=datetime(2024, 1, 1),
+        firstauthor="a@example.com",
+        latestmessage=datetime(2024, 1, 1),
+        latestauthor="a@example.com",
+        latestsubject="Test",
+        latestmsgid="test@example.com",
+    )
+
+
+def test_attachment_id_updated_on_reindex():
+    """Regression test: attachmentid must be updated when the archive re-indexes."""
+    thread = _make_thread()
+
+    old_api_response = [
+        {
+            "msgid": "msg1@example.com",
+            "date": datetime(2024, 1, 1),
+            "from": "author@example.com",
+            "subj": "Test",
+            "atts": [{"id": 37937, "name": "patch_v1.diff"}],
+        },
+    ]
+    parse_and_add_attachments(old_api_response, thread)
+
+    att = MailThreadAttachment.objects.get(
+        mailthread=thread, messageid="msg1@example.com"
+    )
+    assert att.attachmentid == 37937
+    assert att.filename == "patch_v1.diff"
+
+    # Simulate archive re-indexing: same message, new attachment ID
+    new_api_response = [
+        {
+            "msgid": "msg1@example.com",
+            "date": datetime(2024, 1, 1),
+            "from": "author@example.com",
+            "subj": "Test",
+            "atts": [{"id": 90866, "name": "patch_v1.diff"}],
+        },
+    ]
+    parse_and_add_attachments(new_api_response, thread)
+
+    att.refresh_from_db()
+    assert att.attachmentid == 90866, (
+        f"Expected attachmentid to be updated to 90866, got {att.attachmentid}"
+    )
+
+
+def test_attachment_filename_updated_on_reindex():
+    """Filename should also be updated if it changes."""
+    thread = _make_thread()
+
+    api_response = [
+        {
+            "msgid": "msg2@example.com",
+            "date": datetime(2024, 2, 1),
+            "from": "author@example.com",
+            "subj": "Test",
+            "atts": [{"id": 100, "name": "old_name.patch"}],
+        },
+    ]
+    parse_and_add_attachments(api_response, thread)
+
+    att = MailThreadAttachment.objects.get(
+        mailthread=thread, messageid="msg2@example.com"
+    )
+    assert att.filename == "old_name.patch"
+
+    api_response[0]["atts"][0]["name"] = "new_name.patch"
+    api_response[0]["atts"][0]["id"] = 200
+    parse_and_add_attachments(api_response, thread)
+
+    att.refresh_from_db()
+    assert att.filename == "new_name.patch"
+    assert att.attachmentid == 200
+
+
+def test_attachment_created_when_new():
+    """New attachments should still be created correctly."""
+    thread = _make_thread()
+
+    api_response = [
+        {
+            "msgid": "new@example.com",
+            "date": datetime(2024, 3, 1),
+            "from": "author@example.com",
+            "subj": "Test",
+            "atts": [{"id": 555, "name": "feature.patch"}],
+        },
+    ]
+    parse_and_add_attachments(api_response, thread)
+
+    att = MailThreadAttachment.objects.get(
+        mailthread=thread, messageid="new@example.com"
+    )
+    assert att.attachmentid == 555
+    assert att.filename == "feature.patch"
+
+
+def test_messages_without_attachments_ignored():
+    """Messages with no attachments should not create any records."""
+    thread = _make_thread()
+
+    api_response = [
+        {
+            "msgid": "noatt@example.com",
+            "date": datetime(2024, 4, 1),
+            "from": "author@example.com",
+            "subj": "Test",
+            "atts": [],
+        },
+    ]
+    parse_and_add_attachments(api_response, thread)
+
+    assert not MailThreadAttachment.objects.filter(
+        mailthread=thread, messageid="noatt@example.com"
+    ).exists()