Merge pull request #1540 from AlanCoding/modified_by_alice

Do not update modified_by for system fields

awx/main/models/base.py

@@ -256,6 +256,7 @@ class PrimordialModel(CreatedModifiedModel):
 
     def save(self, *args, **kwargs):
         update_fields = kwargs.get('update_fields', [])
+        fields_are_specified = bool(update_fields)
         user = get_current_user()
         if user and not user.id:
             user = None
@@ -263,9 +264,14 @@ class PrimordialModel(CreatedModifiedModel):
             self.created_by = user
             if 'created_by' not in update_fields:
                 update_fields.append('created_by')
-        self.modified_by = user
-        if 'modified_by' not in update_fields:
-            update_fields.append('modified_by')
+        # Update modified_by if not called with update_fields, or if any
+        # editable fields are present in update_fields
+        if (
+                (not fields_are_specified) or
+                any(getattr(self._meta.get_field(name), 'editable', True) for name in update_fields)):
+            self.modified_by = user
+            if 'modified_by' not in update_fields:
+                update_fields.append('modified_by')
         super(PrimordialModel, self).save(*args, **kwargs)
 
     def clean_description(self):

awx/main/models/unified_jobs.py

@@ -263,14 +263,7 @@ class UnifiedJobTemplate(PolymorphicModel, CommonModelNameNotUnique, Notificatio
                 if field not in update_fields:
                     update_fields.append(field)
         # Do the actual save.
-        try:
-            super(UnifiedJobTemplate, self).save(*args, **kwargs)
-        except ValueError:
-            # A fix for https://trello.com/c/S4rU1F21
-            # Does not resolve the root cause. Tis merely a bandaid.
-            if 'scm_delete_on_next_update' in update_fields:
-                update_fields.remove('scm_delete_on_next_update')
-                super(UnifiedJobTemplate, self).save(*args, **kwargs)
+        super(UnifiedJobTemplate, self).save(*args, **kwargs)
 
 
     def _get_current_status(self):
@@ -722,7 +715,10 @@ class UnifiedJob(PolymorphicModel, PasswordFieldsModel, CommonModelNameNotUnique
     def _get_parent_instance(self):
         return getattr(self, self._get_parent_field_name(), None)
 
-    def _update_parent_instance_no_save(self, parent_instance, update_fields=[]):
+    def _update_parent_instance_no_save(self, parent_instance, update_fields=None):
+        if update_fields is None:
+            update_fields = []
+
         def parent_instance_set(key, val):
             setattr(parent_instance, key, val)
             if key not in update_fields:

awx/main/tests/functional/models/test_job.py

@@ -1,6 +1,7 @@
 import pytest
 
 from awx.main.models import JobTemplate, Job
+from crum import impersonate
 
 
 @pytest.mark.django_db
@@ -49,3 +50,18 @@ def test_awx_custom_virtualenv_without_jt(project):
 
     job = Job.objects.get(pk=job.id)
     assert job.ansible_virtualenv_path == '/venv/fancy-proj'
+
+
+@pytest.mark.django_db
+def test_update_parent_instance(job_template, alice):
+    # jobs are launched as a particular user, user not saved as modified_by
+    with impersonate(alice):
+        assert job_template.current_job is None
+        assert job_template.status == 'never updated'
+        assert job_template.modified_by is None
+        job = job_template.jobs.create(status='new')
+        job.status = 'pending'
+        job.save()
+        assert job_template.current_job == job
+        assert job_template.status == 'pending'
+        assert job_template.modified_by is None