Replace `storage.update_annotation` with an `AnnotationWriteService` method #7967

jon-betts · 2023-05-03T18:48:38Z

For:

Move annotation CRUD actions into a service #7932

Requires:

Replace storage.create_annotation with an AnnotationWriteService #7966

Testing notes

Start h, the client, Via and ViaHTML

Update annotation

Visit: http://localhost:9083/http://example.com/
Login as devdata_user / pass
Make an annotation
Edit the annotation
Refresh the page
The content of the annotation should remain as you last edited it

Migrate annotation URL

Visit: http://localhost:5000/login
Login as devdata_admin / pass
Visit: http://localhost:5000/admin/documents
Enter:

{
    "http://example.com": {
        "url": "http://example.net",
        "document": {"title": "Example Domain"}
    }
}

Start h-periodic to enable re-indexing of the annotations
Visit: http://localhost:9083/http://example.com/
The annotations should have gone
Visit: http://localhost:9083/http://example.net/
The annotations should be there

jon-betts · 2023-05-24T18:02:19Z

h/services/annotation_write.py

+        data: dict,
+        update_timestamp: bool = True,
+        reindex_tag: str = "storage.update_annotation",
+        enforce_write_permission: bool = True,


I'd like to move the migration stuff to this service, at which point a lot of these args would become private.

The default of enforce_write_permission here is a change in behavior. Previously we never did this. Now we do it when updating on behalf of the user.

jon-betts · 2023-05-24T18:02:46Z

h/services/annotation_write.py

+            # the target URI must match one of a group's defined scopes
+            and not url_in_scope(
+                annotation.target_uri, [scope.scope for scope in group.scopes]
+            )


Small un-related refactor to point out these are two parts of the same condition.

jon-betts · 2023-05-24T18:08:20Z

tests/h/tasks/url_migration_test.py

+    def celery(self, patch, pyramid_request):
+        celery = patch("h.tasks.url_migration.celery")
+        celery.request = pyramid_request
+        return celery


I was having some untraceable coverage issues, and this seemed related, and a good thing to fix.

jon-betts · 2023-05-24T18:08:33Z

tests/h/views/api/bulk/_ndjson_test.py

@@ -34,6 +34,7 @@ def test_it_captures_initial_errors(self):
        def failing_method():
            raise ValueError("Oh no!")

+            # pragma: nocover
            yield 1  # pylint: disable=unreachable


Random coverage fix

marcospri

Noting of value to add here. Changes make sense and the code is in much better shape now.

Tried the testing instruction and discovered that URL migration interface in the process.

marcospri · 2023-06-01T12:54:32Z

h/storage.py

@@ -5,97 +5,15 @@
 for storing and retrieving annotations. Data passed to these functions is
 assumed to be validated.
 """
-# FIXME: This module was originally written to be a single point of
-#        indirection through which the storage backend could be swapped out on


originally written to be a single point of indirection

Oh well, that didn't happen

marcospri · 2023-06-01T12:56:04Z

h/services/url_migration.py

                # Don't update "edited" timestamp on annotation cards.
                update_timestamp=False,
                reindex_tag="URLMigrationService.move_annotations",
+                # This action is taken by the admin user most of the time, who


Most of the time?

marcospri · 2023-06-01T12:58:11Z

h/services/annotation_write.py

+        # pylint: disable=too-many-arguments
+        self,
+        annotation: Annotation,
+        data: dict,


Not in the scope of this PR but these APIs where we just have some "data" in with no shape or form are difficult to change.

Some changes work because everything is very abstract but other times is tricky to spot a problem because it will silently fail or just do nothing.

Anyway, just rambling.

Yeah, it's confusing as the data is a very specific shape, but what shape is that?

This matches the formulation in the tests and keeps this a unitary piece of logic, rather than split over two separate tests. This form has less chance of getting split up in a weird way in a refactor.

We can't enforce the group based on the current user when that user is an admin.

jon-betts added WIP technical debt labels May 3, 2023

jon-betts self-assigned this May 3, 2023

jon-betts force-pushed the anno-write-svc-create branch from 6dd5530 to 6c02aa7 Compare May 3, 2023 19:20

jon-betts force-pushed the anno-write-svc-update branch from bd0cf21 to f7b8ffd Compare May 3, 2023 19:40

jon-betts force-pushed the anno-write-svc-create branch from 6c02aa7 to 85e6b8d Compare May 4, 2023 12:58

jon-betts force-pushed the anno-write-svc-update branch from f7b8ffd to c2c782e Compare May 4, 2023 12:59

jon-betts force-pushed the anno-write-svc-create branch 2 times, most recently from f06bb09 to aa848a2 Compare May 4, 2023 16:52

Base automatically changed from anno-write-svc-create to main May 4, 2023 17:06

jon-betts force-pushed the anno-write-svc-update branch from c2c782e to 9b3abf2 Compare May 4, 2023 17:26

jon-betts force-pushed the anno-write-svc-update branch 3 times, most recently from ae2d4fd to 8d3b6d1 Compare May 24, 2023 17:23

jon-betts removed the WIP label May 24, 2023

jon-betts commented May 24, 2023

View reviewed changes

jon-betts requested a review from marcospri May 31, 2023 12:45

marcospri approved these changes Jun 1, 2023

View reviewed changes

jon-betts force-pushed the anno-write-svc-update branch from 8d3b6d1 to 53d904e Compare June 8, 2023 11:28

Jon Betts added 8 commits June 15, 2023 19:04

Add update_annotation method to AnnotationWriteService

f9e42db

Replace storage annotation update in the API views

6cdeab5

Replace update annotation in the URLMigrationService

f5509a0

Remove the storage update annotation method

bc37e5e

Fix the name of the URL migration service and add tests

f722293

Add test coverage for the URL migration tasks

49c3960

Use a simpler single if statement for group scope

7043db3

This matches the formulation in the tests and keeps this a unitary piece of logic, rather than split over two separate tests. This form has less chance of getting split up in a weird way in a refactor.

Only enforce group membership when updating annos on behalf of a user

bac2495

We can't enforce the group based on the current user when that user is an admin.

jon-betts force-pushed the anno-write-svc-update branch from 53d904e to bac2495 Compare June 15, 2023 18:04

jon-betts merged commit 13a37dc into main Jun 15, 2023

jon-betts deleted the anno-write-svc-update branch June 15, 2023 18:08

jon-betts mentioned this pull request Jun 19, 2023

Move annotation CRUD actions into a service #7932

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `storage.update_annotation` with an `AnnotationWriteService` method #7967

Replace `storage.update_annotation` with an `AnnotationWriteService` method #7967

jon-betts commented May 3, 2023 •

edited

Loading

jon-betts May 24, 2023

jon-betts May 24, 2023

jon-betts May 24, 2023

jon-betts May 24, 2023

jon-betts May 24, 2023

marcospri left a comment

marcospri Jun 1, 2023

marcospri Jun 1, 2023

marcospri Jun 1, 2023

jon-betts Jun 8, 2023

Replace storage.update_annotation with an AnnotationWriteService method #7967

Replace storage.update_annotation with an AnnotationWriteService method #7967

Conversation

jon-betts commented May 3, 2023 • edited Loading

Testing notes

Update annotation

Migrate annotation URL

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcospri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `storage.update_annotation` with an `AnnotationWriteService` method #7967

Replace `storage.update_annotation` with an `AnnotationWriteService` method #7967

jon-betts commented May 3, 2023 •

edited

Loading