Merge branch 'devel' into the_clone_wars

ansible_base/lib/abstract_models/common.py

@@ -2,6 +2,7 @@
 from collections import OrderedDict
 
 from django.conf import settings
+from django.contrib.auth.models import AbstractUser
 from django.db import models
 from django.urls.exceptions import NoReverseMatch
 from django.utils.translation import gettext_lazy as _
@@ -59,15 +60,40 @@ def save(self, *args, **kwargs):
         '''
         This save function will provide the following features automatically.
           * It will automatically add the modified fields for changing items
-        '''
-        update_fields = list(kwargs.get('update_fields', []))
 
-        if 'modified_by' not in update_fields:
+        There are several edge cases to be aware of:
+          * If the user is logging in and their "last_login" field is getting
+            changed (in their user model instance), then we do not update
+            "modified_by". (This is so they can see if someone changed their
+            user object while they were away).
+          * If no fields in update_fields are "editable", then we don't update
+            "modified_by".
+          * "modified" is updated in every case, since it's an auto_now
+            timestamp.
+        '''
+        has_update_fields = kwargs.get('update_fields') is not None
+        update_fields = kwargs.get('update_fields', [])
+        if update_fields is None:
+            update_fields = []
+        else:
+            update_fields = list(update_fields)
+        is_user_logging_in = isinstance(self, AbstractUser) and update_fields == ['last_login']
+        has_editable_field = any(self._meta.get_field(field).editable for field in update_fields)
+
+        if 'modified_by' in update_fields:
+            # Explicit update of modified_by, don't mess with it
+            pass
+        elif is_user_logging_in:
+            # User is logging in, only last_login is changing, don't update modified_by
+            pass
+        elif has_update_fields and not has_editable_field:
+            # No editable fields are changing, don't update modified_by
+            pass
+        else:
             self.modified_by = current_user_or_system_user()
             update_fields.append('modified_by')
-
-        if kwargs.get('update_fields') is not None:
-            kwargs['update_fields'] = update_fields
+            if has_update_fields:
+                kwargs['update_fields'] = update_fields
 
         return super().save(*args, **kwargs)
 

test_app/migrations/0009_city_state.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.8 on 2024-04-15 20:06
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('test_app', '0008_secretcolor'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='city',
+            name='state',
+            field=models.CharField(editable=False, max_length=100, null=True),
+        ),
+    ]

test_app/models.py

@@ -292,6 +292,7 @@ class Meta:
     country = prevent_search(models.CharField(max_length=100, null=True, default='USA'))
     population = models.PositiveIntegerField(null=True, default=1000)
     extra_data = JSONField(null=True, default=dict)
+    state = models.CharField(max_length=100, null=True, editable=False)
 
 
 class SecretColor(AuditableModel):

test_app/tests/lib/abstract_models/test_common.py

@@ -10,7 +10,7 @@
 from rest_framework.reverse import reverse
 
 from ansible_base.rbac.models import RoleDefinition
-from test_app.models import EncryptionModel, Organization, RelatedFieldsTestModel, User
+from test_app.models import City, EncryptionModel, Organization, RelatedFieldsTestModel, User
 
 
 @pytest.mark.django_db
@@ -147,7 +147,6 @@ def test_cascade_behavior_for_created_by(user, user_api_client):
     connection.check_constraints()
 
 
-@pytest.mark.xfail(reaason="https://github.com/ansible/django-ansible-base/issues/286")
 def test_do_not_update_modified_by_on_login(system_user, user, user_api_client):
     user.refresh_from_db()
     assert user.modified_by == system_user
@@ -167,6 +166,31 @@ def test_modified_by_respects_given_value(system_user, random_user, user, animal
     assert animal.modified_by == random_user
 
 
+@pytest.mark.parametrize(
+    'update_fields, expected_modified_by',
+    [
+        pytest.param(['population'], 'user', id='modified_by saved even if not in update_fields'),
+        pytest.param(['state'], 'system_user', id='update_fields only lists non-editable fields, modified_by does not get set'),
+        pytest.param(['state', 'population'], 'user', id='update_fields lists some non-editable fields, modified_by gets set'),
+        pytest.param(None, 'user', id='update_fields is None, modified_by gets set'),
+        pytest.param(False, 'user', id='update_fields not passed, modified_by gets set'),
+        pytest.param([], 'system_user', id='update_fields empty, modified_by does not get set'),
+    ],
+)
+def test_modified_by_not_set_if_update_fields_are_all_uneditable(system_user, user, update_fields, expected_modified_by):
+    city = City.objects.create(name='Boston', state='MA')
+    assert city.modified_by == system_user
+    city.state = 'Ohio'
+    city.population = 38
+    with impersonate(user):
+        if update_fields is False:
+            city.save()
+        else:
+            city.save(update_fields=update_fields)
+    city.refresh_from_db()
+    assert city.modified_by == (user if expected_modified_by == 'user' else system_user)
+
+
 def test_modified_by_gets_saved_even_if_not_in_update_fields(system_user, user, animal):
     animal.save()
     assert animal.modified_by == system_user