diff mbox

[06/14] series: Add a Series model

Message ID 1445378383-16977-7-git-send-email-damien.lespiau@intel.com
State Rejected
Headers show

Commit Message

Damien Lespiau Oct. 20, 2015, 9:59 p.m. UTC
v2: Add 'order' metadata to revisions as they do have a natural
    ordering.
v3: Add a couple of utility functions to Series and SeriesRevision
v4: Provide a get_absolute_url() method on Series
v5: Add a dump() method to series, handy for debugging
v6: Use 'Series without cover letter' as default series name instead
    of 'Untitled series' to clearly indicated what's hapening to the
    user (Belen Pena)
v7: Squash in the migration (Stephen Finucane)
v8: Add (untested) manual SQL migration paths (Stephen Finucane)

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Acked-by: Stephen Finucane <stephen.finucane@intel.com>
---
 lib/sql/grant-all.mysql.sql          |  6 +++
 lib/sql/grant-all.postgres.sql       | 17 ++++++--
 lib/sql/migration/016-add-series.sql | 23 ++++++++++
 patchwork/migrations/0003_series.py  | 73 +++++++++++++++++++++++++++++++
 patchwork/models.py                  | 85 ++++++++++++++++++++++++++++++++++++
 5 files changed, 201 insertions(+), 3 deletions(-)
 create mode 100644 lib/sql/migration/016-add-series.sql
 create mode 100644 patchwork/migrations/0003_series.py

Comments

Stephen Finucane Nov. 5, 2015, 6:24 p.m. UTC | #1
> v2: Add 'order' metadata to revisions as they do have a natural

>     ordering.

> v3: Add a couple of utility functions to Series and SeriesRevision

> v4: Provide a get_absolute_url() method on Series

> v5: Add a dump() method to series, handy for debugging

> v6: Use 'Series without cover letter' as default series name instead

>     of 'Untitled series' to clearly indicated what's hapening to the

>     user (Belen Pena)

> v7: Squash in the migration (Stephen Finucane)

> v8: Add (untested) manual SQL migration paths (Stephen Finucane)

> 

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

> Acked-by: Stephen Finucane <stephen.finucane@intel.com>


I've spotted a few issues I missed first go around when I was attempting to merge this. Can you address them?

Stephen

PS: I stripped the migrations section as these may/may not be able to go away now.

> --- a/patchwork/models.py

> +++ b/patchwork/models.py

> @@ -410,6 +410,91 @@ class BundlePatch(models.Model):

>          unique_together = [('bundle', 'patch')]

>          ordering = ['order']

> 

> +SERIES_DEFAULT_NAME = "Series without cover letter"

> +

> +# This Model represents the "top level" Series, an object that doesn't

> change

> +# with the various versions of patches sent to the mailing list.

> +class Series(models.Model):

> +    project = models.ForeignKey(Project)

> +    name = models.CharField(max_length=200, default=SERIES_DEFAULT_NAME)


I really am not happy with this. I understand your rationale and I realize that this means people need to account for this in the interfaces (API + UI), but this is still better than storing lies, figuratively speaking. Similarly i18n might not be an issue now but I'd like to keep the doors open for that down the line. Can we please allow 'blank' and 'null' instead?

> +    submitter = models.ForeignKey(Person, related_name='submitters')

> +    reviewer = models.ForeignKey(User, related_name='reviewers',

> null=True,

> +                                 blank=True)

> +    submitted = models.DateTimeField(default=datetime.datetime.now)

> +    last_updated = models.DateTimeField(auto_now=True)

> +    # Caches the latest version so we can display it without looking at

> the max

> +    # of all SeriesRevision.version

> +    version = models.IntegerField(default=1)

> +    # This is the number of patches of the latest version.

> +    n_patches = models.IntegerField(default=0)

> +

> +    def __unicode__(self):

> +        return self.name

> +

> +    def revisions(self):

> +        return SeriesRevision.objects.filter(series=self)

> +

> +    def latest_revision(self):

> +        return self.revisions().reverse()[0]

> +

> +    def get_absolute_url(self):

> +        return reverse('series', kwargs={ 'series': self.pk })

> +

> +    def dump(self):

> +        print('')

> +        print('===')

> +        print('Series: %s' % self)

> +        print('    version %d' % self.version)

> +        for rev in self.revisions():

> +            print('    rev %d:' % rev.version)

> +            i = 1

> +            for patch in rev.ordered_patches():

> +                print('        patch %d:' % i)

> +                print('            subject: %s' % patch.name)

> +                print('            msgid  : %s' % patch.msgid)

> +                i += 1

> +

> +# A 'revision' of a series. Resending a new version of a patch or a full

> new

> +# iteration of a series will create a new revision.

> +class SeriesRevision(models.Model):

> +    series = models.ForeignKey(Series)

> +    version = models.IntegerField(default=1)

> +    root_msgid = models.CharField(max_length=255)

> +    cover_letter = models.TextField(null = True, blank = True)

> +    patches = models.ManyToManyField(Patch, through =

> 'SeriesRevisionPatch')


Is it possible for a patch to belong to more than one Series(Revision)?
 
To be consistent, can you make the following changes:

	CHANGE root_msgid -> msgid
	CHANGE cover_letter -> content
	ADD    submitter  (it's possible that a series revision could come from a different person, yes?)
	ADD    date       (if there's a cover letter, we should store the date IMO. Even if not, knowing the date/time a revision was created is helpful)
	ADD    headers    (if there's a cover letter, we should store these headers for consistency)

This would allow us to treat SeriesRevision, Patch and Comment as Email-type objects, which most of the time they are. If we do this, we can start pulling out common behaviors into mixins.

	https://github.com/stephenfin/patchwork/commit/4912b19790da69e5cddd081119e34f5a194a63c0

> +

> +    class Meta:

> +        unique_together = [('series', 'version')]

> +        ordering = ['version']

> +

> +    def ordered_patches(self):

> +        return self.patches.order_by('seriesrevisionpatch__order')

> +

> +    def add_patch(self, patch, order):

> +        # see if the patch is already in this revision

> +        if SeriesRevisionPatch.objects.filter(revision=self,

> +                                              patch=patch).count():

> +            raise Exception("patch is already in revision")

> +

> +        sp = SeriesRevisionPatch.objects.create(revision=self,

> patch=patch,

> +                                                order=order)

> +        sp.save()

> +

> +    def __unicode__(self):

> +        if hasattr(self, 'series'):

> +            return self.series.name + " (rev " + str(self.version) + ")"

> +        else:

> +            return "New revision" + " (rev " + str(self.version) + ")"

> +

> +class SeriesRevisionPatch(models.Model):

> +    patch = models.ForeignKey(Patch)

> +    revision = models.ForeignKey(SeriesRevision)

> +    order = models.IntegerField()

> +

> +    class Meta:

> +        unique_together = [('revision', 'patch'), ('revision', 'order')]

> +        ordering = ['order']

> +

>  class EmailConfirmation(models.Model):

>      validity = datetime.timedelta(days =

> settings.CONFIRMATION_VALIDITY_DAYS)

>      type = models.CharField(max_length = 20, choices = [

> --

> 2.4.3

> 

> _______________________________________________

> Patchwork mailing list

> Patchwork@lists.ozlabs.org

> https://lists.ozlabs.org/listinfo/patchwork
diff mbox

Patch

diff --git a/lib/sql/grant-all.mysql.sql b/lib/sql/grant-all.mysql.sql
index 6a3d547..fa6128f 100644
--- a/lib/sql/grant-all.mysql.sql
+++ b/lib/sql/grant-all.mysql.sql
@@ -25,6 +25,9 @@  GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_emailoptout TO 'www-data'@loca
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchchangenotification TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_tag TO 'www-data'@localhost;
 GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_patchtag TO 'www-data'@localhost;
+GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_series TO 'www-data'@localhost;
+GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_seriesrevision TO 'www-data'@localhost;
+GRANT SELECT, UPDATE, INSERT, DELETE ON patchwork_seriesrevisionpatch TO 'www-data'@localhost;
 
 -- allow the mail user (in this case, 'nobody') to add patches
 GRANT INSERT, SELECT ON patchwork_patch TO 'nobody'@localhost;
@@ -34,6 +37,9 @@  GRANT INSERT, SELECT, UPDATE, DELETE ON patchwork_patchtag TO 'nobody'@localhost
 GRANT SELECT ON	patchwork_project TO 'nobody'@localhost;
 GRANT SELECT ON patchwork_state TO 'nobody'@localhost;
 GRANT SELECT ON patchwork_tag TO 'nobody'@localhost;
+GRANT INSERT, SELECT, UPDATE ON patchwork_series TO 'nobody'@localhost;
+GRANT INSERT, SELECT ON patchwork_seriesrevision TO 'nobody'@localhost;
+GRANT INSERT, SELECT ON patchwork_seriesrevisionpatch TO 'nobody'@localhost;
 
 COMMIT;
 
diff --git a/lib/sql/grant-all.postgres.sql b/lib/sql/grant-all.postgres.sql
index 477e10a..ff72c7f 100644
--- a/lib/sql/grant-all.postgres.sql
+++ b/lib/sql/grant-all.postgres.sql
@@ -25,7 +25,10 @@  GRANT SELECT, UPDATE, INSERT, DELETE ON
 	patchwork_emailoptout,
 	patchwork_patchchangenotification,
 	patchwork_tag,
-	patchwork_patchtag
+	patchwork_patchtag,
+        patchwork_series,
+        patchwork_seriesrevision,
+        patchwork_seriesrevisionpatch
 TO "www-data";
 GRANT SELECT, UPDATE ON
 	auth_group_id_seq,
@@ -48,14 +51,22 @@  GRANT SELECT, UPDATE ON
 	patchwork_userprofile_id_seq,
 	patchwork_userprofile_maintainer_projects_id_seq,
 	patchwork_tag_id_seq,
-	patchwork_patchtag_id_seq
+	patchwork_patchtag_id_seq,
+        patchwork_series_id_seq,
+        patchwork_seriesrevision_id_seq,
+        patchwork_seriesrevisionpatch_id_seq
 TO "www-data";
 
 -- allow the mail user (in this case, 'nobody') to add patches
 GRANT INSERT, SELECT ON
 	patchwork_patch,
 	patchwork_comment,
-	patchwork_person
+	patchwork_person,
+	patchwork_seriesrevision,
+	patchwork_seriesrevisionpatch
+TO "nobody";
+GRANT INSERT, SELECT, UPDATE ON
+	patchwork_series
 TO "nobody";
 GRANT INSERT, SELECT, UPDATE, DELETE ON
 	patchwork_patchtag
diff --git a/lib/sql/migration/016-add-series.sql b/lib/sql/migration/016-add-series.sql
new file mode 100644
index 0000000..de01be1
--- /dev/null
+++ b/lib/sql/migration/016-add-series.sql
@@ -0,0 +1,23 @@ 
+BEGIN;
+CREATE TABLE "patchwork_series" ("id" serial NOT NULL PRIMARY KEY, "name" varchar(200) NOT NULL, "submitted" timestamp with time zone NOT NULL, "last_updated" timestamp with time zone NOT NULL, "version" integer NOT NULL, "n_patches" integer NOT NULL, "project_id" integer NOT NULL, "reviewer_id" integer NULL, "submitter_id" integer NOT NULL);
+CREATE TABLE "patchwork_seriesrevision" ("id" serial NOT NULL PRIMARY KEY, "version" integer NOT NULL, "root_msgid" varchar(255) NOT NULL, "cover_letter" text NULL);
+CREATE TABLE "patchwork_seriesrevisionpatch" ("id" serial NOT NULL PRIMARY KEY, "order" integer NOT NULL, "patch_id" integer NOT NULL, "revision_id" integer NOT NULL);
+ALTER TABLE "patchwork_seriesrevision" ADD COLUMN "series_id" integer NOT NULL;
+ALTER TABLE "patchwork_seriesrevision" ALTER COLUMN "series_id" DROP DEFAULT;
+ALTER TABLE "patchwork_seriesrevisionpatch" ADD CONSTRAINT "patchwork_seriesrevisionpatch_revision_id_1109258846b6e0ac_uniq" UNIQUE ("revision_id", "order");
+ALTER TABLE "patchwork_seriesrevisionpatch" ADD CONSTRAINT "patchwork_seriesrevisionpatch_revision_id_6908277fcf2dea5b_uniq" UNIQUE ("revision_id", "patch_id");
+ALTER TABLE "patchwork_seriesrevision" ADD CONSTRAINT "patchwork_seriesrevision_series_id_7548e7889275a653_uniq" UNIQUE ("series_id", "version");
+ALTER TABLE "patchwork_series" ADD CONSTRAINT "patchwork_se_project_id_86fd55162c0ef1b_fk_patchwork_project_id" FOREIGN KEY ("project_id") REFERENCES "patchwork_project" ("id") DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE "patchwork_series" ADD CONSTRAINT "patchwork_series_reviewer_id_42567a6911b21fc8_fk_auth_user_id" FOREIGN KEY ("reviewer_id") REFERENCES "auth_user" ("id") DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE "patchwork_series" ADD CONSTRAINT "patchwork_s_submitter_id_8055a5db31fae17_fk_patchwork_person_id" FOREIGN KEY ("submitter_id") REFERENCES "patchwork_person" ("id") DEFERRABLE INITIALLY DEFERRED;
+CREATE INDEX "patchwork_series_b098ad43" ON "patchwork_series" ("project_id");
+CREATE INDEX "patchwork_series_071d8141" ON "patchwork_series" ("reviewer_id");
+CREATE INDEX "patchwork_series_a8919bbb" ON "patchwork_series" ("submitter_id");
+ALTER TABLE "patchwork_seriesrevisionpatch" ADD CONSTRAINT "patchwork_serie_patch_id_75fd1ec59eecf9e6_fk_patchwork_patch_id" FOREIGN KEY ("patch_id") REFERENCES "patchwork_patch" ("id") DEFERRABLE INITIALLY DEFERRED;
+ALTER TABLE "patchwork_seriesrevisionpatch" ADD CONSTRAINT "pat_revision_id_3227811f80e535ce_fk_patchwork_seriesrevision_id" FOREIGN KEY ("revision_id") REFERENCES "patchwork_seriesrevision" ("id") DEFERRABLE INITIALLY DEFERRED;
+CREATE INDEX "patchwork_seriesrevisionpatch_f70d6c4e" ON "patchwork_seriesrevisionpatch" ("patch_id");
+CREATE INDEX "patchwork_seriesrevisionpatch_5de09a8d" ON "patchwork_seriesrevisionpatch" ("revision_id");
+CREATE INDEX "patchwork_seriesrevision_a08cee2d" ON "patchwork_seriesrevision" ("series_id");
+ALTER TABLE "patchwork_seriesrevision" ADD CONSTRAINT "patchwork_ser_series_id_2b977316803e35df_fk_patchwork_series_id" FOREIGN KEY ("series_id") REFERENCES "patchwork_series" ("id") DEFERRABLE INITIALLY DEFERRED;
+
+COMMIT;
diff --git a/patchwork/migrations/0003_series.py b/patchwork/migrations/0003_series.py
new file mode 100644
index 0000000..4e5e5dd
--- /dev/null
+++ b/patchwork/migrations/0003_series.py
@@ -0,0 +1,73 @@ 
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import models, migrations
+import datetime
+from django.conf import settings
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        migrations.swappable_dependency(settings.AUTH_USER_MODEL),
+        ('patchwork', '0002_fix_patch_state_default_values'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='Series',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('name', models.CharField(default=b'Series without cover letter', max_length=200)),
+                ('submitted', models.DateTimeField(default=datetime.datetime.now)),
+                ('last_updated', models.DateTimeField(auto_now=True)),
+                ('version', models.IntegerField(default=1)),
+                ('n_patches', models.IntegerField(default=0)),
+                ('project', models.ForeignKey(to='patchwork.Project')),
+                ('reviewer', models.ForeignKey(related_name='reviewers', blank=True, to=settings.AUTH_USER_MODEL, null=True)),
+                ('submitter', models.ForeignKey(related_name='submitters', to='patchwork.Person')),
+            ],
+        ),
+        migrations.CreateModel(
+            name='SeriesRevision',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('version', models.IntegerField(default=1)),
+                ('root_msgid', models.CharField(max_length=255)),
+                ('cover_letter', models.TextField(null=True, blank=True)),
+            ],
+            options={
+                'ordering': ['version'],
+            },
+        ),
+        migrations.CreateModel(
+            name='SeriesRevisionPatch',
+            fields=[
+                ('id', models.AutoField(verbose_name='ID', serialize=False, auto_created=True, primary_key=True)),
+                ('order', models.IntegerField()),
+                ('patch', models.ForeignKey(to='patchwork.Patch')),
+                ('revision', models.ForeignKey(to='patchwork.SeriesRevision')),
+            ],
+            options={
+                'ordering': ['order'],
+            },
+        ),
+        migrations.AddField(
+            model_name='seriesrevision',
+            name='patches',
+            field=models.ManyToManyField(to='patchwork.Patch', through='patchwork.SeriesRevisionPatch'),
+        ),
+        migrations.AddField(
+            model_name='seriesrevision',
+            name='series',
+            field=models.ForeignKey(to='patchwork.Series'),
+        ),
+        migrations.AlterUniqueTogether(
+            name='seriesrevisionpatch',
+            unique_together=set([('revision', 'order'), ('revision', 'patch')]),
+        ),
+        migrations.AlterUniqueTogether(
+            name='seriesrevision',
+            unique_together=set([('series', 'version')]),
+        ),
+    ]
diff --git a/patchwork/models.py b/patchwork/models.py
index a1c2974..feb82c0 100644
--- a/patchwork/models.py
+++ b/patchwork/models.py
@@ -410,6 +410,91 @@  class BundlePatch(models.Model):
         unique_together = [('bundle', 'patch')]
         ordering = ['order']
 
+SERIES_DEFAULT_NAME = "Series without cover letter"
+
+# This Model represents the "top level" Series, an object that doesn't change
+# with the various versions of patches sent to the mailing list.
+class Series(models.Model):
+    project = models.ForeignKey(Project)
+    name = models.CharField(max_length=200, default=SERIES_DEFAULT_NAME)
+    submitter = models.ForeignKey(Person, related_name='submitters')
+    reviewer = models.ForeignKey(User, related_name='reviewers', null=True,
+                                 blank=True)
+    submitted = models.DateTimeField(default=datetime.datetime.now)
+    last_updated = models.DateTimeField(auto_now=True)
+    # Caches the latest version so we can display it without looking at the max
+    # of all SeriesRevision.version
+    version = models.IntegerField(default=1)
+    # This is the number of patches of the latest version.
+    n_patches = models.IntegerField(default=0)
+
+    def __unicode__(self):
+        return self.name
+
+    def revisions(self):
+        return SeriesRevision.objects.filter(series=self)
+
+    def latest_revision(self):
+        return self.revisions().reverse()[0]
+
+    def get_absolute_url(self):
+        return reverse('series', kwargs={ 'series': self.pk })
+
+    def dump(self):
+        print('')
+        print('===')
+        print('Series: %s' % self)
+        print('    version %d' % self.version)
+        for rev in self.revisions():
+            print('    rev %d:' % rev.version)
+            i = 1
+            for patch in rev.ordered_patches():
+                print('        patch %d:' % i)
+                print('            subject: %s' % patch.name)
+                print('            msgid  : %s' % patch.msgid)
+                i += 1
+
+# A 'revision' of a series. Resending a new version of a patch or a full new
+# iteration of a series will create a new revision.
+class SeriesRevision(models.Model):
+    series = models.ForeignKey(Series)
+    version = models.IntegerField(default=1)
+    root_msgid = models.CharField(max_length=255)
+    cover_letter = models.TextField(null = True, blank = True)
+    patches = models.ManyToManyField(Patch, through = 'SeriesRevisionPatch')
+
+    class Meta:
+        unique_together = [('series', 'version')]
+        ordering = ['version']
+
+    def ordered_patches(self):
+        return self.patches.order_by('seriesrevisionpatch__order')
+
+    def add_patch(self, patch, order):
+        # see if the patch is already in this revision
+        if SeriesRevisionPatch.objects.filter(revision=self,
+                                              patch=patch).count():
+            raise Exception("patch is already in revision")
+
+        sp = SeriesRevisionPatch.objects.create(revision=self, patch=patch,
+                                                order=order)
+        sp.save()
+
+    def __unicode__(self):
+        if hasattr(self, 'series'):
+            return self.series.name + " (rev " + str(self.version) + ")"
+        else:
+            return "New revision" + " (rev " + str(self.version) + ")"
+
+class SeriesRevisionPatch(models.Model):
+    patch = models.ForeignKey(Patch)
+    revision = models.ForeignKey(SeriesRevision)
+    order = models.IntegerField()
+
+    class Meta:
+        unique_together = [('revision', 'patch'), ('revision', 'order')]
+        ordering = ['order']
+
 class EmailConfirmation(models.Model):
     validity = datetime.timedelta(days = settings.CONFIRMATION_VALIDITY_DAYS)
     type = models.CharField(max_length = 20, choices = [