Merge pull request #1851 from AlanCoding/sch_modified_by

Fix schedules modified_by getting nulled

awx/main/fields.py

@@ -218,6 +218,7 @@ class ImplicitRoleField(models.ForeignKey):
         kwargs.setdefault('to', 'Role')
         kwargs.setdefault('related_name', '+')
         kwargs.setdefault('null', 'True')
+        kwargs.setdefault('editable', False)
         super(ImplicitRoleField, self).__init__(*args, **kwargs)
 
     def deconstruct(self):

awx/main/models/base.py

@@ -254,9 +254,13 @@ class PrimordialModel(CreatedModifiedModel):
 
     tags = TaggableManager(blank=True)
 
+    def __init__(self, *args, **kwargs):
+        r = super(PrimordialModel, self).__init__(*args, **kwargs)
+        self._prior_values_store = self._get_fields_snapshot()
+        return r
+
     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
@@ -264,15 +268,43 @@ class PrimordialModel(CreatedModifiedModel):
             self.created_by = user
             if 'created_by' not in update_fields:
                 update_fields.append('created_by')
-        # Update modified_by if not called with update_fields, or if any
+        # Update modified_by if any editable fields have changed
-        # editable fields are present in update_fields
+        new_values = self._get_fields_snapshot()
-        if (
+        if (not self.pk and not self.modified_by) or self.has_user_edits(new_values):
-                (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)
+        self._prior_values_store = new_values
+
+    def has_user_edits(self, new_values):
+        return any(
+            new_values.get(fd_name, None) != self._prior_values_store.get(fd_name, None)
+            for fd_name in new_values.keys()
+        )
+
+    @classmethod
+    def _get_editable_fields(cls):
+        fds = set([])
+        for field in cls._meta.concrete_fields:
+            if hasattr(field, 'attname'):
+                if field.attname == 'id':
+                    continue
+                elif field.attname.endswith('ptr_id'):
+                    # polymorphic fields should always be non-editable, see:
+                    # https://github.com/django-polymorphic/django-polymorphic/issues/349
+                    continue
+                if getattr(field, 'editable', True):
+                    fds.add(field.attname)
+        return fds
+
+    def _get_fields_snapshot(self):
+        new_values = {}
+        editable_set = self._get_editable_fields()
+        for attr, val in self.__dict__.items():
+            if attr in editable_set:
+                new_values[attr] = val
+        return new_values
 
     def clean_description(self):
         # Description should always be empty string, never null.

awx/main/models/projects.py

@@ -324,13 +324,9 @@ class Project(UnifiedJobTemplate, ProjectOptions, ResourceMixin, CustomVirtualEn
             ['name', 'description', 'schedule']
         )
 
-    def __init__(self, *args, **kwargs):
-        r = super(Project, self).__init__(*args, **kwargs)
-        self._prior_values_store = self._current_sensitive_fields()
-        return r
-
     def save(self, *args, **kwargs):
         new_instance = not bool(self.pk)
+        pre_save_vals = getattr(self, '_prior_values_store', {})
         # If update_fields has been specified, add our field names to it,
         # if it hasn't been specified, then we're just doing a normal save.
         update_fields = kwargs.get('update_fields', [])
@@ -361,21 +357,13 @@ class Project(UnifiedJobTemplate, ProjectOptions, ResourceMixin, CustomVirtualEn
                     self.save(update_fields=update_fields)
         # If we just created a new project with SCM, start the initial update.
         # also update if certain fields have changed
-        relevant_change = False
+        relevant_change = any(
-        new_values = self._current_sensitive_fields()
+            pre_save_vals.get(fd_name, None) != self._prior_values_store.get(fd_name, None)
-        if hasattr(self, '_prior_values_store') and self._prior_values_store != new_values:
+            for fd_name in self.FIELDS_TRIGGER_UPDATE
-            relevant_change = True
+        )
-        self._prior_values_store = new_values
         if (relevant_change or new_instance) and (not skip_update) and self.scm_type:
             self.update()
 
-    def _current_sensitive_fields(self):
-        new_values = {}
-        for attr, val in self.__dict__.items():
-            if attr in Project.FIELDS_TRIGGER_UPDATE:
-                new_values[attr] = val
-        return new_values
-
     def _get_current_status(self):
         if self.scm_type:
             if self.current_job and self.current_job.status:

awx/main/signals.py

@@ -10,7 +10,6 @@ import json
 # Django
 from django.conf import settings
 from django.db.models.signals import (
-    post_init,
     post_save,
     pre_delete,
     post_delete,
@@ -200,14 +199,6 @@ def cleanup_detached_labels_on_deleted_parent(sender, instance, **kwargs):
             l.delete()
 
 
-def set_original_organization(sender, instance, **kwargs):
-    '''set_original_organization is used to set the original, or
-    pre-save organization, so we can later determine if the organization
-    field is dirty.
-    '''
-    instance.__original_org_id = instance.organization_id
-
-
 def save_related_job_templates(sender, instance, **kwargs):
     '''save_related_job_templates loops through all of the
     job templates that use an Inventory or Project that have had their
@@ -217,7 +208,7 @@ def save_related_job_templates(sender, instance, **kwargs):
     if sender not in (Project, Inventory):
         raise ValueError('This signal callback is only intended for use with Project or Inventory')
 
-    if instance.__original_org_id != instance.organization_id:
+    if instance._prior_values_store.get('organization_id') != instance.organization_id:
         jtq = JobTemplate.objects.filter(**{sender.__name__.lower(): instance})
         for jt in jtq:
             update_role_parentage_for_instance(jt)
@@ -240,8 +231,6 @@ def connect_computed_field_signals():
 
 connect_computed_field_signals()
 
-post_init.connect(set_original_organization, sender=Project)
-post_init.connect(set_original_organization, sender=Inventory)
 post_save.connect(save_related_job_templates, sender=Project)
 post_save.connect(save_related_job_templates, sender=Inventory)
 post_save.connect(emit_job_event_detail, sender=JobEvent)

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

@@ -2,6 +2,7 @@ import pytest
 import mock
 
 from awx.main.models import Project
+from awx.main.models.organization import Organization
 
 
 @pytest.mark.django_db
@@ -31,3 +32,10 @@ def test_sensitive_change_triggers_update(project):
         project.scm_url = 'https://foo2.invalid'
         project.save()
     mock_update.assert_called_once_with()
+
+
+@pytest.mark.django_db
+def test_foreign_key_change_changes_modified_by(project, organization):
+    assert project._get_fields_snapshot()['organization_id'] == organization.id
+    project.organization = Organization(name='foo', pk=41)
+    assert project._get_fields_snapshot()['organization_id'] == 41

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

@@ -7,6 +7,8 @@ import pytz
 
 from awx.main.models import JobTemplate, Schedule
 
+from crum import impersonate
+
 
 @pytest.fixture
 def job_template(inventory, project):
@@ -18,6 +20,22 @@ def job_template(inventory, project):
     )
 
 
+@pytest.mark.django_db
+def test_computed_fields_modified_by_retained(job_template, admin_user):
+    with impersonate(admin_user):
+        s = Schedule.objects.create(
+            name='Some Schedule',
+            rrule='DTSTART:20300112T210000Z RRULE:FREQ=DAILY;INTERVAL=1',
+            unified_job_template=job_template
+        )
+    s.refresh_from_db()
+    assert s.created_by == admin_user
+    assert s.modified_by == admin_user
+    s.update_computed_fields()
+    s.save()
+    assert s.modified_by == admin_user
+
+
 @pytest.mark.django_db
 def test_repeats_forever(job_template):
     s = Schedule(