-
Notifications
You must be signed in to change notification settings - Fork 435
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
base: main
Are you sure you want to change the base?
Mentions PoC #9279
Changes from 20 commits
cc776b1
461ea5f
0ce8703
45be341
a1ce156
0129a3c
72c0690
bd27f5a
6494f05
06605be
9d0c2c2
66f5a0d
99769cd
5bd068b
d16bb03
4f44ce4
4dd1e68
9fd2ebe
d089ea0
c5a2a27
8fc7df1
740ff49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 |
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") |
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): | ||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mentions aren't being used yet so nothing actually destructive here |
||
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, | ||
), | ||
) |
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]: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly follows |
||
# 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
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, | ||
"display_name": self._mention.user.display_name, | ||
"link": self._mention.user.uri, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -22,6 +24,7 @@ def __init__( | |
links_service: LinksService, | ||
flag_service: FlagService, | ||
user_service: UserService, | ||
mention_service: MentionService, | ||
): | ||
""" | ||
Instantiate the service. | ||
|
@@ -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): | ||
""" | ||
|
@@ -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": [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed from here |
||
MentionJSONPresenter(mention).asdict() | ||
for mention in annotation.mentions | ||
], | ||
} | ||
) | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is the main pain point for using AnnotationSlim vs Annotation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, feels a bit awkward as is |
||
Annotation.mentions, | ||
], | ||
) | ||
|
||
|
@@ -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), | ||
) |
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=\"([^\"]+)\"") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps a bit too loose, but could work for prototyping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In future, I think someone suggested the idea of extracting the ids from the annotation rendered HTML, via DOM selector ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I guess we use a real parser for the real production code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I just noticed you commented precisely this here #9279 (comment) |
||
|
||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making sure that single user is mentioned only once |
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll probably want to parse userids from |
||
|
||
|
||
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") | ||
) |
There was a problem hiding this comment.
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