refactored and covered with tests patch action controller

misago/core/apipatchrouter.py → misago/core/apipatch.py

@@ -12,7 +12,7 @@ class InvalidAction(Exception):
     pass
 
 
-class ApiPatchRouter(object):
+class ApiPatch(object):
     def __init__(self):
         self._actions = []
 
@@ -38,47 +38,55 @@ class ApiPatchRouter(object):
         })
 
     def dispatch(self, request, target):
-        try:
-            return Response(self.run_actions(request, target))
-        except Http404:
-            pass
-        except PermissionDenied as e:
-            pass
-        except InvalidAction as e:
-            pass
-        return Response({})
-
-    def run_actions(self, request, target):
         if not isinstance(request.data, list):
-            raise InvalidAction("PATCH request should be list of operations")
+            return Response({
+                'detail': "PATCH request should be list of operations"
+            }, status=400)
 
+        detail = []
+        is_errored = False
+
+        patch = {'id': target.pk}
         for action in request.data:
-            self.validate_action(action)
-            return self.dispatch_action(request, target, action)
+            try:
+                self.validate_action(action)
+                self.dispatch_action(patch, request, target, action)
+                detail.append('ok')
+            except Http404:
+                is_errored = True
+                detail.append('NOT FOUND')
+                break
+            except (InvalidAction, PermissionDenied) as e:
+                is_errored = True
+                detail.append(e.args[0])
+                break
+
+        patch['detail'] = detail
+        if is_errored:
+            return Response(patch, status=400)
+        else:
+            return Response(patch)
 
     def validate_action(self, action):
+        if not action.get('op'):
+            raise InvalidAction(u"undefined op")
+
         if action.get('op') not in ALLOWED_OPS:
-            if action.get('op'):
-                raise InvalidAction(
-                    u"\"%s\" op is unsupported" % action.get('op'))
-            else:
-                raise InvalidAction(u"server didn't receive valid op")
+            raise InvalidAction(
+                u'"%s" op is unsupported' % action.get('op'))
 
         if not action.get('path'):
             raise InvalidAction(
-                u"\"%s\" op has to define path" % action.get('op'))
+                u'"%s" op has to specify path' % action.get('op'))
 
         if 'value' not in action:
             raise InvalidAction(
-                u"\"%s\" op has to define value" % action.get('op'))
+                u'"%s" op has to specify value' % action.get('op'))
 
-    def dispatch_action(self, request, target, action):
-        patch = {'id': target.pk}
+    def dispatch_action(self, patch, request, target, action):
         for handler in self._actions:
             if (action['op'] == handler['op'] and
                     action['path'] == handler['path']):
                 with transaction.atomic():
                     patch.update(
-                        handler['handler'](request, target, action['value'])
-                    )
-        return patch
+                        handler['handler'](request, target, action['value']))

misago/core/tests/test_apipatch.py

@@ -0,0 +1,241 @@
+from django.core.exceptions import PermissionDenied
+from django.http import Http404
+from django.test import TestCase
+
+from misago.core.apipatch import ApiPatch, InvalidAction
+
+
+class MockRequest(object):
+    def __init__(self, data=None):
+        self.data = data
+
+
+class MockObject(object):
+    def __init__(self, pk):
+        self.id = pk
+        self.pk = pk
+
+
+class ApiPatchTests(TestCase):
+    def test_add(self):
+        """add method adds function to patch object"""
+        patch = ApiPatch()
+
+        def test_function():
+            pass
+        patch.add('test-add', test_function)
+
+        self.assertEqual(len(patch._actions), 1)
+        self.assertEqual(patch._actions[0]['op'], 'add')
+        self.assertEqual(patch._actions[0]['path'], 'test-add')
+        self.assertEqual(patch._actions[0]['handler'], test_function)
+
+    def test_remove(self):
+        """remove method adds function to patch object"""
+        patch = ApiPatch()
+
+        def test_function():
+            pass
+        patch.remove('test-remove', test_function)
+
+        self.assertEqual(len(patch._actions), 1)
+        self.assertEqual(patch._actions[0]['op'], 'remove')
+        self.assertEqual(patch._actions[0]['path'], 'test-remove')
+        self.assertEqual(patch._actions[0]['handler'], test_function)
+
+    def test_replace(self):
+        """replace method adds function to patch object"""
+        patch = ApiPatch()
+
+        def test_function():
+            pass
+        patch.replace('test-replace', test_function)
+
+        self.assertEqual(len(patch._actions), 1)
+        self.assertEqual(patch._actions[0]['op'], 'replace')
+        self.assertEqual(patch._actions[0]['path'], 'test-replace')
+        self.assertEqual(patch._actions[0]['handler'], test_function)
+
+    def test_validate_action(self):
+        """validate_action method validates action dict"""
+        patch = ApiPatch()
+
+        VALID_ACTIONS = (
+            {'op': 'add', 'path': 'test', 'value': 42},
+            {'op': 'remove', 'path': 'other-test', 'value': 'Lorem'},
+            {'op': 'replace', 'path': 'false-test', 'value': None},
+        )
+
+        for action in VALID_ACTIONS:
+            patch.validate_action(action)
+
+        # undefined op
+        UNSUPPORTED_ACTIONS = (
+            {},
+            {'op': ''},
+            {'no': 'op'},
+        )
+
+        for action in UNSUPPORTED_ACTIONS:
+            try:
+                patch.validate_action(action)
+            except InvalidAction as e:
+                self.assertEqual(e.args[0], u"undefined op")
+
+        # unsupported op
+        try:
+            patch.validate_action({'op': 'nope'})
+        except InvalidAction as e:
+            self.assertEqual(e.args[0], u'"nope" op is unsupported')
+
+        # op lacking patch
+        try:
+            patch.validate_action({'op': 'add'})
+        except InvalidAction as e:
+            self.assertEqual(e.args[0], u'"add" op has to specify path')
+
+        # op lacking value
+        try:
+            patch.validate_action({'op': 'add', 'path': 'yolo'})
+        except InvalidAction as e:
+            self.assertEqual(e.args[0], u'"add" op has to specify value')
+
+        # empty value is allowed
+        try:
+            patch.validate_action({'op': 'add', 'path': 'yolo', 'value': ''})
+        except InvalidAction as e:
+            self.assertEqual(e.args[0], u'"add" op has to specify value')
+
+    def test_dispatch_action(self):
+        """dispatch_action calls specified actions"""
+        patch = ApiPatch()
+
+        mock_target = MockObject(13)
+
+        def action_a(request, target, value):
+            self.assertEqual(request, 'request')
+            self.assertEqual(target, mock_target)
+            return {'a': value * 2, 'b': 111}
+        patch.replace('abc', action_a)
+
+        def action_b(request, target, value):
+            self.assertEqual(request, 'request')
+            self.assertEqual(target, mock_target)
+            return {'b': value * 10}
+        patch.replace('abc', action_b)
+
+        def action_fail(request, target, value):
+            self.fail("unrequired action was called")
+
+        patch.add('c', action_fail)
+        patch.remove('c', action_fail)
+        patch.replace('c', action_fail)
+
+        patch_dict = {
+            'id': 123
+        }
+
+        patch.dispatch_action(patch_dict, 'request', mock_target, {
+            'op': 'replace',
+            'path': 'abc',
+            'value': 5,
+        })
+
+        self.assertEqual(len(patch_dict), 3)
+        self.assertEqual(patch_dict['id'], 123)
+        self.assertEqual(patch_dict['a'], 10)
+        self.assertEqual(patch_dict['b'], 50)
+
+    def test_dispatch(self):
+        """dispatch calls actions and returns response"""
+        patch = ApiPatch()
+
+        def action_error(request, target, value):
+            if value == '404':
+                raise Http404()
+            if value == 'perm':
+                raise PermissionDenied("yo ain't doing that!")
+            return {}
+        patch.replace('error', action_error)
+
+        def action_mutate(request, target, value):
+            return {'value': value * 2}
+        patch.replace('mutate', action_mutate)
+
+        # dispatch requires list as an argument
+        response = patch.dispatch(MockRequest({}), {})
+        self.assertEqual(response.status_code, 400)
+
+        self.assertEqual(
+            response.data['detail'],
+            "PATCH request should be list of operations")
+
+        # valid dispatch
+        response = patch.dispatch(MockRequest([
+            {'op': 'replace', 'path': 'mutate', 'value': 2},
+            {'op': 'replace', 'path': 'mutate', 'value': 6},
+            {'op': 'replace', 'path': 'mutate', 'value': 7},
+        ]), MockObject(13))
+
+        self.assertEqual(response.status_code, 200)
+
+        self.assertEqual(len(response.data['detail']), 3)
+        self.assertEqual(response.data['detail'][0], 'ok')
+        self.assertEqual(response.data['detail'][1], 'ok')
+        self.assertEqual(response.data['detail'][2], 'ok')
+        self.assertEqual(response.data['id'], 13)
+        self.assertEqual(response.data['value'], 14)
+
+        # invalid action in dispatch
+        response = patch.dispatch(MockRequest([
+            {'op': 'replace', 'path': 'mutate', 'value': 2},
+            {'op': 'replace', 'path': 'mutate', 'value': 6},
+            {'op': 'replace'},
+            {'op': 'replace', 'path': 'mutate', 'value': 7},
+        ]), MockObject(13))
+
+        self.assertEqual(response.status_code, 400)
+
+        self.assertEqual(len(response.data['detail']), 3)
+        self.assertEqual(response.data['detail'][0], 'ok')
+        self.assertEqual(response.data['detail'][1], 'ok')
+        self.assertEqual(
+            response.data['detail'][2], '"replace" op has to specify path')
+        self.assertEqual(response.data['id'], 13)
+        self.assertEqual(response.data['value'], 12)
+
+        # action in dispatch raised 404
+        response = patch.dispatch(MockRequest([
+            {'op': 'replace', 'path': 'mutate', 'value': 2},
+            {'op': 'replace', 'path': 'error', 'value': '404'},
+            {'op': 'replace', 'path': 'mutate', 'value': 6},
+            {'op': 'replace', 'path': 'mutate', 'value': 7},
+        ]), MockObject(13))
+
+        self.assertEqual(response.status_code, 400)
+
+        self.assertEqual(len(response.data['detail']), 2)
+        self.assertEqual(response.data['detail'][0], 'ok')
+        self.assertEqual(
+            response.data['detail'][1], "NOT FOUND")
+        self.assertEqual(response.data['id'], 13)
+        self.assertEqual(response.data['value'], 4)
+
+        # action in dispatch raised perm denied
+        response = patch.dispatch(MockRequest([
+            {'op': 'replace', 'path': 'mutate', 'value': 2},
+            {'op': 'replace', 'path': 'mutate', 'value': 6},
+            {'op': 'replace', 'path': 'mutate', 'value': 9},
+            {'op': 'replace', 'path': 'error', 'value': 'perm'},
+        ]), MockObject(13))
+
+        self.assertEqual(response.status_code, 400)
+
+        self.assertEqual(len(response.data['detail']), 4)
+        self.assertEqual(response.data['detail'][0], 'ok')
+        self.assertEqual(response.data['detail'][1], 'ok')
+        self.assertEqual(response.data['detail'][2], 'ok')
+        self.assertEqual(
+            response.data['detail'][3], "yo ain't doing that!")
+        self.assertEqual(response.data['id'], 13)
+        self.assertEqual(response.data['value'], 18)

misago/threads/api/threadendpoints/patch.py

@@ -1,7 +1,7 @@
-from misago.core.apipatchrouter import ApiPatchRouter
+from misago.core.apipatch import ApiPatch
 from misago.threads.moderation import threads as moderation
 
-thread_patch_endpoint = ApiPatchRouter()
+thread_patch_endpoint = ApiPatch()
 
 
 def patch_weight(request, thread, value):