diff --git a/h/services/url_migration.py b/h/services/url_migration.py index bcdca5dd109..adb6ea87bce 100644 --- a/h/services/url_migration.py +++ b/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), ) diff --git a/h/views/api/annotations.py b/h/views/api/annotations.py index 40bf730f359..1b2ad3d45f8 100644 --- a/h/views/api/annotations.py +++ b/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") diff --git a/tests/h/services/url_migration_test.py b/tests/h/services/url_migration_test.py index e9f02aacb37..b4cd51d7789 100644 --- a/tests/h/services/url_migration_test.py +++ b/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, ) diff --git a/tests/h/views/api/annotations_test.py b/tests/h/views/api/annotations_test.py index 4a7e50ab9de..75d391601b0 100644 --- a/tests/h/views/api/annotations_test.py +++ b/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,84 +215,54 @@ 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 = {} @@ -299,7 +270,7 @@ def pyramid_request(self, pyramid_request): 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")