diff --git a/h/activity/bucketing.py b/h/activity/bucketing.py index 832e10a13c5..9b163786fed 100644 --- a/h/activity/bucketing.py +++ b/h/activity/bucketing.py @@ -7,7 +7,8 @@ import newrelic.agent from pyramid import i18n -from h import links, presenters +from h import presenters +from h.services.links import incontext_link _ = i18n.TranslationStringFactory(__package__) @@ -48,7 +49,7 @@ def incontext_link(self, request): """ if not self.annotations: return None - return links.incontext_link(request, self.annotations[0]) + return incontext_link(request, self.annotations[0]) def append(self, annotation): self.annotations.append(annotation) diff --git a/h/emails/mention_notification.py b/h/emails/mention_notification.py index a1b9525940c..f1a905feb73 100644 --- a/h/emails/mention_notification.py +++ b/h/emails/mention_notification.py @@ -1,12 +1,12 @@ from pyramid.renderers import render from pyramid.request import Request -from h import links from h.emails.util import email_subject from h.models import Subscriptions from h.notification.mention import MentionNotification from h.services import SubscriptionService from h.services.email import EmailData, EmailTag +from h.services.links import incontext_link def generate(request: Request, notification: MentionNotification) -> EmailData: @@ -20,7 +20,7 @@ def generate(request: Request, notification: MentionNotification) -> EmailData: "username": username, "user_display_name": notification.mentioning_user.display_name or f"@{username}", - "annotation_url": links.incontext_link(request, notification.annotation) + "annotation_url": incontext_link(notification.annotation) or request.route_url("annotation", id=notification.annotation.id), "document_title": notification.document.title or notification.annotation.target_uri, diff --git a/h/emails/reply_notification.py b/h/emails/reply_notification.py index c9afa12f39e..ebdf757bb63 100644 --- a/h/emails/reply_notification.py +++ b/h/emails/reply_notification.py @@ -1,12 +1,12 @@ from pyramid.renderers import render from pyramid.request import Request -from h import links from h.emails.util import email_subject, get_user_url from h.models import Subscriptions from h.notification.reply import Notification from h.services import SubscriptionService from h.services.email import EmailData, EmailTag +from h.services.links import incontext_link def generate(request: Request, notification: Notification) -> EmailData: @@ -15,7 +15,6 @@ def generate(request: Request, notification: Notification) -> EmailData: :param request: the current request :param notification: the reply notification data structure """ - unsubscribe_token = request.find_service(SubscriptionService).get_unsubscribe_token( user_id=notification.parent_user.userid, type_=Subscriptions.Type.REPLY ) @@ -34,7 +33,7 @@ def generate(request: Request, notification: Notification) -> EmailData: ), # Reply related "reply": notification.reply, - "reply_url": links.incontext_link(request, notification.reply) + "reply_url": incontext_link(notification.reply) or request.route_url("annotation", id=notification.reply.id), "reply_user_display_name": notification.reply_user.display_name or notification.reply_user.username, diff --git a/h/links.py b/h/links.py deleted file mode 100644 index ad55bf6464a..00000000000 --- a/h/links.py +++ /dev/null @@ -1,77 +0,0 @@ -"""Provides links to different representations of annotations.""" - -from urllib.parse import unquote, urljoin, urlparse - - -def pretty_link(url): - """ - Return a nicely formatted version of a URL. - - This strips off 'visual noise' from the URL including common schemes - (HTTP, HTTPS), domain prefixes ('www.') and query strings. - """ - parsed = urlparse(url) - if parsed.scheme not in ["http", "https"]: - return url - netloc = parsed.netloc - netloc = netloc.removeprefix("www.") - return unquote(netloc + parsed.path) - - -def html_link(request, annotation): - """Return a link to an HTML representation of the given annotation, or None.""" - is_third_party_annotation = annotation.authority != request.default_authority - if is_third_party_annotation: - # We don't currently support HTML representations of third party - # annotations. - return None - return request.route_url("annotation", id=annotation.id) - - -def incontext_link(request, annotation): - """Generate a link to an annotation on the page where it was made.""" - bouncer_url = request.registry.settings.get("h.bouncer_url") - if not bouncer_url: - return None - - link = urljoin(bouncer_url, annotation.thread_root_id) - uri = annotation.target_uri - if uri.startswith(("http://", "https://")): - # We can't use urljoin here, because if it detects the second argument - # is a URL it will discard the base URL, breaking the link entirely. - link += "/" + uri[uri.index("://") + 3 :] - elif uri.startswith("urn:x-pdf:") and annotation.document: # pragma: no cover - for docuri in annotation.document.document_uris: - uri = docuri.uri - if uri.startswith(("http://", "https://")): - link += "/" + uri[uri.index("://") + 3 :] - break - - return link - - -def json_link(request, annotation): - return request.route_url("api.annotation", id=annotation.id) - - -def jsonld_id_link(request, annotation): - return request.route_url("annotation", id=annotation.id) - - -def includeme(config): # pragma: no cover - # Add an annotation link generator for the `annotation` view -- this adds a - # named link called "html" to API rendered views of annotations. See - # :py:mod:`h.presenters` for details. - config.add_annotation_link_generator("html", html_link) - - # Add an annotation link generator for viewing annotations in context on - # the page on which they were made. - config.add_annotation_link_generator("incontext", incontext_link) - - # Add a default 'json' link type - config.add_annotation_link_generator("json", json_link) - - # Add a 'jsonld_id' link type for generating the "id" field for JSON-LD - # annotations. This is hidden, and so not rendered in the annotation's - # "links" field. - config.add_annotation_link_generator("jsonld_id", jsonld_id_link, hidden=True) diff --git a/h/presenters/annotation_jsonld.py b/h/presenters/annotation_jsonld.py index 63084627f8d..45472acc17a 100644 --- a/h/presenters/annotation_jsonld.py +++ b/h/presenters/annotation_jsonld.py @@ -1,3 +1,4 @@ +from h.services.links import LinksService from h.util.datetime import utc_iso8601 @@ -10,7 +11,7 @@ class AnnotationJSONLDPresenter: https://www.w3.org/TR/annotation-model/ """ - def __init__(self, annotation, links_service): + def __init__(self, annotation, links_service: LinksService): self.annotation = annotation self._links_service = links_service @@ -20,7 +21,7 @@ def asdict(self): return { "@context": self.CONTEXT_URL, "type": "Annotation", - "id": self._links_service.get(self.annotation, "jsonld_id"), + "id": self._links_service.jsonld_id(self.annotation), "created": utc_iso8601(self.annotation.created), "modified": utc_iso8601(self.annotation.updated), "creator": self.annotation.userid, diff --git a/h/services/__init__.py b/h/services/__init__.py index 88be8b3683c..8d5ce6bddec 100644 --- a/h/services/__init__.py +++ b/h/services/__init__.py @@ -112,10 +112,6 @@ def includeme(config): # pragma: no cover ) # Other services - config.add_directive( - "add_annotation_link_generator", - "h.services.links.add_annotation_link_generator", - ) config.register_service_factory("h.services.links.links_factory", name="links") config.register_service_factory( "h.services.list_organizations.list_organizations_factory", diff --git a/h/services/annotation_json.py b/h/services/annotation_json.py index 00769e72693..ea8b9b67452 100644 --- a/h/services/annotation_json.py +++ b/h/services/annotation_json.py @@ -80,7 +80,11 @@ def present(self, annotation: Annotation, with_metadata: bool = False): # noqa: }, "target": annotation.target, "document": DocumentJSONPresenter(annotation.document).asdict(), - "links": self._links_service.get_all(annotation), + "links": { + "html": self._links_service.html_link(annotation), + "incontext": self._links_service.incontext_link(annotation), + "json": self._links_service.json_link(annotation), + }, } ) diff --git a/h/services/links.py b/h/services/links.py index 28b0caa1ab2..0e29b372386 100644 --- a/h/services/links.py +++ b/h/services/links.py @@ -1,16 +1,56 @@ """Tools for generating links to domain objects.""" +from urllib.parse import urljoin + from pyramid.request import Request from h.security.request_methods import default_authority -LINK_GENERATORS_KEY = "h.links.link_generators" + +def json_link(request, annotation) -> str: + return request.route_url("api.annotation", id=annotation.id) + + +def jsonld_id_link(request, annotation) -> str: + return request.route_url("annotation", id=annotation.id) + + +def html_link(request, annotation) -> str | None: + """Return a link to an HTML representation of the given annotation, or None.""" + is_third_party_annotation = annotation.authority != request.default_authority + if is_third_party_annotation: + # We don't currently support HTML representations of third party + # annotations. + return None + return request.route_url("annotation", id=annotation.id) + + +def incontext_link(request, annotation) -> str | None: + """Generate a link to an annotation on the page where it was made.""" + bouncer_url = request.registry.settings.get("h.bouncer_url") + if not bouncer_url: + return None + + link = urljoin(bouncer_url, annotation.thread_root_id) + uri = annotation.target_uri + if uri.startswith(("http://", "https://")): + # We can't use urljoin here, because if it detects the second argument + # is a URL it will discard the base URL, breaking the link entirely. + link += "/" + uri[uri.index("://") + 3 :] + elif uri.startswith("urn:x-pdf:") and annotation.document: # pragma: no cover + for docuri in annotation.document.document_uris: + uri = docuri.uri + if uri.startswith(("http://", "https://")): + link += "/" + uri[uri.index("://") + 3 :] + break + + return link class LinksService: """A service for generating links to annotations.""" - def __init__(self, base_url, registry): + def __init__(self, base_url): """ Create a new links service. @@ -19,7 +59,6 @@ def __init__(self, base_url, registry): :type registry: pyramid.registry.Registry """ self.base_url = base_url - self.registry = registry # It would be absolutely fair if at this point you asked yourself any # of the following questions: @@ -41,7 +80,6 @@ def __init__(self, base_url, registry): # error-prone way to get access to the route_url function, which can # be used by link generators. self._request = Request.blank("/", base_url=base_url) - self._request.registry = registry # Allow retrieval of the authority from the fake request object, the # same as we do for real requests. @@ -49,45 +87,21 @@ def __init__(self, base_url, registry): default_authority, name="default_authority", reify=True ) - def get(self, annotation, name): - """Get the link named `name` for the passed `annotation`.""" - link_generator, _ = self.registry[LINK_GENERATORS_KEY][name] - return link_generator(self._request, annotation) - - def get_all(self, annotation): - """Get all (non-hidden) links for the passed `annotation`.""" - links = {} - for name, (link_generator, hidden) in self.registry[ - LINK_GENERATORS_KEY - ].items(): - if hidden: - continue - link = link_generator(self._request, annotation) - if link is not None: - links[name] = link - return links + def json_link(self, annotation): + return json_link(self._request, annotation) + + def jsonld_id_link(self, annotation) -> str: + return jsonld_id_link(self._request, annotation) + + def html_link(self, annotation): + return html_link(self._request, annotation) + + def incontext_link(self, annotation): + return incontext_link(self._request, annotation) def links_factory(_context, request): """Return a LinksService instance for the passed context and request.""" - base_url = request.registry.settings.get("h.app_url", "http://localhost:5000") - return LinksService(base_url=base_url, registry=request.registry) - - -def add_annotation_link_generator(config, name, generator, hidden=False): # noqa: FBT002 - """ - Register a function which generates a named link for an annotation. - - Annotation hypermedia links are added to the rendered annotations in a - `links` property or similar. `name` is the unique identifier for the link - type, and `generator` is a callable which accepts two arguments -- the - current request, and the annotation for which to generate a link -- and - returns a string. - - If `hidden` is True, then the link generator will not be included in the - default links output when rendering annotations. - """ - registry = config.registry - if LINK_GENERATORS_KEY not in registry: - registry[LINK_GENERATORS_KEY] = {} - registry[LINK_GENERATORS_KEY][name] = (generator, hidden) + return LinksService( + base_url=request.registry.settings.get("h.app_url", "http://localhost:5000") + ) diff --git a/h/views/activity.py b/h/views/activity.py index b7d69067bbd..7f93b6090e0 100644 --- a/h/views/activity.py +++ b/h/views/activity.py @@ -1,6 +1,6 @@ """Activity pages views.""" -from urllib.parse import urlparse +from urllib.parse import unquote, urlparse from markupsafe import Markup from pyramid import httpexceptions @@ -9,7 +9,6 @@ from h import util from h.activity import query from h.i18n import TranslationString as _ -from h.links import pretty_link from h.paginator import paginate from h.presenters.organization_json import OrganizationJSONPresenter from h.search import parser @@ -21,6 +20,21 @@ PAGE_SIZE = 200 +def pretty_link(url): + """ + Return a nicely formatted version of a URL. + + This strips off 'visual noise' from the URL including common schemes + (HTTP, HTTPS), domain prefixes ('www.') and query strings. + """ + parsed = urlparse(url) + if parsed.scheme not in ["http", "https"]: + return url + netloc = parsed.netloc + netloc = netloc.removeprefix("www.") + return unquote(netloc + parsed.path) + + @view_defaults( route_name="activity.search", renderer="h:templates/activity/search.html.jinja2" ) diff --git a/tests/unit/h/emails/mention_notification_test.py b/tests/unit/h/emails/mention_notification_test.py index a028437ca12..8ee624d5a64 100644 --- a/tests/unit/h/emails/mention_notification_test.py +++ b/tests/unit/h/emails/mention_notification_test.py @@ -17,7 +17,7 @@ def test_it( pyramid_request, html_renderer, text_renderer, - links, + links_service, ): app_url = "https://example.com" pyramid_request.registry.settings.update( @@ -28,14 +28,12 @@ def test_it( generate(pyramid_request, notification) - links.incontext_link.assert_called_once_with( - pyramid_request, notification.annotation - ) + links_service.incontext_link.assert_called_once_with(notification.annotation) expected_context = { "username": mentioning_user.username, "user_display_name": mentioning_user.display_name, - "annotation_url": links.incontext_link.return_value, + "annotation_url": links_service.incontext_link.return_value, "document_title": document.title, "document_url": annotation.target_uri, "annotation": notification.annotation, @@ -52,9 +50,9 @@ def test_falls_back_to_individual_page_if_no_bouncer( pyramid_request, html_renderer, text_renderer, - links, + links_service, ): - links.incontext_link.return_value = None + links_service.incontext_link.return_value = None generate(pyramid_request, notification) @@ -158,10 +156,6 @@ def annotation(self): **common, ) - @pytest.fixture(autouse=True) - def links(self, patch): - return patch("h.emails.mention_notification.links") - @pytest.fixture(autouse=True) def html_renderer(self, pyramid_config): return pyramid_config.testing_add_renderer( diff --git a/tests/unit/h/emails/reply_notification_test.py b/tests/unit/h/emails/reply_notification_test.py index 6f4f1b479ef..78efc479a9c 100644 --- a/tests/unit/h/emails/reply_notification_test.py +++ b/tests/unit/h/emails/reply_notification_test.py @@ -16,14 +16,12 @@ def test_it( reply_user, html_renderer, text_renderer, - links, + links_service, subscription_service, ): generate(pyramid_request, notification) - links.incontext_link.assert_called_once_with( - pyramid_request, notification.reply - ) + links_service.incontext_link.assert_called_once_with(notification.reply) subscription_service.get_unsubscribe_token.assert_called_once_with( user_id=notification.parent_user.userid, type_=Subscriptions.Type.REPLY @@ -36,7 +34,7 @@ def test_it( "parent_user_display_name": parent_user.display_name, "parent_user_url": "http://example.com/stream/user/patricia", "reply": notification.reply, - "reply_url": links.incontext_link.return_value, + "reply_url": links_service.incontext_link.return_value, "reply_user_display_name": reply_user.display_name, "reply_user_url": "http://example.com/stream/user/ron", "unsubscribe_url": "http://example.com/unsub/FAKETOKEN", @@ -62,7 +60,7 @@ def test_falls_back_to_individual_page_if_no_bouncer( reply_user, html_renderer, text_renderer, - links, + links_service, ): # It links to individual pages if bouncer isn't available. # If bouncer isn't enabled direct links in reply notification emails @@ -70,7 +68,7 @@ def test_falls_back_to_individual_page_if_no_bouncer( # the bouncer direct link. # incontext_link() returns None if bouncer isn't available. - links.incontext_link.return_value = None + links_service.incontext_link.return_value = None generate(pyramid_request, notification) @@ -173,10 +171,6 @@ def document(self, db_session): db_session.flush() return doc - @pytest.fixture - def links(self, patch): - return patch("h.emails.reply_notification.links") - @pytest.fixture def notification(self, reply, reply_user, parent, parent_user, document): return Notification( diff --git a/tests/unit/h/services/links_test.py b/tests/unit/h/services/links_test.py index a1dddc6b9c3..649b9b1c803 100644 --- a/tests/unit/h/services/links_test.py +++ b/tests/unit/h/services/links_test.py @@ -2,7 +2,7 @@ import pytest -from h.services.links import LinksService, add_annotation_link_generator, links_factory +from h.services.links import LinksService, links_factory class TestLinksService: @@ -87,33 +87,4 @@ def registry(pyramid_config): pyramid_config.add_route("some.named.route", "/some/path") pyramid_config.add_route("param.route", "/annotations/{id}") - add_annotation_link_generator( - pyramid_config, - "giraffe", - lambda r, a: "http://giraffes.com", # noqa: ARG005 - ) - add_annotation_link_generator( - pyramid_config, - "elephant", - lambda r, a: "https://elephant.org", # noqa: ARG005 - ) - add_annotation_link_generator( - pyramid_config, - "kiwi", - lambda r, a: "http://kiwi.net", # noqa: ARG005 - hidden=True, - ) - add_annotation_link_generator(pyramid_config, "returnsnone", lambda r, a: None) # noqa: ARG005 - add_annotation_link_generator( - pyramid_config, - "namedroute", - lambda r, a: r.route_url("some.named.route"), # noqa: ARG005 - ) - add_annotation_link_generator( - pyramid_config, - "paramroute", - lambda r, a: r.route_url("param.route", id=a.id), - hidden=True, - ) - return pyramid_config.registry