]> git.ipfire.org Git - thirdparty/patchwork.git/commitdiff
models: Split 'CoverLetter' from 'Submission'
authorStephen Finucane <stephen@that.guru>
Sat, 29 Feb 2020 17:20:50 +0000 (17:20 +0000)
committerStephen Finucane <stephen@that.guru>
Sun, 26 Apr 2020 12:45:45 +0000 (13:45 +0100)
We want to get rid of the split between 'Patch' and 'Submission' because
of the cost of using JOINs basically everywhere we use 'Patch'. Before
we do that, we need to move the other users of 'Submission' to other
models and other models that rely on these users sharing the common
'Submission' base. For the former, there is only one user,
'CoverLetter', while for the latter there is only the 'Comment' model.
As a result, we must do the following:

- Create a new 'Cover' model
- Create a new 'CoverComment' model
- Move everything from 'CoverLetter' to 'Cover' and all entries
  associated with a 'CoverLetter' from 'Comment' to 'CoverComment'
- Delete the 'CoverLetter' model
- Rename the 'Comment' model to 'PatchComment'

This means our model "hierarchy" goes from:

  Submission
    Patch
    CoverLetter
    Comment

To:

  Submission
    Patch
    PatchComment
  Cover
    CoverComment

A future change will flatten the 'Submission' and 'Patch' model.

Note that this actually highlighted a bug in Django, which has since
been reported upstream [1]. As noted there, the issue stems from MySQL's
refusal to remove an index from a foreign key when DB constraints are
used and the workaround is to remove the foreign key constraint before
altering the indexes and then re-add the constraint after.

[1] https://code.djangoproject.com/ticket/31335

Signed-off-by: Stephen Finucane <stephen@that.guru>
22 files changed:
patchwork/admin.py
patchwork/api/comment.py
patchwork/api/cover.py
patchwork/api/embedded.py
patchwork/api/filters.py
patchwork/management/commands/parsearchive.py
patchwork/migrations/0042_add_cover_model.py [new file with mode: 0644]
patchwork/models.py
patchwork/parser.py
patchwork/signals.py
patchwork/tests/api/test_comment.py
patchwork/tests/test_detail.py
patchwork/tests/test_mboxviews.py
patchwork/tests/test_parser.py
patchwork/tests/test_series.py
patchwork/tests/test_tags.py
patchwork/tests/utils.py
patchwork/urls.py
patchwork/views/comment.py
patchwork/views/cover.py
patchwork/views/patch.py
patchwork/views/utils.py

index fb798085dce21faa321da62fbac46c848b257bc8..a65022245bcf618e11983727e73fd980f91e279d 100644 (file)
@@ -10,10 +10,11 @@ from django.db.models import Prefetch
 
 from patchwork.models import Bundle
 from patchwork.models import Check
-from patchwork.models import Comment
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
+from patchwork.models import CoverComment
 from patchwork.models import DelegationRule
 from patchwork.models import Patch
+from patchwork.models import PatchComment
 from patchwork.models import PatchRelation
 from patchwork.models import Person
 from patchwork.models import Project
@@ -75,14 +76,14 @@ class StateAdmin(admin.ModelAdmin):
 admin.site.register(State, StateAdmin)
 
 
-class CoverLetterAdmin(admin.ModelAdmin):
+class CoverAdmin(admin.ModelAdmin):
     list_display = ('name', 'submitter', 'project', 'date')
     list_filter = ('project', )
     search_fields = ('name', 'submitter__name', 'submitter__email')
     date_hierarchy = 'date'
 
 
-admin.site.register(CoverLetter, CoverLetterAdmin)
+admin.site.register(Cover, CoverAdmin)
 
 
 class PatchAdmin(admin.ModelAdmin):
@@ -104,13 +105,22 @@ class PatchAdmin(admin.ModelAdmin):
 admin.site.register(Patch, PatchAdmin)
 
 
-class CommentAdmin(admin.ModelAdmin):
-    list_display = ('submission', 'submitter', 'date')
-    search_fields = ('submission__name', 'submitter__name', 'submitter__email')
+class CoverCommentAdmin(admin.ModelAdmin):
+    list_display = ('cover', 'submitter', 'date')
+    search_fields = ('cover__name', 'submitter__name', 'submitter__email')
     date_hierarchy = 'date'
 
 
-admin.site.register(Comment, CommentAdmin)
+admin.site.register(CoverComment, CoverCommentAdmin)
+
+
+class PatchCommentAdmin(admin.ModelAdmin):
+    list_display = ('patch', 'submitter', 'date')
+    search_fields = ('patch__name', 'submitter__name', 'submitter__email')
+    date_hierarchy = 'date'
+
+
+admin.site.register(PatchComment, PatchCommentAdmin)
 
 
 class PatchInline(admin.StackedInline):
index 290b9cd3f3cecb85dda7fcd20c505c2be56ac2ce..3802dab979ce468d871b373459b1765ca7cfdc8f 100644 (file)
@@ -12,11 +12,13 @@ from rest_framework.serializers import SerializerMethodField
 from patchwork.api.base import BaseHyperlinkedModelSerializer
 from patchwork.api.base import PatchworkPermission
 from patchwork.api.embedded import PersonSerializer
-from patchwork.models import Comment
+from patchwork.models import Cover
+from patchwork.models import CoverComment
 from patchwork.models import Submission
+from patchwork.models import PatchComment
 
 
-class CommentListSerializer(BaseHyperlinkedModelSerializer):
+class BaseCommentListSerializer(BaseHyperlinkedModelSerializer):
 
     web_url = SerializerMethodField()
     subject = SerializerMethodField()
@@ -46,7 +48,6 @@ class CommentListSerializer(BaseHyperlinkedModelSerializer):
         return headers
 
     class Meta:
-        model = Comment
         fields = ('id', 'web_url', 'msgid', 'list_archive_url', 'date',
                   'subject', 'submitter', 'content', 'headers')
         read_only_fields = fields
@@ -56,11 +57,48 @@ class CommentListSerializer(BaseHyperlinkedModelSerializer):
         }
 
 
-class CommentList(ListAPIView):
+class CoverCommentListSerializer(BaseCommentListSerializer):
+
+    class Meta:
+        model = CoverComment
+        fields = BaseCommentListSerializer.Meta.fields
+        read_only_fields = fields
+        versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
+
+
+class PatchCommentListSerializer(BaseCommentListSerializer):
+
+    class Meta:
+        model = PatchComment
+        fields = BaseCommentListSerializer.Meta.fields
+        read_only_fields = fields
+        versioned_fields = BaseCommentListSerializer.Meta.versioned_fields
+
+
+class CoverCommentList(ListAPIView):
+    """List cover comments"""
+
+    permission_classes = (PatchworkPermission,)
+    serializer_class = CoverCommentListSerializer
+    search_fields = ('subject',)
+    ordering_fields = ('id', 'subject', 'date', 'submitter')
+    ordering = 'id'
+    lookup_url_kwarg = 'pk'
+
+    def get_queryset(self):
+        if not Cover.objects.filter(pk=self.kwargs['pk']).exists():
+            raise Http404
+
+        return CoverComment.objects.filter(
+            cover=self.kwargs['pk']
+        ).select_related('submitter')
+
+
+class PatchCommentList(ListAPIView):
     """List comments"""
 
     permission_classes = (PatchworkPermission,)
-    serializer_class = CommentListSerializer
+    serializer_class = PatchCommentListSerializer
     search_fields = ('subject',)
     ordering_fields = ('id', 'subject', 'date', 'submitter')
     ordering = 'id'
@@ -70,6 +108,6 @@ class CommentList(ListAPIView):
         if not Submission.objects.filter(pk=self.kwargs['pk']).exists():
             raise Http404
 
-        return Comment.objects.filter(
-            submission=self.kwargs['pk']
+        return PatchComment.objects.filter(
+            patch=self.kwargs['pk']
         ).select_related('submitter')
index 66d2146cc9b2b9b670a78272490a9fce50e82a1f..b4a3a8f737330b349cc091fb920f64109854b7d8 100644 (file)
@@ -15,7 +15,7 @@ from patchwork.api.filters import CoverFilterSet
 from patchwork.api.embedded import PersonSerializer
 from patchwork.api.embedded import ProjectSerializer
 from patchwork.api.embedded import SeriesSerializer
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
 
 
 class CoverListSerializer(BaseHyperlinkedModelSerializer):
@@ -49,7 +49,7 @@ class CoverListSerializer(BaseHyperlinkedModelSerializer):
         return data
 
     class Meta:
-        model = CoverLetter
+        model = Cover
         fields = ('id', 'url', 'web_url', 'project', 'msgid',
                   'list_archive_url', 'date', 'name', 'submitter', 'mbox',
                   'series', 'comments')
@@ -82,7 +82,7 @@ class CoverDetailSerializer(CoverListSerializer):
         return headers
 
     class Meta:
-        model = CoverLetter
+        model = Cover
         fields = CoverListSerializer.Meta.fields + (
             'headers', 'content')
         read_only_fields = fields
@@ -100,7 +100,7 @@ class CoverList(ListAPIView):
     ordering = 'id'
 
     def get_queryset(self):
-        return CoverLetter.objects.all()\
+        return Cover.objects.all()\
             .prefetch_related('series__project')\
             .select_related('project', 'submitter', 'series')\
             .defer('content', 'headers')
@@ -112,5 +112,5 @@ class CoverDetail(RetrieveAPIView):
     serializer_class = CoverDetailSerializer
 
     def get_queryset(self):
-        return CoverLetter.objects.all()\
+        return Cover.objects.all()\
             .select_related('project', 'submitter', 'series')
index 1478e748865e9b2a9f724f4d69a6f69d237924de..3f32bd42128b668d4f5823c8019316452c19470c 100644 (file)
@@ -108,7 +108,7 @@ class CoverSerializer(SerializedRelatedField):
     class _Serializer(MboxMixin, WebURLMixin, BaseHyperlinkedModelSerializer):
 
         class Meta:
-            model = models.CoverLetter
+            model = models.Cover
             fields = ('id', 'url', 'web_url', 'msgid', 'list_archive_url',
                       'date', 'name', 'mbox')
             read_only_fields = fields
index a328d594f5b5a01615978193e62113ec45d36950..bd3b326538cfdc04b4fc9dbddedee67f9cb65a4c 100644 (file)
@@ -18,7 +18,7 @@ from rest_framework import exceptions
 from patchwork.api import utils
 from patchwork.models import Bundle
 from patchwork.models import Check
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
 from patchwork.models import Event
 from patchwork.models import Patch
 from patchwork.models import Person
@@ -199,7 +199,7 @@ class CoverFilterSet(TimestampMixin, BaseFilterSet):
     msgid = CharFilter(method=msgid_filter)
 
     class Meta:
-        model = CoverLetter
+        model = Cover
         fields = ('project', 'series', 'submitter')
 
 
@@ -253,7 +253,7 @@ class EventFilterSet(TimestampMixin, BaseFilterSet):
     patch = BaseFilter(queryset=Patch.objects.all(),
                        widget=MultipleHiddenInput,
                        distinct=False)
-    cover = BaseFilter(queryset=CoverLetter.objects.all(),
+    cover = BaseFilter(queryset=Cover.objects.all(),
                        widget=MultipleHiddenInput,
                        distinct=False)
 
index b7f1ea7313c28abbfc95b17086dd2d3b2ad21282..f53b8db4a1390c333c32e4a6a2b67ca1095e48ca 100644 (file)
@@ -32,8 +32,9 @@ class Command(BaseCommand):
     def handle(self, *args, **options):
         results = {
             models.Patch: 0,
-            models.CoverLetter: 0,
-            models.Comment: 0,
+            models.Cover: 0,
+            models.PatchComment: 0,
+            models.CoverComment: 0,
         }
         duplicates = 0
         dropped = 0
@@ -118,9 +119,11 @@ class Command(BaseCommand):
             '  %(errors)4d errors\n'
             'Total: %(new)s new entries' % {
                 'total': count,
-                'covers': results[models.CoverLetter],
+                'covers': results[models.Cover],
                 'patches': results[models.Patch],
-                'comments': results[models.Comment],
+                'comments': (
+                    results[models.CoverComment] + results[models.PatchComment]
+                ),
                 'duplicates': duplicates,
                 'dropped': dropped,
                 'errors': errors,
diff --git a/patchwork/migrations/0042_add_cover_model.py b/patchwork/migrations/0042_add_cover_model.py
new file mode 100644 (file)
index 0000000..53196c1
--- /dev/null
@@ -0,0 +1,247 @@
+import datetime
+
+from django.db import migrations, models
+import django.db.models.deletion
+import patchwork.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0041_python3'),
+    ]
+
+    operations = [
+        # create a new, separate cover (letter) model
+
+        migrations.CreateModel(
+            name='Cover',
+            fields=[
+                (
+                    'id',
+                    models.AutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name='ID',
+                    ),
+                ),
+                ('msgid', models.CharField(max_length=255)),
+                (
+                    'date',
+                    models.DateTimeField(default=datetime.datetime.utcnow),
+                ),
+                ('headers', models.TextField(blank=True)),
+                ('content', models.TextField(blank=True, null=True)),
+                ('name', models.CharField(max_length=255)),
+                (
+                    'project',
+                    models.ForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE,
+                        to='patchwork.Project',
+                    ),
+                ),
+                (
+                    'submitter',
+                    models.ForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE,
+                        to='patchwork.Person',
+                    ),
+                ),
+            ],
+            options={'ordering': ['date']},
+            bases=(patchwork.models.FilenameMixin, models.Model),
+        ),
+        migrations.AddIndex(
+            model_name='cover',
+            index=models.Index(
+                fields=['date', 'project', 'submitter', 'name'],
+                name='cover_covering_idx',
+            ),
+        ),
+        migrations.AlterUniqueTogether(
+            name='cover', unique_together=set([('msgid', 'project')]),
+        ),
+
+        # create a new, separate cover (letter) comment model
+
+        migrations.CreateModel(
+            name='CoverComment',
+            fields=[
+                (
+                    'id',
+                    models.AutoField(
+                        auto_created=True,
+                        primary_key=True,
+                        serialize=False,
+                        verbose_name='ID',
+                    ),
+                ),
+                ('msgid', models.CharField(max_length=255)),
+                (
+                    'date',
+                    models.DateTimeField(default=datetime.datetime.utcnow),
+                ),
+                ('headers', models.TextField(blank=True)),
+                ('content', models.TextField(blank=True, null=True)),
+                (
+                    'cover',
+                    models.ForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE,
+                        related_name='comments',
+                        related_query_name='comment',
+                        to='patchwork.Cover',
+                    ),
+                ),
+                (
+                    'submitter',
+                    models.ForeignKey(
+                        on_delete=django.db.models.deletion.CASCADE,
+                        to='patchwork.Person',
+                    ),
+                ),
+            ],
+            options={'ordering': ['date']},
+        ),
+        migrations.AddIndex(
+            model_name='covercomment',
+            index=models.Index(
+                fields=['cover', 'date'], name='cover_date_idx'
+            ),
+        ),
+        migrations.AlterUniqueTogether(
+            name='covercomment', unique_together=set([('msgid', 'cover')]),
+        ),
+
+        # copy all entries from the 'CoverLetter' model to the new 'Cover'
+        # model; note that it's not possible to reverse this since we can't
+        # guarantee IDs will be unique after the split
+
+        migrations.RunSQL(
+            """
+            INSERT INTO patchwork_cover
+                (id, msgid, name, date, headers, project_id, submitter_id,
+                 content)
+            SELECT s.id, s.msgid, s.name, s.date, s.headers, s.project_id,
+                s.submitter_id, s.content
+            FROM patchwork_coverletter c
+            INNER JOIN patchwork_submission s ON s.id = c.submission_ptr_id
+            """,
+            None,
+        ),
+
+        # copy all 'CoverLetter'-related comments to the new 'CoverComment'
+        # table; as above, this is non-reversible
+
+        migrations.RunSQL(
+            """
+            INSERT INTO patchwork_covercomment
+                (id, msgid, date, headers, content, cover_id, submitter_id)
+            SELECT c.id, c.msgid, c.date, c.headers, c.content,
+                c.submission_id, c.submitter_id
+            FROM patchwork_comment c
+            INNER JOIN patchwork_coverletter p
+                ON p.submission_ptr_id = c.submission_id
+            """,
+            None,
+        ),
+
+        # update all references to the 'CoverLetter' model to point to the new
+        # 'Cover' model instead
+
+        migrations.AlterField(
+            model_name='event',
+            name='cover',
+            field=models.ForeignKey(
+                blank=True,
+                help_text='The cover letter that this event was created for.',
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name='+',
+                to='patchwork.Cover',
+            ),
+        ),
+        migrations.AlterField(
+            model_name='series',
+            name='cover_letter',
+            field=models.OneToOneField(
+                null=True,
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name='series',
+                to='patchwork.Cover'
+            ),
+        ),
+
+        # remove all the old 'CoverLetter'-related entries from the 'Comment'
+        # table
+
+        migrations.RunSQL(
+            """
+            DELETE patchwork_comment FROM
+                patchwork_comment
+            INNER JOIN patchwork_coverletter
+                ON patchwork_coverletter.submission_ptr_id = patchwork_comment.submission_id
+            """,  # noqa
+            None
+        ),
+
+        # delete the old 'CoverLetter' model
+
+        migrations.DeleteModel(
+            name='CoverLetter',
+        ),
+
+        # rename the 'Comment.submission' field to 'Comment.patch'; note our
+        # use of 'AlterField' before and after to work around bug #31335
+        #
+        # https://code.djangoproject.com/ticket/31335
+
+        migrations.AlterField(
+            model_name='comment',
+            name='submission',
+            field=models.ForeignKey(
+                db_constraint=False,
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name='comments',
+                related_query_name='comment',
+                to='patchwork.Submission',
+            ),
+        ),
+        migrations.RemoveIndex(
+            model_name='comment',
+            name='submission_date_idx',
+        ),
+        migrations.RenameField(
+            model_name='comment',
+            old_name='submission',
+            new_name='patch',
+        ),
+        migrations.AlterUniqueTogether(
+            name='comment',
+            unique_together=set([('msgid', 'patch')]),
+        ),
+        migrations.AddIndex(
+            model_name='comment',
+            index=models.Index(
+                fields=['patch', 'date'],
+                name='patch_date_idx',
+            ),
+        ),
+        migrations.AlterField(
+            model_name='comment',
+            name='patch',
+            field=models.ForeignKey(
+                on_delete=django.db.models.deletion.CASCADE,
+                related_name='comments',
+                related_query_name='comment',
+                to='patchwork.Submission',
+            ),
+        ),
+
+        # rename the 'Comment' model to 'PatchComment'
+
+        migrations.RenameModel(
+            old_name='Comment',
+            new_name='PatchComment',
+        ),
+    ]
index 769f602f318e139447c0b989cb0839f80008bdf2..3755b654ebd4a188bc23c8a11b6b5b946cd268fc 100644 (file)
@@ -358,7 +358,7 @@ class FilenameMixin(object):
         return fname
 
 
-class Submission(FilenameMixin, EmailMixin, models.Model):
+class SubmissionMixin(models.Model):
     # parent
 
     project = models.ForeignKey(Project, on_delete=models.CASCADE)
@@ -385,19 +385,10 @@ class Submission(FilenameMixin, EmailMixin, models.Model):
         return self.name
 
     class Meta:
-        ordering = ['date']
-        unique_together = [('msgid', 'project')]
-        indexes = [
-            # This is a covering index for the /list/ query
-            # Like what we have for Patch, but used for displaying what we want
-            # rather than for working out the count (of course, this all
-            # depends on the SQL optimiser of your db engine)
-            models.Index(fields=['date', 'project', 'submitter', 'name'],
-                         name='submission_covering_idx'),
-        ]
+        abstract = True
 
 
-class CoverLetter(Submission):
+class Cover(FilenameMixin, EmailMixin, SubmissionMixin):
 
     def get_absolute_url(self):
         return reverse('cover-detail',
@@ -409,6 +400,35 @@ class CoverLetter(Submission):
                        kwargs={'project_id': self.project.linkname,
                                'msgid': self.url_msgid})
 
+    class Meta:
+        ordering = ['date']
+        unique_together = [('msgid', 'project')]
+        indexes = [
+            # This is a covering index for the /list/ query
+            # Like what we have for Patch, but used for displaying what we want
+            # rather than for working out the count (of course, this all
+            # depends on the SQL optimiser of your DB engine)
+            models.Index(
+                fields=['date', 'project', 'submitter', 'name'],
+                name='cover_covering_idx',
+            ),
+        ]
+
+
+class Submission(SubmissionMixin, FilenameMixin, EmailMixin):
+
+    class Meta:
+        ordering = ['date']
+        unique_together = [('msgid', 'project')]
+        indexes = [
+            # This is a covering index for the /list/ query
+            # Like what we have for Patch, but used for displaying what we want
+            # rather than for working out the count (of course, this all
+            # depends on the SQL optimiser of your db engine)
+            models.Index(fields=['date', 'project', 'submitter', 'name'],
+                         name='submission_covering_idx'),
+        ]
+
 
 class Patch(Submission):
     # patch metadata
@@ -619,44 +639,79 @@ class Patch(Submission):
         ]
 
 
-class Comment(EmailMixin, models.Model):
+class CoverComment(EmailMixin, models.Model):
+
+    cover = models.ForeignKey(
+        Cover,
+        related_name='comments',
+        related_query_name='comment',
+        on_delete=models.CASCADE,
+    )
+
+    @property
+    def list_archive_url(self):
+        if not self.cover.project.list_archive_url_format:
+            return None
+        if not self.msgid:
+            return None
+        return self.project.list_archive_url_format.format(self.url_msgid)
+
+    def get_absolute_url(self):
+        return reverse('comment-redirect', kwargs={'comment_id': self.id})
+
+    def is_editable(self, user):
+        return False
+
+    class Meta:
+        ordering = ['date']
+        unique_together = [('msgid', 'cover')]
+        indexes = [
+            models.Index(name='cover_date_idx', fields=['cover', 'date']),
+        ]
+
+
+class PatchComment(EmailMixin, models.Model):
     # parent
 
-    submission = models.ForeignKey(Submission, related_name='comments',
-                                   related_query_name='comment',
-                                   on_delete=models.CASCADE)
+    patch = models.ForeignKey(
+        Submission,
+        related_name='comments',
+        related_query_name='comment',
+        on_delete=models.CASCADE,
+    )
 
     @property
     def list_archive_url(self):
-        if not self.submission.project.list_archive_url_format:
+        if not self.patch.project.list_archive_url_format:
             return None
         if not self.msgid:
             return None
-        return self.project.list_archive_url_format.format(
+        return self.patch.list_archive_url_format.format(
             self.url_msgid)
 
     def get_absolute_url(self):
         return reverse('comment-redirect', kwargs={'comment_id': self.id})
 
     def save(self, *args, **kwargs):
-        super(Comment, self).save(*args, **kwargs)
-        if hasattr(self.submission, 'patch'):
-            self.submission.patch.refresh_tag_counts()
+        super(PatchComment, self).save(*args, **kwargs)
+        # TODO(stephenfin): Update this once patch is flattened
+        if hasattr(self.patch, 'patch'):
+            self.patch.patch.refresh_tag_counts()
 
     def delete(self, *args, **kwargs):
-        super(Comment, self).delete(*args, **kwargs)
-        if hasattr(self.submission, 'patch'):
-            self.submission.patch.refresh_tag_counts()
+        super(PatchComment, self).delete(*args, **kwargs)
+        # TODO(stephenfin): Update this once patch is flattened
+        if hasattr(self.patch, 'patch'):
+            self.patch.patch.refresh_tag_counts()
 
     def is_editable(self, user):
         return False
 
     class Meta:
         ordering = ['date']
-        unique_together = [('msgid', 'submission')]
+        unique_together = [('msgid', 'patch')]
         indexes = [
-            models.Index(name='submission_date_idx',
-                         fields=['submission', 'date'])
+            models.Index(name='patch_date_idx', fields=['patch', 'date']),
         ]
 
 
@@ -668,9 +723,12 @@ class Series(FilenameMixin, models.Model):
                                 blank=True, on_delete=models.CASCADE)
 
     # content
-    cover_letter = models.OneToOneField(CoverLetter, related_name='series',
-                                        null=True,
-                                        on_delete=models.CASCADE)
+    cover_letter = models.OneToOneField(
+        Cover,
+        related_name='series',
+        null=True,
+        on_delete=models.CASCADE
+    )
 
     # metadata
     name = models.CharField(max_length=255, blank=True, null=True,
@@ -987,7 +1045,7 @@ class Event(models.Model):
         on_delete=models.CASCADE,
         help_text='The series that this event was created for.')
     cover = models.ForeignKey(
-        CoverLetter, related_name='+', null=True, blank=True,
+        Cover, related_name='+', null=True, blank=True,
         on_delete=models.CASCADE,
         help_text='The cover letter that this event was created for.')
 
index 63ff680162e9e21a8ff09277120fe3c66eb8ec98..3e2d2fff4892a4be9c9a7a9f3c0fa2d9a04c2cea 100644 (file)
@@ -18,11 +18,12 @@ from django.contrib.auth.models import User
 from django.db.utils import IntegrityError
 from django.db import transaction
 
-from patchwork.models import Comment
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
+from patchwork.models import CoverComment
 from patchwork.models import DelegationRule
 from patchwork.models import get_default_initial_patch_state
 from patchwork.models import Patch
+from patchwork.models import PatchComment
 from patchwork.models import Person
 from patchwork.models import Project
 from patchwork.models import Series
@@ -664,10 +665,12 @@ def find_submission_for_comment(project, refs):
 
         # see if we have comments that refer to a patch
         try:
-            comment = Comment.objects.get(submission__project=project,
-                                          msgid=ref)
+            comment = PatchComment.objects.get(
+                patch__project=project,
+                msgid=ref,
+            )
             return comment.submission
-        except Comment.MultipleObjectsReturned:
+        except PatchComment.MultipleObjectsReturned:
             # NOTE(stephenfin): This is a artifact of prior lack of support
             # for cover letters in Patchwork. Previously all replies to
             # patches were saved as comments. However, it's possible that
@@ -681,11 +684,36 @@ def find_submission_for_comment(project, refs):
             # apply the comment to the cover letter. Note that this only
             # happens when running 'parsearchive' or similar, so it should not
             # affect every day use in any way.
-            comments = Comment.objects.filter(submission__project=project,
-                                              msgid=ref)
+            comments = PatchComment.objects.filter(
+                patch__project=project,
+                msgid=ref,
+            )
             # The latter item will be the cover letter
             return comments.reverse()[0].submission
-        except Comment.DoesNotExist:
+        except PatchComment.DoesNotExist:
+            pass
+
+    return None
+
+
+def find_cover_for_comment(project, refs):
+    for ref in refs:
+        ref = ref[:255]
+        # first, check for a direct reply
+        try:
+            cover = Cover.objects.get(project=project, msgid=ref)
+            return cover
+        except Cover.DoesNotExist:
+            pass
+
+        # see if we have comments that refer to a patch
+        try:
+            comment = CoverComment.objects.get(
+                cover__project=project,
+                msgid=ref,
+            )
+            return comment.cover
+        except CoverComment.DoesNotExist:
             pass
 
     return None
@@ -1190,11 +1218,11 @@ def parse_mail(mail, list_id=None):
         if not is_comment:
             if not refs == []:
                 try:
-                    CoverLetter.objects.all().get(name=name)
-                except CoverLetter.DoesNotExist:
+                    Cover.objects.all().get(name=name)
+                except Cover.DoesNotExist:
                     # if no match, this is a new cover letter
                     is_cover_letter = True
-                except CoverLetter.MultipleObjectsReturned:
+                except Cover.MultipleObjectsReturned:
                     # if multiple cover letters are found, just ignore
                     pass
             else:
@@ -1228,10 +1256,10 @@ def parse_mail(mail, list_id=None):
                     msgid=msgid, project=project, series=series)
 
             with transaction.atomic():
-                if CoverLetter.objects.filter(project=project, msgid=msgid):
+                if Cover.objects.filter(project=project, msgid=msgid):
                     raise DuplicateMailError(msgid=msgid)
 
-                cover_letter = CoverLetter.objects.create(
+                cover_letter = Cover.objects.create(
                     msgid=msgid,
                     project=project,
                     name=name[:255],
@@ -1250,16 +1278,35 @@ def parse_mail(mail, list_id=None):
 
     # we only save comments if we have the parent email
     submission = find_submission_for_comment(project, refs)
-    if not submission:
+    if submission:
+        author = get_or_create_author(mail, project)
+
+        with transaction.atomic():
+            if PatchComment.objects.filter(patch=patch, msgid=msgid):
+                raise DuplicateMailError(msgid=msgid)
+            comment = PatchComment.objects.create(
+                patch=submission,
+                msgid=msgid,
+                date=date,
+                headers=headers,
+                submitter=author,
+                content=message)
+
+        logger.debug('Comment saved')
+
+        return comment
+
+    cover = find_cover_for_comment(project, refs)
+    if not cover:
         return
 
     author = get_or_create_author(mail, project)
 
     with transaction.atomic():
-        if Comment.objects.filter(submission=submission, msgid=msgid):
+        if CoverComment.objects.filter(cover=cover, msgid=msgid):
             raise DuplicateMailError(msgid=msgid)
-        comment = Comment.objects.create(
-            submission=submission,
+        comment = CoverComment.objects.create(
+            cover=cover,
             msgid=msgid,
             date=date,
             headers=headers,
index 3a2f0fbdd3a44a0a1884925f5fc4aa4c40cb4691..4db9909f6063096bc9d41b0fe707c9afea23a0d9 100644 (file)
@@ -10,7 +10,7 @@ from django.db.models.signals import pre_save
 from django.dispatch import receiver
 
 from patchwork.models import Check
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
 from patchwork.models import Event
 from patchwork.models import Patch
 from patchwork.models import PatchChangeNotification
@@ -54,7 +54,7 @@ def patch_change_callback(sender, instance, raw, **kwargs):
     notification.save()
 
 
-@receiver(post_save, sender=CoverLetter)
+@receiver(post_save, sender=Cover)
 def create_cover_created_event(sender, instance, created, raw, **kwargs):
 
     def create_event(cover):
index f48bfce1abf06ac948d51dd0956bc8c9b9069368..dfbf90497b70a9d6808b714bb2e1671937c98779 100644 (file)
@@ -10,9 +10,10 @@ from django.urls import NoReverseMatch
 from django.urls import reverse
 
 from patchwork.tests.api import utils
-from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_cover
+from patchwork.tests.utils import create_cover_comment
 from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patch_comment
 from patchwork.tests.utils import SAMPLE_CONTENT
 
 if settings.ENABLE_REST_API:
@@ -47,7 +48,7 @@ class TestCoverComments(utils.APITestCase):
     def test_list(self):
         """List cover letter comments."""
         cover = create_cover()
-        comment = create_comment(submission=cover)
+        comment = create_cover_comment(cover=cover)
 
         resp = self.client.get(self.api_url(cover))
         self.assertEqual(status.HTTP_200_OK, resp.status_code)
@@ -57,7 +58,7 @@ class TestCoverComments(utils.APITestCase):
     def test_list_version_1_0(self):
         """List cover letter comments using API v1.0."""
         cover = create_cover()
-        create_comment(submission=cover)
+        create_cover_comment(cover=cover)
 
         # check we can't access comments using the old version of the API
         with self.assertRaises(NoReverseMatch):
@@ -98,7 +99,7 @@ class TestPatchComments(utils.APITestCase):
     def test_list(self):
         """List patch comments."""
         patch = create_patch()
-        comment = create_comment(submission=patch)
+        comment = create_patch_comment(patch=patch)
 
         resp = self.client.get(self.api_url(patch))
         self.assertEqual(status.HTTP_200_OK, resp.status_code)
@@ -108,7 +109,7 @@ class TestPatchComments(utils.APITestCase):
     def test_list_version_1_0(self):
         """List patch comments using API v1.0."""
         patch = create_patch()
-        create_comment(submission=patch)
+        create_patch_comment(patch=patch)
 
         # check we can't access comments using the old version of the API
         with self.assertRaises(NoReverseMatch):
index ddc2b937b074451883f4e798e43b49bf7c729cde..393ebc76eaa9835cf4c22687b3c2295b4f6db7d4 100644 (file)
@@ -6,9 +6,10 @@
 from django.test import TestCase
 from django.urls import reverse
 
-from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_cover
+from patchwork.tests.utils import create_cover_comment
 from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patch_comment
 from patchwork.tests.utils import create_project
 
 
@@ -159,24 +160,32 @@ class PatchViewTest(TestCase):
 
 class CommentRedirectTest(TestCase):
 
-    def _test_redirect(self, submission, submission_url):
-        comment_id = create_comment(submission=submission).id
+    def test_patch_redirect(self):
+        patch = create_patch()
+        comment_id = create_patch_comment(patch=patch).id
 
         requested_url = reverse('comment-redirect',
                                 kwargs={'comment_id': comment_id})
         redirect_url = '%s#%d' % (
-            reverse(submission_url,
-                    kwargs={'project_id': submission.project.linkname,
-                            'msgid': submission.url_msgid}),
+            reverse('patch-detail',
+                    kwargs={'project_id': patch.project.linkname,
+                            'msgid': patch.url_msgid}),
             comment_id)
 
         response = self.client.get(requested_url)
         self.assertRedirects(response, redirect_url)
 
-    def test_patch_redirect(self):
-        patch = create_patch()
-        self._test_redirect(patch, 'patch-detail')
-
     def test_cover_redirect(self):
         cover = create_cover()
-        self._test_redirect(cover, 'cover-detail')
+        comment_id = create_cover_comment(cover=cover).id
+
+        requested_url = reverse('comment-redirect',
+                                kwargs={'comment_id': comment_id})
+        redirect_url = '%s#%d' % (
+            reverse('cover-detail',
+                    kwargs={'project_id': cover.project.linkname,
+                            'msgid': cover.url_msgid}),
+            comment_id)
+
+        response = self.client.get(requested_url)
+        self.assertRedirects(response, redirect_url)
index 1e7bfb010dbb060dd5ad3bf3c61438dad83b513f..a7b0186ae96f48eabfa49e0fc91699fca08a3781 100644 (file)
@@ -13,8 +13,8 @@ import email
 from django.test import TestCase
 from django.urls import reverse
 
-from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patch_comment
 from patchwork.tests.utils import create_project
 from patchwork.tests.utils import create_person
 from patchwork.tests.utils import create_series
@@ -34,8 +34,8 @@ class MboxPatchResponseTest(TestCase):
             project=self.project,
             submitter=self.person,
             content='comment 1 text\nAcked-by: 1\n')
-        create_comment(
-            submission=patch,
+        create_patch_comment(
+            patch=patch,
             submitter=self.person,
             content='comment 2 text\nAcked-by: 2\n')
         response = self.client.get(
@@ -48,8 +48,8 @@ class MboxPatchResponseTest(TestCase):
             project=self.project,
             submitter=self.person,
             content='patch text\n')
-        create_comment(
-            submission=patch,
+        create_patch_comment(
+            patch=patch,
             submitter=self.person,
             content=u'comment\nAcked-by:\u00A0 foo')
         response = self.client.get(
@@ -71,8 +71,8 @@ class MboxPatchSplitResponseTest(TestCase):
             submitter=self.person,
             diff='',
             content='comment 1 text\nAcked-by: 1\n---\nupdate\n')
-        self.comment = create_comment(
-            submission=self.patch,
+        self.comment = create_patch_comment(
+            patch=self.patch,
             submitter=self.person,
             content='comment 2 text\nAcked-by: 2\n')
 
index 07c2b979640f1d399fb8d74f453331ffc29b165e..99e27f10da8031dcba5abab7f67c25ea27b5bacc 100644 (file)
@@ -17,11 +17,11 @@ from django.test import TransactionTestCase
 from django.db.transaction import atomic
 from django.db import connection
 
-from patchwork.models import Comment
+from patchwork.models import Cover
 from patchwork.models import Patch
+from patchwork.models import PatchComment
 from patchwork.models import Person
 from patchwork.models import State
-from patchwork.models import CoverLetter
 from patchwork.parser import clean_subject
 from patchwork.parser import get_or_create_author
 from patchwork.parser import find_patch_content as find_content
@@ -551,7 +551,7 @@ class MultipleProjectPatchCommentTest(MultipleProjectPatchTest):
             patch = Patch.objects.filter(project=project)[0]
             # we should see the reply comment only
             self.assertEqual(
-                Comment.objects.filter(submission=patch).count(), 1)
+                PatchComment.objects.filter(patch=patch).count(), 1)
 
 
 class ListIdHeaderTest(TestCase):
@@ -1157,7 +1157,7 @@ class DuplicateMailTest(TestCase):
         self._test_duplicate_mail(m2)
 
         self.assertEqual(Patch.objects.count(), 1)
-        self.assertEqual(Comment.objects.count(), 1)
+        self.assertEqual(PatchComment.objects.count(), 1)
 
     def test_duplicate_coverletter(self):
         m = create_email('test', listid=self.listid, msgid='1@example.com')
@@ -1166,4 +1166,4 @@ class DuplicateMailTest(TestCase):
 
         self._test_duplicate_mail(m)
 
-        self.assertEqual(CoverLetter.objects.count(), 1)
+        self.assertEqual(Cover.objects.count(), 1)
index 2e86e47cc5a15d2edf38685ae542296252671059..e68ee88e92ab33130081d044ca36c5b8af29882b 100644 (file)
@@ -35,7 +35,7 @@ class _BaseTestCase(TestCase):
         mbox = mailbox.mbox(os.path.join(TEST_SERIES_DIR, name), create=False)
         for msg in mbox:
             obj = parser.parse_mail(msg, project.listid)
-            if type(obj) == models.CoverLetter:
+            if type(obj) == models.Cover:
                 results[0].append(obj)
             elif type(obj) == models.Patch:
                 results[1].append(obj)
index 97afba0af26845cbad0800f441b933cb015d7561..6c13687f893acf27cde73c1b30c92aa4d41d5a73 100644 (file)
@@ -9,8 +9,8 @@ from django.test import TransactionTestCase
 from patchwork.models import Patch
 from patchwork.models import PatchTag
 from patchwork.models import Tag
-from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_patch
+from patchwork.tests.utils import create_patch_comment
 
 
 class ExtractTagsTest(TestCase):
@@ -107,8 +107,8 @@ class PatchTagsTest(TransactionTestCase):
         return '%s-by: Test Tagger <tagger@example.com>\n' % tags[tagtype]
 
     def create_tag_comment(self, patch, tagtype=None):
-        comment = create_comment(
-            submission=patch,
+        comment = create_patch_comment(
+            patch=patch,
             content=self.create_tag(tagtype))
         return comment
 
index 1a7baa285a99588d690ccc394b64749daa474ad3..83bd66a0d7f3bc465f4946349849ea619adaf14f 100644 (file)
@@ -13,9 +13,10 @@ from django.contrib.auth.models import User
 
 from patchwork.models import Bundle
 from patchwork.models import Check
-from patchwork.models import Comment
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
+from patchwork.models import CoverComment
 from patchwork.models import Patch
+from patchwork.models import PatchComment
 from patchwork.models import PatchRelation
 from patchwork.models import Person
 from patchwork.models import Project
@@ -203,8 +204,8 @@ def create_patch(**kwargs):
 
 
 def create_cover(**kwargs):
-    """Create 'CoverLetter' object."""
-    num = CoverLetter.objects.count()
+    """Create 'Cover' object."""
+    num = Cover.objects.count()
 
     # NOTE(stephenfin): Despite first appearances, passing 'series' to the
     # 'create' function doesn't actually cause the relationship to be created.
@@ -232,7 +233,7 @@ def create_cover(**kwargs):
     }
     values.update(kwargs)
 
-    cover = CoverLetter.objects.create(**values)
+    cover = Cover.objects.create(**values)
 
     if series:
         series.add_cover_letter(cover)
@@ -240,17 +241,30 @@ def create_cover(**kwargs):
     return cover
 
 
-def create_comment(**kwargs):
-    """Create 'Comment' object."""
+def create_cover_comment(**kwargs):
+    """Create 'CoverComment' object."""
     values = {
         'submitter': create_person() if 'submitter' not in kwargs else None,
-        'submission': create_patch() if 'submission' not in kwargs else None,
+        'cover': create_cover() if 'cover' not in kwargs else None,
         'msgid': make_msgid(),
         'content': SAMPLE_CONTENT,
     }
     values.update(kwargs)
 
-    return Comment.objects.create(**values)
+    return CoverComment.objects.create(**values)
+
+
+def create_patch_comment(**kwargs):
+    """Create 'PatchComment' object."""
+    values = {
+        'submitter': create_person() if 'submitter' not in kwargs else None,
+        'patch': create_patch() if 'patch' not in kwargs else None,
+        'msgid': make_msgid(),
+        'content': SAMPLE_CONTENT,
+    }
+    values.update(kwargs)
+
+    return PatchComment.objects.create(**values)
 
 
 def create_check(**kwargs):
index 9c3b85f007a74369bf8332ea00a9c7ca9851fc27..7d888d4a3dc0152003a7ce4c2c6d92e164e03641 100644 (file)
@@ -249,10 +249,10 @@ if settings.ENABLE_REST_API:
 
     api_1_1_patterns = [
         url(r'^patches/(?P<pk>[^/]+)/comments/$',
-            api_comment_views.CommentList.as_view(),
+            api_comment_views.PatchCommentList.as_view(),
             name='api-patch-comment-list'),
         url(r'^covers/(?P<pk>[^/]+)/comments/$',
-            api_comment_views.CommentList.as_view(),
+            api_comment_views.CoverCommentList.as_view(),
             name='api-cover-comment-list'),
     ]
 
index 7dee8dc40c7ecc7abf52ea9e4578bd8dc015c19a..4f699224df601f1df5f5dbfe651ebbe0e15166d3 100644 (file)
@@ -4,20 +4,41 @@
 # SPDX-License-Identifier: GPL-2.0-or-later
 
 from django import http
-from django import shortcuts
 from django.urls import reverse
 
 from patchwork import models
 
 
 def comment(request, comment_id):
-    submission = shortcuts.get_object_or_404(models.Comment,
-                                             id=comment_id).submission
-    if models.Patch.objects.filter(id=submission.id).exists():
-        url = 'patch-detail'
-    else:
-        url = 'cover-detail'
-
-    return http.HttpResponseRedirect('%s#%s' % (
-        reverse(url, kwargs={'project_id': submission.project.linkname,
-                             'msgid': submission.url_msgid}), comment_id))
+    patch = None
+    cover = None
+
+    try:
+        patch = models.PatchComment.objects.get(id=comment_id).patch
+    except models.PatchComment.DoesNotExist:
+        try:
+            cover = models.CoverComment.objects.get(id=comment_id).cover
+        except models.CoverComment.DoesNotExist:
+            pass
+
+    if not patch and not cover:
+        raise http.Http404('No comment matches the given query.')
+
+    if patch:
+        url = reverse(
+            'patch-detail',
+            kwargs={
+                'project_id': patch.project.linkname,
+                'msgid': patch.url_msgid,
+            },
+        )
+    else:  # cover
+        url = reverse(
+            'cover-detail',
+            kwargs={
+                'project_id': cover.project.linkname,
+                'msgid': cover.url_msgid,
+            },
+        )
+
+    return http.HttpResponseRedirect('%s#%s' % (url, comment_id))
index e90b73735e8967ee993dca5e922574731668776a..8ab0ba993650d4b6298c755d1ac78e0ac664ebd0 100644 (file)
@@ -10,7 +10,7 @@ from django.shortcuts import get_object_or_404
 from django.shortcuts import render
 from django.urls import reverse
 
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
 from patchwork.models import Patch
 from patchwork.models import Project
 from patchwork.views.utils import cover_to_mbox
@@ -22,7 +22,7 @@ def cover_detail(request, project_id, msgid):
 
     # redirect to patches where necessary
     try:
-        cover = get_object_or_404(CoverLetter, project_id=project.id,
+        cover = get_object_or_404(Cover, project_id=project.id,
                                   msgid=db_msgid)
     except Http404 as exc:
         patches = Patch.objects.filter(
@@ -44,7 +44,7 @@ def cover_detail(request, project_id, msgid):
     comments = cover.comments.all()
     comments = comments.select_related('submitter')
     comments = comments.only('submitter', 'date', 'id', 'content',
-                             'submission')
+                             'cover')
     context['comments'] = comments
 
     return render(request, 'patchwork/submission.html', context)
@@ -53,7 +53,7 @@ def cover_detail(request, project_id, msgid):
 def cover_mbox(request, project_id, msgid):
     db_msgid = ('<%s>' % msgid)
     project = get_object_or_404(Project, linkname=project_id)
-    cover = get_object_or_404(CoverLetter, project_id=project.id,
+    cover = get_object_or_404(Cover, project_id=project.id,
                               msgid=db_msgid)
 
     response = HttpResponse(content_type='text/plain')
@@ -65,7 +65,7 @@ def cover_mbox(request, project_id, msgid):
 
 
 def cover_by_id(request, cover_id):
-    cover = get_object_or_404(CoverLetter, id=cover_id)
+    cover = get_object_or_404(Cover, id=cover_id)
 
     url = reverse('cover-detail', kwargs={'project_id': cover.project.linkname,
                                           'msgid': cover.url_msgid})
@@ -74,7 +74,7 @@ def cover_by_id(request, cover_id):
 
 
 def cover_mbox_by_id(request, cover_id):
-    cover = get_object_or_404(CoverLetter, id=cover_id)
+    cover = get_object_or_404(Cover, id=cover_id)
 
     url = reverse('cover-mbox', kwargs={'project_id': cover.project.linkname,
                                         'msgid': cover.url_msgid})
index 9fdbbf9bf9da257f80956f42523f848e5a5096a8..5d772fdf6adc664fa4b0be88f87eac7d969d8232 100644 (file)
@@ -15,7 +15,7 @@ from django.urls import reverse
 from patchwork.forms import CreateBundleForm
 from patchwork.forms import PatchForm
 from patchwork.models import Bundle
-from patchwork.models import CoverLetter
+from patchwork.models import Cover
 from patchwork.models import Patch
 from patchwork.models import Project
 from patchwork.views import generic_list
@@ -42,7 +42,7 @@ def patch_detail(request, project_id, msgid):
     try:
         patch = Patch.objects.get(project_id=project.id, msgid=db_msgid)
     except Patch.DoesNotExist:
-        covers = CoverLetter.objects.filter(
+        covers = Cover.objects.filter(
             project_id=project.id,
             msgid=db_msgid,
         )
@@ -109,8 +109,7 @@ def patch_detail(request, project_id, msgid):
 
     comments = patch.comments.all()
     comments = comments.select_related('submitter')
-    comments = comments.only('submitter', 'date', 'id', 'content',
-                             'submission')
+    comments = comments.only('submitter', 'date', 'id', 'content', 'patch')
 
     if patch.related:
         related_same_project = patch.related.patches.only(
index 4419702ee8c0d6666daf5cbcd59730b57f469b02..f02948c2391d4f1320ec3ed1e51ce4d34aad2a5a 100644 (file)
@@ -15,8 +15,9 @@ import re
 from django.conf import settings
 from django.http import Http404
 
-from patchwork.models import Comment
+from patchwork.models import CoverComment
 from patchwork.models import Patch
+from patchwork.models import PatchComment
 from patchwork.parser import split_from_header
 
 if settings.ENABLE_REST_API:
@@ -34,12 +35,12 @@ class PatchMbox(MIMENonMultipart):
 
 
 def _submission_to_mbox(submission):
-    """Get an mbox representation of a single Submission.
+    """Get an mbox representation of a single submission.
 
-    Handles both Patch and CoverLetter objects.
+    Handles both Patch and Cover objects.
 
     Arguments:
-        submission: The Patch object to convert.
+        submission: The Patch or Cover object to convert.
 
     Returns:
         A string for the mbox file.
@@ -61,8 +62,12 @@ def _submission_to_mbox(submission):
         postscript = ''
 
     # TODO(stephenfin): Make this use the tags infrastructure
-    for comment in Comment.objects.filter(submission=submission):
-        body += comment.patch_responses
+    if is_patch:
+        for comment in PatchComment.objects.filter(patch=submission):
+            body += comment.patch_responses
+    else:
+        for comment in CoverComment.objects.filter(cover=submission):
+            body += comment.patch_responses
 
     if postscript:
         body += '---\n' + postscript + '\n'