[PATCH 2/3] models: Merge patch and first comment

[Finucane, Stephen]

On 12 Mar 02:37, Andy Doan wrote:
> On 03/08/2016 03:54 PM, Stephen Finucane wrote:
> >At the moment a patch is split into two model entries: a Patch and a
> >linked Comment. The original rationale for this was that a Patch is
> >really a sub-class of Comment. A comment is a record of the text
> >content of an incoming mail, while a patch is that, plus the patch
> >content too. Hence the separation and a one-to-one relationship when a
> >patch is present. However, this decision was made before Django added
> >support for model inheritance and is no longer necessary. This change
> >flatten the models in preparation for some email subclassing work. This
> >is achieved by copying over the non-duplicated fields from the Comment
> >to the linked Patch, then deleting the Comment.
> >
> >The migrations are broken into two steps: a schema migration and a data
> >migration, per the recommendations of the Django documentation [1]. SQL
> >migration scripts, where necessary, will need to be created manually as
> >there appears to be no way to do this in a way that is
> >RDBMS-independant [2][3].
> >
> >[1] https://docs.djangoproject.com/en/1.9/topics/migrations/#data-migrations
> >[2] https://stackoverflow.com/q/6856849/
> >[3] https://stackoverflow.com/q/224732/
> >
> >Signed-off-by: Stephen Finucane <stephen.finucane at intel.com>
> 
> Reviewed-by: Andy Doan <andy.doan at linaro.org>
>
> >diff --git a/patchwork/migrations/0007_move_comment_content_to_patch_content.py b/patchwork/migrations/0007_move_comment_content_to_patch_content.py
> 
> >+from django.db import migrations, models
> 
> Hate to nitpick over migration warnings since they are usually
> machine generated, but "models" is an unused import for this module.

Nope: this was handrolled, and the import was unnecessary. Removed.

> >+def copy_comment_field(apps, schema_editor):
> >+    Comment = apps.get_model('patchwork', 'Comment')
> >+    Patch = apps.get_model('patchwork', 'Patch')
> >+
> >+    for patch in Patch.objects.all():
> >+        # this can only return one entry due to the unique_together
> >+        # constraint
> >+        comment = Comment.objects.get(patch=patch, msgid=patch.msgid)
> >+        patch.content = comment.content
> >+        patch.save()
> 
> This should work based on the normal flow of patchwork. However,
> I've found a place where instance can't complete this migration.
> We've added some hacks to track AOSP contributions[1]. We don't
> actually create a comment linked to the patch, we just use the
> "content" field to point to the AOSP change. I *think* if you
> changed this to Comment.objects.filter(....) and if count() == 1 it
> wouldn't break my instance. However, I'm not sure that's fair to ask
> you to add baggage to deal with me when its probably possible for me
> to add an internal migration to "fix" our AOSP stuff. Thought it
> would hurt to ask though.
> 
> 1: http://patches.linaro.org/patch/7669/

There was a 'try...except' in 'patch_to_mbox' ('views/__init__.py'), so
the chances are this affects more than you. Updated to make handle the
"comments optional" case. Good spot.

Thanks for the reviews :)

Stephen