diff mbox

[v2,2/3] models: Split Patch into two models

Message ID 1457909744-4502-3-git-send-email-stephen.finucane@intel.com
State Superseded
Headers show

Commit Message

Stephen Finucane March 13, 2016, 10:55 p.m. UTC
There are a lot of similarities between cover letters and patches: so
many, in fact, that it would be helpful to occasionally treat them as
the same thing. Achieve this by extracting out the fields that would be
shared between a Patch and a hypothetical cover letter into a "sub
model". This allows us to do cool stuff like assigning comments to both
patches and cover letters or listing both patches and cover letters on
the main screen in a natural way.

The migrations for this are really the only complicated part. There are
three, broken up into schema and data migrations per Django customs,
and they works as follows:

* Rename the 'Patch' model to 'Submission', then create a subclass
  called 'Patch' that includes duplicates of the patch-specific fields
  of Submission (with changed names to prevent conflicts). Rename
  non-patch specific references to the renamed 'Submission' model
  as necessary.
* Duplicate the contents of the patch-specific fields from 'Submission'
  to 'Patch'
* Remove the patch-specific fields from 'Submission', renaming the
  'Patch' model to take their place. Update the patch-specific
  references to point the new 'Patch' model, rather than 'Submission'.

This comes at the cost of an additional JOIN per item on the main
screen, but this seems a small price to pay for the additional
functionality gained. To minimise this, however, caching will be added.

Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
---
v2: Correct mistake in reverse migration for 0010
---
 patchwork/admin.py                                 |  17 ++-
 patchwork/bin/parsemail.py                         |   8 +-
 patchwork/forms.py                                 |   2 +-
 patchwork/migrations/0009_add_submission_model.py  |  80 ++++++++++++++
 .../0010_migrate_data_from_submission_to_patch.py  |  51 +++++++++
 patchwork/migrations/0011_remove_temp_fields.py    | 121 +++++++++++++++++++++
 patchwork/models.py                                |  52 ++++++---
 patchwork/paginator.py                             |  10 +-
 patchwork/settings/base.py                         |   2 +-
 patchwork/tests/test_mboxviews.py                  |   4 +-
 patchwork/tests/test_patchparser.py                |   3 +-
 patchwork/tests/test_tags.py                       |   3 +-
 patchwork/tests/test_user.py                       |  12 +-
 patchwork/views/__init__.py                        |   2 +-
 14 files changed, 326 insertions(+), 41 deletions(-)
 create mode 100644 patchwork/migrations/0009_add_submission_model.py
 create mode 100644 patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
 create mode 100644 patchwork/migrations/0011_remove_temp_fields.py

Comments

Stephen Finucane March 15, 2016, 3:22 p.m. UTC | #1
On 13 Mar 22:55, Stephen Finucane wrote:

[snip]

> This comes at the cost of an additional JOIN per item on the main
> screen, but this seems a small price to pay for the additional
> functionality gained. To minimise this, however, caching will be added.

[snip]

So now that we have django-debug-toolbar working, I was able to inspect
the actual database queries to see how the INNER JOIN affected
performance. The end result: virtually no difference. This is great
news, as we get the benefits that this change brings with virtually no
downsides. We can still add some caching, but this will give us an
improvement on existing performance.

Stephen
Andy Doan March 16, 2016, 9:28 p.m. UTC | #2
On 03/13/2016 05:55 PM, Stephen Finucane wrote:

>   create mode 100644 patchwork/migrations/0009_add_submission_model.py
>   create mode 100644 patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
>   create mode 100644 patchwork/migrations/0011_remove_temp_fields.py

I see how having these split up makes the code more readable, but aren't 
they sort of all-or-nothing?

> diff --git a/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py

> +def create_patch_instances(apps, schema_editor):
> +    Submission = apps.get_model('patchwork', 'Submission')
> +    Patch = apps.get_model('patchwork', 'Patch')
> +
> +    for submission in Submission.objects.all():
> +        # NOTE(sfinucan) We copy every field _except_ tags, which is
> +        # autogenerated anyway
> +        patch = Patch(submission_ptr=submission,
> +                      diff2=submission.diff,
> +                      commit_ref2=submission.commit_ref,
> +                      pull_url2=submission.pull_url,
> +                      delegate2=submission.delegate,
> +                      state2=submission.state,
> +                      archived2=submission.archived,
> +                      hash2=submission.hash)
> +        patch.__dict__.update(submission.__dict__)
> +        patch.save()

This logic is very time-consuming. It took 20 minutes on my instance. I 
just hacked this together in pure SQL to run in about 8 seconds (not 
really tested):

migrations.RunSQL(
             ['''INSERT INTO patchwork_patch
                   (submission_ptr_id, diff2, commit_ref2, pull_url2,
                    delegate2_id, state2_id, archived2, hash2)
                 SELECT id, diff, commit_ref, pull_url, delegate_id,
                        state_id, archived, hash
                 FROM patchwork_submission
                 '''],
             ['TODO THE reverse migration i haven't figured out yet']
         ),
Andy Doan March 16, 2016, 9:46 p.m. UTC | #3
On 03/16/2016 04:28 PM, Andy Doan wrote:
> This logic is very time-consuming. It took 20 minutes on my instance. I
> just hacked this together in pure SQL to run in about 8 seconds (not
> really tested):
>
> migrations.RunSQL(
>              ['''INSERT INTO patchwork_patch
>                    (submission_ptr_id, diff2, commit_ref2, pull_url2,
>                     delegate2_id, state2_id, archived2, hash2)
>                  SELECT id, diff, commit_ref, pull_url, delegate_id,
>                         state_id, archived, hash
>                  FROM patchwork_submission
>                  '''],
>              ['TODO THE reverse migration i haven't figured out yet']
>          ),

I figured out the reverse migration path also. Its a little more time 
consuming at 30 seconds, but still not too bad:

['''UPDATE patchwork_submission SET
                   diff=diff2, commit_ref=commit_ref2, pull_url=pull_url2,
                   delegate_id=delegate2_id, state_id=state2_id,
                   archived=archived2, hash=hash2
                 FROM patchwork_patch WHERE
                   patchwork_submission.id = 
patchwork_patch.submission_ptr_id
                 ''']

Full source: http://paste.ubuntu.com/15404553/
Stephen Finucane March 21, 2016, 6:12 p.m. UTC | #4
On 16 Mar 16:28, Andy Doan wrote:
> On 03/13/2016 05:55 PM, Stephen Finucane wrote:
> 
> >  create mode 100644 patchwork/migrations/0009_add_submission_model.py
> >  create mode 100644 patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
> >  create mode 100644 patchwork/migrations/0011_remove_temp_fields.py
> 
> I see how having these split up makes the code more readable, but
> aren't they sort of all-or-nothing?

They are, but I was going on the advice of the Django documentation
and breaking up data migrations from schema migrations [1]. In a
similar vein, the few guides I found on the subject recommended
splitting things up, though these were targeting South. TBH, I'm
happy to merge them, but I also don't see any disadvantages to
keeping them separate.

> >diff --git a/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
> 
> >+def create_patch_instances(apps, schema_editor):
> >+    Submission = apps.get_model('patchwork', 'Submission')
> >+    Patch = apps.get_model('patchwork', 'Patch')
> >+
> >+    for submission in Submission.objects.all():
> >+        # NOTE(sfinucan) We copy every field _except_ tags, which is
> >+        # autogenerated anyway
> >+        patch = Patch(submission_ptr=submission,
> >+                      diff2=submission.diff,
> >+                      commit_ref2=submission.commit_ref,
> >+                      pull_url2=submission.pull_url,
> >+                      delegate2=submission.delegate,
> >+                      state2=submission.state,
> >+                      archived2=submission.archived,
> >+                      hash2=submission.hash)
> >+        patch.__dict__.update(submission.__dict__)
> >+        patch.save()
> 
> This logic is very time-consuming. It took 20 minutes on my
> instance. I just hacked this together in pure SQL to run in about 8
> seconds (not really tested):
> 
> migrations.RunSQL(
>             ['''INSERT INTO patchwork_patch
>                   (submission_ptr_id, diff2, commit_ref2, pull_url2,
>                    delegate2_id, state2_id, archived2, hash2)
>                 SELECT id, diff, commit_ref, pull_url, delegate_id,
>                        state_id, archived, hash
>                 FROM patchwork_submission
>                 '''],
>             ['TODO THE reverse migration i haven't figured out yet']
>         ),

Good idea. I'll send an update patchset with this included shortly.

Stephen

[1] https://docs.djangoproject.com/en/1.9/topics/migrations/#data-migrations
diff mbox

Patch

diff --git a/patchwork/admin.py b/patchwork/admin.py
index 22d95e7..707a376 100644
--- a/patchwork/admin.py
+++ b/patchwork/admin.py
@@ -21,8 +21,9 @@  from __future__ import absolute_import
 
 from django.contrib import admin
 
-from patchwork.models import (Project, Person, UserProfile, State, Patch,
-                              Comment, Bundle, Tag, Check, DelegationRule)
+from patchwork.models import (Project, Person, UserProfile, State, Submission,
+                              Patch, Comment, Bundle, Tag, Check,
+                              DelegationRule)
 
 
 class DelegationRuleInline(admin.TabularInline):
@@ -61,6 +62,14 @@  class StateAdmin(admin.ModelAdmin):
 admin.site.register(State, StateAdmin)
 
 
+class SubmissionAdmin(admin.ModelAdmin):
+    list_display = ('name', 'submitter', 'project', 'date')
+    list_filter = ('project', )
+    search_fields = ('name', 'submitter__name', 'submitter__email')
+    date_hierarchy = 'date'
+admin.site.register(Submission, SubmissionAdmin)
+
+
 class PatchAdmin(admin.ModelAdmin):
     list_display = ('name', 'submitter', 'project', 'state', 'date',
                     'archived', 'is_pull_request')
@@ -78,8 +87,8 @@  admin.site.register(Patch, PatchAdmin)
 
 
 class CommentAdmin(admin.ModelAdmin):
-    list_display = ('patch', 'submitter', 'date')
-    search_fields = ('patch__name', 'submitter__name', 'submitter__email')
+    list_display = ('submission', 'submitter', 'date')
+    search_fields = ('submission__name', 'submitter__name', 'submitter__email')
     date_hierarchy = 'date'
 admin.site.register(Comment, CommentAdmin)
 
diff --git a/patchwork/bin/parsemail.py b/patchwork/bin/parsemail.py
index 9640ff3..5fcc6c3 100755
--- a/patchwork/bin/parsemail.py
+++ b/patchwork/bin/parsemail.py
@@ -265,7 +265,8 @@  def find_content(project, mail):
         cpatch = find_patch_for_comment(project, mail)
         if not cpatch:
             return (None, None, None)
-        comment = Comment(patch=cpatch, date=mail_date(mail),
+        comment = Comment(submission=cpatch,
+                          date=mail_date(mail),
                           content=clean_content(commentbuf),
                           headers=mail_headers(mail))
 
@@ -297,8 +298,9 @@  def find_patch_for_comment(project, mail):
 
         # see if we have comments that refer to a patch
         try:
-            comment = Comment.objects.get(patch__project=project, msgid=ref)
-            return comment.patch
+            comment = Comment.objects.get(submission__project=project,
+                                          msgid=ref)
+            return comment.submission
         except Comment.DoesNotExist:
             pass
 
diff --git a/patchwork/forms.py b/patchwork/forms.py
index 628761b..3f876b7 100644
--- a/patchwork/forms.py
+++ b/patchwork/forms.py
@@ -122,7 +122,7 @@  class UserProfileForm(forms.ModelForm):
 
     class Meta:
         model = UserProfile
-        fields = ['primary_project', 'patches_per_page']
+        fields = ['primary_project', 'items_per_page']
 
 
 class OptionalDelegateField(DelegateField):
diff --git a/patchwork/migrations/0009_add_submission_model.py b/patchwork/migrations/0009_add_submission_model.py
new file mode 100644
index 0000000..6bb68fb
--- /dev/null
+++ b/patchwork/migrations/0009_add_submission_model.py
@@ -0,0 +1,80 @@ 
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.conf import settings
+from django.db import migrations, models
+
+import patchwork.models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0008_add_email_mixin'),
+    ]
+
+    operations = [
+        # Rename the 'Patch' to 'Submission'
+        migrations.RenameModel(
+            old_name='Patch',
+            new_name='Submission'
+        ),
+        migrations.AlterModelOptions(
+            name='submission',
+            options={'ordering': ['date']},
+        ),
+
+        # Rename the non-Patch specific references to point to Submission
+        migrations.RenameField(
+            model_name='comment',
+            old_name='patch',
+            new_name='submission',
+        ),
+        migrations.AlterUniqueTogether(
+            name='comment',
+            unique_together=set([('msgid', 'submission')]),
+        ),
+        migrations.RenameField(
+            model_name='userprofile',
+            old_name='patches_per_page',
+            new_name='items_per_page',
+        ),
+        migrations.AlterField(
+            model_name='userprofile',
+            name='items_per_page',
+            field=models.PositiveIntegerField(
+                default=100,
+                help_text=b'Number of items to display per page'),
+        ),
+
+        # Recreate the 'Patch' model as a subclass of 'Submission'. Each field
+        # is given a unique name to prevent it conflicting with the same field
+        # found in the 'Submission' "super model". We will fix this later.
+        migrations.CreateModel(
+            name='Patch',
+            fields=[
+                ('submission_ptr', models.OneToOneField(
+                    parent_link=True, auto_created=True, primary_key=True,
+                    serialize=False, to='patchwork.Submission')),
+                ('diff2', models.TextField(null=True, blank=True)),
+                ('commit_ref2', models.CharField(
+                    max_length=255, null=True, blank=True)),
+                ('pull_url2', models.CharField(
+                    max_length=255, null=True, blank=True)),
+                # we won't migrate the data of this, seeing as it's
+                # automatically recreated every time we save a Patch
+                ('tags2', models.ManyToManyField(
+                    to='patchwork.Tag', through='patchwork.PatchTag')),
+                ('delegate2', models.ForeignKey(
+                    blank=True, to=settings.AUTH_USER_MODEL, null=True)),
+                ('state2', models.ForeignKey(to='patchwork.State')),
+                ('archived2', models.BooleanField(default=False)),
+                ('hash2', patchwork.models.HashField(
+                    max_length=40, null=True, blank=True)),
+            ],
+            options={
+                'verbose_name_plural': 'Patches',
+            },
+            bases=('patchwork.submission',),
+        ),
+    ]
diff --git a/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
new file mode 100644
index 0000000..0da4567
--- /dev/null
+++ b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
@@ -0,0 +1,51 @@ 
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+def create_patch_instances(apps, schema_editor):
+    Submission = apps.get_model('patchwork', 'Submission')
+    Patch = apps.get_model('patchwork', 'Patch')
+
+    for submission in Submission.objects.all():
+        # NOTE(sfinucan) We copy every field _except_ tags, which is
+        # autogenerated anyway
+        patch = Patch(submission_ptr=submission,
+                      diff2=submission.diff,
+                      commit_ref2=submission.commit_ref,
+                      pull_url2=submission.pull_url,
+                      delegate2=submission.delegate,
+                      state2=submission.state,
+                      archived2=submission.archived,
+                      hash2=submission.hash)
+        patch.__dict__.update(submission.__dict__)
+        patch.save()
+
+
+def remove_patch_instances(apps, schema_editor):
+    Patch = apps.get_model('patchwork', 'Patch')
+
+    for patch in Patch.objects.all():
+        patch.diff = patch.diff2
+        patch.commit_ref = patch.commit_ref2
+        patch.pull_url = patch.pull_url2
+        patch.delegate = patch.delegate2
+        patch.state = patch.state2
+        patch.archived = patch.archived2
+        patch.hash = patch.hash2
+        patch.save()
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0009_add_submission_model'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            code=create_patch_instances,
+            reverse_code=remove_patch_instances,
+        ),
+    ]
diff --git a/patchwork/migrations/0011_remove_temp_fields.py b/patchwork/migrations/0011_remove_temp_fields.py
new file mode 100644
index 0000000..6b159c5
--- /dev/null
+++ b/patchwork/migrations/0011_remove_temp_fields.py
@@ -0,0 +1,121 @@ 
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0010_migrate_data_from_submission_to_patch'),
+    ]
+
+    operations = [
+        # Remove duplicate fields from 'Submission' and rename 'Patch' version
+        migrations.RemoveField(
+            model_name='submission',
+            name='diff',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='diff2',
+            new_name='diff',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='commit_ref',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='commit_ref2',
+            new_name='commit_ref',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='pull_url',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='pull_url2',
+            new_name='pull_url',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='tags',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='tags2',
+            new_name='tags',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='delegate',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='delegate2',
+            new_name='delegate',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='state',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='state2',
+            new_name='state',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='archived',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='archived2',
+            new_name='archived',
+        ),
+        migrations.RemoveField(
+            model_name='submission',
+            name='hash',
+        ),
+        migrations.RenameField(
+            model_name='patch',
+            old_name='hash2',
+            new_name='hash',
+        ),
+        # Update any many-to-many fields to point to Patch now
+        migrations.AlterField(
+            model_name='bundle',
+            name='patches',
+            field=models.ManyToManyField(to='patchwork.Patch',
+                                         through='patchwork.BundlePatch'),
+        ),
+        migrations.AlterField(
+            model_name='bundlepatch',
+            name='patch',
+            field=models.ForeignKey(to='patchwork.Patch'),
+        ),
+        migrations.AlterField(
+            model_name='check',
+            name='patch',
+            field=models.ForeignKey(to='patchwork.Patch'),
+        ),
+        migrations.AlterField(
+            model_name='patch',
+            name='state',
+            field=models.ForeignKey(to='patchwork.State', null=True),
+        ),
+        migrations.AlterField(
+            model_name='patchchangenotification',
+            name='patch',
+            field=models.OneToOneField(primary_key=True,
+                                       serialize=False,
+                                       to='patchwork.Patch'),
+        ),
+        migrations.AlterField(
+            model_name='patchtag',
+            name='patch',
+            field=models.ForeignKey(to='patchwork.Patch'),
+        ),
+    ]
diff --git a/patchwork/models.py b/patchwork/models.py
index e54c4fc..b561728 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -131,9 +131,9 @@  class UserProfile(models.Model):
         default=False,
         help_text='Selecting this option allows patchwork to send email on'
         ' your behalf')
-    patches_per_page = models.PositiveIntegerField(
+    items_per_page = models.PositiveIntegerField(
         default=100, null=False, blank=False,
-        help_text='Number of patches to display per page')
+        help_text='Number of items to display per page')
 
     def name(self):
         if self.user.first_name or self.user.last_name:
@@ -144,7 +144,7 @@  class UserProfile(models.Model):
 
     def contributor_projects(self):
         submitters = Person.objects.filter(user=self.user)
-        return Project.objects.filter(id__in=Patch.objects.filter(
+        return Project.objects.filter(id__in=Submission.objects.filter(
             submitter__in=submitters).values('project_id').query)
 
     def sync_person(self):
@@ -283,7 +283,8 @@  class PatchQuerySet(models.query.QuerySet):
             select[tag.attr_name] = (
                 "coalesce("
                 "(SELECT count FROM patchwork_patchtag"
-                " WHERE patchwork_patchtag.patch_id=patchwork_patch.id"
+                " WHERE patchwork_patchtag.patch_id="
+                "patchwork_patch.submission_ptr_id"
                 " AND patchwork_patchtag.tag_id=%s), 0)")
             select_params.append(tag.id)
 
@@ -329,14 +330,35 @@  class EmailMixin(models.Model):
 
 
 @python_2_unicode_compatible
-class Patch(EmailMixin, models.Model):
+class Submission(EmailMixin, models.Model):
     # parent
 
     project = models.ForeignKey(Project)
 
-    # patch metadata
+    # submission metadata
 
     name = models.CharField(max_length=255)
+
+    # patchwork metadata
+
+    def refresh_tag_counts(self):
+        pass  # TODO(sfinucan) Once this is only called for patches, remove
+
+    def is_editable(self, user):
+        return False
+
+    def __str__(self):
+        return self.name
+
+    class Meta:
+        ordering = ['date']
+        unique_together = [('msgid', 'project')]
+
+
+@python_2_unicode_compatible
+class Patch(Submission):
+    # patch metadata
+
     diff = models.TextField(null=True, blank=True)
     commit_ref = models.CharField(max_length=255, null=True, blank=True)
     pull_url = models.CharField(max_length=255, null=True, blank=True)
@@ -355,7 +377,7 @@  class Patch(EmailMixin, models.Model):
         if count == 0:
             self.patchtag_set.filter(tag=tag).delete()
             return
-        (patchtag, _) = PatchTag.objects.get_or_create(patch=self, tag=tag)
+        patchtag, _ = PatchTag.objects.get_or_create(patch=self, tag=tag)
         if patchtag.count != count:
             patchtag.count = count
             patchtag.save()
@@ -477,27 +499,25 @@  class Patch(EmailMixin, models.Model):
 
     class Meta:
         verbose_name_plural = 'Patches'
-        ordering = ['date']
-        unique_together = [('msgid', 'project')]
 
 
 class Comment(EmailMixin, models.Model):
     # parent
 
-    patch = models.ForeignKey(Patch, related_name='comments',
-                              related_query_name='comment')
+    submission = models.ForeignKey(Submission, related_name='comments',
+                                   related_query_name='comment')
 
     def save(self, *args, **kwargs):
         super(Comment, self).save(*args, **kwargs)
-        self.patch.refresh_tag_counts()
+        self.submission.refresh_tag_counts()
 
     def delete(self, *args, **kwargs):
         super(Comment, self).delete(*args, **kwargs)
-        self.patch.refresh_tag_counts()
+        self.submission.refresh_tag_counts()
 
     class Meta:
         ordering = ['date']
-        unique_together = [('msgid', 'patch')]
+        unique_together = [('msgid', 'submission')]
 
 
 class Bundle(models.Model):
@@ -524,7 +544,8 @@  class Bundle(models.Model):
             max_order = 0
 
         # see if the patch is already in this bundle
-        if BundlePatch.objects.filter(bundle=self, patch=patch).count():
+        if BundlePatch.objects.filter(bundle=self,
+                                      patch=patch).count():
             raise Exception('patch is already in bundle')
 
         bp = BundlePatch.objects.create(bundle=self, patch=patch,
@@ -682,7 +703,6 @@  def _patch_change_callback(sender, instance, **kwargs):
     if notification is None:
         notification = PatchChangeNotification(patch=instance,
                                                orig_state=orig_patch.state)
-
     elif notification.orig_state == instance.state:
         # If we're back at the original state, there is no need to notify
         notification.delete()
diff --git a/patchwork/paginator.py b/patchwork/paginator.py
index 0f6d684..5ae0346 100644
--- a/patchwork/paginator.py
+++ b/patchwork/paginator.py
@@ -24,7 +24,7 @@  from django.core import paginator
 from django.utils.six.moves import range
 
 
-DEFAULT_PATCHES_PER_PAGE = 100
+DEFAULT_ITEMS_PER_PAGE = 100
 LONG_PAGE_THRESHOLD = 30
 LEADING_PAGE_RANGE_DISPLAYED = 4
 TRAILING_PAGE_RANGE_DISPLAYED = 2
@@ -41,19 +41,19 @@  class Paginator(paginator.Paginator):
 
     def __init__(self, request, objects):
 
-        patches_per_page = settings.DEFAULT_PATCHES_PER_PAGE
+        items_per_page = settings.DEFAULT_ITEMS_PER_PAGE
 
         if request.user.is_authenticated():
-            patches_per_page = request.user.profile.patches_per_page
+            items_per_page = request.user.profile.items_per_page
 
         ppp = request.META.get('ppp')
         if ppp:
             try:
-                patches_per_page = int(ppp)
+                items_per_page = int(ppp)
             except ValueError:
                 pass
 
-        super(Paginator, self).__init__(objects, patches_per_page)
+        super(Paginator, self).__init__(objects, items_per_page)
 
         try:
             page_no = int(request.GET.get('page'))
diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
index ef2a9ee..4ed73fc 100644
--- a/patchwork/settings/base.py
+++ b/patchwork/settings/base.py
@@ -126,7 +126,7 @@  STATICFILES_DIRS = [
 # Patchwork settings
 #
 
-DEFAULT_PATCHES_PER_PAGE = 100
+DEFAULT_ITEMS_PER_PAGE = 100
 
 CONFIRMATION_VALIDITY_DAYS = 7
 
diff --git a/patchwork/tests/test_mboxviews.py b/patchwork/tests/test_mboxviews.py
index 0bba9e2..7c6e9fc 100644
--- a/patchwork/tests/test_mboxviews.py
+++ b/patchwork/tests/test_mboxviews.py
@@ -47,7 +47,7 @@  class MboxPatchResponseTest(TestCase):
                            content='comment 1 text\nAcked-by: 1\n')
         self.patch.save()
 
-        comment = Comment(patch=self.patch,
+        comment = Comment(submission=self.patch,
                           msgid='p2',
                           submitter=self.person,
                           content='comment 2 text\nAcked-by: 2\n')
@@ -78,7 +78,7 @@  class MboxPatchSplitResponseTest(TestCase):
             content='comment 1 text\nAcked-by: 1\n---\nupdate\n')
         self.patch.save()
 
-        comment = Comment(patch=self.patch,
+        comment = Comment(submission=self.patch,
                           msgid='p2',
                           submitter=self.person,
                           content='comment 2 text\nAcked-by: 2\n')
diff --git a/patchwork/tests/test_patchparser.py b/patchwork/tests/test_patchparser.py
index 1fba35c..760341c 100644
--- a/patchwork/tests/test_patchparser.py
+++ b/patchwork/tests/test_patchparser.py
@@ -327,7 +327,8 @@  class MultipleProjectPatchCommentTest(MultipleProjectPatchTest):
         for project in [self.p1, self.p2]:
             patch = Patch.objects.filter(project=project)[0]
             # we should see the reply comment only
-            self.assertEqual(Comment.objects.filter(patch=patch).count(), 1)
+            self.assertEqual(
+                Comment.objects.filter(submission=patch).count(), 1)
 
 
 class ListIdHeaderTest(TestCase):
diff --git a/patchwork/tests/test_tags.py b/patchwork/tests/test_tags.py
index b57d5fd..417a5b3 100644
--- a/patchwork/tests/test_tags.py
+++ b/patchwork/tests/test_tags.py
@@ -114,7 +114,8 @@  class PatchTagsTest(TransactionTestCase):
         return '%s-by: %s\n' % (tags[tagtype], self.tagger)
 
     def create_tag_comment(self, patch, tagtype=None):
-        comment = Comment(patch=patch, msgid=str(datetime.datetime.now()),
+        comment = Comment(submission=patch,
+                          msgid=str(datetime.datetime.now()),
                           submitter=defaults.patch_author_person,
                           content=self.create_tag(tagtype))
         comment.save()
diff --git a/patchwork/tests/test_user.py b/patchwork/tests/test_user.py
index 2d8ebf6..1a42659 100644
--- a/patchwork/tests/test_user.py
+++ b/patchwork/tests/test_user.py
@@ -178,23 +178,23 @@  class UserProfileTest(TestCase):
 
     def testUserProfileValidPost(self):
         user_profile = UserProfile.objects.get(user=self.user.user.id)
-        old_ppp = user_profile.patches_per_page
+        old_ppp = user_profile.items_per_page
         new_ppp = old_ppp + 1
 
-        self.client.post('/user/', {'patches_per_page': new_ppp})
+        self.client.post('/user/', {'items_per_page': new_ppp})
 
         user_profile = UserProfile.objects.get(user=self.user.user.id)
-        self.assertEqual(user_profile.patches_per_page, new_ppp)
+        self.assertEqual(user_profile.items_per_page, new_ppp)
 
     def testUserProfileInvalidPost(self):
         user_profile = UserProfile.objects.get(user=self.user.user.id)
-        old_ppp = user_profile.patches_per_page
+        old_ppp = user_profile.items_per_page
         new_ppp = -1
 
-        self.client.post('/user/', {'patches_per_page': new_ppp})
+        self.client.post('/user/', {'items_per_page': new_ppp})
 
         user_profile = UserProfile.objects.get(user=self.user.user.id)
-        self.assertEqual(user_profile.patches_per_page, old_ppp)
+        self.assertEqual(user_profile.items_per_page, old_ppp)
 
 
 class UserPasswordChangeTest(TestCase):
diff --git a/patchwork/views/__init__.py b/patchwork/views/__init__.py
index 7d31a06..3927150 100644
--- a/patchwork/views/__init__.py
+++ b/patchwork/views/__init__.py
@@ -332,7 +332,7 @@  def patch_to_mbox(patch):
     # TODO(stephenfin): Make this use the tags infrastructure
     body += patch.patch_responses()
 
-    for comment in Comment.objects.filter(patch=patch):
+    for comment in Comment.objects.filter(submission=patch):
         body += comment.patch_responses()
 
     if postscript: