From e5248bb57795ad48fb7c4b8228d10b5b117f9039 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Aug 25 2017 11:23:19 +0000 Subject: Fix links in the feed The HTML links for the feed items were generated when the backend received a message. At that point, the backend did not now on which URL the front-end is mounted, so it couldn't generate proper links. On `hubs-dev`, the links were of the form `http://0.0.0.0:5000/hubname/`. This commit has the front-end generate the links instead of the backend, so the request context is known. --- diff --git a/hubs/feed.py b/hubs/feed.py index 5fb9546..1cbe510 100644 --- a/hubs/feed.py +++ b/hubs/feed.py @@ -60,22 +60,44 @@ def on_new_message(msg): feed.add(msg) -def format_msg(msg): +def add_dom_id(msg): + """Compute a deterministic dom_id. + + Since this is the identifier stored when the message is saved by the user + in the SQL DB, it has to be invariant through conglomerate() calls. + """ if "msg_ids" not in msg: msg = fedmsg.meta.conglomerate([msg])[0] - # Compute a deterministic dom_id. Since this is the identifier stored - # when the message is saved by the user in the SQL DB, it has to be - # invariant through conglomerate() calls. msg["dom_id"] = hashlib.sha1( b":".join( [mid.encode("utf-8") for mid in sorted(msg["msg_ids"])] )).hexdigest() - # TODO: generate markup - msg["markup"] = _make_hub_links(msg, "subtitle") - msg["markup_subjective"] = _make_hub_links(msg, "subjective") return msg +def format_msgs(msgs): + """Add keys with message text formatted in HTML to a list of messages. + + This method must not be called by the backed, or the links generated by + ``url_for()`` will be wrong (no proper request context). + + Arguments: + msgs (list): a list of messages to format in HTML + + Returns: + list: the list of messages with additional ``markup*`` keys. + """ + if not msgs: + return [] + # TODO: generate more markup + existing_usernames = [r[0] for r in User.query.values(User.username)] + for msg in msgs: + msg["markup"] = _make_hub_links(msg, "subtitle", existing_usernames) + msg["markup_subjective"] = _make_hub_links( + msg, "subjective", existing_usernames) + return msgs + + _word_split_re = re.compile(r'(\s+)') _punctuation_re = re.compile( '^(?P(?:%s)*)(?P.*?)(?P(?:%s)*)$' % ( @@ -85,11 +107,13 @@ _punctuation_re = re.compile( ) -def _make_hub_links(msg, attr): +def _make_hub_links(msg, attr, existing_usernames): if not msg.get(attr): return "" - existing_usernames = [r[0] for r in User.query.values(User.username)] - usernames = [u for u in msg["usernames"] if u in existing_usernames] + usernames = [u for u in msg.get("usernames", []) + if u in existing_usernames] + if not usernames: + return msg[attr] words = _word_split_re.split(msg[attr]) for i, word in enumerate(words): match = _punctuation_re.match(word) @@ -148,9 +172,13 @@ class Feed(object): if self.db is None: self.connect() log.debug("Adding message %s to %s", msg["msg_id"], self.key) - self.db.lpush(self.key, dumps(self._format_msg(msg))) + self.db.lpush(self.key, dumps(self._preprocess_msg(msg))) self.db.ltrim(self.key, 0, self.max_items) + def _preprocess_msg(self, msg): + # Default implementation: no-op. + return msg + def length(self): if self.db is None: self.connect() @@ -183,18 +211,16 @@ class Notifications(Feed): msgtype = "notif" - def _format_msg(self, msg): - return format_msg(msg) + def _preprocess_msg(self, msg): + return add_dom_id(msg) class Activity(Feed): msgtype = "activity" - def _format_msg(self, msg): - # No-op, we want raw messages in the DB to conglomerate and format them - # later. - return msg + # We leave a no-op implementation of_preprocess_msg because we want + # raw messages in the DB to conglomerate and format them later. def _load_json(username): diff --git a/hubs/tests/test_feed.py b/hubs/tests/test_feed.py index 042ff55..4df4ca4 100644 --- a/hubs/tests/test_feed.py +++ b/hubs/tests/test_feed.py @@ -7,7 +7,7 @@ from mock import Mock, patch from hubs.app import app from hubs.models import User, Hub, Association from hubs.feed import ( - Notifications, Activity, format_msg, get_hubs_for_msg, + Notifications, Activity, add_dom_id, format_msgs, get_hubs_for_msg, on_new_message, on_new_notification) from hubs.tests import APPTest @@ -118,7 +118,7 @@ class FeedTest(APPTest): ["infra", "ralph", "testhub"] ) - def test_format_msg(self): + def test_add_dom_id(self): msg = { "msg_ids": { "testid1": {"msg_id": "testid1"}, @@ -130,10 +130,19 @@ class FeedTest(APPTest): "subjective": "your ticket was commented by decause", } with app.test_request_context(): - result = format_msg(msg) + result = add_dom_id(msg) self.assertEqual( result["dom_id"], "067306d0b091ec48d9e2dded1ca9b1f8b1b2ac93") + + def test_format_msgs(self): + msg = { + "usernames": ["ralph", "decause"], + "subtitle": "ralph's ticket was commented by decause", + "subjective": "your ticket was commented by decause", + } + with app.test_request_context(): + result = format_msgs([msg])[0] self.assertEqual( result["markup"], """ralph's ticket was commented by """ diff --git a/hubs/tests/views/test_user.py b/hubs/tests/views/test_user.py index f15192c..127fd36 100644 --- a/hubs/tests/views/test_user.py +++ b/hubs/tests/views/test_user.py @@ -18,7 +18,7 @@ class TestStreamExisting(hubs.tests.APPTest): feed = Mock() Notifications.return_value = feed feed.get.return_value = [{ - "markup": "foo", "link": "bar", + "subtitle": "foo", "link": "bar", }] with hubs.tests.auth_set(app, self.user): resp = self.app.get('/stream/existing') @@ -27,6 +27,7 @@ class TestStreamExisting(hubs.tests.APPTest): data = json.loads(resp.get_data(as_text=True)) self.assertEqual(data["status"], "OK") self.assertEqual(len(data["data"]), 1) + self.assertEqual(data["data"][0]['subtitle'], 'foo') self.assertEqual(data["data"][0]['markup'], 'foo') self.assertEqual(data["data"][0]['link'], 'bar') diff --git a/hubs/views/user.py b/hubs/views/user.py index e33c638..c0e2b99 100644 --- a/hubs/views/user.py +++ b/hubs/views/user.py @@ -26,7 +26,7 @@ def stream(): def stream_existing(): username = flask.g.user.username feed = hubs.feed.Notifications(username) - existing = feed.get() # TODO: paging? + existing = hubs.feed.format_msgs(feed.get()) # TODO: paging? # Right now, stream and actions are the same. # Once mentions is implemented, then each will be its own. return flask.jsonify(dict( diff --git a/hubs/widgets/feed/__init__.py b/hubs/widgets/feed/__init__.py index 9a58891..f37ec2e 100644 --- a/hubs/widgets/feed/__init__.py +++ b/hubs/widgets/feed/__init__.py @@ -3,6 +3,7 @@ from __future__ import unicode_literals, absolute_import import logging +from hubs.feed import format_msgs from hubs.widgets import validators from hubs.widgets.base import Widget from hubs.widgets.view import WidgetView, RootWidgetView @@ -46,7 +47,7 @@ class ExistingView(WidgetView): def get_context(self, instance, *args, **kwargs): get_data = GetData(instance) - existing = get_data() + existing = format_msgs(get_data()) return { "status": "OK", "data": existing, diff --git a/hubs/widgets/feed/functions.py b/hubs/widgets/feed/functions.py index da6d8df..b323ca8 100644 --- a/hubs/widgets/feed/functions.py +++ b/hubs/widgets/feed/functions.py @@ -3,7 +3,7 @@ from __future__ import unicode_literals, absolute_import import fedmsg.meta -from hubs.feed import Activity, format_msg +from hubs.feed import Activity, add_dom_id from hubs.widgets.caching import CachedFunction @@ -15,9 +15,8 @@ class GetData(CachedFunction): feed = Activity(hub_name) raw_msgs = feed.get() # TODO: paging? msgs = fedmsg.meta.conglomerate(raw_msgs) - msgs = [format_msg(msg) for msg in msgs] limit = self.instance.config["message_limit"] - return msgs[:limit] + return [add_dom_id(msg) for msg in msgs[:limit]] def should_invalidate(self, message): if "_hubs" not in message: