Fix for AC-284. Delete groups when they are disassociated and have no more parents. Remove all group and host associations from a group when it is marked inactive.

awx/main/models/__init__.py

@@ -269,6 +269,16 @@ class Group(CommonModelNameNotUnique):
     def get_absolute_url(self):
         return reverse('main:group_detail', args=(self.pk,))
 
+    def mark_inactive(self, save=True):
+        '''
+        When marking groups inactive, remove all associations to related
+        groups/hosts.
+        '''
+        super(Group, self).mark_inactive(save=save)
+        self.parents.clear()
+        self.children.clear()
+        self.hosts.clear()
+
     def update_has_active_failures(self):
         failed_hosts = self.all_hosts.filter(active=True,
                                              last_job_host_summary__job__active=True,

awx/main/signals.py

@@ -8,7 +8,7 @@ import threading
 # Django
 from django.contrib.auth.models import User
 from django.db import DatabaseError
-from django.db.models.signals import post_save, pre_delete, post_delete, m2m_changed
+from django.db.models.signals import pre_save, post_save, pre_delete, post_delete, m2m_changed
 from django.dispatch import receiver
 
 # Django-REST-Framework
@@ -45,6 +45,7 @@ def update_inventory_has_active_failures(sender, **kwargs):
     prevent unnecessary recursive calls.
     '''
     if not getattr(_inventory_updating, 'is_updating', False):
+        instance = kwargs['instance']
         if sender == Group.hosts.through:
             sender_name = 'group.hosts'
         elif sender == Group.parents.through:
@@ -53,15 +54,19 @@ def update_inventory_has_active_failures(sender, **kwargs):
             sender_name = unicode(sender._meta.verbose_name)
         if kwargs['signal'] == post_save:
             sender_action = 'saved'
+            if instance.active: # No need to update for active instances.
+                return
         elif kwargs['signal'] == post_delete:
             sender_action = 'deleted'
-        else:
+        elif kwargs['signal'] == m2m_changed and kwargs['action'] in ('post_add', 'post_remove', 'post_clear'):
             sender_action = 'changed'
+        else:
+            return
         logger.debug('%s %s, updating inventory has_active_failures: %r %r',
                      sender_name, sender_action, sender, kwargs)
         try:
             _inventory_updating.is_updating = True
-            inventory = kwargs['instance'].inventory
+            inventory = instance.inventory
             update_hosts = issubclass(sender, Job)
             inventory.update_has_active_failures(update_hosts=update_hosts)
         finally:
@@ -102,18 +107,30 @@ def migrate_children_from_deleted_group_to_parent_groups(sender, **kwargs):
                          child_group, parent_group)
             parent_group.children.add(child_group)
 
+@receiver(pre_save, sender=Group)
+def save_related_pks_before_group_marked_inactive(sender, **kwargs):
+    instance = kwargs['instance']
+    if not instance.pk:
+        return
+    instance._saved_parents_pks = set(instance.parents.values_list('pk', flat=True))
+    instance._saved_hosts_pks = set(instance.hosts.values_list('pk', flat=True))
+    instance._saved_children_pks = set(instance.children.values_list('pk', flat=True))
+
 @receiver(post_save, sender=Group)
 def migrate_children_from_inactive_group_to_parent_groups(sender, **kwargs):
     instance = kwargs['instance']
     if instance.active:
         return
-    for parent_group in instance.parents.all():
+    parents_pks = getattr(instance, '_saved_parents_pks', [])
-        for child_host in instance.hosts.all():
+    hosts_pks = getattr(instance, '_saved_hosts_pks', [])
-            logger.debug('moving host %s to parent %s after making group %s inactive',
+    children_pks = getattr(instance, '_saved_children_pks', [])
+    for parent_group in Group.objects.filter(pk__in=parents_pks):
+        for child_host in Host.objects.filter(pk__in=hosts_pks):
+            logger.debug('moving host %s to parent %s after marking group %s inactive',
                          child_host, parent_group, instance)
             parent_group.hosts.add(child_host)
-        for child_group in instance.children.all():
+        for child_group in Group.objects.filter(pk__in=children_pks):
-            logger.debug('moving group %s to parent %s after making group %s inactive',
+            logger.debug('moving group %s to parent %s after marking group %s inactive',
                          child_group, parent_group, instance)
             parent_group.children.add(child_group)
         parent_group.children.remove(instance)

awx/main/tests/inventory.py

@@ -566,6 +566,17 @@ class InventoryTest(BaseTest):
         # try to double disassociate to see what happens (should no-op)
         self.post(subgroups_url3, data=result, expect=204, auth=self.get_other_credentials())
 
+        # removed group should be automatically marked inactive once it no longer has any parents.
+        removed_group = Group.objects.get(pk=result['id'])
+        self.assertTrue(removed_group.parents.count())
+        self.assertTrue(removed_group.active)
+        for parent in removed_group.parents.all():
+            parent_children_url = reverse('main:group_children_list', args=(parent.pk,))
+            data = {'id': removed_group.pk, 'disassociate': 1}
+            self.post(parent_children_url, data, expect=204, auth=self.get_super_credentials())
+        removed_group = Group.objects.get(pk=result['id'])
+        self.assertFalse(removed_group.active)
+
         #########################################################
         # FIXME: TAGS
 
@@ -628,11 +639,15 @@ class InventoryTest(BaseTest):
         self.assertFalse(h_d in g_c.hosts.all())
 
         # Mark group C inactive. Its child groups and hosts should now also be
-        # attached to group A. Group D hosts should be unchanged.
+        # attached to group A. Group D hosts should be unchanged.  Group C
+        # should also no longer have any group or host relationships.
         g_c.mark_inactive()
         self.assertTrue(g_d in g_a.children.all())
         self.assertTrue(h_c in g_a.hosts.all())
         self.assertFalse(h_d in g_a.hosts.all())
+        self.assertFalse(g_c.parents.all())
+        self.assertFalse(g_c.children.all())
+        self.assertFalse(g_c.hosts.all())
 
     def test_group_parents_and_children(self):
         # Test for various levels of group parent/child relations, with hosts,

awx/main/tests/tasks.py

@@ -377,11 +377,13 @@ class RunJobTest(BaseCeleryTest):
         self.assertFalse(self.group.has_active_failures)
         self.inventory = Inventory.objects.get(pk=self.inventory.pk)
         self.assertFalse(self.inventory.has_active_failures)
-        # Un-mark host as inactive (should set flag on group and inventory)
+        # Un-mark host as inactive (need to force update of flag on group and
+        # inventory)
         host = self.host
         host.name = '_'.join(host.name.split('_')[3:]) or 'undeleted host'
         host.active = True
         host.save()
+        host.update_has_active_failures()
         self.group = Group.objects.get(pk=self.group.pk)
         self.assertTrue(self.group.has_active_failures)
         self.inventory = Inventory.objects.get(pk=self.inventory.pk)
@@ -404,10 +406,11 @@ class RunJobTest(BaseCeleryTest):
         self.assertFalse(self.group.has_active_failures)
         self.inventory = Inventory.objects.get(pk=self.inventory.pk)
         self.assertFalse(self.inventory.has_active_failures)
-        # Un-mark job as inactive (should set flag on host, group and inventory)
+        # Un-mark job as inactive (need to force update of flag)
         job.name = '_'.join(job.name.split('_')[3:]) or 'undeleted job'
         job.active = True
         job.save()
+        job.inventory.update_has_active_failures()
         self.host = Host.objects.get(pk=self.host.pk)
         self.assertTrue(self.host.has_active_failures)
         self.group = Group.objects.get(pk=self.group.pk)

awx/main/views.py

@@ -457,6 +457,19 @@ class GroupChildrenList(SubListCreateAPIView):
     parent_model = Group
     relationship = 'children'
 
+    def unattach(self, request, *args, **kwargs):
+        '''
+        Special case for disassociating a child group from the parent. If the
+        child group has no more parents, then automatically mark it inactive.
+        '''
+        response = super(GroupChildrenList, self).unattach(request, *args, **kwargs)
+        if response.status_code != status.HTTP_204_NO_CONTENT:
+            return response
+        sub = self.model.objects.get(pk=request.DATA.get('id', None))
+        if sub.parents.filter(active=True).count() == 0:
+            sub.mark_inactive()
+        return response
+
 class GroupHostsList(SubListCreateAPIView):
     ''' the list of hosts directly below a group '''