From 2f18a9f2c142728639c63f2dfc42568766529fb0 Mon Sep 17 00:00:00 2001 From: Chris Meyers Date: Mon, 25 Apr 2016 17:14:09 -0400 Subject: [PATCH 1/3] delete label on last disassociate --- awx/api/generics.py | 30 +++++++- awx/api/views.py | 2 +- awx/main/models/label.py | 10 +++ awx/main/tests/unit/api/test_generics.py | 96 +++++++++++++++++++++++- awx/main/tests/unit/models/test_label.py | 22 ++++++ 5 files changed, 154 insertions(+), 6 deletions(-) diff --git a/awx/api/generics.py b/awx/api/generics.py index cdd10e497d..08ec1dc356 100644 --- a/awx/api/generics.py +++ b/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,19 @@ 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) + + if Label.is_detached(sub_id): + Label.objects.get(id=sub_id).delete() + + return res + class SubDetailAPIView(generics.RetrieveAPIView, GenericAPIView, ParentMixin): pass diff --git a/awx/api/views.py b/awx/api/views.py index 3e70cb8099..fa4743887b 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -3359,7 +3359,7 @@ class NotificationDetail(RetrieveAPIView): serializer_class = NotificationSerializer new_in_300 = True -class LabelList(ListCreateAPIView): +class LabelList(ListCreateAPIView, DeleteLastUnattachLabelMixin): model = Label serializer_class = LabelSerializer diff --git a/awx/main/models/label.py b/awx/main/models/label.py index 8458e4cb23..376e5249bf 100644 --- a/awx/main/models/label.py +++ b/awx/main/models/label.py @@ -39,3 +39,13 @@ class Label(CommonModelNameNotUnique): jobtemplate_labels__isnull=True ) + @staticmethod + def is_detached(id): + return bool( + Label.objects.filter( + id=id, + unifiedjob_labels__isnull=True, + unifiedjobtemplate_labels__isnull=True + ).count()) + + diff --git a/awx/main/tests/unit/api/test_generics.py b/awx/main/tests/unit/api/test_generics.py index 42fe141c49..50ece17556 100644 --- a/awx/main/tests/unit/api/test_generics.py +++ b/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_sub_id) + 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() diff --git a/awx/main/tests/unit/models/test_label.py b/awx/main/tests/unit/models/test_label.py index c5c6e50b91..f96f5f54af 100644 --- a/awx/main/tests/unit/models/test_label.py +++ b/awx/main/tests/unit/models/test_label.py @@ -8,3 +8,25 @@ 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 + + ret = Label.is_detached(37) + + 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 + + ret = Label.is_detached(37) + + 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() From 1925742da13a33d16c57c500f06bb5d85a240ea8 Mon Sep 17 00:00:00 2001 From: Chris Meyers Date: Tue, 26 Apr 2016 17:48:23 -0400 Subject: [PATCH 2/3] delete orphaned labels when jt or j deleted --- awx/api/generics.py | 6 ++-- awx/main/models/label.py | 13 +++++++-- awx/main/signals.py | 7 ++++- awx/main/tests/unit/api/test_generics.py | 2 +- awx/main/tests/unit/models/test_label.py | 37 ++++++++++++++++++++++-- awx/main/tests/unit/test_signals.py | 17 +++++++++++ 6 files changed, 73 insertions(+), 9 deletions(-) create mode 100644 awx/main/tests/unit/test_signals.py diff --git a/awx/api/generics.py b/awx/api/generics.py index 08ec1dc356..326b6032d5 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -449,8 +449,10 @@ class DeleteLastUnattachLabelMixin(object): res = super(DeleteLastUnattachLabelMixin, self).unattach_by_id(request, sub_id) - if Label.is_detached(sub_id): - Label.objects.get(id=sub_id).delete() + label = Label.objects.get(id=sub_id) + + if label.is_detached(): + label.delete() return res diff --git a/awx/main/models/label.py b/awx/main/models/label.py index 376e5249bf..af9a2241b7 100644 --- a/awx/main/models/label.py +++ b/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,13 +40,19 @@ class Label(CommonModelNameNotUnique): jobtemplate_labels__isnull=True ) - @staticmethod - def is_detached(id): + def is_detached(self): return bool( Label.objects.filter( - id=id, + 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 diff --git a/awx/main/signals.py b/awx/main/signals.py index 068efb4ece..9b574fea65 100644 --- a/awx/main/signals.py +++ b/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 diff --git a/awx/main/tests/unit/api/test_generics.py b/awx/main/tests/unit/api/test_generics.py index 50ece17556..42d755abcf 100644 --- a/awx/main/tests/unit/api/test_generics.py +++ b/awx/main/tests/unit/api/test_generics.py @@ -140,7 +140,7 @@ class TestDeleteLastUnattachLabelMixin: 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_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() diff --git a/awx/main/tests/unit/models/test_label.py b/awx/main/tests/unit/models/test_label.py index f96f5f54af..20da73e9ad 100644 --- a/awx/main/tests/unit/models/test_label.py +++ b/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() @@ -14,7 +18,8 @@ def test_is_detached(mocker): Label.objects.filter = mocker.MagicMock(return_value=mock_query_set) mock_query_set.count.return_value = 1 - ret = Label.is_detached(37) + 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) @@ -25,8 +30,36 @@ def test_is_detached_not(mocker): Label.objects.filter = mocker.MagicMock(return_value=mock_query_set) mock_query_set.count.return_value = 0 - ret = Label.is_detached(37) + 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 + diff --git a/awx/main/tests/unit/test_signals.py b/awx/main/tests/unit/test_signals.py new file mode 100644 index 0000000000..c3830ee525 --- /dev/null +++ b/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() From e449237d8a90a101690eaa7e3e69804f631704b1 Mon Sep 17 00:00:00 2001 From: Chris Meyers Date: Wed, 27 Apr 2016 12:07:57 -0400 Subject: [PATCH 3/3] move delete last unattach mixin to job template sublabel --- awx/api/views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/awx/api/views.py b/awx/api/views.py index fa4743887b..141655b10b 100644 --- a/awx/api/views.py +++ b/awx/api/views.py @@ -2263,7 +2263,7 @@ class JobTemplateNotifiersSuccessList(SubListCreateAttachDetachAPIView): parent_model = JobTemplate relationship = 'notifiers_success' -class JobTemplateLabelList(SubListCreateAttachDetachAPIView): +class JobTemplateLabelList(SubListCreateAttachDetachAPIView, DeleteLastUnattachLabelMixin): model = Label serializer_class = LabelSerializer @@ -3359,7 +3359,7 @@ class NotificationDetail(RetrieveAPIView): serializer_class = NotificationSerializer new_in_300 = True -class LabelList(ListCreateAPIView, DeleteLastUnattachLabelMixin): +class LabelList(ListCreateAPIView): model = Label serializer_class = LabelSerializer