Merge e230802 into cde9da9

docs/installation.rst

@@ -66,3 +66,22 @@ The internal name of the log database. You'll need to use this in the
 Set this to ``True`` to enable the ``anonymise_db`` management command. You
 will want this to be ``False`` on your production deployment.
 
+
+```SILENCED_SYSTEM_CHECKS``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+By default, gdpr-assist performs migration checks to ensure that you've followed
+the upgrade instructions correctly to avoid accidental data loss.
+
+See :doc:`upgrading` for more details of the specific checks.
+
+They may cause a slight performance hit to management command which run checks, so while
+we recommend you leave them on while upgrading, once the upgrade has been completed and
+succesfully deployed the checks can safely be disabled afterwards by adding them to
+Django's `SILENCED_SYSTEMS_CHECKS`__ setting::
+
+    SILENCED_SYSTEM_CHECKS = [
+        "gdpr_assist.E001",
+    ]
+
+__ https://docs.djangoproject.com/en/3.0/ref/settings/#silenced-system-checks

docs/upgrading.rst

@@ -70,9 +70,16 @@ If the ``anonymised`` field was not added by gdpr-assist, and you do not want to
 ``MigrateGdprAnonymised``, you can tell the check to ignore the failing migration by
 adding ``gdpr_assist_safe = True`` to the migration class; for example::
 
-        class Migration(migrations.Migration):
-            gdpr_assist_safe = True
-            dependencies = [...
+    class Migration(migrations.Migration):
+        gdpr_assist_safe = True
+        dependencies = [...
+
+Alternatively if you are happy that all your migrations are safe, you can add the check
+to ``SILENCED_SYSTEM_CHECKS`` in your project settings to disable the migration check::
+
+    SILENCED_SYSTEM_CHECKS = [
+        "gdpr_assist.E001",
+    ]
 
 
 Changes to your code

example/example/migrations/0001_initial.py

@@ -27,6 +27,7 @@ class Migration(migrations.Migration):
                         verbose_name="ID",
                     ),
                 ),
+                ("anonymised", models.BooleanField(default=False)),
                 ("notes", models.CharField(max_length=255)),
             ],
         ),
@@ -42,6 +43,7 @@ class Migration(migrations.Migration):
                         verbose_name="ID",
                     ),
                 ),
+                ("anonymised", models.BooleanField(default=False)),
                 ("email", models.EmailField(max_length=254)),
                 ("sent_at", models.DateTimeField()),
             ],
@@ -58,6 +60,7 @@ class Migration(migrations.Migration):
                         verbose_name="ID",
                     ),
                 ),
+                ("anonymised", models.BooleanField(default=False)),
                 ("name", models.CharField(max_length=255)),
                 ("email", models.EmailField(max_length=254)),
             ],
@@ -75,6 +78,7 @@ class Migration(migrations.Migration):
                         verbose_name="ID",
                     ),
                 ),
+                ("anonymised", models.BooleanField(default=False)),
                 ("age", models.IntegerField(blank=True, null=True)),
                 ("address", models.TextField(blank=True)),
                 ("has_children", models.NullBooleanField()),

example/example/migrations/0002_migrate_anonymised.py

@@ -0,0 +1,17 @@
+# -*- coding: utf-8 -*-
+from django.db import migrations
+
+from gdpr_assist.upgrading import MigrateGdprAnonymised
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("example", "0001_initial"),
+        ("gdpr_assist", "0002_privacyanonymised"),
+    ]
+    operations = [
+        MigrateGdprAnonymised("HealthRecord"),
+        MigrateGdprAnonymised("MailingListLog"),
+        MigrateGdprAnonymised("Person"),
+        MigrateGdprAnonymised("PersonProfile"),
+    ]

example/example/migrations/0003_auto_20200723_1128.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.16 on 2020-07-23 11:28
+from __future__ import unicode_literals
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("example", "0002_migrate_anonymised"),
+    ]
+
+    operations = [
+        migrations.RemoveField(model_name="healthrecord", name="anonymised"),
+        migrations.RemoveField(model_name="mailinglistlog", name="anonymised"),
+        migrations.RemoveField(model_name="person", name="anonymised"),
+        migrations.RemoveField(model_name="personprofile", name="anonymised"),
+    ]

gdpr_assist/upgrading.py

@@ -3,6 +3,7 @@
 """
 from collections import defaultdict
 
+from django.conf import settings
 from django.core.checks import Error, Tags, register
 from django.core.exceptions import ImproperlyConfigured
 from django.db import DEFAULT_DB_ALIAS, connections, migrations, router
@@ -76,7 +77,13 @@ def check_migrate_gdpr_anonymised(app_configs, **kwargs):
     Check that the developers using gdpr-assist have read the instructions for upgrading
     from version 1.1.0
     """
+    # Check is optional
+    # Normally SILENCED_SYSTEM_CHECKS would let the check run, and errors couldn't be
+    # silenced. We'll abuse the setting in the interest of keeping configuration simple.
+    if "gdpr_assist.E001" in settings.SILENCED_SYSTEM_CHECKS:
+        return []
 
+    # Import here instead of top level as django.apps won't be available at import time
     from django.db.migrations.executor import MigrationExecutor
 
     errors = []
@@ -91,23 +98,13 @@ def check_migrate_gdpr_anonymised(app_configs, **kwargs):
         return
 
     # Step through migration plan looking for relevant migrations
-    is_upgrading = False
     migrated_models = defaultdict(dict)
-    plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
+    plan = executor.migration_plan(executor.loader.graph.leaf_nodes(), clean_start=True)
     for migration, is_backwards in plan:
         # Don't worry about reverse migrations
         if is_backwards:
             continue
 
-        # Start examining migrations once we've seen the v1.2.0 migration
-        if not is_upgrading:
-            if (
-                migration.app_label == "gdpr_assist"
-                and migration.name == "0002_privacyanonymised"
-            ):
-                is_upgrading = True
-            continue
-
         # Look at operations
         for op in migration.operations:
             app_label = migration.app_label.lower()

tests/test_upgrading.py

@@ -13,18 +13,30 @@ class MigrationTestCase(TransactionTestCase):
     Base class for migration-related tests
     """
 
+    # Test apps we're working work
+    test_apps = {
+        "anonymised_migration",
+        "missing_anonymised_migration",
+        "no_anonymised_migration",
+    }
+
     # Migrate back to this
     migrate_from = None
 
     # Migrate forward to this (optional)
     migrate_to = None
 
-    def setUp(self):
-        # Have to do it in the class model before we enter the transaction
+    @classmethod
+    def setUpClass(cls):
+        # Capture current state before adding any apps, so we know where to restore to
+        executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
+        cls._original_state = executor.loader.graph.leaf_nodes()
 
-        # Capture current state
-        self.executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
-        self._original_state = self.executor.loader.graph.leaf_nodes()
+        super().setUpClass()
+
+    def setUp(self):
+        # Keep track of any test apps we migrate
+        self.test_apps_migrated = set()
 
         # Migrate back
         self._migrate(self.migrate_from)
@@ -38,16 +50,26 @@ def setUp(self):
         super().setUp()
 
     def tearDown(self):
-        super().tearDown()
-
         # Migrate back
         if self.migrate_to is not None:
             self._migrate(self.migrate_from)
 
-        self.tearDownData()
+        # Make sure we've migrated out the test apps we've spotted
+        if self.test_apps_migrated:
+            self._migrate([(test_app, None) for test_app in self.test_apps_migrated])
+
+        # Restore back to the original
+        # The MigrationLoader will check the active django.apps.apps, so we need to
+        # create a modified migration plan to exclude our test apps
+        executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
+        executor.loader.build_graph()
+        plan = executor.migration_plan(self._original_state)
+        filtered_plan = [
+            pair for pair in plan if pair[0].app_label not in self.test_apps
+        ]
+        executor.migrate(self._original_state, plan=filtered_plan)
 
-        # Restore
-        self._migrate(self._original_state)
+        super().tearDown()
 
     def setUpData(self):
         """
@@ -56,17 +78,21 @@ def setUpData(self):
         Run after migrate_from, before migrate_to
         """
 
-    def tearDownData(self):
-        """
-        Subclasses need to tidy up their data themselves as we're outside transactions
+    def _migrate(self, target):
+        executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
+        executor.loader.build_graph()
 
-        Run in the state of migrate_from - after reversing migrate_to, before restoring
-        state
-        """
+        # See if the migration plan includes any of our test apps, so we know which to
+        # clean up in the tearDown
+        plan_apps = [
+            pair[0] for pair in self.plan_to_names(executor.migration_plan(target))
+        ]
+        test_apps = self.test_apps.intersection(set(plan_apps))
+        self.test_apps_migrated.update(test_apps)
 
-    def _migrate(self, target):
-        self.executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
-        self.executor.migrate(target)
+        # Perform the actual migration and refresh the graph
+        executor.migrate(target)
+        executor.loader.build_graph()
 
     def plan_to_names(self, plan):
         """
@@ -76,6 +102,10 @@ def plan_to_names(self, plan):
 
 
 class TestMigrateGdprAnonymisedCheck(MigrationTestCase):
+    """
+    Assume we're migrating everything at once
+    """
+
     # Roll back migrations to gdpr_assist.0001_initial
     migrate_from = [("gdpr_assist", "0001_initial")]
 
@@ -85,7 +115,6 @@ class TestMigrateGdprAnonymisedCheck(MigrationTestCase):
         }
     )
     def test_anonymised_migration_uses_operator__check_passes(self):
-
         executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
         plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
 
@@ -110,7 +139,6 @@ def test_anonymised_migration_uses_operator__check_passes(self):
         }
     )
     def test_no_anonymised_migration_uses_flag__check_passes(self):
-
         executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
         plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
 
@@ -157,6 +185,72 @@ def test_missing_anonymised_migration__check_fails(self):
         self.assertEqual(error.id, "gdpr_assist.E001")
 
 
+@modify_settings(
+    INSTALLED_APPS={"append": "tests.gdpr_assist_test_migrations.anonymised_migration"}
+)
+class TestMGASeparateMigratesAnonymisedCheck(MigrationTestCase):
+    """
+    Assume we're migrating gdpr-assist, then creating migrations, then migrating our
+    models
+    """
+
+    # Simulate migration gdpr-assist first
+    migrate_from = [("gdpr_assist", "0001_initial")]
+    migrate_to = [("gdpr_assist", "0002_privacyanonymised")]
+
+    def test_anonymised_migration_uses_operator__check_passes(self):
+        executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
+        plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
+
+        # Sanity check - confirm plan is what we expected from Django
+        self.assertEqual(
+            self.plan_to_names(plan),
+            [
+                ("anonymised_migration", "0002_migrate_anonymised"),
+                ("anonymised_migration", "0003_auto_20201020_0102"),
+            ],
+        )
+
+        # Run check
+        errors = check_migrate_gdpr_anonymised(executor.loader.project_state().apps)
+        self.assertEqual(len(errors), 0)
+
+
+@modify_settings(
+    INSTALLED_APPS={
+        "append": "tests.gdpr_assist_test_migrations.missing_anonymised_migration"
+    }
+)
+class TestMGASeparateMigratesMissingAnonymisedCheck(MigrationTestCase):
+    """
+    Assume we're migrating gdpr-assist, then creating migrations, then migrating our
+    models
+    """
+
+    # Simulate migration gdpr-assist first
+    migrate_from = [("gdpr_assist", "0001_initial")]
+    migrate_to = [("gdpr_assist", "0002_privacyanonymised")]
+
+    def test_missing_anonymised_migration__check_fails(self):
+        executor = MigrationExecutor(connections[DEFAULT_DB_ALIAS])
+        plan = executor.migration_plan(executor.loader.graph.leaf_nodes())
+
+        # Sanity check - confirm plan is what we expected from Django
+        self.assertEqual(
+            self.plan_to_names(plan),
+            [("missing_anonymised_migration", "0002_auto_20201020_0102")],
+        )
+
+        # Run check
+        errors = check_migrate_gdpr_anonymised(executor.loader.project_state().apps)
+        self.assertEqual(len(errors), 1)
+        error = errors[0]
+        self.assertEqual(
+            error.msg, "Removing anonymised field before its data is migrated"
+        )
+        self.assertEqual(error.id, "gdpr_assist.E001")
+
+
 @modify_settings(
     INSTALLED_APPS={"append": "tests.gdpr_assist_test_migrations.anonymised_migration"}
 )