[PATCH 02/13] models: Split Patch into two models
Stephen Finucane
stephen.finucane at intel.com
Fri Mar 11 23:08:06 AEDT 2016
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 at intel.com>
---
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 | 48 ++++++++
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, 323 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
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..7be1e04
--- /dev/null
+++ b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
@@ -0,0 +1,48 @@
+# -*- 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):
+ Submission = apps.get_model('patchwork', 'Submission')
+ Patch = apps.get_model('patchwork', 'Patch')
+
+ for submission in Submission.objects.all():
+ # TODO(stephenfin) This assumes all submissions are patches, but is
+ # that always going to be true at this step?
+ patch = Patch.objects.get(submission_ptr=submission)
+ patch.delete()
+
+
+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 480459b..3eacebf 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')]
+
+
+ at 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:
--
2.0.0
More information about the Patchwork
mailing list