Merge pull request #305 from HackSoftware/update-model-m2m

Add support for m2m fields in `model_update`

Author: RadoRado

styleguide_example/common/factories.py

@@ -0,0 +1,19 @@
+import factory
+
+from styleguide_example.common.models import RandomModel, SimpleModel
+from styleguide_example.utils.tests import faker
+
+
+class RandomModelFactory(factory.django.DjangoModelFactory):
+    class Meta:
+        model = RandomModel
+
+    end_date = factory.LazyAttribute(lambda self: faker.date_object())
+    start_date = factory.LazyAttribute(lambda self: faker.date_object(end_datetime=self.end_date))
+
+
+class SimpleModelFactory(factory.django.DjangoModelFactory):
+    class Meta:
+        model = SimpleModel
+
+    name = factory.LazyAttribute(lambda self: faker.word())

styleguide_example/common/migrations/0005_simplemodel_randommodel_simple_objects.py

@@ -0,0 +1,31 @@
+# Generated by Django 4.1.3 on 2022-11-28 11:58
+
+from django.db import migrations, models
+import django.utils.timezone
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('common', '0004_remove_randommodel_start_date_before_end_date_and_more'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='SimpleModel',
+            fields=[
+                ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('created_at', models.DateTimeField(db_index=True, default=django.utils.timezone.now)),
+                ('updated_at', models.DateTimeField(auto_now=True)),
+                ('name', models.CharField(blank=True, max_length=255, null=True)),
+            ],
+            options={
+                'abstract': False,
+            },
+        ),
+        migrations.AddField(
+            model_name='randommodel',
+            name='simple_objects',
+            field=models.ManyToManyField(blank=True, related_name='random_objects', to='common.simplemodel'),
+        ),
+    ]

styleguide_example/common/models.py

@@ -11,6 +11,15 @@ class Meta:
         abstract = True
 
 
+class SimpleModel(BaseModel):
+    """
+    This is a basic model used to illustrate a many-to-many relationship
+    with RandomModel.
+    """
+
+    name = models.CharField(max_length=255, blank=True, null=True)
+
+
 class RandomModel(BaseModel):
     """
     This is an example model, to be used as reference in the Styleguide,
@@ -20,5 +29,7 @@ class RandomModel(BaseModel):
     start_date = models.DateField()
     end_date = models.DateField()
 
+    simple_objects = models.ManyToManyField(SimpleModel, blank=True, related_name="random_objects")
+
     class Meta:
         constraints = [models.CheckConstraint(name="start_date_before_end_date", check=Q(start_date__lt=F("end_date")))]

styleguide_example/common/services.py

@@ -1,11 +1,13 @@
 from typing import Any, Dict, List, Tuple
 
+from django.db import models
+
 from styleguide_example.common.types import DjangoModelType
 
 
 def model_update(*, instance: DjangoModelType, fields: List[str], data: Dict[str, Any]) -> Tuple[DjangoModelType, bool]:
     """
-    Generic update service meant to be reused in local update services
+    Generic update service meant to be reused in local update services.
 
     For example:
 
@@ -18,26 +20,56 @@ def user_update(*, user: User, data) -> User:
         return user
 
     Return value: Tuple with the following elements:
-        1. The instance we updated
+        1. The instance we updated.
         2. A boolean value representing whether we performed an update or not.
+
+    Some important notes:
+
+        - Only keys present in `fields` will be taken from `data`.
+        - If something in present in `fields` but not present in `data`, we simply skip.
+        - There's a strict assertion that all values in `fields` are actual fields in `instance`.
+        - `fields` can support m2m fields, which are handled after the update on `instance`.
     """
     has_updated = False
+    m2m_data = {}
+    update_fields = []
+
+    model_fields = {field.name: field for field in instance._meta.get_fields()}
 
     for field in fields:
         # Skip if a field is not present in the actual data
         if field not in data:
             continue
 
+        # If field is not an actual model field, raise an error
+        model_field = model_fields.get(field)
+
+        assert model_field is not None, f"{field} is not part of {instance.__class__.__name__} fields."
+
+        # If we have m2m field, handle differently
+        if isinstance(model_field, models.ManyToManyField):
+            m2m_data[field] = data[field]
+            continue
+
         if getattr(instance, field) != data[field]:
             has_updated = True
+            update_fields.append(field)
             setattr(instance, field, data[field])
 
-    # Perform an update only if any of the fields was actually changed
+    # Perform an update only if any of the fields were actually changed
     if has_updated:
         instance.full_clean()
         # Update only the fields that are meant to be updated.
         # Django docs reference:
         # https://docs.djangoproject.com/en/dev/ref/models/instances/#specifying-which-fields-to-save
-        instance.save(update_fields=fields)
+        instance.save(update_fields=update_fields)
+
+    for field_name, value in m2m_data.items():
+        related_manager = getattr(instance, field_name)
+        related_manager.set(value)
+
+        # Still not sure about this.
+        # What if we only update m2m relations & nothing on the model? Is this still considered as updated?
+        has_updated = True
 
     return instance, has_updated

styleguide_example/common/tests/services/test_model_update.py

@@ -1,43 +1,101 @@
-import unittest
+from datetime import timedelta
 from unittest.mock import Mock
 
+from django.db import connection
+from django.test import TestCase
+from django.test.utils import CaptureQueriesContext
+
+from styleguide_example.common.factories import RandomModelFactory, SimpleModelFactory
 from styleguide_example.common.services import model_update
 
 
-class ModelUpdateTests(unittest.TestCase):
+class ModelUpdateTests(TestCase):
     def setUp(self):
+        self.model_instance = RandomModelFactory()
+        self.simple_object = SimpleModelFactory()
         self.instance = Mock(field_a=None, field_b=None, field_c=None)
 
-    def test_model_update_does_not_update_if_none_of_the_fields_are_in_the_data(self):
-        update_fields = ["non_existing_field"]
-        data = {"field_a": "value_a"}
+    def test_model_update_does_nothing(self):
+        with self.subTest("when no fields are provided"):
+            instance = RandomModelFactory()
+
+            updated_instance, has_updated = model_update(instance=instance, fields=[], data={})
 
-        updated_instance, has_updated = model_update(instance=self.instance, fields=update_fields, data=data)
+            self.assertEqual(instance, updated_instance)
+            self.assertFalse(has_updated)
+            self.assertNumQueries(0)
 
-        self.assertEqual(updated_instance, self.instance)
-        self.assertFalse(has_updated)
+        with self.subTest("when non of the fields are in the data"):
+            instance = RandomModelFactory()
 
-        self.assertIsNone(updated_instance.field_a)
-        self.assertIsNone(updated_instance.field_b)
-        self.assertIsNone(updated_instance.field_c)
+            updated_instance, has_updated = model_update(instance=instance, fields=["start_date"], data={"foo": "bar"})
 
-        self.instance.full_clean.assert_not_called()
-        self.instance.save.assert_not_called()
+            self.assertEqual(instance, updated_instance)
+            self.assertFalse(has_updated)
+            self.assertNumQueries(0)
 
     def test_model_update_updates_only_passed_fields_from_data(self):
-        update_fields = ["field_a"]
-        data = {"field_a": "value_a", "field_b": "value_b"}
+        instance = RandomModelFactory()
+
+        update_fields = ["start_date"]
+        data = {
+            "field_a": "value_a",
+            "start_date": instance.start_date - timedelta(days=1),
+            "end_date": instance.end_date + timedelta(days=1),
+        }
+
+        self.assertNotEqual(instance.start_date, data["start_date"])
+
+        update_query = None
 
-        updated_instance, has_updated = model_update(instance=self.instance, fields=update_fields, data=data)
+        with CaptureQueriesContext(connection) as ctx:
+            updated_instance, has_updated = model_update(instance=instance, fields=update_fields, data=data)
+            update_query = ctx.captured_queries[-1]
 
         self.assertTrue(has_updated)
+        self.assertEqual(updated_instance.start_date, data["start_date"])
+        self.assertNotEqual(updated_instance.end_date, data["end_date"])
+
+        self.assertFalse(hasattr(updated_instance, "field_a"))
+
+        self.assertNotIn("end_date", update_query)
+
+    def test_model_update_raises_error_when_called_with_non_existent_field(self):
+        instance = RandomModelFactory()
+
+        update_fields = ["non_existing_field"]
+        data = {"non_existing_field": "foo"}
+
+        with self.assertRaises(AssertionError):
+            updated_instance, has_updated = model_update(instance=instance, fields=update_fields, data=data)
 
-        self.assertEqual(updated_instance.field_a, "value_a")
-        # Even though `field_b` is passed in `data` - it does not get updated
-        # because it is not present in the `fields` list.
-        self.assertIsNone(updated_instance.field_b)
-        # `field_c` remains `None`, because it is not passed anywhere.
-        self.assertIsNone(updated_instance.field_c)
+    def test_model_update_updates_many_to_many_fields(self):
+        instance = RandomModelFactory()
+        simple_obj = SimpleModelFactory()
 
-        self.instance.full_clean.assert_called_once()
-        self.instance.save.assert_called_once_with(update_fields=update_fields)
+        update_fields = ["simple_objects"]
+        data = {"simple_objects": [simple_obj]}
+
+        self.assertNotIn(simple_obj, instance.simple_objects.all())
+
+        updated_instance, has_updated = model_update(instance=instance, fields=update_fields, data=data)
+
+        self.assertEqual(updated_instance, instance)
+        self.assertTrue(has_updated)
+
+        self.assertIn(simple_obj, updated_instance.simple_objects.all())
+
+    def test_model_update_updates_standard_and_many_to_many_fields(self):
+        instance = RandomModelFactory()
+        simple_obj = SimpleModelFactory()
+
+        update_fields = ["start_date", "simple_objects"]
+        data = {"start_date": instance.start_date - timedelta(days=1), "simple_objects": [simple_obj]}
+
+        self.assertNotIn(simple_obj, instance.simple_objects.all())
+
+        updated_instance, has_updated = model_update(instance=instance, fields=update_fields, data=data)
+
+        self.assertTrue(has_updated)
+        self.assertEqual(updated_instance.start_date, data["start_date"])
+        self.assertIn(simple_obj, updated_instance.simple_objects.all())