From c7b7816199bd0be8f1885cd3e35fbd9c655a4c14 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:19 +0000 Subject: [PATCH 1/7] Also split the models tests module --- diff --git a/hubs/tests/models/__init__.py b/hubs/tests/models/__init__.py new file mode 100644 index 0000000..e69de29 --- /dev/null +++ b/hubs/tests/models/__init__.py diff --git a/hubs/tests/models/test_hub.py b/hubs/tests/models/test_hub.py new file mode 100644 index 0000000..6ec668b --- /dev/null +++ b/hubs/tests/models/test_hub.py @@ -0,0 +1,116 @@ +from __future__ import unicode_literals + +import hubs +import hubs.models +import hubs.tests +from hubs.authz import AccessLevel + + +class HubTest(hubs.tests.APPTest): + + def test_delete_hubs(self): + # verify the hub exists + hub_name = 'ralph' + hub = hubs.models.Hub.get(hub_name) + self.assertIsNotNone(hub) + + # check if association exists + username = 'ralph' + user = hubs.models.User.get(username) + assoc = hubs.models.Association.get(hub, user, 'owner') + self.assertIsNotNone(assoc) + + # check if widgets exist + widgets = hubs.models.Widget.by_hub_id_all(hub.name) + self.assertEqual(11, len(widgets)) + + # delete the hub + self.session.delete(hub) + + # check if association is removed + assoc = hubs.models.Association.get(hub, user, 'owner') + self.assertIsNone(assoc) + + # check if widgets are removed + widgets = hubs.models.Widget.by_hub_id_all(hub.name) + self.assertEqual([], widgets) + + # verify hub is deleted + hub = hubs.models.Hub.get(hub_name) + self.assertIsNone(hub) + + # check if user is still intact + user = hubs.models.User.get(username) + self.assertIsNotNone(user) + self.assertEqual('ralph', user.username) + + def test_auth_hub_widget_access_level(self): + username = 'ralph' + ralph = hubs.models.User.get(username) + hub_ralph = hubs.models.Hub.get(username) + widget_ralph = hubs.models.Widget.query.filter_by( + hub=hub_ralph, plugin="contact").one() + hub_decause = hubs.models.Hub.get("decause") + widget_decause = hubs.models.Widget.query.filter_by( + hub=hub_decause, plugin="contact").one() + self.assertEqual( + hub_decause._get_auth_access_level(ralph), + AccessLevel.logged_in) + self.assertEqual( + widget_decause._get_auth_access_level(ralph), + AccessLevel.logged_in) + self.assertEqual( + hub_ralph._get_auth_access_level(ralph), + AccessLevel.owner) + self.assertEqual( + widget_ralph._get_auth_access_level(ralph), + AccessLevel.owner) + + def test_auth_hub_auth_group(self): + username = "ralph" + hub = hubs.models.Hub.get(username) + self.assertEqual(hub._get_auth_group(), username) + # Uncomment this when CAIAPI is active. + # hub.config["auth_group"] = "testing" + # self.assertEqual(hub._get_auth_group(), "testing") + + def test_auth_hub_permission_name(self): + hub = hubs.models.Hub.get("ralph") + self.assertEqual( + hub._get_auth_permission_name("view"), "hub.public.view") + self.assertEqual( + hub._get_auth_permission_name("users.manage"), "hub.users.manage") + self.assertEqual( + hub._get_auth_permission_name("config"), "hub.config") + hub.config["visibility"] = "preview" + self.assertEqual( + hub._get_auth_permission_name("view"), "hub.preview.view") + hub.config["visibility"] = "private" + self.assertEqual( + hub._get_auth_permission_name("view"), "hub.private.view") + + def test_auth_hub_widget_user_roles(self): + username = "ralph" + ralph = hubs.models.User.get(username) + hub = hubs.models.Hub.get(username) + widget = hubs.models.Widget.query.filter_by( + hub=hub, plugin="contact").one() + assert len(hub.associations) == 1 + assert hub.associations[0].user.username == "ralph" + for role in ["owner", "sponsor", "member"]: + hub.associations[0].role = role + self.assertEqual( + hub._get_auth_user_roles(ralph), {"ralph": [role]}) + self.assertEqual( + widget._get_auth_user_roles(ralph), {"ralph": [role]}) + + def test_unsubscribe_owner(self): + # Owners must be turned into regular members when unsubscribed. + hub = hubs.models.Hub.query.get("infra") + ralph = hubs.models.User.query.get("ralph") + self.session.add(hubs.models.Association( + hub=hub, user=ralph, role='owner')) + self.assertIn(ralph, hub.owners) + hub.unsubscribe(ralph, role="owner") + self.assertNotIn(ralph, hub.owners) + self.assertIn(ralph, hub.members) diff --git a/hubs/tests/models/test_user.py b/hubs/tests/models/test_user.py new file mode 100644 index 0000000..948f211 --- /dev/null +++ b/hubs/tests/models/test_user.py @@ -0,0 +1,88 @@ +from __future__ import unicode_literals + +import hubs +import hubs.models +import hubs.tests + + +class UserTest(hubs.tests.APPTest): + + def test_delete_user(self): + # verify user exists + username = 'ralph' + user = hubs.models.User.get(username) + self.assertIsNotNone(user) + + # check if association exists + hub = hubs.models.Hub.get(username) + assoc = hubs.models.Association.get(hub, user, 'owner') + self.assertIsNotNone(assoc) + + # delete the user + self.session.delete(user) + user = hubs.models.User.get(username) + self.assertIsNone(user) + + # checking to see if the hub is still intact + hub = hubs.models.Hub.get(username) + self.assertIsNotNone(hub) + self.assertEqual('ralph', hub.name) + + # check if widgets still are intact + widgets = hubs.models.Widget.by_hub_id_all(hub.name) + self.assertEqual(11, len(widgets)) + + +class BookmarksTest(hubs.tests.APPTest): + + def _add_assoc(self, hubname, username, role): + hub = hubs.models.Hub.query.get(hubname) + user = hubs.models.User.query.get(username) + self.session.add(hubs.models.Association( + hub=hub, user=user, role=role)) + + def test_user_bookmarks(self): + """ + test that when we add bookmarks they show up. + when adding bookmarks for different hubs, we should + see them all in the result. + """ + commops = hubs.models.Hub(name="commops") + self.session.add(commops) + self._add_assoc("infra", "ralph", "stargazer") + self._add_assoc("i18n", "ralph", "member") + self._add_assoc("commops", "ralph", "subscriber") + ralph = hubs.models.User.query.get("ralph") + self.assertEquals(len(ralph.bookmarks["starred"]), 1) + self.assertEquals(ralph.bookmarks["starred"][0].name, "infra") + self.assertEquals(len(ralph.bookmarks["memberships"]), 1) + self.assertEquals(ralph.bookmarks["memberships"][0].name, "i18n") + self.assertEquals(len(ralph.bookmarks["subscriptions"]), 1) + self.assertEquals(ralph.bookmarks["subscriptions"][0].name, "commops") + + def test_user_bookmarks_star(self): + """ + test that when when adding associations for the same hub, + we should just see the stargazer one. + """ + self._add_assoc("infra", "ralph", "stargazer") + self._add_assoc("infra", "ralph", "member") + self._add_assoc("infra", "ralph", "subscriber") + ralph = hubs.models.User.query.get("ralph") + self.assertEquals(len(ralph.bookmarks["starred"]), 1) + self.assertEquals(ralph.bookmarks["starred"][0].name, "infra") + self.assertEquals(len(ralph.bookmarks["memberships"]), 0) + self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0) + + def test_user_bookmarks_memberships(self): + """ + test that when when adding associations for the same hub, + we should just see the memberships one. + """ + self._add_assoc("infra", "ralph", "member") + self._add_assoc("infra", "ralph", "subscriber") + ralph = hubs.models.User.query.get("ralph") + self.assertEquals(len(ralph.bookmarks["starred"]), 0) + self.assertEquals(len(ralph.bookmarks["memberships"]), 1) + self.assertEquals(ralph.bookmarks["memberships"][0].name, "infra") + self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0) diff --git a/hubs/tests/models/test_widget.py b/hubs/tests/models/test_widget.py new file mode 100644 index 0000000..6d9211a --- /dev/null +++ b/hubs/tests/models/test_widget.py @@ -0,0 +1,32 @@ +from __future__ import unicode_literals + +import hubs +import hubs.models +import hubs.tests + + +class WidgetTest(hubs.tests.APPTest): + + def test_auth_widget_permission_name(self): + hub = hubs.models.Hub.get("ralph") + widget = hubs.models.Widget.query.filter_by( + hub=hub, plugin="contact").one() + self.assertEqual( + widget._get_auth_permission_name("view"), "hub.public.view") + hub.config["visibility"] = "private" + self.session.commit() + self.assertEqual( + widget._get_auth_permission_name("view"), "hub.private.view") + hub.config["visibility"] = "preview" + assert widget.visibility == "public" + self.assertEqual( + widget._get_auth_permission_name("view"), "widget.public.view") + widget.visibility = "restricted" + self.assertEqual( + widget._get_auth_permission_name("view"), "widget.restricted.view") + + def test_widget_enabled(self): + hub = hubs.models.Hub.get("ralph") + widget = hubs.models.Widget(hub=hub, plugin="does-not-exist") + self.session.add(widget) + self.assertFalse(widget.enabled) diff --git a/hubs/tests/test_models.py b/hubs/tests/test_models.py deleted file mode 100644 index 51d066e..0000000 --- a/hubs/tests/test_models.py +++ /dev/null @@ -1,241 +0,0 @@ -from __future__ import unicode_literals - -import hubs -import hubs.models -import hubs.tests -from hubs.authz import AccessLevel - - -class ModelTest(hubs.tests.APPTest): - - def test_delete_user(self): - # verify user exists - username = 'ralph' - user = hubs.models.User.get(username) - self.assertIsNotNone(user) - - # check if association exists - hub = hubs.models.Hub.get(username) - assoc = hubs.models.Association.get(hub, user, 'owner') - self.assertIsNotNone(assoc) - - # delete the user - self.session.delete(user) - user = hubs.models.User.get(username) - self.assertIsNone(user) - - # checking to see if the hub is still intact - hub = hubs.models.Hub.get(username) - self.assertIsNotNone(hub) - self.assertEqual('ralph', hub.name) - - # check if widgets still are intact - widgets = hubs.models.Widget.by_hub_id_all(hub.name) - self.assertEqual(11, len(widgets)) - - def test_delete_hubs(self): - # verify the hub exists - hub_name = 'ralph' - hub = hubs.models.Hub.get(hub_name) - self.assertIsNotNone(hub) - - # check if association exists - username = 'ralph' - user = hubs.models.User.get(username) - assoc = hubs.models.Association.get(hub, user, 'owner') - self.assertIsNotNone(assoc) - - # check if widgets exist - widgets = hubs.models.Widget.by_hub_id_all(hub.name) - self.assertEqual(11, len(widgets)) - - # delete the hub - self.session.delete(hub) - - # check if association is removed - assoc = hubs.models.Association.get(hub, user, 'owner') - self.assertIsNone(assoc) - - # check if widgets are removed - widgets = hubs.models.Widget.by_hub_id_all(hub.name) - self.assertEqual([], widgets) - - # verify hub is deleted - hub = hubs.models.Hub.get(hub_name) - self.assertIsNone(hub) - - # check if user is still intact - user = hubs.models.User.get(username) - self.assertIsNotNone(user) - self.assertEqual('ralph', user.username) - - def test_delete_user_then_hubs(self): - username = 'ralph' - user = hubs.models.User.get(username) - self.assertIsNotNone(user) - self.session.delete(user) - user = hubs.models.User.get(username) - self.assertIsNone(user) - - # checking to see if the hub is still intact - hub = hubs.models.Hub.get(username) - self.assertIsNotNone(hub) - - self.session.delete(hub) - - # check if widgets are removed - widgets = hubs.models.Widget.by_hub_id_all(hub.name) - self.assertEqual([], widgets) - - hub = hubs.models.Hub.get(username) - self.assertIsNone(hub) - - def test_auth_hub_widget_access_level(self): - username = 'ralph' - ralph = hubs.models.User.get(username) - hub_ralph = hubs.models.Hub.get(username) - widget_ralph = hubs.models.Widget.query.filter_by( - hub=hub_ralph, plugin="contact").one() - hub_decause = hubs.models.Hub.get("decause") - widget_decause = hubs.models.Widget.query.filter_by( - hub=hub_decause, plugin="contact").one() - self.assertEqual( - hub_decause._get_auth_access_level(ralph), - AccessLevel.logged_in) - self.assertEqual( - widget_decause._get_auth_access_level(ralph), - AccessLevel.logged_in) - self.assertEqual( - hub_ralph._get_auth_access_level(ralph), - AccessLevel.owner) - self.assertEqual( - widget_ralph._get_auth_access_level(ralph), - AccessLevel.owner) - - def test_auth_hub_auth_group(self): - username = "ralph" - hub = hubs.models.Hub.get(username) - self.assertEqual(hub._get_auth_group(), username) - # Uncomment this when CAIAPI is active. - # hub.config["auth_group"] = "testing" - # self.assertEqual(hub._get_auth_group(), "testing") - - def test_auth_hub_permission_name(self): - hub = hubs.models.Hub.get("ralph") - self.assertEqual( - hub._get_auth_permission_name("view"), "hub.public.view") - self.assertEqual( - hub._get_auth_permission_name("users.manage"), "hub.users.manage") - self.assertEqual( - hub._get_auth_permission_name("config"), "hub.config") - hub.config["visibility"] = "preview" - self.assertEqual( - hub._get_auth_permission_name("view"), "hub.preview.view") - hub.config["visibility"] = "private" - self.assertEqual( - hub._get_auth_permission_name("view"), "hub.private.view") - - def test_auth_hub_widget_user_roles(self): - username = "ralph" - ralph = hubs.models.User.get(username) - hub = hubs.models.Hub.get(username) - widget = hubs.models.Widget.query.filter_by( - hub=hub, plugin="contact").one() - assert len(hub.associations) == 1 - assert hub.associations[0].user.username == "ralph" - for role in ["owner", "sponsor", "member"]: - hub.associations[0].role = role - self.assertEqual( - hub._get_auth_user_roles(ralph), {"ralph": [role]}) - self.assertEqual( - widget._get_auth_user_roles(ralph), {"ralph": [role]}) - - def test_auth_widget_permission_name(self): - hub = hubs.models.Hub.get("ralph") - widget = hubs.models.Widget.query.filter_by( - hub=hub, plugin="contact").one() - self.assertEqual( - widget._get_auth_permission_name("view"), "hub.public.view") - hub.config["visibility"] = "private" - self.session.commit() - self.assertEqual( - widget._get_auth_permission_name("view"), "hub.private.view") - hub.config["visibility"] = "preview" - assert widget.visibility == "public" - self.assertEqual( - widget._get_auth_permission_name("view"), "widget.public.view") - widget.visibility = "restricted" - self.assertEqual( - widget._get_auth_permission_name("view"), "widget.restricted.view") - - def test_widget_enabled(self): - hub = hubs.models.Hub.get("ralph") - widget = hubs.models.Widget(hub=hub, plugin="does-not-exist") - self.session.add(widget) - self.assertFalse(widget.enabled) - - def test_unsubscribe_owner(self): - # Owners must be turned into regular members when unsubscribed. - hub = hubs.models.Hub.query.get("infra") - ralph = hubs.models.User.query.get("ralph") - self.session.add(hubs.models.Association( - hub=hub, user=ralph, role='owner')) - self.assertIn(ralph, hub.owners) - hub.unsubscribe(ralph, role="owner") - self.assertNotIn(ralph, hub.owners) - self.assertIn(ralph, hub.members) - - -class ModelBookmarksTest(hubs.tests.APPTest): - - def _add_assoc(self, hubname, username, role): - hub = hubs.models.Hub.query.get(hubname) - user = hubs.models.User.query.get(username) - self.session.add(hubs.models.Association( - hub=hub, user=user, role=role)) - - def test_user_bookmarks(self): - """ - test that when we add bookmarks they show up. - when adding bookmarks for different hubs, we should - see them all in the result. - """ - commops = hubs.models.Hub(name="commops") - self.session.add(commops) - self._add_assoc("infra", "ralph", "stargazer") - self._add_assoc("i18n", "ralph", "member") - self._add_assoc("commops", "ralph", "subscriber") - ralph = hubs.models.User.query.get("ralph") - self.assertEquals(len(ralph.bookmarks["starred"]), 1) - self.assertEquals(ralph.bookmarks["starred"][0].name, "infra") - self.assertEquals(len(ralph.bookmarks["memberships"]), 1) - self.assertEquals(ralph.bookmarks["memberships"][0].name, "i18n") - self.assertEquals(len(ralph.bookmarks["subscriptions"]), 1) - self.assertEquals(ralph.bookmarks["subscriptions"][0].name, "commops") - - def test_user_bookmarks_star(self): - """ - test that when when adding associations for the same hub, - we should just see the stargazer one. - """ - self._add_assoc("infra", "ralph", "stargazer") - self._add_assoc("infra", "ralph", "member") - self._add_assoc("infra", "ralph", "subscriber") - ralph = hubs.models.User.query.get("ralph") - self.assertEquals(len(ralph.bookmarks["starred"]), 1) - self.assertEquals(ralph.bookmarks["starred"][0].name, "infra") - self.assertEquals(len(ralph.bookmarks["memberships"]), 0) - self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0) - - def test_user_bookmarks_memberships(self): - """ - test that when when adding associations for the same hub, - we should just see the memberships one. - """ - self._add_assoc("infra", "ralph", "member") - self._add_assoc("infra", "ralph", "subscriber") - ralph = hubs.models.User.query.get("ralph") - self.assertEquals(len(ralph.bookmarks["starred"]), 0) - self.assertEquals(len(ralph.bookmarks["memberships"]), 1) - self.assertEquals(ralph.bookmarks["memberships"][0].name, "infra") - self.assertEquals(len(ralph.bookmarks["subscriptions"]), 0) From 9bd672e7e4ff6ea55993a000fa721dc7c53b8767 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:19 +0000 Subject: [PATCH 2/7] Fix a bug when validating a non-empty list config value --- diff --git a/hubs/models/hubconfig.py b/hubs/models/hubconfig.py index 10d1df6..7aedbf0 100644 --- a/hubs/models/hubconfig.py +++ b/hubs/models/hubconfig.py @@ -201,7 +201,7 @@ class HubConfigProxy(MutableMapping): # the config. validated[key].append(v) continue - validated[key] = self.VALIDATORS[key](v) + validated[key].append(self.VALIDATORS[key](v)) else: if current_config[key] == value: # optimization: don't validate if it's already in diff --git a/hubs/tests/models/test_hub_config.py b/hubs/tests/models/test_hub_config.py new file mode 100644 index 0000000..5228029 --- /dev/null +++ b/hubs/tests/models/test_hub_config.py @@ -0,0 +1,19 @@ +from __future__ import unicode_literals + +from mock import patch + +import hubs +import hubs.models +import hubs.tests + + +class HubConfigTest(hubs.tests.APPTest): + + @patch.object(hubs.models.hubconfig.HubConfigProxy, "VALIDATORS", + {"pagure": lambda v: v}) + def test_validate_list(self): + hub = hubs.models.Hub.get("ralph") + self.assertEqual( + hub.config.validate({"pagure": ["testrepo"]}), + {"pagure": ["testrepo"]} + ) From 447fe5f08bc0eff312e3ef56a526f596d0045db7 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:19 +0000 Subject: [PATCH 3/7] Better handling of errors in the HubConfig panel --- diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelCalendar.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelCalendar.js index 037ee71..be75510 100644 --- a/hubs/static/client/app/components/HubConfig/HubConfigPanelCalendar.js +++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelCalendar.js @@ -48,7 +48,12 @@ export default class CalendarPanel extends React.Component { const stillLoading = ( typeof this.props.hubConfig.calendar === "undefined" ); - const invalid = this.props.error ? this.props.error.fields.calendar : null; + + let invalid = null; + if (this.props.error && this.props.error.fields) { + invalid = this.props.error.fields.calendar; + } + return (
{e.preventDefault();}}> diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelChat.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelChat.js index 9134b5a..9de4e0b 100644 --- a/hubs/static/client/app/components/HubConfig/HubConfigPanelChat.js +++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelChat.js @@ -71,7 +71,10 @@ export default class ChatPanel extends React.Component { ); } - const invalid = this.props.error ? this.props.error.fields.chat_channel : null; + let invalid = null; + if (this.props.error && this.props.error.fields) { + invalid = this.props.error.fields.chat_channel; + } return ( {e.preventDefault();}}> diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelDevPlatform.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelDevPlatform.js index e7f623f..a600368 100644 --- a/hubs/static/client/app/components/HubConfig/HubConfigPanelDevPlatform.js +++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelDevPlatform.js @@ -170,7 +170,7 @@ export default class DevPlatformPanel extends React.Component { } - { this.props.error && this.props.error.fields.devplatform_project && + { this.props.error && this.props.error.fields && this.props.error.fields.devplatform_project &&
{this.props.error.fields.devplatform_project}
diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelGeneral.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelGeneral.js index 97d4238..76232e8 100644 --- a/hubs/static/client/app/components/HubConfig/HubConfigPanelGeneral.js +++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelGeneral.js @@ -73,12 +73,14 @@ export default class GeneralPanel extends React.Component { const stillLoading = (typeof this.props.hubConfig.summary === "undefined"); const visibilities = this.props.globalConfig.hub_visibility || []; const right_width = this.props.hubConfig ? 12 - this.props.hubConfig.left_width : 4; + let invalid = {}; - if (this.props.error) { + if (this.props.error && this.props.error.fields) { ["summary", "left_width", "visibility", "avatar"].forEach((key) => { invalid[key] = this.props.error.fields[key]; }); } + return ( {e.preventDefault();}}> diff --git a/hubs/static/client/app/components/HubConfig/HubConfigPanelMailingList.js b/hubs/static/client/app/components/HubConfig/HubConfigPanelMailingList.js index df6d416..662010a 100644 --- a/hubs/static/client/app/components/HubConfig/HubConfigPanelMailingList.js +++ b/hubs/static/client/app/components/HubConfig/HubConfigPanelMailingList.js @@ -48,7 +48,11 @@ export default class MailingListPanel extends React.Component { const stillLoading = ( typeof this.props.hubConfig.mailing_list === "undefined" ); - const invalid = this.props.error ? this.props.error.fields.mailing_list : null; + + let invalid = null; + if (this.props.error && this.props.error.fields) { + invalid = this.props.error.fields.mailing_list; + } return ( {e.preventDefault();}}> From 864131ae0be62ec991c75198f7c0d0d7411350a0 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:19 +0000 Subject: [PATCH 4/7] Cosmetics for the Feed widget --- diff --git a/hubs/static/client/app/components/feed/Feed.js b/hubs/static/client/app/components/feed/Feed.js index 338eb2f..e92d55d 100644 --- a/hubs/static/client/app/components/feed/Feed.js +++ b/hubs/static/client/app/components/feed/Feed.js @@ -28,7 +28,9 @@ export default class Feed extends React.Component { return (
{ (items.length == 0 && this.props.loaded) ? - +

+ +

: items } From 29548acedfc0f8b4383cfe282fc9fde97856b364 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:19 +0000 Subject: [PATCH 5/7] Send message to group hubs depending on the hub config Fixes #488 --- diff --git a/hubs/feed.py b/hubs/feed.py index 939f503..6096836 100644 --- a/hubs/feed.py +++ b/hubs/feed.py @@ -10,7 +10,7 @@ import flask import pymongo from fedmsg.encoding import loads, dumps -from hubs.models import Hub, User, Association +from hubs.models import Hub, User, HubConfig from hubs.utils import get_fedmsg_config @@ -19,23 +19,52 @@ log = logging.getLogger(__name__) def get_hubs_for_msg(msg): hubs = [] + # User hubs for username in fedmsg.meta.msg2usernames(msg): user = User.query.get(username) # Only act on existing users if user is None: log.debug("Message concerning an unknown user: %s", username) continue + if Hub.query.get(username) is None: + log.debug("User exists but has no personal hub: %s", username) + continue hubs.append(username) - group_hubs = Hub.query.filter( - Hub.user_hub == False, # noqa:E712 - ).join(Association).filter( - Association.user == user, - Association.role.in_(["owner", "member"]), - ) - hubs.extend([result[0] for result in group_hubs.values(Hub.name)]) + + # Group hubs + if ".meetbot.meeting." in msg["topic"]: + # Chat + hubs.extend(_get_group_hub_names_by_config( + HubConfig.key == "chat_channel", + HubConfig.value == msg["msg"]["channel"], + )) + elif ".mailman.receive" in msg["topic"]: + # Mailing-list + list_name = msg["msg"]["mlist"]["list_name"] + # TODO: deploy a new version of the mailman fedmsg plugin that adds + # this metadata: + # list_address = msg["msg"]["mlist"]["fqdn_listname"] + # in the meantime, we have to use a LIKE (which is very slow) + hubs.extend(_get_group_hub_names_by_config( + HubConfig.key == "mailing_list", + HubConfig.value.like("{}@%".format(list_name)), + )) + elif ".fedocal." in msg["topic"]: + # Calendar + hubs.extend(_get_group_hub_names_by_config( + HubConfig.key == "calendar", + HubConfig.value == msg["msg"]["calendar"]["calendar_name"], + )) return hubs +def _get_group_hub_names_by_config(*args): + query = Hub.query.filter( + Hub.user_hub == False, # noqa:E712 + ).join(HubConfig).filter(*args) + return [result[0] for result in query.values(Hub.name)] + + def on_new_notification(msg): """Only for FMN messages.""" username = fedmsg.meta.msg2agent(msg) diff --git a/hubs/tests/test_feed.py b/hubs/tests/test_feed.py index 526aac7..c1725bb 100644 --- a/hubs/tests/test_feed.py +++ b/hubs/tests/test_feed.py @@ -120,20 +120,6 @@ class FeedTest(APPTest): # User does not exist, no feed instance should have been created. mock_notifications.assert_not_called() - @patch("hubs.feed.fedmsg.meta.msg2usernames") - def test_get_hubs_for_msg(self, msg2usernames): - ralph = User.query.get("ralph") - infra = Hub.query.get("infra") - test_hub = Hub(name="testhub") - self.session.add(test_hub) - self.session.add(Association(hub=test_hub, user=ralph, role="member")) - self.session.add(Association(hub=infra, user=ralph, role="owner")) - msg2usernames.return_value = ["unknown_user", "ralph"] - self.assertListEqual( - sorted(get_hubs_for_msg({"msg_id": "testmsg"})), - ["infra", "ralph", "testhub"] - ) - def test_add_dom_id(self): msg = { "msg_ids": { @@ -168,3 +154,119 @@ class FeedTest(APPTest): result["markup_subjective"], """your ticket was commented by decause""" ) + + +class GetHubsForMsgTestCase(APPTest): + + # See: http://fedora-fedmsg.readthedocs.io/en/latest/topics.html + + def setUp(self): + super(GetHubsForMsgTestCase, self).setUp() + self.msg2usernames_patcher = patch( + "hubs.feed.fedmsg.meta.msg2usernames") + self.msg2usernames = self.msg2usernames_patcher.start() + self.msg2usernames.return_value = [] + self.dummy_msg = {"msg_id": "testmsg", "topic": "testtopic"} + + def tearDown(self): + self.msg2usernames_patcher.stop() + super(GetHubsForMsgTestCase, self).tearDown() + + def test_unknown_user(self): + self.msg2usernames.return_value = ["unknown_user"] + self.assertListEqual(get_hubs_for_msg(self.dummy_msg), []) + + def test_user_hub_owner(self): + self.msg2usernames.return_value = ["ralph"] + self.assertListEqual( + get_hubs_for_msg(self.dummy_msg), ["ralph"]) + + def test_group_hub_owner(self): + # Don't send a message to a group hub just because the user is the + # owner. + ralph = User.query.get("ralph") + infra = Hub.query.get("infra") + self.session.add(Association(hub=infra, user=ralph, role="owner")) + self.msg2usernames.return_value = ["ralph"] + self.assertListEqual( + get_hubs_for_msg(self.dummy_msg), ["ralph"]) + + def test_group_hub_member(self): + ralph = User.query.get("ralph") + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + self.session.add(Association(hub=test_hub, user=ralph, role="member")) + self.msg2usernames.return_value = ["ralph"] + self.assertListEqual( + get_hubs_for_msg(self.dummy_msg), ["ralph"]) + + def test_group_hub_irc(self): + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + test_hub.config["chat_network"] = "irc.freenode.net" + test_hub.config["chat_channel"] = "testchannel" + messages = [{ + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.meetbot.meeting.start", + "msg": { + "channel": "testchannel", + }, + }, { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.meetbot.meeting.topic.update", + "msg": { + "channel": "testchannel", + }, + }, { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.meetbot.meeting.item.link", + "msg": { + "channel": "testchannel", + }, + }, { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.meetbot.meeting.item.help", + "msg": { + "channel": "testchannel", + }, + }, { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.meetbot.meeting.complete", + "msg": { + "channel": "testchannel", + }, + }] + for msg in messages: + self.assertListEqual(get_hubs_for_msg(msg), ["testhub"]) + + def test_group_hub_mailinglist(self): + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + test_hub.config["mailing_list"] = "testlist@lists.fpo" + msg = { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.mailman.receive", + "msg": { + "mlist": {"list_name": "testlist"}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), ["testhub"]) + + def test_group_hub_calendar(self): + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + test_hub.config["calendar"] = "testcal" + topics = [ + "calendar.clear", "calendar.delete", "calendar.new", + "calendar.update", "calendar.upload", "meeting.delete", + "meeting.new", "meeting.reminder", "meeting.update", + ] + for topic in topics: + msg = { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.fedocal.{}".format(topic), + "msg": { + "calendar": {"calendar_name": "testcal"}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), ["testhub"]) From be55c72a16e458e139d2eec42ec6a518bc016248 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:20 +0000 Subject: [PATCH 6/7] Handle the Pagure messages --- diff --git a/hubs/feed.py b/hubs/feed.py index 6096836..691a4cf 100644 --- a/hubs/feed.py +++ b/hubs/feed.py @@ -11,7 +11,7 @@ import pymongo from fedmsg.encoding import loads, dumps from hubs.models import Hub, User, HubConfig -from hubs.utils import get_fedmsg_config +from hubs.utils import get_fedmsg_config, pagure log = logging.getLogger(__name__) @@ -55,6 +55,17 @@ def get_hubs_for_msg(msg): HubConfig.key == "calendar", HubConfig.value == msg["msg"]["calendar"]["calendar_name"], )) + elif ".pagure." in msg["topic"]: + # Pagure + try: + project_name = pagure.msg2projectname(msg) + except KeyError: + pass + else: + hubs.extend(_get_group_hub_names_by_config( + HubConfig.key == "pagure", + HubConfig.value == project_name, + )) return hubs diff --git a/hubs/tests/test_feed.py b/hubs/tests/test_feed.py index c1725bb..168c113 100644 --- a/hubs/tests/test_feed.py +++ b/hubs/tests/test_feed.py @@ -270,3 +270,55 @@ class GetHubsForMsgTestCase(APPTest): }, } self.assertListEqual(get_hubs_for_msg(msg), ["testhub"]) + + def test_group_hub_pagure(self): + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + projects_ok = [ + "testproject-1", + "testproject-2", + "namespace/testproject-3", + ] + projects_fail = [ + "testproject-3", + "testproject-4", + "namespace/testproject-1", + ] + test_hub.config["pagure"] = projects_ok + + def _do_test_project(project, expected): + msg = { + "msg_id": "testmsg", + "topic": "io.pagure.prod.pagure.commit.flag.added", + "msg": { + "repo": {"fullname": project}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), expected) + msg = { + "msg_id": "testmsg", + "topic": "io.pagure.prod.pagure.issue.assigned.added", + "msg": { + "project": {"fullname": project}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), expected) + msg = { + "msg_id": "testmsg", + "topic": "io.pagure.prod.pagure.pull-request.closed", + "msg": { + "pullrequest": {"project": {"fullname": project}}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), expected) + for project in projects_ok: + _do_test_project(project, ["testhub"]) + for project in projects_fail: + _do_test_project(project, []) + # Handle new and/or differently formatted messages + msg = { + "msg_id": "testmsg", + "topic": "io.pagure.prod.pagure.dummy", + "msg": {}, + } + self.assertListEqual(get_hubs_for_msg(msg), []) diff --git a/hubs/tests/widgets/test_pagure_pr.py b/hubs/tests/widgets/test_pagure_pr.py index 540a10e..88ed547 100644 --- a/hubs/tests/widgets/test_pagure_pr.py +++ b/hubs/tests/widgets/test_pagure_pr.py @@ -26,8 +26,10 @@ class TestPagurePr(WidgetTest): msg = { 'topic': 'tests.pagure.pull-request.new', 'msg': { - "project": { - "name": "fedora-hubs", + "pullrequest": { + "project": { + "name": "fedora-hubs", + }, }, }, } @@ -35,8 +37,10 @@ class TestPagurePr(WidgetTest): msg = { 'topic': 'tests.pagure.pull-request.closed', 'msg': { - "project": { - "name": "fedora-hubs", + "pullrequest": { + "project": { + "name": "fedora-hubs", + }, }, }, } @@ -46,8 +50,10 @@ class TestPagurePr(WidgetTest): msg = { 'topic': 'tests.pagure.pull-request.new', 'msg': { - "project": { - "name": "not-fedora-hubs", + "pullrequest": { + "project": { + "name": "not-fedora-hubs", + }, }, }, } diff --git a/hubs/utils/pagure.py b/hubs/utils/pagure.py new file mode 100644 index 0000000..12dbdf9 --- /dev/null +++ b/hubs/utils/pagure.py @@ -0,0 +1,13 @@ +from __future__ import unicode_literals + + +PAGURE_URL = "https://pagure.io" + + +def msg2projectname(msg): + # Thanks Pagure for this very coherent API. + if ".pagure.commit.flag." in msg["topic"]: + return msg["msg"]["repo"]["fullname"] + if "pagure.pull-request" in msg["topic"]: + return msg["msg"]["pullrequest"]["project"]["fullname"] + return msg["msg"]["project"]["fullname"] diff --git a/hubs/utils/validators.py b/hubs/utils/validators.py index 49d0426..4d5ec86 100644 --- a/hubs/utils/validators.py +++ b/hubs/utils/validators.py @@ -17,6 +17,7 @@ import requests import six from hubs.utils.github import github_org_is_valid, github_repo_is_valid +from hubs.utils.pagure import PAGURE_URL def Noop(value): @@ -108,10 +109,13 @@ def FMNContext(value): def PagureRepo(value): """Fails if the Pagure repository name does not exist.""" - response = requests.get("https://pagure.io/%s" % value, timeout=5) - if response.ok: - return value - raise ValueError('Invalid Pagure repo: {}'.format(value)) + try: + response = requests.get("/".join([PAGURE_URL, value]), timeout=5) + except requests.exceptions.ReadTimeout: + raise ValueError("Could not connect to Pagure, please try again.") + if not response.ok: + raise ValueError('Invalid Pagure repo: {}'.format(value)) + return value def CommaSeparatedList(value): diff --git a/hubs/widgets/pagure_pr/__init__.py b/hubs/widgets/pagure_pr/__init__.py index 1515d35..9988b57 100644 --- a/hubs/widgets/pagure_pr/__init__.py +++ b/hubs/widgets/pagure_pr/__init__.py @@ -1,14 +1,12 @@ from __future__ import unicode_literals -from hubs.utils import validators +from hubs.utils import validators, pagure from hubs.widgets.base import Widget from hubs.widgets.view import RootWidgetView from hubs.widgets.caching import CachedFunction import requests -pagure_url = "https://pagure.io/api/0" - # TODO: use a checkbox set to select which repos to use from the hub's # configuration. @@ -43,7 +41,7 @@ class GetPRs(CachedFunction): def execute(self): repo = self.instance.config["repo"] - url = '/'.join([pagure_url, repo, "pull-requests"]) + url = '/'.join([pagure.PAGURE_URL, "api", "0", repo, "pull-requests"]) response = requests.get(url) try: data = response.json() @@ -79,7 +77,7 @@ class GetPRs(CachedFunction): and ".pagure.pull-request.closed" not in message["topic"]): return False try: - project = message['msg']['project']['name'] + project = message['msg']['pullrequest']['project']['name'] except KeyError: return False return (project == self.instance.config['repo']) diff --git a/hubs/widgets/pagureissues/__init__.py b/hubs/widgets/pagureissues/__init__.py index ba6f8c7..4afcaed 100644 --- a/hubs/widgets/pagureissues/__init__.py +++ b/hubs/widgets/pagureissues/__init__.py @@ -2,13 +2,11 @@ from __future__ import unicode_literals import requests -from hubs.utils import validators +from hubs.utils import validators, pagure from hubs.widgets.base import Widget from hubs.widgets.view import RootWidgetView from hubs.widgets.caching import CachedFunction -pagure_url = "https://pagure.io/api/0" - # TODO: use a checkbox set to select which repos to use from the hub's # configuration. @@ -46,7 +44,7 @@ class GetIssues(CachedFunction): def execute(self): repo = self.instance.config["repo"] - url = '/'.join([pagure_url, repo, "issues"]) + url = '/'.join([pagure.PAGURE_URL, "api", "0", repo, "issues"]) issue_response = requests.get(url) data = issue_response.json() total = data['total_issues'] From 58e7682cd80346ec0366f721eb72be61d2d613f2 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 20 2017 11:36:20 +0000 Subject: [PATCH 7/7] Handle Github messages --- diff --git a/hubs/feed.py b/hubs/feed.py index 691a4cf..cda0ada 100644 --- a/hubs/feed.py +++ b/hubs/feed.py @@ -66,6 +66,17 @@ def get_hubs_for_msg(msg): HubConfig.key == "pagure", HubConfig.value == project_name, )) + elif ".github." in msg["topic"]: + # Github + try: + project_name = msg["msg"]["repository"]["full_name"] + except KeyError: + pass + else: + hubs.extend(_get_group_hub_names_by_config( + HubConfig.key == "github", + HubConfig.value == project_name, + )) return hubs diff --git a/hubs/tests/test_feed.py b/hubs/tests/test_feed.py index 168c113..afc4609 100644 --- a/hubs/tests/test_feed.py +++ b/hubs/tests/test_feed.py @@ -322,3 +322,36 @@ class GetHubsForMsgTestCase(APPTest): "msg": {}, } self.assertListEqual(get_hubs_for_msg(msg), []) + + def test_group_hub_github(self): + test_hub = Hub(name="testhub", user_hub=False) + self.session.add(test_hub) + projects_ok = [ + "testgroup/testproject-1", + "testgroup/testproject-2", + ] + projects_fail = [ + "testgroup/testproject-3", + "othergroup/testproject-1", + ] + test_hub.config["github"] = projects_ok + + def _do_test_project(project, expected): + msg = { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.github.commit_comment", + "msg": { + "repository": {"full_name": project}, + }, + } + self.assertListEqual(get_hubs_for_msg(msg), expected) + for project in projects_ok: + _do_test_project(project, ["testhub"]) + for project in projects_fail: + _do_test_project(project, []) + msg = { + "msg_id": "testmsg", + "topic": "org.fedoraproject.prod.github.webhook", + "msg": {}, + } + self.assertListEqual(get_hubs_for_msg(msg), [])