[PATCH v4 2/3] models: Split Patch into two models
Stephen Finucane
stephen.finucane at intel.com
Sat Mar 26 04:29:29 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>
Signed-off-by: Andy Doan <andy.doan at linaro.org>
---
v2: Correct mistake in reverse migration for 0010
v3: Replace Python data migrations with SQL equivalents
v4: Remove unused variable
---
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 | 30 +++++
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, 305 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..1d4b6e1
--- /dev/null
+++ b/patchwork/migrations/0010_migrate_data_from_submission_to_patch.py
@@ -0,0 +1,30 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+ dependencies = [
+ ('patchwork', '0009_add_submission_model'),
+ ]
+
+ operations = [
+ 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
+ '''],
+ ['''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
+ ''']
+ ),
+ ]
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 bdd797a..5b5ce5b 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -130,9 +130,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:
@@ -143,7 +143,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):
@@ -243,7 +243,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)
@@ -289,14 +290,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)
@@ -315,7 +337,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()
@@ -437,27 +459,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):
@@ -484,7 +504,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,
@@ -642,7 +663,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 7150e21..26236d7 100644
--- a/patchwork/settings/base.py
+++ b/patchwork/settings/base.py
@@ -138,7 +138,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 f558ed0..a3f61cd 100644
--- a/patchwork/tests/test_tags.py
+++ b/patchwork/tests/test_tags.py
@@ -113,7 +113,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 2c67408..ddddf63 100644
--- a/patchwork/views/__init__.py
+++ b/patchwork/views/__init__.py
@@ -365,7 +365,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