From b7f0bc54917a51e6770027d6e18afb74e454f9d5 Mon Sep 17 00:00:00 2001 From: Melove Date: Mon, 2 Mar 2026 16:20:34 +0530 Subject: [PATCH 1/2] Fix stale attachment IDs causing 404 errors in email section Change get_or_create to update_or_create in parse_and_add_attachments() so that attachmentid and filename are refreshed when the mail archive re-indexes attachments. Previously, defaults were only applied on record creation, leaving stale IDs that pointed to non-existent attachments. Add regression tests verifying attachment metadata is updated correctly. --- pgcommitfest/commitfest/ajax.py | 2 +- pgcommitfest/commitfest/forms.py | 15 +- .../tests/test_attachment_update.py | 131 ++++++++++++++++++ 3 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 pgcommitfest/commitfest/tests/test_attachment_update.py diff --git a/pgcommitfest/commitfest/ajax.py b/pgcommitfest/commitfest/ajax.py index 8137dfdf..1c30ad0b 100644 --- a/pgcommitfest/commitfest/ajax.py +++ b/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={ diff --git a/pgcommitfest/commitfest/forms.py b/pgcommitfest/commitfest/forms.py index 49d7366f..a01b437e 100644 --- a/pgcommitfest/commitfest/forms.py +++ b/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. diff --git a/pgcommitfest/commitfest/tests/test_attachment_update.py b/pgcommitfest/commitfest/tests/test_attachment_update.py new file mode 100644 index 00000000..cf4ad08f --- /dev/null +++ b/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() From aaa10208dd66673ce87589b8e70b727fd462a3ec Mon Sep 17 00:00:00 2001 From: Jelterminator FennemAI-Neo Date: Sun, 18 Oct 2026 11:43:45 +0200 Subject: [PATCH 2/2] Fold the attachment update test into the refresh thread test The separate test file mostly covered behaviour that was already there. The one new thing, refreshing a stale attachment id, is now checked as part of the existing `refresh_single_thread` test, which exercises the real code path. This also drops the unrelated reformatting of `forms.py`, which is already on main. --- pgcommitfest/commitfest/forms.py | 15 +- .../tests/test_attachment_update.py | 131 ------------------ .../commitfest/tests/test_refresh_thread.py | 20 ++- 3 files changed, 22 insertions(+), 144 deletions(-) delete mode 100644 pgcommitfest/commitfest/tests/test_attachment_update.py diff --git a/pgcommitfest/commitfest/forms.py b/pgcommitfest/commitfest/forms.py index a01b437e..49d7366f 100644 --- a/pgcommitfest/commitfest/forms.py +++ b/pgcommitfest/commitfest/forms.py @@ -107,13 +107,10 @@ 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" @@ -147,8 +144,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. diff --git a/pgcommitfest/commitfest/tests/test_attachment_update.py b/pgcommitfest/commitfest/tests/test_attachment_update.py deleted file mode 100644 index cf4ad08f..00000000 --- a/pgcommitfest/commitfest/tests/test_attachment_update.py +++ /dev/null @@ -1,131 +0,0 @@ -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() diff --git a/pgcommitfest/commitfest/tests/test_refresh_thread.py b/pgcommitfest/commitfest/tests/test_refresh_thread.py index 55d22a22..3ebeeaa5 100644 --- a/pgcommitfest/commitfest/tests/test_refresh_thread.py +++ b/pgcommitfest/commitfest/tests/test_refresh_thread.py @@ -4,13 +4,13 @@ import pytest from pgcommitfest.commitfest.ajax import refresh_single_thread -from pgcommitfest.commitfest.models import MailThread, Patch +from pgcommitfest.commitfest.models import MailThread, MailThreadAttachment, Patch pytestmark = pytest.mark.django_db -def test_refresh_single_thread_updates_patch_lastmail(): - """Regression test: patch.lastmail should get the new date, not the old one.""" +def test_refresh_single_thread(): + """A new mail updates patch.lastmail and refreshes stale attachment info.""" old_date = datetime(2024, 1, 1) new_date = datetime(2024, 6, 15) @@ -26,6 +26,14 @@ def test_refresh_single_thread_updates_patch_lastmail(): ) p = Patch.objects.create(name="Test Patch", lastmail=old_date) p.mailthread_set.add(thread) + attachment = MailThreadAttachment.objects.create( + mailthread=thread, + messageid="old@example.com", + attachmentid=1, + filename="old.patch", + date=old_date, + author="a", + ) api_response = [ { @@ -33,7 +41,8 @@ def test_refresh_single_thread_updates_patch_lastmail(): "date": old_date, "from": "a", "subj": "T", - "atts": [], + # The archives assigned a new id to the same attachment + "atts": [{"id": 2, "name": "new.patch"}], }, { "msgid": "new@example.com", @@ -49,3 +58,6 @@ def test_refresh_single_thread_updates_patch_lastmail(): p.refresh_from_db() assert p.lastmail == new_date + attachment.refresh_from_db() + assert (attachment.attachmentid, attachment.filename) == (2, "new.patch") + assert MailThreadAttachment.objects.filter(mailthread=thread).count() == 1