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.finuc...@intel.com> Signed-off-by: Andy Doan <andy.d...@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')] + + +@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork