Refactor Patch Workflow. (#24)

Add validation method to PatchJSONParameters to perform common checks. Rewrite a little bit resources according to changes in PatchJSONParameters.
Add a few tests to check the changes I've made. Changed in some tests tuples to lists to make reading easier.
Also moved _process_patch_operation() to PatchJSONParameters and remove it from resources. Add another common logic to perform patch and @classmethod perform_patch().
Refactored resources to use new method.
Add dummy methods for different operations in PatchJSONParatemeters

app/modules/teams/resources.py

@@ -1,4 +1,5 @@
 # encoding: utf-8
+# pylint: disable=bad-continuation
 """
 RESTful API Team resources
 --------------------------
@@ -98,10 +99,7 @@ def patch(self, args, team):
                 db.session,
                 default_error_message="Failed to update team details."
             ):
-            for operation in args:
-                if not self._process_patch_operation(operation, team=team):
-                    log.info("Team patching has ignored unknown operation %s", operation)
-
+            parameters.PatchTeamDetailsParameters.perform_patch(args, obj=team)
             db.session.merge(team)
         return team
 
@@ -124,30 +122,6 @@ def delete(self, team):
             db.session.delete(team)
         return None
 
-    def _process_patch_operation(self, operation, team):
-        """
-        Args:
-            operation (dict) - one patch operation in RFC 6902 format.
-            team (Team) - team instance which is needed to be patched.
-            state (dict) - inter-operations state storage.
-
-        Returns:
-            processing_status (bool) - True if operation was handled, otherwise False.
-        """
-        if 'value' not in operation:
-            # TODO: handle errors better
-            abort(code=http_exceptions.UnprocessableEntity.code, message="value is required")
-
-        assert operation['path'][0] == '/', "Path must always begin with /"
-        field_name = operation['path'][1:]
-        field_value = operation['value']
-
-        if operation['op'] == parameters.PatchTeamDetailsParameters.OP_REPLACE:
-            setattr(team, field_name, field_value)
-            return True
-
-        return False
-
 
 @api.route('/<int:team_id>/members/')
 @api.response(

app/modules/users/parameters.py

@@ -1,9 +1,11 @@
 # encoding: utf-8
+# pylint: disable=too-many-ancestors,bad-continuation
 """
 Input arguments (Parameters) for User resources RESTful API
 -----------------------------------------------------------
 """
 
+from flask_login import current_user
 from flask_marshmallow import base_fields
 from marshmallow import validates_schema
 
@@ -72,3 +74,41 @@ class PatchUserDetailsParameters(PatchJSONParameters):
             User.is_admin.fget.__name__,
         )
     )
+
+    @classmethod
+    def test(cls, obj, field, value, state=None):
+        """
+        Additional check for 'current_password' as User hasn't field 'current_password'
+        """
+        if field == 'current_password':
+            if current_user.password != value and obj.password != value:
+                abort(code=http_exceptions.Forbidden.code, message="Wrong password")
+            else:
+                state['current_password'] = value
+                return True
+        return PatchJSONParameters.test(obj, field, value)
+
+    @classmethod
+    def replace(cls, obj, field, value, state=None):
+        """
+        Some fields require extra permissions to be changed.
+        Current user has to have at least a Supervisor role to change
+        'is_active' and 'is_readonly' property
+        And 'is_admin' requires Admin role
+        """
+        if field in {'is_active', 'is_readonly'}:
+            with permissions.SupervisorRolePermission(
+                    obj=obj,
+                    password_required=True,
+                    password=state['current_password']
+                ):
+                # Access granted
+                pass
+        elif field == 'is_admin':
+            with permissions.AdminRolePermission(
+                    password_required=True,
+                    password=state['current_password']
+                ):
+                # Access granted
+                pass
+        return PatchJSONParameters.replace(obj, field, value, state=state)

app/modules/users/resources.py

@@ -48,7 +48,6 @@ def post(self, args):
         """
         Create a new user.
         """
-
         with api.commit_or_abort(
                 db.session,
                 default_error_message="Failed to create a new user."
@@ -116,73 +115,10 @@ def patch(self, args, user):
                 db.session,
                 default_error_message="Failed to update user details."
             ):
-            for operation in args:
-                try:
-                    if not self._process_patch_operation(operation, user=user, state=state):
-                        log.info("User patching has ignored unknown operation %s", operation)
-                except ValueError as exception:
-                    abort(code=http_exceptions.Conflict.code, message=str(exception))
-
+            parameters.PatchUserDetailsParameters.perform_patch(args, user, state)
             db.session.merge(user)
         return user
 
-    def _process_patch_operation(self, operation, user, state):
-        """
-        Args:
-            operation (dict) - one patch operation in RFC 6902 format
-            user (User) - user instance which is needed to be patched
-            state (dict) - inter-operations state storage
-
-        Returns:
-            processing_status (bool) - True if operation was handled, otherwise False.
-        """
-        if 'value' not in operation:
-            # TODO: handle errors better
-            abort(code=http_exceptions.UnprocessableEntity.code, message="value is required")
-
-        if operation['op'] == parameters.PatchUserDetailsParameters.OP_TEST:
-            # User has to provide the admin/supervisor password (if the current
-            # user has an admin or a supervisor role) or the current password
-            # of the user that is edited.
-            if operation['path'] == '/current_password':
-                current_password = operation['value']
-
-                if current_user.password != current_password and user.password != current_password:
-                    abort(code=http_exceptions.Forbidden.code, message="Wrong password")
-
-                state['current_password'] = current_password
-                return True
-
-        elif operation['op'] == parameters.PatchUserDetailsParameters.OP_REPLACE:
-            assert operation['path'][0] == '/', "Path must always begin with /"
-            field_name = operation['path'][1:]
-            field_value = operation['value']
-
-            # Some fields require extra permissions to be changed.
-            # Current user has to have at least a Supervisor role to change
-            # 'is_active' and 'is_readonly' property
-            if field_name in {'is_active', 'is_readonly'}:
-                with permissions.SupervisorRolePermission(
-                    obj=user,
-                    password_required=True,
-                    password=state['current_password']
-                ):
-                    # Access granted
-                    pass
-
-            # Current user has to have an Admin role to change 'is_admin' property
-            elif field_name == 'is_admin':
-                with permissions.AdminRolePermission(
-                    password_required=True,
-                    password=state['current_password']
-                ):
-                    # Access granted
-                    pass
-
-            setattr(user, field_name, field_value)
-            return True
-        return False
-
 
 @api.route('/me')
 class UserMe(Resource):

flask_restplus_patched/parameters.py

@@ -1,9 +1,13 @@
 # encoding: utf-8
 # pylint: disable=missing-docstring
+import logging
+
 from six import itervalues
 
 from flask_marshmallow import Schema, base_fields
-from marshmallow import validate
+from marshmallow import validate, validates_schema, ValidationError
+
+log = logging.getLogger(__name__)  # pylint: disable=invalid-name
 
 
 class Parameters(Schema):
@@ -55,11 +59,14 @@ class PatchJSONParameters(Parameters):
         OP_REMOVE,
         OP_REPLACE,
     )
-    op = base_fields.String(required=True) # pylint: disable=invalid-name
+    op = base_fields.String(required=True)  # pylint: disable=invalid-name
 
     PATH_CHOICES = None
+
     path = base_fields.String(required=True)
 
+    NO_VALUE_OPERATIONS = (OP_REMOVE,)
+
     value = base_fields.Raw(required=False)
 
     def __init__(self, *args, **kwargs):
@@ -72,3 +79,125 @@ def __init__(self, *args, **kwargs):
             self.fields['op'].validators + [validate.OneOf(self.OPERATION_CHOICES)]
         self.fields['path'].validators = \
             self.fields['path'].validators + [validate.OneOf(self.PATH_CHOICES)]
+
+    @validates_schema
+    def validate_patch_structure(self, data):
+        """
+        Common validation of PATCH structure
+
+        Provide check that 'value' present in all operations expect it.
+
+        Provide check if 'path' is present. 'path' can be absent if provided
+        without '/' at the start. Supposed that if 'path' is present than it
+        is prepended with '/'.
+        Removing '/' in the beginning to simplify usage in resource.
+        """
+        if data['op'] not in self.NO_VALUE_OPERATIONS and 'value' not in data:
+            raise ValidationError('value is required')
+
+        if 'path' not in data:
+            raise ValidationError('Path is required and must always begin with /')
+        else:
+            data['field_name'] = data['path'][1:]
+
+    @classmethod
+    def perform_patch(cls, operations, obj, state=None):
+        """
+        Performs all necessary operations by calling class methods with corresponding names
+        """
+        for operation in operations:
+            if not cls._process_patch_operation(operation, obj=obj, state=state):
+                log.info("%s patching has stopped because of unknown operation %s", (obj.__name__, operation))
+                raise ValidationError("Failed to update %s details." % obj.__name__)
+        return True
+
+    @classmethod
+    def _process_patch_operation(cls, operation, obj, state=None):
+        """
+        Args:
+            operation (dict) - one patch operation in RFC 6902 format.
+            obj (Team) - team instance which is needed to be patched.
+            state (dict) - inter-operations state storage
+
+        Returns:
+            processing_status (bool) - True if operation was handled, otherwise False.
+        """
+        field_operaion = operation['op']
+
+        if field_operaion == cls.OP_REPLACE:
+            return cls.replace(obj, operation['field_name'], operation['value'], state=state)
+
+        elif field_operaion == cls.OP_TEST:
+            return cls.test(obj, operation['field_name'], operation['value'], state=state)
+
+        elif field_operaion == cls.OP_ADD:
+            return cls.add(obj, operation['field_name'], operation['value'], state=state)
+
+        elif field_operaion == cls.OP_MOVE:
+            return cls.move(obj, operation['field_name'], operation['value'], state=state)
+
+        elif field_operaion == cls.OP_COPY:
+            return cls.copy(obj, operation['field_name'], operation['value'], state=state)
+
+        elif field_operaion == cls.OP_REMOVE:
+            return cls.remove(obj, operation['field_name'], state=state)
+
+        return False
+
+    @classmethod
+    def replace(cls, obj, field, value, state=None):
+        """
+        This is method for replace operation. It is separated to have a possibility to easily
+        override it in your Parameters
+
+        Args:
+            obj: instance to change
+            field: (str) name
+            value: (str) value
+            state: (dict) inter-operations state storage
+
+        Returns:
+            processing_status: (bool) - True
+        """
+        setattr(obj, field, value)
+        return True
+
+    @classmethod
+    def test(cls, obj, field, value, state=None):
+        """
+        This is method for test operation. It is separated to have a possibility to easily
+        override it in your Parameters
+
+        Args:
+            obj: instance to change
+            field: (str) name
+            value: (str) value
+            state: (dict) inter-operations state storage
+
+        Returns:
+            processing_status: (bool) - True
+        """
+        if getattr(obj, field) == value:
+            return True
+        else:
+            return False
+
+    @classmethod
+    def add(cls, obj, field, value, state=None):
+        # pylint: disable=abstract-method
+        raise NotImplementedError()
+
+    @classmethod
+    def remove(cls, obj, field, state=None):
+        # pylint: disable=abstract-method
+        raise NotImplementedError()
+
+    @classmethod
+    def move(cls, obj, field, value, state=None):
+        # pylint: disable=abstract-method
+        raise NotImplementedError()
+
+    @classmethod
+    def copy(cls, obj, field, value, state=None):
+        # pylint: disable=abstract-method
+        raise NotImplementedError()