Rework tagging infrastructure

Solve getpatchwork#113 and getpatchwork#57 GitHub issues, fix up returning tags in the API,
keep track of tag origin to be able to add tags to comments in the API.

Use relations Tag-Patch and Tag-CoverLetter to avoid duplication of
tags for each patch in series, and use `series` attribute of
SubmissionTag as a notion of a tag which is related to each patch in the
series (because it comes from cover letter or it's comments)

Signed-off-by: Veronika Kabatova <vkabatov@redhat.com>

docs/usage/overview.rst

@@ -119,10 +119,11 @@ one delegate can be assigned to a patch.
 Tags
 ~~~~
 
-Tags are specially formatted metadata appended to the foot the body of a patch
-or a comment on a patch. Patchwork extracts these tags at parse time and
-associates them with the patch. You add extra tags to an email by replying to
-the email. The following tags are available on a standard Patchwork install:
+Tags are specially formatted metadata appended to the foot the body of a patch,
+cover letter or a comment related to them. Patchwork extracts these tags at
+parse time and associates them with patches. You add extra tags to an email by
+replying to the email. The following tags are available on a standard Patchwork
+install:
 
 ``Acked-by:``
   For example::

patchwork/management/commands/retag.py

@@ -19,11 +19,13 @@
 
 from django.core.management.base import BaseCommand
 
+from patchwork.models import Cover
 from patchwork.models import Patch
+from patchwork.models import SeriesPatch
 
 
 class Command(BaseCommand):
-    help = 'Update the tag (Ack/Review/Test) counts on existing patches'
+    help = 'Update tags on existing patches'
     args = '[<patch_id>...]'
 
     def handle(self, *args, **options):
@@ -37,7 +39,17 @@ def handle(self, *args, **options):
         count = query.count()
 
         for i, patch in enumerate(query.iterator()):
-            patch.refresh_tag_counts()
+            patch.refresh_tags()
+            for comment in patch.comments.all():
+                comment.refresh_tags()
+
+            series_patches = SeriesPatch.objects.filter(patch_id=patch.id)
+            for series_patch in series_patches:
+                cover = series_patch.series.cover_letter
+                cover.refresh_tags()
+                for comment in cover.comments.all():
+                    comment.refresh_tags()
+
             if (i % 10) == 0:
                 self.stdout.write('%06d/%06d\r' % (i, count), ending='')
                 self.stdout.flush()

patchwork/migrations/0027_rework_tagging.py

@@ -0,0 +1,146 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+import re
+
+
+# Django migrations don't allow us to call models' methods because the
+# migration will break if the methods change. Therefore we need to use an
+# altered copy of all the code needed.
+def extract_tags(extract_from, tags):
+    found_tags = {}
+
+    if not extract_from.content:
+        return found_tags
+
+    for tag in tags:
+        regex = re.compile(tag.pattern + r'\s(.*)', re.M | re.I)
+        found_tags[tag] = regex.findall(extract_from.content)
+
+    return found_tags
+
+
+def add_tags(apps, submission, tag, values, comment=None):
+    if not values:
+        # We don't need to delete tags since none exist yet and we can't
+        # delete comments etc. during the migration
+        return
+
+    if hasattr(submission, 'patch'):
+        series = None
+    else:
+        series = submission.coverletter.series.first()
+
+    SubmissionTag = apps.get_model('patchwork', 'SubmissionTag')
+    current_objs = SubmissionTag.objects.filter(submission=self,
+                                                comment=comment,
+                                                tag=tag,
+                                                series=series)
+
+    # Only create nonexistent tags
+    values_to_add = set(values) - set(current_objs.values_list('value',
+                                                               flat=True))
+    SubmissionTag.objects.bulk_create([SubmissionTag(
+        submission=submission,
+        tag=tag,
+        value=value,
+        comment=comment,
+        series=series
+    ) for value in values_to_add])
+
+
+def create_all(apps, schema_editor):
+    Tag = apps.get_model('patchwork', 'Tag')
+    tags = Tag.objects.all()
+
+    Submission = apps.get_model('patchwork', 'Submission')
+    for submission in Submission.objects.all():
+        extracted = extract_tags(submission, tags)
+        for tag in extracted:
+            add_tags(apps, submission, tag, extracted[tag])
+
+    Comment = apps.get_model('patchwork', 'Comment')
+    for comment in Comment.objects.all():
+        extracted = extract_tags(comment, tags)
+        for tag in extracted:
+            add_tags(apps,
+                     comment.submission,
+                     tag,
+                     extracted[tag],
+                     comment=comment)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('patchwork', '0026_add_user_bundles_backref'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='SubmissionTag',
+            fields=[
+                ('id', models.AutoField(auto_created=True,
+                                        primary_key=True,
+                                        serialize=False,
+                                        verbose_name='ID')),
+                ('value', models.CharField(max_length=255)),
+                ('comment', models.ForeignKey(
+                    on_delete=django.db.models.deletion.CASCADE,
+                    to='patchwork.Comment',
+                    null=True
+                )),
+                ('submission', models.ForeignKey(
+                    on_delete=django.db.models.deletion.CASCADE,
+                    to='patchwork.Submission'
+                )),
+                ('tag', models.ForeignKey(
+                    on_delete=django.db.models.deletion.CASCADE,
+                    to='patchwork.Tag'
+                )),
+                ('series', models.ForeignKey(
+                    on_delete=django.db.models.deletion.CASCADE,
+                    to='patchwork.Series',
+                    null=True
+                )),
+            ],
+        ),
+        migrations.AlterUniqueTogether(
+            name='patchtag',
+            unique_together=set([]),
+        ),
+        migrations.RemoveField(
+            model_name='patchtag',
+            name='patch',
+        ),
+        migrations.RemoveField(
+            model_name='patchtag',
+            name='tag',
+        ),
+        migrations.RemoveField(
+            model_name='patch',
+            name='tags',
+        ),
+        migrations.DeleteModel(
+            name='PatchTag',
+        ),
+        migrations.AddField(
+            model_name='submission',
+            name='related_tags',
+            field=models.ManyToManyField(
+                through='patchwork.SubmissionTag',
+                to='patchwork.Tag'
+            ),
+        ),
+        migrations.AlterUniqueTogether(
+            name='submissiontag',
+            unique_together=set([('submission',
+                                  'tag',
+                                  'value',
+                                  'comment')]),
+        ),
+        migrations.RunPython(create_all, atomic=False),
+    ]

patchwork/tests/api/test_patch.py

@@ -166,7 +166,7 @@ def test_detail(self):
 
         self.assertEqual(patch.content, resp.data['content'])
         self.assertEqual(patch.diff, resp.data['diff'])
-        self.assertEqual(0, len(resp.data['tags']))
+        self.assertEqual(1, len(resp.data['tags']))
 
     def test_detail_version_1_0(self):
         patch = create_patch()

patchwork/tests/test_mboxviews.py

@@ -39,6 +39,8 @@ class MboxPatchResponseTest(TestCase):
 
     """Test that the mbox view appends the Acked-by from a patch comment."""
 
+    fixtures = ['default_tags']
+
     def setUp(self):
         self.project = create_project()
         self.person = create_person()
@@ -53,7 +55,9 @@ def test_patch_response(self):
             submitter=self.person,
             content='comment 2 text\nAcked-by: 2\n')
         response = self.client.get(reverse('patch-mbox', args=[patch.id]))
-        self.assertContains(response, 'Acked-by: 1\nAcked-by: 2\n')
+        # Can't guarantee the order in which the tags are returned
+        self.assertContains(response, 'Acked-by: 1\n')
+        self.assertContains(response, 'Acked-by: 2\n')
 
     def test_patch_utf8_nbsp(self):
         patch = create_patch(
@@ -73,6 +77,8 @@ class MboxPatchSplitResponseTest(TestCase):
     """Test that the mbox view appends the Acked-by from a patch comment,
        and places it before an '---' update line."""
 
+    fixtures = ['default_tags']
+
     def setUp(self):
         project = create_project()
         self.person = create_person()
@@ -88,7 +94,15 @@ def setUp(self):
 
     def test_patch_response(self):
         response = self.client.get(reverse('patch-mbox', args=[self.patch.id]))
-        self.assertContains(response, 'Acked-by: 1\nAcked-by: 2\n')
+        # Can't guarantee the order in which the tags are returned
+        self.assertContains(response, 'Acked-by: 1\n')
+        self.assertContains(response, 'Acked-by: 2\n')
+        # We need to check for 3 Acked-by strings, one comes from the body of
+        # the patch and the other two are the tags themselves.
+        self.assertRegex(
+            response.content.decode(),
+            '(?s).*Acked-by: 1\n.*Acked-by.*Acked-by.*---\nupdate.*'
+        )
 
 
 class MboxHeaderTest(TestCase):

patchwork/tests/test_parser.py

@@ -802,12 +802,12 @@ def setUp(self):
     def test_tags(self):
         self.assertEqual(Patch.objects.count(), 1)
         patch = Patch.objects.all()[0]
-        self.assertEqual(patch.patchtag_set.filter(
-            tag__name='Acked-by').count(), 0)
-        self.assertEqual(patch.patchtag_set.get(
-            tag__name='Reviewed-by').count, 1)
-        self.assertEqual(patch.patchtag_set.get(
-            tag__name='Tested-by').count, 1)
+        self.assertEqual(patch.related_tags.filter(name='Acked-by').count(),
+                         0)
+        self.assertEqual(patch.related_tags.filter(name='Reviewed-by').count(),
+                         1)
+        self.assertEqual(patch.related_tags.filter(name='Tested-by').count(),
+                         1)
 
 
 class ParseCommentTagsTest(PatchTest):
@@ -830,12 +830,11 @@ def setUp(self):
     def test_tags(self):
         self.assertEqual(Patch.objects.count(), 1)
         patch = Patch.objects.all()[0]
-        self.assertEqual(patch.patchtag_set.filter(
-            tag__name='Acked-by').count(), 0)
-        self.assertEqual(patch.patchtag_set.get(
-            tag__name='Reviewed-by').count, 1)
-        self.assertEqual(patch.patchtag_set.get(
-            tag__name='Tested-by').count, 1)
+        self.assertEqual(patch.related_tags.filter(name='Acked-by').count(), 0)
+        self.assertEqual(patch.related_tags.filter(name='Reviewed-by')count(),
+                         1)
+        self.assertEqual(patch.related_tags.filter(name='Tested-by').count(),
+                         1)
 
 
 class SubjectTest(TestCase):

patchwork/tests/test_tags.py

@@ -21,7 +21,7 @@
 from django.test import TransactionTestCase
 
 from patchwork.models import Patch
-from patchwork.models import PatchTag
+from patchwork.models import SubmissionTag
 from patchwork.models import Tag
 from patchwork.tests.utils import create_comment
 from patchwork.tests.utils import create_patch
@@ -34,11 +34,14 @@ class ExtractTagsTest(TestCase):
     name_email = 'test name <' + email + '>'
 
     def assertTagsEqual(self, str, acks, reviews, tests):  # noqa
-        counts = Patch.extract_tags(str, Tag.objects.all())
-        self.assertEqual((acks, reviews, tests),
-                         (counts[Tag.objects.get(name='Acked-by')],
-                          counts[Tag.objects.get(name='Reviewed-by')],
-                          counts[Tag.objects.get(name='Tested-by')]))
+        patch = create_patch(content=str)
+        extracted = patch._extract_tags(Tag.objects.all())
+        self.assertEqual(
+            (acks, reviews, tests),
+            (len(extracted.get(Tag.objects.get(name='Acked-by'), [])),
+             len(extracted.get(Tag.objects.get(name='Reviewed-by'), [])),
+             len(extracted.get(Tag.objects.get(name='Tested-by'), [])))
+        )
 
     def test_empty(self):
         self.assertTagsEqual('', 0, 0, 0)
@@ -80,7 +83,7 @@ def test_ack_in_reply(self):
         self.assertTagsEqual('> Acked-by: %s\n' % self.name_email, 0, 0, 0)
 
 
-class PatchTagsTest(TransactionTestCase):
+class SubmissionTagsTest(TransactionTestCase):
 
     fixtures = ['default_tags']
     ACK = 1
@@ -95,16 +98,14 @@ def setUp(self):
     def assertTagsEqual(self, patch, acks, reviews, tests):  # noqa
         patch = Patch.objects.get(pk=patch.pk)
 
-        def count(name):
-            try:
-                return patch.patchtag_set.get(tag__name=name).count
-            except PatchTag.DoesNotExist:
-                return 0
+        def count(submission, name):
+            return SubmissionTag.objects.filter(submission=patch,
+                                                tag__name=name).count()
 
         counts = (
-            count(name='Acked-by'),
-            count(name='Reviewed-by'),
-            count(name='Tested-by'),
+            count(patch, 'Acked-by'),
+            count(patch, 'Reviewed-by'),
+            count(patch, 'Tested-by'),
         )
 
         self.assertEqual(counts, (acks, reviews, tests))
@@ -118,7 +119,12 @@ def create_tag(self, tagtype=None):
         if tagtype not in tags:
             return ''
 
-        return '%s-by: Test Tagger <tagger@example.com>\n' % tags[tagtype]
+        index = SubmissionTag.objects.filter(
+            tag__name=tags[tagtype] + '-by'
+        ).count()
+        return '%s-by: Test Taggeri%d <tagger@example.com>\n' % (
+            tags[tagtype], index + 1
+        )
 
     def create_tag_comment(self, patch, tagtype=None):
         comment = create_comment(
@@ -179,29 +185,3 @@ def test_multiple_comments_multiple_tags(self):
         c1.content += self.create_tag(self.REVIEW)
         c1.save()
         self.assertTagsEqual(self.patch, 1, 1, 0)
-
-
-class PatchTagManagerTest(PatchTagsTest):
-
-    def assertTagsEqual(self, patch, acks, reviews, tests):  # noqa
-        tagattrs = {}
-        for tag in Tag.objects.all():
-            tagattrs[tag.name] = tag.attr_name
-
-        # force project.tags to be queried outside of the assertNumQueries
-        patch.project.tags
-
-        # we should be able to do this with two queries: one for
-        # the patch table lookup, and the prefetch_related for the
-        # projects table.
-        with self.assertNumQueries(2):
-            patch = Patch.objects.with_tag_counts(project=patch.project) \
-                .get(pk=patch.pk)
-
-            counts = (
-                getattr(patch, tagattrs['Acked-by']),
-                getattr(patch, tagattrs['Reviewed-by']),
-                getattr(patch, tagattrs['Tested-by']),
-            )
-
-        self.assertEqual(counts, (acks, reviews, tests))