Use the model_update service for user_update

Author: wencakisa

styleguide_example/common/services.py

@@ -1,39 +1,50 @@
-from typing import TypeVar
+from typing import List, Dict, Any, Tuple
 
-from django.db import transaction
 
-T = TypeVar('T')
-
-
-@transaction.atomic
-def generic_update(*, instance: T, **fields_to_update) -> T:
+# TODO: Decide typing for the `instance` argument
+def model_update(
+    *,
+    instance: Any,
+    fields: List[str],
+    data: Dict[str, Any]
+) -> Tuple[Any, bool]:
     """
     Generic update service meant to be reused in local update services
 
     For example:
 
-    def user_update(*, user: User, **fields_to_update) -> User:
-        user = generic_update(instance=user, **fields_to_update)
+    def user_update(*, user: User, **data) -> User:
+        fields = ['first_name', 'last_name']
+        user = model_update(instance=user, fields=fields, data=data)
 
         // Do other actions with the user here
 
         return user
-    """
-    # If the passed instance is `None` - do nothing.
-    if instance is None:
-        return instance
 
-    # If there's nothing to update - do not perform unnecessary actions.
-    if not fields_to_update:
-        return instance
-
-    for attr, value in fields_to_update.items():
-        setattr(instance, attr, value)
-
-    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
-    update_fields = list(fields_to_update.keys())
-    instance.save(update_fields=update_fields)
+    Return value: Tuple with the following elements:
+        1. The instance we updated
+        2. A boolean value representing whether we performed an update or not.
+    """
+    has_updated = False
 
-    return instance
+    if instance is None:
+        return instance, has_updated
+
+    for field in fields:
+        # Skip if a field is not present in the actual data
+        if field not in data:
+            continue
+
+        if getattr(instance, field) != data[field]:
+            has_updated = True
+            setattr(instance, field, data[field])
+
+    # Perform an update only if any of the fields was 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)
+
+    return instance, has_updated

styleguide_example/common/tests/services/test_generic_update.py

@@ -0,0 +1,75 @@
+import unittest
+from unittest.mock import Mock
+
+from styleguide_example.common.services import model_update
+
+
+class ModelUpdateTests(unittest.TestCase):
+    def setUp(self):
+        self.instance = Mock(
+            field_a=None,
+            field_b=None,
+            field_c=None
+        )
+
+    def test_model_update_does_not_update_if_instance_is_none(self):
+        updated_instance, has_updated = model_update(
+            instance=self.instance,
+            fields=[],
+            data={}
+        )
+
+        self.assertEqual(updated_instance, self.instance)
+        self.assertFalse(has_updated)
+
+        self.assertIsNone(updated_instance.field_a)
+        self.assertIsNone(updated_instance.field_b)
+        self.assertIsNone(updated_instance.field_c)
+
+        self.instance.full_clean.assert_not_called()
+        self.instance.save.assert_not_called()
+
+    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'}
+
+        updated_instance, has_updated = model_update(
+            instance=self.instance,
+            fields=update_fields,
+            data=data
+        )
+
+        self.assertEqual(updated_instance, self.instance)
+        self.assertFalse(has_updated)
+
+        self.assertIsNone(updated_instance.field_a)
+        self.assertIsNone(updated_instance.field_b)
+        self.assertIsNone(updated_instance.field_c)
+
+        self.instance.full_clean.assert_not_called()
+        self.instance.save.assert_not_called()
+
+    def test_model_update_updates_only_passed_fields_from_data(self):
+        update_fields = ['field_a']
+        data = {
+            'field_a': 'value_a',
+            'field_b': 'value_b'
+        }
+
+        updated_instance, has_updated = model_update(
+            instance=self.instance,
+            fields=update_fields,
+            data=data
+        )
+
+        self.assertTrue(has_updated)
+
+        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)
+
+        self.instance.full_clean.assert_called_once()
+        self.instance.save.assert_called_once_with(update_fields=update_fields)

styleguide_example/common/tests/services/test_model_update.py

@@ -1,5 +1,9 @@
 from typing import Optional
 
+from django.db import transaction
+
+from styleguide_example.common.services import model_update
+
 from styleguide_example.users.models import BaseUser
 
 
@@ -18,3 +22,20 @@ def user_create(
     )
 
     return user
+
+
+@transaction.atomic
+def user_update(*, user: BaseUser, **data) -> BaseUser:
+    non_side_effect_fields = ['first_name', 'last_name']
+
+    user, has_updated = model_update(
+        instance=user,
+        fields=non_side_effect_fields,
+        data=data
+    )
+
+    # Side-effect fields update here (e.g. username is generated based on first & last name)
+
+    # ... some additional tasks with the user ...
+
+    return user