Move annotation preparation/validation into h.api.schemas

With a view to turning `h.api.logic` into `h.api.storage` -- and as a
result the central point of indirection for all `h.api` storage
accesses, I've moved some of the annotation payload prep and validation
out of `h.api.logic` and alongside the other validation logic in
`h.api.schemas`.

h/api/logic.py

@@ -11,48 +11,18 @@
 log = logging.getLogger(__name__)
 
 
-def create_annotation(fields, userid):
+def create_annotation(fields):
     """Create and store an annotation."""
-    # Create Annotation instance
     annotation = Annotation(fields)
-    annotation['user'] = userid
-
-    # Save it in the database
     search_lib.prepare(annotation)
     annotation.save()
 
-    log.debug('Created annotation; user: %s', annotation['user'])
-
     return annotation
 
 
-def update_annotation(annotation, fields, userid):
-    """Update the given annotation with the given new fields.
-
-    :raises RuntimeError: if the fields attempt to change the annotation's
-        permissions and has_admin_permission is False, or if they are
-        attempting to move the annotation between groups.
-
-    """
-    # If the user is changing access permissions, check if it's allowed.
-    permissions = annotation.get('permissions', {})
-    changing_permissions = (
-        'permissions' in fields and
-        fields['permissions'] != permissions
-    )
-    if changing_permissions and userid not in permissions.get('admin', []):
-        raise RuntimeError(
-            _('Not authorized to change annotation permissions.'), 401)
-
-    if 'group' in fields and 'group' in annotation:
-        if fields['group'] != annotation.get('group'):
-            raise RuntimeError(
-                _("You can't move annotations between groups."), 401)
-
-    # Update the annotation with the new data
+def update_annotation(annotation, fields):
+    """Update the given annotation with the given new fields."""
     annotation.update(fields)
-
-    # Save the annotation in the database, overwriting the old version.
     search_lib.prepare(annotation)
     annotation.save()
 

h/api/schemas.py

@@ -4,7 +4,9 @@
 import copy
 import jsonschema
 from jsonschema.exceptions import best_match
+from pyramid import i18n
 
+_ = i18n.TranslationStringFactory(__package__)
 
 # These annotation fields are not to be set by the user.
 PROTECTED_FIELDS = ['created', 'updated', 'user', 'id']
@@ -78,13 +80,69 @@ class AnnotationSchema(JSONSchema):
         },
     }
 
+
+class CreateAnnotationSchema(object):
+
+    """
+    Validate the payload from a user when creating an annotation.
+    """
+
+    def __init__(self, request):
+        self.request = request
+        self.structure = AnnotationSchema()
+
     def validate(self, data):
-        appstruct = super(AnnotationSchema, self).validate(data)
+        appstruct = self.structure.validate(data)
 
-        # Some fields are not to be set by the user, ignore them
+        # Some fields are not to be set by the user, ignore them.
         for field in PROTECTED_FIELDS:
             appstruct.pop(field, None)
 
+        # Set the annotation user field to the request user.
+        appstruct['user'] = self.request.authenticated_userid
+
+        return appstruct
+
+
+class UpdateAnnotationSchema(object):
+
+    """
+    Validate the payload from a user when updating an annotation.
+    """
+
+    def __init__(self, request, annotation):
+        self.request = request
+        self.annotation = annotation
+        self.structure = AnnotationSchema()
+
+    def validate(self, data):
+        appstruct = self.structure.validate(data)
+
+        # Some fields are not to be set by the user, ignore them.
+        for field in PROTECTED_FIELDS:
+            appstruct.pop(field, None)
+
+        # The user may not change the permissions of an annotation on which
+        # they are lacking 'admin' rights.
+        userid = self.request.authenticated_userid
+        permissions = self.annotation.get('permissions', {})
+        changing_permissions = (
+            'permissions' in appstruct and
+            appstruct['permissions'] != permissions
+        )
+        if changing_permissions and userid not in permissions.get('admin', []):
+            raise ValidationError('permissions: ' +
+                                  _('You may not change the permissions on '
+                                    'an annotation unless you have the '
+                                    '"admin" permission on that annotation!'))
+
+        # Annotations may not be moved between groups.
+        if 'group' in appstruct and 'group' in self.annotation:
+            if appstruct['group'] != self.annotation['group']:
+                raise ValidationError('group: ' +
+                                      _('You may not move annotations between '
+                                        'groups!'))
+
         return appstruct
 
 

h/api/test/logic_test.py

@@ -25,141 +25,52 @@ def _mock_annotation(**kwargs):
 @create_annotation_fixtures
 def test_create_annotation_calls_Annotation(Annotation):
     fields = mock.MagicMock()
-    logic.create_annotation(fields, userid='acct:joe@example.org')
+    logic.create_annotation(fields)
 
     Annotation.assert_called_once_with(fields)
 
 
-@create_annotation_fixtures
-def test_create_annotation_sets_user(Annotation):
-    """It should set the annotation's 'user' field to the userid."""
-    userid = 'acct:sofia@example.net'
-    Annotation.return_value = _mock_annotation()
-
-    annotation = logic.create_annotation({}, userid)
-
-    assert annotation['user'] == userid
-
-
 @create_annotation_fixtures
 def test_create_annotation_calls_prepare(Annotation, search_lib):
     """It should call prepare() once with the annotation."""
-    logic.create_annotation({}, userid='acct:gustave@example.com')
+    logic.create_annotation({})
 
     search_lib.prepare.assert_called_once_with(Annotation.return_value)
 
 
 @create_annotation_fixtures
 def test_create_annotation_calls_save(Annotation):
     """It should call save() once."""
-    logic.create_annotation({}, userid='acct:francois@example.com')
+    logic.create_annotation({})
 
     Annotation.return_value.save.assert_called_once_with()
 
 
 @create_annotation_fixtures
 def test_create_annotation_returns_the_annotation(Annotation):
-    result = logic.create_annotation({}, userid='acct:satyarupa@example.org')
+    result = logic.create_annotation({})
     assert result == Annotation.return_value
 
 
-@create_annotation_fixtures
-def test_create_annotation_does_not_crash_if_annotation_has_no_group(
-        Annotation):
-    assert 'group' not in Annotation.return_value
-    fields = {}  # No group here either.
-
-    logic.create_annotation(fields, userid='acct:maya@example.net')
-
-
-@create_annotation_fixtures
-def test_create_annotation_does_not_crash_if_annotations_parent_has_no_group(
-        Annotation):
-    """It shouldn't crash if the parent annotation has no group.
-
-    It shouldn't crash if the annotation is a reply and its parent annotation
-    has no 'group' field.
-
-    """
-    # No group in the original annotation/reply itself.
-    Annotation.return_value = _mock_annotation()
-    assert 'group' not in Annotation.return_value
-    fields = {}  # No group here either.
-
-    # And no group in the parent annotation either.
-    Annotation.fetch.return_value = {}
-
-    logic.create_annotation(fields, userid='acct:filip@example.com')
-
-
 # The fixtures required to mock all of update_annotation()'s dependencies.
 update_annotation_fixtures = pytest.mark.usefixtures('search_lib')
 
 
-@update_annotation_fixtures
-def test_update_annotation_raises_if_non_admin_changes_perms():
-    with pytest.raises(RuntimeError):
-        logic.update_annotation(
-            _mock_annotation(permissions={}),
-            fields={'permissions': {'read': ['someone']}},
-            userid='alice')
-
-
-@update_annotation_fixtures
-def test_update_annotation_admins_can_change_permissions():
-    annotation = _mock_annotation(
-        permissions={'admin': ['alice']},
-        user='alice')
-
-    logic.update_annotation(
-        annotation,
-        fields={'permissions': 'changed'},
-        userid='alice')
-
-    assert annotation['permissions'] == 'changed'
-
-
-@update_annotation_fixtures
-def test_update_annotation_non_admins_can_make_non_permissions_changes():
-    annotation = _mock_annotation(
-        foo='bar',
-        permissions={'admin': ['alice']},
-        user='alice')
-
-    logic.update_annotation(
-        annotation,
-        fields={
-            'foo': 'changed',
-        },
-        userid='bob')
-
-    assert annotation['foo'] == 'changed'
-
-
 @update_annotation_fixtures
 def test_update_annotation_calls_update():
     annotation = _mock_annotation()
     fields = {'foo': 'bar'}
 
-    logic.update_annotation(annotation, fields, 'foo')
+    logic.update_annotation(annotation, fields)
 
     annotation.update.assert_called_once_with(fields)
 
 
-@update_annotation_fixtures
-def test_update_annotation_user_cannot_change_group():
-    annotation = _mock_annotation(group='old')
-    fields = {'group': 'new'}
-
-    with pytest.raises(RuntimeError):
-        logic.update_annotation(annotation, fields, 'foo')
-
-
 @update_annotation_fixtures
 def test_update_annotation_calls_prepare(search_lib):
     annotation = _mock_annotation()
 
-    logic.update_annotation(annotation, {}, 'foo')
+    logic.update_annotation(annotation, {})
 
     search_lib.prepare.assert_called_once_with(annotation)
 
@@ -168,52 +79,15 @@ def test_update_annotation_calls_prepare(search_lib):
 def test_update_annotation_calls_save():
     annotation = _mock_annotation()
 
-    logic.update_annotation(annotation, {}, 'foo')
+    logic.update_annotation(annotation, {})
 
     annotation.save.assert_called_once_with()
 
 
-@update_annotation_fixtures
-def test_update_annotation_does_not_crash_if_annotation_has_no_group():
-    annotation = _mock_annotation()
-    assert 'group' not in annotation
-
-    logic.update_annotation(annotation, {}, 'foo')
-
-
-@update_annotation_fixtures
-def test_update_annotation_does_not_crash_if_annotations_parent_has_no_group(
-        Annotation):
-    """It shouldn't crash if the parent annotation has no group.
-
-    It shouldn't crash if the annotation is a reply and its parent annotation
-    has no 'group' field.
-
-    """
-    # No group in the original annotation/reply itself.
-    annotation = _mock_annotation()
-    assert 'group' not in annotation
-
-    # And no group in the parent annotation either.
-    Annotation.fetch.return_value = {}
-
-    logic.update_annotation(annotation, {}, 'foo')
-
-
 # The fixtures required to mock all of delete_annotation()'s dependencies.
 delete_annotation_fixtures = pytest.mark.usefixtures()
 
 
-@delete_annotation_fixtures
-def test_delete_does_not_crash_if_annotation_has_no_group():
-    annotation = mock.MagicMock()
-    annotation_data = {}  # No 'group' key.
-    annotation.get.side_effect = annotation_data.get
-    annotation.__getitem__.side_effect = annotation_data.__getitem__
-
-    logic.delete_annotation(annotation)
-
-
 @delete_annotation_fixtures
 def test_delete_annotation_calls_delete():
     annotation = mock.MagicMock()