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

Mentions PoC #9279

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/_extra/api-reference/schemas/annotation.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -199,3 +199,24 @@ Annotation:
description: The annotation creator's display name
example: "Felicity Nunsun"
- type: null
mentions:
type: array
items:
type: object
properties:
userid:
type: string
pattern: "acct:^[A-Za-z0-9._]{3,30}@.*$"
description: user account ID in the format `"acct:<username>@<authority>"`
example: "acct:[email protected]"
username:
type: string
description: The username of the user
display_name:
type: string
description: The display name of the user
link:
type: string
format: uri
description: The link to the user profile
description: An array of user mentions the annotation text
4 changes: 2 additions & 2 deletions h/emails/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
from h.emails import reply_notification, reset_password, signup
from h.emails import mention_notification, reply_notification, reset_password, signup

__all__ = ("reply_notification", "reset_password", "signup")
__all__ = ("reply_notification", "reset_password", "signup", "mention_notification")
36 changes: 36 additions & 0 deletions h/emails/mention_notification.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from pyramid.renderers import render
from pyramid.request import Request

from h import links
from h.notification.mention import Notification


def generate(request: Request, notification: Notification):
context = {
"user_url": _get_user_url(notification.mentioning_user, request),
"user_display_name": notification.mentioning_user.display_name
or notification.mentioning_user.username,
"annotation_url": links.incontext_link(request, notification.annotation)
or request.route_url("annotation", id=notification.annotation.id),
"document_title": notification.document.title
or notification.annotation.target_uri,
"document_url": notification.annotation.target_uri,
"annotation": notification.annotation,
}

subject = f"{context['user_display_name']} has mentioned you in an annotation"
text = render(
"h:templates/emails/mention_notification.txt.jinja2", context, request=request
)
html = render(
"h:templates/emails/mention_notification.html.jinja2", context, request=request
)

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


def _get_user_url(user, request):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied from emails/reply_notifcation.py for now

if user.authority == request.default_authority:
return request.route_url("stream.user_query", user=user.username)

return None
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
"""Update mention to reference annotation.

Revision ID: 022ea4bf4a27
Revises: 39cc1025a3a2
"""

import sqlalchemy as sa
from alembic import op

from h.db import types

revision = "022ea4bf4a27"
down_revision = "39cc1025a3a2"


def upgrade() -> None:
op.drop_column("mention", "annotation_id")
mtomilov marked this conversation as resolved.
Show resolved Hide resolved
op.add_column(
"mention",
sa.Column(
"annotation_id",
types.URLSafeUUID(),
sa.ForeignKey("annotation.id", ondelete="CASCADE"),
nullable=False,
),
)


def downgrade() -> None:
op.drop_column("mention", "annotation_id")
op.add_column(
"mention",
sa.Column(
"annotation_id",
sa.INTEGER(),
sa.ForeignKey("annotation_slim.id", ondelete="CASCADE"),
nullable=False,
),
)
2 changes: 2 additions & 0 deletions h/models/annotation.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ class Annotation(Base):
uselist=True,
)

mentions = sa.orm.relationship("Mention", back_populates="annotation")

@property
def uuid(self):
"""
Expand Down
12 changes: 6 additions & 6 deletions h/models/mention.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import sqlalchemy as sa
from sqlalchemy.orm import Mapped, mapped_column

from h.db import Base
from h.db import Base, types
from h.db.mixins import Timestamps
from h.models import helpers

Expand All @@ -11,13 +11,13 @@ class Mention(Base, Timestamps): # pragma: nocover

id: Mapped[int] = mapped_column(sa.Integer, autoincrement=True, primary_key=True)

annotation_id: Mapped[int] = mapped_column(
sa.Integer,
sa.ForeignKey("annotation_slim.id", ondelete="CASCADE"),
annotation_id: Mapped[types.URLSafeUUID] = mapped_column(
types.URLSafeUUID,
sa.ForeignKey("annotation.id", ondelete="CASCADE"),
nullable=False,
)
"""FK to annotation_slim.id"""
annotation = sa.orm.relationship("AnnotationSlim")
"""FK to annotation.id"""
annotation = sa.orm.relationship("Annotation", back_populates="mentions")

user_id: Mapped[int] = mapped_column(
sa.Integer,
Expand Down
61 changes: 61 additions & 0 deletions h/notification/mention.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import logging
from dataclasses import dataclass

from h.models import Annotation, Document, User

logger = logging.getLogger(__name__)


@dataclass
class Notification:
"""A data structure representing a mention notification in an annotation."""

mentioning_user: User
mentioned_user: User
annotation: Annotation
document: Document


def get_notifications(request, annotation: Annotation, action) -> list[Notification]:
mtomilov marked this conversation as resolved.
Show resolved Hide resolved
# Only send notifications when new annotations are created
if action != "create":
return []

user_service = request.find_service(name="user")

# If the mentioning user doesn't exist (anymore), we can't send emails, but
# this would be super weird, so log a warning.
mentioning_user = user_service.fetch(annotation.userid)
if mentioning_user is None:
logger.warning(
"user who just mentioned another user no longer exists: %s",
annotation.userid,
)
return []

notifications = []
for mention in annotation.mentions:
Copy link
Contributor Author

@mtomilov mtomilov Jan 29, 2025

Choose a reason for hiding this comment

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

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

# If the mentioning user doesn't exist (anymore), we can't send emails
mentioned_user = user_service.fetch(mention.user.userid)
if mentioned_user is None:
continue

# If mentioned user doesn't have an email address we can't email them.
if not mention.user.email:
continue

# Do not notify users about their own replies
if mentioning_user == mentioned_user:
continue

# If the annotation doesn't have a document, we can't send an email.
if annotation.document is None:
continue

notifications.append(
Notification(
mentioning_user, mentioned_user, annotation, annotation.document
)
)

return notifications
18 changes: 18 additions & 0 deletions h/presenters/mention_json.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
from typing import Any

from h.models import Mention


class MentionJSONPresenter:
"""Present a mention in the JSON format returned by API requests."""

def __init__(self, mention: Mention):
self._mention = mention

def asdict(self) -> dict[str, Any]:
return {
"userid": self._mention.annotation.userid,
"username": self._mention.user.username,
Copy link
Contributor

Choose a reason for hiding this comment

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

In hypothesis/client#6787 (comment), we were discussing that perhaps we don't need to expose the username here, is it can be extracted from the userid.

I don't have a strong opinion though.

Copy link
Contributor Author

@mtomilov mtomilov Feb 4, 2025

Choose a reason for hiding this comment

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

It will be coming from the mention itself like this. It seems better to just store the info from the get go, especially considering the work Marcos has done on user renames already.
So we'll be storing the username at the time of mention on the mention.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. This is the username the user had at the time the mention was created, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's missing from the PoC due to some back-and-forth going at the time. Sorry for the confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

"display_name": self._mention.user.display_name,
"link": self._mention.user.uri,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can any of these be None? (Other than the display_name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question
uri can also apparently be false at least in the db, not sure when it makes sense but still.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it actually makes sense. We will not want to link users in the LMS context, for example.

}
4 changes: 4 additions & 0 deletions h/services/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
BulkLMSStatsService,
)
from h.services.job_queue import JobQueueService
from h.services.mention import MentionService
from h.services.subscription import SubscriptionService


Expand Down Expand Up @@ -42,6 +43,9 @@ def includeme(config): # pragma: no cover
config.register_service_factory(
"h.services.annotation_write.service_factory", iface=AnnotationWriteService
)
config.register_service_factory(
"h.services.mention.service_factory", iface=MentionService
)

# Other services
config.register_service_factory(
Expand Down
12 changes: 12 additions & 0 deletions h/services/annotation_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

from h.models import Annotation, User
from h.presenters import DocumentJSONPresenter
from h.presenters.mention_json import MentionJSONPresenter
from h.security import Identity, identity_permits
from h.security.permissions import Permission
from h.services import MentionService
from h.services.annotation_read import AnnotationReadService
from h.services.flag import FlagService
from h.services.links import LinksService
Expand All @@ -22,6 +24,7 @@ def __init__(
links_service: LinksService,
flag_service: FlagService,
user_service: UserService,
mention_service: MentionService,
):
"""
Instantiate the service.
Expand All @@ -30,11 +33,13 @@ def __init__(
:param links_service: LinksService instance
:param flag_service: FlagService instance
:param user_service: UserService instance
:param mention_service: MentionService instance
"""
self._annotation_read_service = annotation_read_service
self._links_service = links_service
self._flag_service = flag_service
self._user_service = user_service
self._mention_service = mention_service

def present(self, annotation: Annotation):
"""
Expand Down Expand Up @@ -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": [
mtomilov marked this conversation as resolved.
Show resolved Hide resolved
MentionJSONPresenter(mention).asdict()
for mention in annotation.mentions
],
}
)

Expand Down Expand Up @@ -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
mtomilov marked this conversation as resolved.
Show resolved Hide resolved
Annotation.mentions,
],
)

Expand Down Expand Up @@ -184,4 +195,5 @@ def factory(_context, request):
links_service=request.find_service(name="links"),
flag_service=request.find_service(name="flag"),
user_service=request.find_service(name="user"),
mention_service=request.find_service(MentionService),
)
8 changes: 8 additions & 0 deletions h/services/annotation_write.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from h.services.annotation_metadata import AnnotationMetadataService
from h.services.annotation_read import AnnotationReadService
from h.services.job_queue import JobQueueService
from h.services.mention import MentionService
from h.traversal.group import GroupContext
from h.util.group_scope import url_in_scope

Expand All @@ -29,12 +30,14 @@ def __init__(
queue_service: JobQueueService,
annotation_read_service: AnnotationReadService,
annotation_metadata_service: AnnotationMetadataService,
mention_service: MentionService,
):
self._db = db_session
self._has_permission = has_permission
self._queue_service = queue_service
self._annotation_read_service = annotation_read_service
self._annotation_metadata_service = annotation_metadata_service
self._mention_service = mention_service

def create_annotation(self, data: dict) -> Annotation:
"""
Expand Down Expand Up @@ -88,6 +91,8 @@ def create_annotation(self, data: dict) -> Annotation:
schedule_in=60,
)

self._mention_service.update_mentions(annotation)

return annotation

def update_annotation(
Expand Down Expand Up @@ -151,6 +156,8 @@ def update_annotation(
force=not update_timestamp,
)

self._mention_service.update_mentions(annotation)

return annotation

def hide(self, annotation):
Expand Down Expand Up @@ -281,4 +288,5 @@ def service_factory(_context, request) -> AnnotationWriteService:
queue_service=request.find_service(name="queue_service"),
annotation_read_service=request.find_service(AnnotationReadService),
annotation_metadata_service=request.find_service(AnnotationMetadataService),
mention_service=request.find_service(MentionService),
)
43 changes: 43 additions & 0 deletions h/services/mention.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import logging
import re

from sqlalchemy import delete
from sqlalchemy.orm import Session

from h.models import Annotation, Mention
from h.services.user import UserService

USERID_PAT = re.compile(r"data-userid=\"([^\"]+)\"")
mtomilov marked this conversation as resolved.
Show resolved Hide resolved

logger = logging.getLogger(__name__)


class MentionService:
"""A service for managing user mentions."""

def __init__(self, session: Session, user_service: UserService):
self._session = session
self._user_service = user_service

def update_mentions(self, annotation: Annotation) -> None:
self._session.flush()

userids = set(self._parse_userids(annotation.text))
mtomilov marked this conversation as resolved.
Show resolved Hide resolved
users = self._user_service.fetch_all(userids)
self._session.execute(
delete(Mention).where(Mention.annotation_id == annotation.id)
)
for user in users:
mention = Mention(annotation_id=annotation.id, user_id=user.id)
self._session.add(mention)

@staticmethod
def _parse_userids(text: str) -> list[str]:
return USERID_PAT.findall(text)
Copy link
Contributor Author

@mtomilov mtomilov Jan 27, 2025

Choose a reason for hiding this comment

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

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



def service_factory(_context, request) -> MentionService:
"""Return a MentionService instance for the passed context and request."""
return MentionService(
session=request.db, user_service=request.find_service(name="user")
)
Loading
Loading