Mentions PoC #9279

mtomilov · 2025-01-27T09:05:01Z

Adds @mentions support in the API, emails will be addressed separately.
All of create / get / edit / search endpoints now support them.

Testing API

Run migrations make db
Login as devdata_admin and generate API token from http://localhost:5000/account/developer

Create annotation

 curl -X POST --location "http://localhost:5000/api/annotations" \
 -H "Authorization: Bearer <token>" \
 -H "Content-Type: application/json" \
 -d '{
         "uri": "https://www.google.com/",
         "text": "**hello** <span>world</span>"
     }'

Mentions can be already put in the text here and will be returned in the response.

Edit annotation

 curl -X PATCH --location "http://localhost:5000/api/annotations/<url safe id>" \
     -H "Authorization: Bearer <token>" \
     -H "Content-Type: application/json" \
     -d '{
             "text": "Hello <a data-hyp-mention data-userid=\"acct:devdata_admin@localhost\">@devdata_admin</a>, take a look at this\nHello <a data-hyp-mention data-userid=\"acct:devdata_user@localhost\">@devdata_user</a>, take a look at this"
         }'

But I think it's easier to test via edit on already created annotation.

Search annotations
curl -X GET --location "http://localhost:5000/api/search" \ -H "Authorization: Bearer <token>"
Mentions will be returned here as well

Testing emails

Notifications are sent only on creation for now

Create annotation

 curl -X POST --location "http://localhost:5000/api/annotations" \
 -H "Authorization: Bearer <token>" \
 -H "Content-Type: application/json" \
 -d '{
         "uri": "https://www.google.com/",
         "text": "Hello <a data-hyp-mention data-userid=\"acct:devdata_admin@localhost\">@devdata_admin</a>, take a look at this\nHello <a data-hyp-mention data-userid=\"acct:devdata_user@localhost\">@devdata_user</a>, take a look at this"
     }'

Check h/mail folder for incoming mail

mtomilov · 2025-01-27T11:49:53Z

h/services/annotation_read.py

@@ -54,7 +54,12 @@ def _annotation_search_query(
        query = query.where(Annotation.id.in_(ids))

        if eager_load:
-            query = query.options(*(subqueryload(prop) for prop in eager_load))
+            for prop in eager_load:
+                if isinstance(prop, tuple) and len(prop) == 2:


Quick and dirty query optimization to load all mentions at once

mtomilov · 2025-01-27T11:50:42Z

h/services/mention.py

+        userids = set(self._parse_userids(annotation.text))
+        users = self._user_service.fetch_all(userids)
+        self._session.execute(
+            sa.delete(Mention).where(Mention.annotation_id == annotation.slim.id)


Maybe I should've put mentions on Annotation model directly instead of its slim sibling

Yeah, I think they should be linked with the regular annotation.

AnnotationSlim has better FKs to user, group etc.

I think it should be the default go to for new functionality.

That being said if we are using AnnotationSlim at the model we should also use it a the service level, ie, this method should take one annotation_slim: AnnotationSlim.

We have used AnnotasionSlim for features that mostly make expensive queries, it might be a bit awkward to use it something like this where you have to keep track of both tables. IMO that's not that bad if most of the code around mentions uses AnnotationSlim and the callers are the ones that pass the right type of model.

As more features that use AnnotationSlim it should be easier to make it the default but I don't think is a hard requirement for mentions to use it, if actually using it becomes a problem there's no issue of going back to Annotation.

Let me rethink that

Switched to Annotation to try it out

mtomilov · 2025-01-27T12:17:55Z

h/services/mention.py

+
+    @staticmethod
+    def _parse_userids(text: str) -> list[str]:
+        return USERID_PAT.findall(text)


We'll probably want to parse userids from text_rendered html.

acelaya · 2025-01-28T08:08:21Z

h/schemas/annotation.py

+            "mentions": {
+                "type": "array",
+                "items": copy.deepcopy(MENTION_SCHEMA),
+            },


Is this used to validate annotations on creation? If so, I was not expecting the mentions list to be sent on creation, only to be returned on fetch.

Yeah this seems to be for validation.
@marcospri Could you please point me to where we describe response schema?

I don't know the answer. We have

https://github.com/hypothesis/h/blob/mentions-poc/docs/_extra/api-reference/schemas/annotation.yaml#L1

which seems to be part of the docs.

Thank you, I've removed it from here and added to the yaml.

acelaya · 2025-01-28T08:13:58Z

h/services/mention.py

+from h.models import Annotation, Mention
+from h.services.user import UserService
+
+USERID_PAT = re.compile(r"data-userid=\"([^\"]+)\"")


Perhaps a bit too loose, but could work for prototyping

In future, I think someone suggested the idea of extracting the ids from the annotation rendered HTML, via DOM selector (html.find('a[data-hyp-mention]).dataset.userid)

Yes, I guess we use a real parser for the real production code.

Oh, I just noticed you commented precisely this here #9279 (comment)

acelaya · 2025-01-28T08:19:26Z

h/services/mention.py

+        userids = set(self._parse_userids(annotation.text))
+        users = self._user_service.fetch_all(userids)
+        self._session.execute(
+            sa.delete(Mention).where(Mention.annotation_id == annotation.slim.id)


Yeah, I think they should be linked with the regular annotation.

marcospri · 2025-01-28T09:07:40Z

h/migrations/versions/bfc54f0844aa_add_username_to_mention.py

+
+
+def upgrade() -> None:
+    op.add_column("mention", sa.Column("username", sa.UnicodeText(), nullable=False))


Not sure I understand what this is for. Is it related to supporting changing usernames?

Are we going to support that on v1?

I wonder if we should store the history of usernames at the user level instead of at the mentions one, ie a table/column with the history of a user's usernames.

Yeah it doesn't have to be in v1, removed for now.

As for full support sure, I was thinking something along the lines of UserDelete table but for renames in addition to storing usernames here. Then we'd be able to handle all the edge cases.

marcospri · 2025-01-28T09:08:18Z

h/models/mention.py

@@ -28,5 +28,8 @@ class Mention(Base, Timestamps):  # pragma: nocover
    """FK to user.id"""
    user = sa.orm.relationship("User")

+    username = sa.Column("username", sa.UnicodeText(), nullable=False)
+    """Current username of the user mentioned"""


Is it "current" or the one that appears on the annotation text?

Yep, might not have been the best name.

marcospri · 2025-01-28T09:21:10Z

h/services/mention.py

+        userids = set(self._parse_userids(annotation.text))
+        users = self._user_service.fetch_all(userids)
+        self._session.execute(
+            sa.delete(Mention).where(Mention.annotation_id == annotation.slim.id)


AnnotationSlim has better FKs to user, group etc.

I think it should be the default go to for new functionality.

That being said if we are using AnnotationSlim at the model we should also use it a the service level, ie, this method should take one annotation_slim: AnnotationSlim.

We have used AnnotasionSlim for features that mostly make expensive queries, it might be a bit awkward to use it something like this where you have to keep track of both tables. IMO that's not that bad if most of the code around mentions uses AnnotationSlim and the callers are the ones that pass the right type of model.

As more features that use AnnotationSlim it should be easier to make it the default but I don't think is a hard requirement for mentions to use it, if actually using it becomes a problem there's no issue of going back to Annotation.

marcospri · 2025-01-28T09:23:08Z

h/services/mention.py

+import re
+from typing import Iterable, Sequence
+
+import sqlalchemy as sa


I wouldn't be shy about importing anything from sqlalchemy. I mean speling the whole from sqlalchemy import select, ....

No problem, is this the recommended way?

marcospri · 2025-01-28T09:23:37Z

h/services/mention.py

+from h.models import Annotation, Mention
+from h.services.user import UserService
+
+USERID_PAT = re.compile(r"data-userid=\"([^\"]+)\"")


Yes, I guess we use a real parser for the real production code.

marcospri · 2025-01-28T09:38:38Z

h/services/annotation_json.py

@@ -151,6 +160,8 @@ def present_all_for_user(self, annotation_ids, user: User):
                # which ultimately depends on group permissions, causing a
                # group lookup for every annotation without this
                Annotation.group,
+                # Optimise access to the mentions


I guess this is the main pain point for using AnnotationSlim vs Annotation

Right, feels a bit awkward as is

marcospri · 2025-01-28T10:01:35Z

h/schemas/annotation.py

+            "mentions": {
+                "type": "array",
+                "items": copy.deepcopy(MENTION_SCHEMA),
+            },


I don't know the answer. We have

https://github.com/hypothesis/h/blob/mentions-poc/docs/_extra/api-reference/schemas/annotation.yaml#L1

which seems to be part of the docs.

marcospri · 2025-01-28T10:03:55Z

h/services/annotation_json.py

@@ -71,6 +76,10 @@ def present(self, annotation: Annotation):
                "target": annotation.target,
                "document": DocumentJSONPresenter(annotation.document).asdict(),
                "links": self._links_service.get_all(annotation),
+                "mentions": [


Do we want mentions to be in all responses or only on the ones that do have mentions?

Removed from here

mtomilov · 2025-01-29T12:25:40Z

h/migrations/versions/022ea4bf4a27_update_mention_to_reference_annotation.py

+
+
+def upgrade() -> None:
+    op.drop_column("mention", "annotation_id")


Mentions aren't being used yet so nothing actually destructive here

mtomilov · 2025-01-29T15:15:42Z

h/emails/mention_notification.py

+    return [notification.mentioned_user.email], subject, text, html
+
+
+def _get_user_url(user, request):


Copied from emails/reply_notifcation.py for now

mtomilov · 2025-01-29T15:17:05Z

h/notification/mention.py

+    document: Document
+
+
+def get_notifications(request, annotation: Annotation, action) -> list[Notification]:


Mostly follows notification/reply.py

mtomilov · 2025-01-29T15:18:41Z

h/notification/mention.py

+        return []
+
+    notifications = []
+    for mention in annotation.mentions:


We will need to factor in groups / permissions / limits here as well to avoid sending notifications when not appropriate.

mtomilov · 2025-01-29T15:26:48Z

h/templates/emails/mention_notification.html.jinja2

+  commented:
+</p>
+
+<blockquote>{{ annotation.text or "" }}</blockquote>


Taking the same approach we already have in emails/reply_notification.html.jinja2

mtomilov · 2025-01-29T15:30:34Z

h/services/mention.py

+    def update_mentions(self, annotation: Annotation) -> None:
+        self._session.flush()
+
+        userids = set(self._parse_userids(annotation.text))


Making sure that single user is mentioned only once

mtomilov · 2025-01-29T15:42:15Z

h/templates/emails/mention_notification.html.jinja2

+  commented:
+</p>
+
+<blockquote>{{ annotation.text|striptags or "" }}</blockquote>


Stripping away our custom mention tags

mtomilov · 2025-01-29T15:47:21Z

h/subscribers.py

@@ -89,3 +89,28 @@ def send_reply_notifications(event):
        except OperationalError as err:  # pragma: no cover
            # We could not connect to rabbit! So carry on
            report_exception(err)
+
+
+@subscriber(AnnotationEvent)


Another option could be creating a new MentionEvent instead

mtomilov · 2025-01-29T16:16:37Z

h/templates/emails/reply_notification.html.jinja2

@@ -21,7 +21,7 @@
  commented:
 </p>

-<blockquote>{{ parent.text or "" }}</blockquote>
+<blockquote>{{ parent.text|striptags or "" }}</blockquote>


Now that we have custom tags in annotations we need to strip them in replies as well

mtomilov added 7 commits January 24, 2025 16:55

Add MentionService

cc776b1

Fix MentionService

461ea5f

Add username to mention

0ce8703

Optimize MentionService

45be341

Add MENTION_SCHEMA

a1ce156

Fix migration

0129a3c

Optimize mentions query

72c0690

mtomilov commented Jan 27, 2025

View reviewed changes

mtomilov changed the title ~~Mentions POC~~ Mentions PoC Jan 27, 2025

mtomilov added 2 commits January 27, 2025 13:18

Fix format

bd27f5a

Clean up

6494f05

mtomilov requested a review from marcospri January 27, 2025 16:16

acelaya reviewed Jan 28, 2025

View reviewed changes

marcospri reviewed Jan 28, 2025

View reviewed changes

mtomilov added 7 commits January 29, 2025 09:15

Clean up

06605be

Update annotation.yaml with mentions

9d0c2c2

Clean up

66f5a0d

Clean up

99769cd

Format

5bd068b

Update mention to reference annotation

d16bb03

Clean up

4f44ce4

mtomilov commented Jan 29, 2025

View reviewed changes

mtomilov added 3 commits January 29, 2025 16:08

Send mention email notifications

4dd1e68

Update docstring

9fd2ebe

Fix MentionJSONPresenter

d089ea0

mtomilov commented Jan 29, 2025

View reviewed changes

mtomilov requested review from acelaya and marcospri January 29, 2025 15:31

Strip html tags

c5a2a27

mtomilov commented Jan 29, 2025

View reviewed changes

Strip html tags in replies

8fc7df1

mtomilov commented Jan 29, 2025

View reviewed changes



		def upgrade() -> None:
		op.add_column("mention", sa.Column("username", sa.UnicodeText(), nullable=False))



		def upgrade() -> None:
		op.drop_column("mention", "annotation_id")

		return [notification.mentioned_user.email], subject, text, html


		def _get_user_url(user, request):

		document: Document


		def get_notifications(request, annotation: Annotation, action) -> list[Notification]:

Mentions PoC #9279

Are you sure you want to change the base?

Mentions PoC #9279

Conversation

mtomilov commented Jan 27, 2025 • edited Loading

Testing API

Testing emails

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

Choose a reason for hiding this comment

mtomilov Jan 27, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

mtomilov Jan 29, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomilov Jan 29, 2025 • edited Loading

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

mtomilov Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomilov Jan 29, 2025 • edited Loading

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

mtomilov Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtomilov commented Jan 27, 2025 •

edited

Loading

mtomilov Jan 27, 2025 •

edited

Loading

mtomilov Jan 29, 2025 •

edited

Loading

mtomilov Jan 29, 2025 •

edited

Loading

mtomilov Jan 29, 2025 •

edited

Loading

mtomilov Jan 29, 2025 •

edited

Loading

mtomilov Jan 29, 2025 •

edited

Loading