Migrate consumers of update_annotation to the annotation service

h/services/url_migration.py

@@ -3,7 +3,6 @@
 
 from celery.utils import chunks
 
-import h.storage
 from h.schemas.annotation import transform_document
 from h.services import AnnotationService
 from h.tasks.url_migration import move_annotations
@@ -17,8 +16,8 @@ class URLMigrationService:
     BATCH_SIZE = 50
     """How many annotations to migrate at once."""
 
-    def __init__(self, request, annotation_service: AnnotationService):
-        self.request = request
+    def __init__(self, transaction_manager, annotation_service: AnnotationService):
+        self._transaction_manager = transaction_manager
         self._annotation_service = annotation_service
 
     def move_annotations(self, annotation_ids, current_uri, new_url_info):
@@ -69,9 +68,8 @@ def move_annotations(self, annotation_ids, current_uri, new_url_info):
             # Update the annotation's `target_uri` and associated document,
             # and create `Document*` entities for the new URL if they don't
             # already exist.
-            h.storage.update_annotation(
-                self.request,
-                annotation.id,
+            self._annotation_service.update_annotation(
+                annotation,
                 update_data,
                 # Don't update "edited" timestamp on annotation cards.
                 update_timestamp=False,
@@ -104,7 +102,7 @@ def move_annotations_by_url(self, url, new_url_info):
 
         # Ensure new document is visible in tasks that move remaining
         # annotations.
-        self.request.tm.commit()
+        self._transaction_manager.commit()
 
         # Schedule async tasks to move the remaining annotations in chunks
         for batch in chunks(iter(annotation_ids), n=self.BATCH_SIZE):
@@ -120,5 +118,6 @@ def move_annotations_by_url(self, url, new_url_info):
 
 def url_migration_factory(_context, request):
     return URLMigrationService(
-        request=request, annotation_service=request.find_service(AnnotationService)
+        transaction_manager=request.tm,
+        annotation_service=request.find_service(AnnotationService),
     )

h/views/api/annotations.py

@@ -27,6 +27,7 @@
 )
 from h.schemas.util import validate_query_params
 from h.security import Permission
+from h.services import AnnotationService
 from h.views.api.config import api_config
 from h.views.api.exceptions import PayloadError
 
@@ -135,7 +136,9 @@ def update(context, request):
     )
     appstruct = schema.validate(_json_payload(request))
 
-    annotation = storage.update_annotation(request, context.annotation.id, appstruct)
+    annotation = request.find_service(AnnotationService).update_annotation(
+        context.annotation, data=appstruct
+    )
 
     _publish_annotation_event(request, annotation, "update")
 

tests/h/services/url_migration_test.py

@@ -7,7 +7,7 @@
 
 class TestURLMigrationService:
     def test_move_annotations(
-        self, svc, annotation_service, factories, update_annotation, transform_document
+        self, svc, annotation_service, factories, transform_document
     ):
         annotation = factories.Annotation(
             target_selectors=[
@@ -34,9 +34,8 @@ def test_move_annotations(
             ids=sentinel.annotation_ids, target_uri=sentinel.current_uri
         )
         transform_document.assert_called_once_with(sentinel.document, sentinel.new_url)
-        update_annotation.assert_called_once_with(
-            svc.request,
-            annotation.id,
+        annotation_service.update_annotation.assert_called_once_with(
+            annotation,
             {
                 "target_uri": sentinel.new_url,
                 "document": transform_document.return_value,
@@ -70,7 +69,7 @@ def test_move_annotations_by_url(
         self,
         svc,
         factories,
-        pyramid_request,
+        transaction_manager,
         annotation_service,
         move_annotations,
         move_annotations_task,
@@ -88,7 +87,7 @@ def test_move_annotations_by_url(
         move_annotations.assert_called_once_with(
             [annotations[-1].id], sentinel.url, new_url_info
         )
-        pyramid_request.tm.commit.assert_called_once()
+        transaction_manager.commit.assert_called_once()
 
         move_annotations_task.delay.assert_has_calls(
             [
@@ -120,10 +119,6 @@ def test_move_annotations_by_url_with_no_annotations(
     def transform_document(self, patch):
         return patch("h.services.url_migration.transform_document")
 
-    @pytest.fixture(autouse=True)
-    def update_annotation(self, patch):
-        return patch("h.storage.update_annotation")
-
     @pytest.fixture
     def move_annotations_task(self, patch):
         return patch("h.services.url_migration.move_annotations")
@@ -134,12 +129,12 @@ def move_annotations(self, svc):
             yield move_annotations
 
     @pytest.fixture
-    def pyramid_request(self, pyramid_request):
-        pyramid_request.tm = Mock(spec_set=["commit"])
-        return pyramid_request
+    def transaction_manager(self):
+        return Mock(spec_set=["commit"])
 
     @pytest.fixture
-    def svc(self, pyramid_request, annotation_service):
+    def svc(self, annotation_service, transaction_manager):
         return URLMigrationService(
-            request=pyramid_request, annotation_service=annotation_service
+            transaction_manager=transaction_manager,
+            annotation_service=annotation_service,
         )

tests/h/views/api/annotations_test.py

@@ -8,6 +8,7 @@
 from h.services.annotation_delete import AnnotationDeleteService
 from h.traversal import AnnotationContext
 from h.views.api import annotations as views
+from h.views.api.exceptions import PayloadError
 
 
 @pytest.mark.usefixtures("annotation_json_service", "search_lib")
@@ -214,92 +215,62 @@ def AnnotationJSONLDPresenter(self, patch):
         return patch("h.views.api.annotations.AnnotationJSONLDPresenter")
 
 
-@pytest.mark.usefixtures(
-    "AnnotationEvent",
-    "links_service",
-    "annotation_json_service",
-    "update_schema",
-    "storage",
-)
 class TestUpdate:
     def test_it(
         self,
         annotation_context,
         pyramid_request,
-        update_schema,
-        storage,
+        UpdateAnnotationSchema,
+        annotation_service,
         annotation_json_service,
+        AnnotationEvent,
     ):
         returned = views.update(annotation_context, pyramid_request)
 
-        update_schema.assert_called_once_with(
+        # Check it validates the annotation
+        UpdateAnnotationSchema.assert_called_once_with(
             pyramid_request,
             annotation_context.annotation.target_uri,
             annotation_context.annotation.groupid,
         )
-        update_schema.return_value.validate.assert_called_once_with(
-            pyramid_request.json_body
-        )
-
-        storage.update_annotation.assert_called_once_with(
-            pyramid_request,
-            annotation_context.annotation.id,
-            update_schema.return_value.validate.return_value,
-        )
-
-        annotation_json_service.present_for_user.assert_called_once_with(
-            annotation=storage.update_annotation.return_value, user=pyramid_request.user
+        schema = UpdateAnnotationSchema.return_value
+        # Check it updates the annotation
+        schema.validate.assert_called_once_with(pyramid_request.json_body)
+        annotation_service.update_annotation.assert_called_once_with(
+            annotation_context.annotation,
+            schema.validate.return_value,
         )
-
-        assert returned == annotation_json_service.present_for_user.return_value
-
-    def test_it_publishes_annotation_event(
-        self, annotation_context, AnnotationEvent, storage, pyramid_request
-    ):
-        views.update(annotation_context, pyramid_request)
-
+        # Check it publishes event
         AnnotationEvent.assert_called_once_with(
-            pyramid_request, storage.update_annotation.return_value.id, "update"
+            pyramid_request,
+            annotation_service.update_annotation.return_value.id,
+            "update",
         )
         pyramid_request.notify_after_commit.assert_called_once_with(
             AnnotationEvent.return_value
         )
+        # Check it presents the annotation
+        annotation_json_service.present_for_user.assert_called_once_with(
+            annotation=annotation_service.update_annotation.return_value,
+            user=pyramid_request.user,
+        )
+        assert returned == annotation_json_service.present_for_user.return_value
 
-    def test_it_raises_if_storage_raises(
-        self, annotation_context, pyramid_request, storage
-    ):
-        storage.update_annotation.side_effect = ValidationError("asplode")
-
-        with pytest.raises(ValidationError):
-            views.update(annotation_context, pyramid_request)
-
-    def test_it_raises_if_validate_raises(
-        self, annotation_context, pyramid_request, update_schema
+    @pytest.mark.usefixtures("with_invalid_json_body")
+    def test_it_raises_with_invalid_json_body(
+        self, annotation_context, pyramid_request
     ):
-        update_schema.return_value.validate.side_effect = ValidationError("asplode")
-
-        with pytest.raises(ValidationError):
+        with pytest.raises(PayloadError):
             views.update(annotation_context, pyramid_request)
 
-    def test_it_raises_if_json_parsing_fails(self, annotation_context, pyramid_request):
-        """It raises PayloadError if parsing of the request body fails."""
-        # Make accessing the request.json_body property raise ValueError.
-        type(pyramid_request).json_body = {}
-        with mock.patch.object(
-            type(pyramid_request), "json_body", new_callable=mock.PropertyMock
-        ) as json_body:
-            json_body.side_effect = ValueError()
-            with pytest.raises(views.PayloadError):
-                views.update(annotation_context, pyramid_request)
-
     @pytest.fixture
     def pyramid_request(self, pyramid_request):
         pyramid_request.json_body = {}
         pyramid_request.notify_after_commit = mock.Mock()
         return pyramid_request
 
     @pytest.fixture
-    def update_schema(self, patch):
+    def UpdateAnnotationSchema(self, patch):
         return patch("h.views.api.annotations.UpdateAnnotationSchema")
 
 
@@ -345,6 +316,16 @@ def pyramid_request(pyramid_request):
     return pyramid_request
 
 
+@pytest.fixture
+def with_invalid_json_body(pyramid_request):
+    type(pyramid_request).json_body = None
+    with mock.patch.object(
+        type(pyramid_request), "json_body", new_callable=mock.PropertyMock
+    ) as json_body:
+        json_body.side_effect = ValueError()
+        yield json_body
+
+
 @pytest.fixture
 def storage(patch):
     return patch("h.views.api.annotations.storage")