Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace storage.fetch_annotation with an AnnotationReadService #7947

Merged
merged 5 commits into from
May 4, 2023

Conversation

jon-betts
Copy link
Contributor

@jon-betts jon-betts commented Apr 20, 2023

For:

Testing notes

In H / Via / ViaHTML / Client:

make services
make dev

In one tab:

In another private tab:

Websocket

  • Create an annotation in one tab
  • Switch to the other and see the websocket update icon
  • Click on it
  • The annotation should appear

Notification reply

  • Remove /mail/* in your h directory
  • In the other tab reply to the annotation
  • Look in the mail/ dir, it should have an *.eml file
  • There should be some details related to your reply

Nearly anything which has a route on the annotation traversal

There are a bunch in routes which use the traversal, but flag is probably the easiest

  • In the admin tab flag an annotation
  • Reload the page
  • It should still be flagged

Copy link
Member

@marcospri marcospri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, going in the right direction with the service approach.

@@ -62,7 +62,7 @@ def get_notification(
# Now we know we're dealing with a reply
reply = annotation

parent = storage.fetch_annotation(request.db, parent_id)
parent = request.find_service(AnnotationReadService).get_annotation_by_id(parent_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh why not just annotation.parent? because the whole references thing makes that not trivial 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think there's any good reason really. I've swapped this over.

@jon-betts jon-betts merged commit 0aabf26 into main May 4, 2023
@jon-betts jon-betts deleted the anno-read-svc-fetch branch May 4, 2023 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants