Addressing comments, updating tests, etc.

awx/api/permissions.py

@@ -198,8 +198,8 @@ class TaskPermission(ModelAccessPermission):
 
 class WorkflowApprovalPermission(ModelAccessPermission):
     '''
-    Permission check used by workflow approval and deny views
+    Permission check used by workflow `approval` and `deny` views to determine
-    to determine who can has access to approve and deny paused workflow nodes
+    who has access to approve and deny paused workflow nodes
     '''
 
     def check_post_permissions(self, request, view, obj=None):

awx/api/serializers.py

@@ -3473,15 +3473,6 @@ class WorkflowApprovalTemplateSerializer(UnifiedJobTemplateSerializer):
         return res
 
 
-class WorkflowJobTemplateApprovalSerializer(UnifiedJobTemplateSerializer):
-    class Meta:
-        model = WorkflowApprovalTemplate
-        fields = ('*',)
-
-    def post(self, obj):
-        return  # POST only!!!
-
-
 class LaunchConfigurationBaseSerializer(BaseSerializer):
     scm_branch = serializers.CharField(allow_blank=True, allow_null=True, required=False, default=None)
     job_type = serializers.ChoiceField(allow_blank=True, allow_null=True, required=False, default=None,

awx/api/views/__init__.py

@@ -4473,7 +4473,7 @@ class WorkflowApprovalApprove(RetrieveAPIView):
         if not request.user.can_access(models.WorkflowApproval, 'approve_or_deny', obj):
             raise PermissionDenied(detail=_("User does not have permission to approve or deny this workflow."))
         if obj.status != 'pending':
-            return Response("This workflow step has already been approved or denied.", status=status.HTTP_400_BAD_REQUEST)
+            return Response({"error": _("This workflow step has already been approved or denied.")}, status=status.HTTP_400_BAD_REQUEST)
         obj.approve(request)
         return Response(status=status.HTTP_204_NO_CONTENT)
 
@@ -4488,6 +4488,6 @@ class WorkflowApprovalDeny(RetrieveAPIView):
         if not request.user.can_access(models.WorkflowApproval, 'approve_or_deny', obj):
             raise PermissionDenied(detail=_("User does not have permission to approve or deny this workflow."))
         if obj.status != 'pending':
-            return Response("This workflow step has already been approved or denied.", status=status.HTTP_400_BAD_REQUEST)
+            return Response({"error": _("This workflow step has already been approved or denied.")}, status=status.HTTP_400_BAD_REQUEST)
         obj.deny(request)
         return Response(status=status.HTTP_204_NO_CONTENT)

awx/main/access.py

@@ -2809,8 +2809,6 @@ class WorkflowApprovalAccess(BaseAccess):
                 self.user, 'read_role'))
 
     def can_approve_or_deny(self, obj):
-        if obj.status != 'pending':
-            return False
         if self.user in obj.workflow_job_template.approval_role or self.user.is_superuser:
             return True
 

awx/main/scheduler/task_manager.py

@@ -527,10 +527,11 @@ class TaskManager():
             if task.timeout == 0:
                 continue
             if (now - task.created) >= approval_timeout_seconds:
-                logger.warn("The approval node {} ({}) has expired after {} seconds.".format(task.name, task.pk, task.timeout))
+                timeout_message = "The approval node {} ({}) has expired after {} seconds.".format(task.name, task.pk, task.timeout)
+                logger.warn(timeout_message)
                 task.timed_out = True
                 task.status = 'failed'
-                task.job_explanation = _("This approval node has timed out.")
+                task.job_explanation = _(timeout_message)
                 task.save(update_fields=['status', 'job_explanation', 'timed_out'])
 
     def calculate_capacity_consumed(self, tasks):

awx/main/tests/functional/api/test_workflow_node.py

@@ -152,7 +152,7 @@ class TestApprovalNodes():
         node = wfjt.workflow_nodes.create(unified_job_template=job_template)
         url = reverse('api:workflow_job_template_node_create_approval',
                       kwargs={'pk': node.pk, 'version': 'v2'})
-        post(url, {'name': 'Approve/Deny Test1', 'description': '', 'timeout': 0},
+        post(url, {'name': 'Approve Test', 'description': '', 'timeout': 0},
              user=admin_user, expect=200)
         post(reverse('api:workflow_job_template_launch', kwargs={'pk': wfjt.pk}),
              user=admin_user, expect=201)
@@ -161,7 +161,7 @@ class TestApprovalNodes():
         TaskManager().schedule()
         wfj_node = wf_job.workflow_nodes.first()
         approval = wfj_node.job
-        assert approval.name == 'Approve/Deny Test1'
+        assert approval.name == 'Approve Test'
         post(reverse('api:workflow_approval_approve', kwargs={'pk': approval.pk}),
              user=admin_user, expect=204)
         # Test that there is an activity stream entry that was created for the "approve" action.
@@ -171,7 +171,7 @@ class TestApprovalNodes():
         assert WorkflowApproval.objects.get(pk=approval.pk).status == 'successful'
         assert qs.operation == 'update'
         post(reverse('api:workflow_approval_approve', kwargs={'pk': approval.pk}),
-             user=admin_user, expect=403)
+             user=admin_user, expect=400)
 
     @pytest.mark.django_db
     def test_approval_node_deny(self, post, admin_user, job_template):
@@ -182,7 +182,7 @@ class TestApprovalNodes():
         node = wfjt.workflow_nodes.create(unified_job_template=job_template)
         url = reverse('api:workflow_job_template_node_create_approval',
                       kwargs={'pk': node.pk, 'version': 'v2'})
-        post(url, {'name': 'Approve/Deny Test2', 'description': '', 'timeout': 0},
+        post(url, {'name': 'Deny Test', 'description': '', 'timeout': 0},
              user=admin_user, expect=200)
         post(reverse('api:workflow_job_template_launch', kwargs={'pk': wfjt.pk}),
              user=admin_user, expect=201)
@@ -191,7 +191,7 @@ class TestApprovalNodes():
         TaskManager().schedule()
         wfj_node = wf_job.workflow_nodes.first()
         approval = wfj_node.job
-        assert approval.name == 'Approve/Deny Test2'
+        assert approval.name == 'Deny Test'
         post(reverse('api:workflow_approval_deny', kwargs={'pk': approval.pk}),
              user=admin_user, expect=204)
         # Test that there is an activity stream entry that was created for the "deny" action.
@@ -201,7 +201,7 @@ class TestApprovalNodes():
         assert WorkflowApproval.objects.get(pk=approval.pk).status == 'failed'
         assert qs.operation == 'update'
         post(reverse('api:workflow_approval_deny', kwargs={'pk': approval.pk}),
-             user=admin_user, expect=403)
+             user=admin_user, expect=400)
 
     def test_approval_node_cleanup(self, post, approval_node, admin_user, get):
         workflow_job_template = WorkflowJobTemplate.objects.create()