Merge pull request #1690 from chrismeyersfsu/fix-labels_disassociate

delete label on last disassociate

awx/api/generics.py

@@ -25,6 +25,7 @@ from rest_framework import views
 
 # AWX
 from awx.main.models import *  # noqa
+from awx.main.models import Label
 from awx.main.utils import * # noqa
 from awx.api.serializers import ResourceAccessListElementSerializer
 
@@ -35,7 +36,8 @@ __all__ = ['APIView', 'GenericAPIView', 'ListAPIView', 'SimpleListAPIView',
            'RetrieveUpdateDestroyAPIView', 'DestroyAPIView',
            'SubDetailAPIView',
            'ResourceAccessList',
-           'ParentMixin',]
+           'ParentMixin',
+           'DeleteLastUnattachLabelMixin',]
 
 logger = logging.getLogger('awx.api.generics')
 
@@ -399,12 +401,15 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView):
         else:
             return Response(status=status.HTTP_204_NO_CONTENT)
 
-    def unattach(self, request, *args, **kwargs):
+    def unattach_validate(self, request):
         sub_id = request.data.get('id', None)
+        res = None
         if not sub_id:
             data = dict(msg='"id" is required to disassociate')
-            return Response(data, status=status.HTTP_400_BAD_REQUEST)
+            res = Response(data, status=status.HTTP_400_BAD_REQUEST)
+        return (sub_id, res)
 
+    def unattach_by_id(self, request, sub_id):
         parent = self.get_parent_object()
         parent_key = getattr(self, 'parent_key', None)
         relationship = getattrd(parent, self.relationship)
@@ -421,6 +426,12 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView):
 
         return Response(status=status.HTTP_204_NO_CONTENT)
 
+    def unattach(self, request, *args, **kwargs):
+        (sub_id, res) = self.unattach_validate(request)
+        if res:
+            return res
+        return self.unattach_by_id(request, sub_id)
+       
     def post(self, request, *args, **kwargs):
         if not isinstance(request.data, dict):
             return Response('invalid type for post data',
@@ -430,6 +441,21 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView):
         else:
             return self.attach(request, *args, **kwargs)
 
+class DeleteLastUnattachLabelMixin(object):
+    def unattach(self, request, *args, **kwargs):
+        (sub_id, res) = super(DeleteLastUnattachLabelMixin, self).unattach_validate(request, *args, **kwargs)
+        if res:
+            return res
+
+        res = super(DeleteLastUnattachLabelMixin, self).unattach_by_id(request, sub_id)
+
+        label = Label.objects.get(id=sub_id)
+
+        if label.is_detached():
+            label.delete()
+
+        return res
+
 class SubDetailAPIView(generics.RetrieveAPIView, GenericAPIView, ParentMixin):
     pass
 

awx/api/views.py

@@ -2282,7 +2282,7 @@ class JobTemplateNotifiersSuccessList(SubListCreateAttachDetachAPIView):
     parent_model = JobTemplate
     relationship = 'notifiers_success'
 
-class JobTemplateLabelList(SubListCreateAttachDetachAPIView):
+class JobTemplateLabelList(SubListCreateAttachDetachAPIView, DeleteLastUnattachLabelMixin):
 
     model = Label
     serializer_class = LabelSerializer

awx/main/models/label.py

@@ -8,6 +8,7 @@ from django.utils.translation import ugettext_lazy as _
 
 # AWX
 from awx.main.models.base import CommonModelNameNotUnique
+from awx.main.models.unified_jobs import UnifiedJobTemplate, UnifiedJob
 
 __all__ = ('Label', )
 
@@ -39,3 +40,19 @@ class Label(CommonModelNameNotUnique):
                 jobtemplate_labels__isnull=True
             )
 
+    def is_detached(self):
+        return bool(
+            Label.objects.filter(
+                id=self.id, 
+                unifiedjob_labels__isnull=True,
+                unifiedjobtemplate_labels__isnull=True
+            ).count())
+
+    def is_candidate_for_detach(self):
+        c1 = UnifiedJob.objects.filter(labels__in=[self.id]).count()
+        c2 = UnifiedJobTemplate.objects.filter(labels__in=[self.id]).count()
+        if (c1 + c2 - 1) == 0:
+            return True
+        else:
+            return False
+

awx/main/signals.py

@@ -156,6 +156,10 @@ def org_admin_edit_members(instance, action, model, reverse, pk_set, **kwargs):
                 if action == 'pre_remove':
                     instance.content_object.admin_role.children.remove(user.admin_role)
 
+def cleanup_detached_labels_on_deleted_parent(sender, instance, **kwargs):
+    for l in instance.labels.all():
+        if l.is_candidate_for_detach():
+            l.delete()
 
 post_save.connect(emit_update_inventory_on_created_or_deleted, sender=Host)
 post_delete.connect(emit_update_inventory_on_created_or_deleted, sender=Host)
@@ -175,7 +179,8 @@ m2m_changed.connect(rebuild_role_ancestor_list, Role.parents.through)
 m2m_changed.connect(org_admin_edit_members, Role.members.through)
 post_save.connect(sync_superuser_status_to_rbac, sender=User)
 post_save.connect(create_user_role, sender=User)
-
+pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJob)
+pre_delete.connect(cleanup_detached_labels_on_deleted_parent, sender=UnifiedJobTemplate)
 
 # Migrate hosts, groups to parent group(s) whenever a group is deleted
 

awx/main/tests/unit/api/test_generics.py

@@ -1,13 +1,14 @@
 
 # Python
 import pytest
+import mock
 
 # DRF
 from rest_framework import status
 from rest_framework.response import Response
 
 # AWX
-from awx.api.generics import ParentMixin, SubListCreateAttachDetachAPIView
+from awx.api.generics import ParentMixin, SubListCreateAttachDetachAPIView, DeleteLastUnattachLabelMixin
 
 @pytest.fixture
 def get_object_or_404(mocker):
@@ -37,7 +38,7 @@ def parent_relationship_factory(mocker):
         return (serializer, mock_parent_relationship)
     return rf
 
-# TODO: Test create and associate failure (i.e. id doesn't exist or record already exists)
+# TODO: Test create and associate failure (i.e. id doesn't exist, record already exists, permission denied)
 # TODO: Mock and check return (Response)
 class TestSubListCreateAttachDetachAPIView:
     def test_attach_create_and_associate(self, mocker, get_object_or_400, parent_relationship_factory, mock_response_new):
@@ -65,6 +66,97 @@ class TestSubListCreateAttachDetachAPIView:
         mock_parent_relationship.wife.add.assert_called_with(get_object_or_400.return_value)
         mock_response_new.assert_called_with(Response, status=status.HTTP_204_NO_CONTENT)
 
+    def test_unattach_validate_ok(self, mocker):
+        mock_request = mocker.MagicMock(data=dict(id=1))
+        serializer = SubListCreateAttachDetachAPIView()
+
+        (sub_id, res) = serializer.unattach_validate(mock_request)
+
+        assert sub_id == 1
+        assert res is None
+    
+    def test_unattach_validate_missing_id(self, mocker):
+        mock_request = mocker.MagicMock(data=dict())
+        serializer = SubListCreateAttachDetachAPIView()
+
+        (sub_id, res) = serializer.unattach_validate(mock_request)
+
+        assert sub_id is None
+        assert type(res) is Response
+ 
+    def test_unattach_by_id_ok(self, mocker, parent_relationship_factory, get_object_or_400):
+        (serializer, mock_parent_relationship) = parent_relationship_factory(SubListCreateAttachDetachAPIView, 'wife')
+        mock_request = mocker.MagicMock()
+        mock_sub = mocker.MagicMock(name="object to unattach")
+        get_object_or_400.return_value = mock_sub
+
+        res = serializer.unattach_by_id(mock_request, 1)
+
+        assert type(res) is Response
+        assert res.status_code == status.HTTP_204_NO_CONTENT
+        mock_parent_relationship.wife.remove.assert_called_with(mock_sub)
+
+    def test_unattach_ok(self, mocker):
+        mock_request = mocker.MagicMock()
+        mock_sub_id = mocker.MagicMock()
+        view = SubListCreateAttachDetachAPIView()
+        view.unattach_validate = mocker.MagicMock()
+        view.unattach_by_id = mocker.MagicMock()
+        view.unattach_validate.return_value = (mock_sub_id, None)
+
+        view.unattach(mock_request)
+
+        view.unattach_validate.assert_called_with(mock_request)
+        view.unattach_by_id.assert_called_with(mock_request, mock_sub_id)
+
+    def test_unattach_invalid(self, mocker):
+        mock_request = mocker.MagicMock()
+        mock_res = mocker.MagicMock()
+        view = SubListCreateAttachDetachAPIView()
+        view.unattach_validate = mocker.MagicMock()
+        view.unattach_by_id = mocker.MagicMock()
+        view.unattach_validate.return_value = (None, mock_res)
+
+        view.unattach(mock_request)
+
+        view.unattach_validate.assert_called_with(mock_request)
+        view.unattach_by_id.assert_not_called()
+
+class TestDeleteLastUnattachLabelMixin:
+    @mock.patch('awx.api.generics.super')
+    def test_unattach_ok(self, super, mocker):
+        mock_request = mocker.MagicMock()
+        mock_sub_id = mocker.MagicMock()
+        super.return_value = super
+        super.unattach_validate = mocker.MagicMock(return_value=(mock_sub_id, None))
+        super.unattach_by_id = mocker.MagicMock()
+        mock_label = mocker.patch('awx.api.generics.Label')
+        mock_label.objects.get.return_value = mock_label
+        mock_label.is_detached.return_value = True
+
+        view = DeleteLastUnattachLabelMixin()
+
+        view.unattach(mock_request, None, None)
+
+        super.unattach_validate.assert_called_with(mock_request, None, None)
+        super.unattach_by_id.assert_called_with(mock_request, mock_sub_id)
+        mock_label.is_detached.assert_called_with()
+        mock_label.objects.get.assert_called_with(id=mock_sub_id)
+        mock_label.delete.assert_called_with()
+
+    @mock.patch('awx.api.generics.super')
+    def test_unattach_fail(self, super, mocker):
+        mock_request = mocker.MagicMock()
+        mock_response = mocker.MagicMock()
+        super.return_value = super
+        super.unattach_validate = mocker.MagicMock(return_value=(None, mock_response))
+        view = DeleteLastUnattachLabelMixin()
+
+        res = view.unattach(mock_request, None, None)
+
+        super.unattach_validate.assert_called_with(mock_request, None, None)
+        assert mock_response == res
+
 class TestParentMixin:
     def test_get_parent_object(self, mocker, get_object_or_404):
         parent_mixin = ParentMixin()

awx/main/tests/unit/models/test_label.py

@@ -1,4 +1,8 @@
+import pytest
+
 from awx.main.models.label import Label
+from awx.main.models.unified_jobs import UnifiedJobTemplate, UnifiedJob
+
 
 def test_get_orphaned_labels(mocker):
     mock_query_set = mocker.MagicMock()
@@ -8,3 +12,54 @@ def test_get_orphaned_labels(mocker):
 
     assert mock_query_set == ret
     Label.objects.filter.assert_called_with(organization=None, jobtemplate_labels__isnull=True)
+
+def test_is_detached(mocker):
+    mock_query_set = mocker.MagicMock()
+    Label.objects.filter = mocker.MagicMock(return_value=mock_query_set)
+    mock_query_set.count.return_value = 1
+
+    label = Label(id=37)
+    ret = label.is_detached()
+
+    assert ret is True
+    Label.objects.filter.assert_called_with(id=37, unifiedjob_labels__isnull=True, unifiedjobtemplate_labels__isnull=True)
+    mock_query_set.count.assert_called_with()
+
+def test_is_detached_not(mocker):
+    mock_query_set = mocker.MagicMock()
+    Label.objects.filter = mocker.MagicMock(return_value=mock_query_set)
+    mock_query_set.count.return_value = 0
+
+    label = Label(id=37)
+    ret = label.is_detached()
+
+    assert ret is False
+    Label.objects.filter.assert_called_with(id=37, unifiedjob_labels__isnull=True, unifiedjobtemplate_labels__isnull=True)
+    mock_query_set.count.assert_called_with()
+
+@pytest.mark.parametrize("jt_count,j_count,expected", [
+    (1, 0, True),
+    (0, 1, True),
+    (1, 1, False),
+])
+def test_is_candidate_for_detach(mocker, jt_count, j_count, expected):
+    mock_job_qs = mocker.MagicMock()
+    mock_job_qs.count = mocker.MagicMock(return_value=j_count)
+    UnifiedJob.objects = mocker.MagicMock()
+    UnifiedJob.objects.filter = mocker.MagicMock(return_value=mock_job_qs)
+
+    mock_jt_qs = mocker.MagicMock()
+    mock_jt_qs.count = mocker.MagicMock(return_value=jt_count)
+    UnifiedJobTemplate.objects = mocker.MagicMock()
+    UnifiedJobTemplate.objects.filter = mocker.MagicMock(return_value=mock_jt_qs)
+
+    label = Label(id=37)
+    ret = label.is_candidate_for_detach()
+
+    UnifiedJob.objects.filter.assert_called_with(labels__in=[label.id])
+    UnifiedJobTemplate.objects.filter.assert_called_with(labels__in=[label.id])
+    mock_job_qs.count.assert_called_with()
+    mock_jt_qs.count.assert_called_with()
+
+    assert ret is expected
+

awx/main/tests/unit/test_signals.py

@@ -0,0 +1,17 @@
+from awx.main import signals
+
+class TestCleanupDetachedLabels:
+    def test_cleanup_detached_labels_on_deleted_parent(self, mocker):
+        mock_labels = [mocker.MagicMock(), mocker.MagicMock()]
+        mock_instance = mocker.MagicMock()
+        mock_instance.labels.all = mocker.MagicMock()
+        mock_instance.labels.all.return_value = mock_labels
+        mock_labels[0].is_candidate_for_detach.return_value = True
+        mock_labels[1].is_candidate_for_detach.return_value = False
+
+        signals.cleanup_detached_labels_on_deleted_parent(None, mock_instance)
+       
+        mock_labels[0].is_candidate_for_detach.assert_called_with()
+        mock_labels[1].is_candidate_for_detach.assert_called_with()
+        mock_labels[0].delete.assert_called_with()
+        mock_labels[1].delete.assert_not_called()