From 7eadb97cf01f6d794af233f65c04241163770391 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Mar 04 2017 08:40:35 +0000 Subject: [PATCH 1/4] Move the plus-plus views to the contact widget --- diff --git a/hubs/tests/test_fedora_hubs_flask_api.py b/hubs/tests/test_fedora_hubs_flask_api.py index 9e66de1..ea33575 100644 --- a/hubs/tests/test_fedora_hubs_flask_api.py +++ b/hubs/tests/test_fedora_hubs_flask_api.py @@ -4,7 +4,6 @@ import unittest from six.moves.urllib.parse import urlparse from flask import json -from mock import mock from werkzeug.datastructures import ImmutableMultiDict import hubs @@ -393,120 +392,3 @@ class HubsAPITest(hubs.tests.APPTest): url = '/source/notexistent' result = self.app.get(url) self.assertEqual(result.status_code, 404) - - def mocked_requests_get(*args, **kwargs): - class MockResponse: # flake8: noqa - def __init__(self, json_data, status_code): - self.json_data = json_data - self.status_code = status_code - self.text = str(json_data) - def json(self): - return self.json_data - - if '/decause' in args[0]: - data = { - "current": 0, - "decrements": 0, - "increments": 0, - "release": "f24", - "total": 0, - "username": "decause" - } - return MockResponse(json_data=data, status_code=200) - - return MockResponse({}, 404) - - def mocked_requests_post(*args, **kwargs): - class MockResponse: - def __init__(self, json_data, status_code): - self.json_data = json_data - self.status_code = status_code - self.text = str(json_data) - - def json(self): - return self.json_data - if '/decause' in kwargs['url']: - data = { - "current": 1, - "decrements": 0, - "increments": 1, - "release": "f24", - "total": 1, - "username": "decause" - } - return MockResponse(json_data=data, status_code=200) - - return MockResponse({}, 404) - - @mock.patch('requests.get', side_effect=mocked_requests_get) - def test_plus_plus_get_valid(self, mock_get): - url = '/plus_plus/decause/status' - result = self.app.get(url) - expected = { - "current": 0, - "decrements": 0, - "increments": 0, - "release": "f24", - "total": 0, - "username": "decause" - } - self.assertEqual( - json.loads(result.get_data(as_text=True)), - expected) - - @mock.patch('requests.post', side_effect=mocked_requests_post) - def test_plus_plus_post_increment_valid(self, mock_post): - user = tests.FakeAuthorization('ralph') - with tests.auth_set(app, user): - url = '/plus_plus/decause/update' - result = self.app.post(url, data={'increment': True}) - expected = { - "current": 1, - "decrements": 0, - "increments": 1, - "release": "f24", - "total": 1, - "username": "decause" - } - self.assertEqual( - json.loads(result.get_data(as_text=True)), - expected) - - @mock.patch('requests.post', side_effect=mocked_requests_post) - def test_plus_plus_post_increment_myself_error(self, mock_post): - user = tests.FakeAuthorization('ralph') - with tests.auth_set(app, user): - url = '/plus_plus/ralph/update' - result = self.app.post(url, data={'increment': True}) - self.assertEqual(result.status_code, 403) - self.assertEqual( - result.get_data(as_text=True), - 'You may not modify your own karma.') - - @mock.patch('requests.post', side_effect=mocked_requests_post) - def test_plus_plus_post_increment_user_does_not_exist(self, mock_post): - user = tests.FakeAuthorization('ralph') - with tests.auth_set(app, user): - url = '/plus_plus/doesnotexist/update' - result = self.app.post(url, data={'increment': True}) - self.assertEqual(result.status_code, 403) - self.assertEqual( - result.get_data(as_text=True), - 'User does not exist') - - @mock.patch('requests.post', side_effect=mocked_requests_post) - def test_plus_plus_post_increment_no_data_error(self, mock_post): - user = tests.FakeAuthorization('ralph') - with tests.auth_set(app, user): - url = '/plus_plus/decause/update' - result = self.app.post(url, data={}) - self.assertEqual(result.status_code, 403) - exp_str = "You must set 'decrement' or 'increment' " \ - "with a boolean value in the body" - self.assertEqual(result.get_data(as_text=True), exp_str) - - def test_plus_plus_receiver_does_not_exist(self): - url = '/plus_plus/doesnotexist/status' - result = self.app.get(url) - self.assertEqual(result.status_code, 403) - self.assertEqual(result.get_data(as_text=True), 'User does not exist') diff --git a/hubs/tests/test_widgets/test_contact.py b/hubs/tests/test_widgets/test_contact.py new file mode 100644 index 0000000..d338e18 --- /dev/null +++ b/hubs/tests/test_widgets/test_contact.py @@ -0,0 +1,158 @@ +from __future__ import unicode_literals + +import flask +import hubs +import json + +from hubs.models import Hub, Widget +from hubs.tests import APPTest, FakeAuthorization, auth_set +from hubs.tests.test_widgets import WidgetTest +from mock import mock + + +def mocked_requests_get(*args, **kwargs): + class MockResponse: # flake8: noqa + def __init__(self, json_data, status_code): + self.json_data = json_data + self.status_code = status_code + self.text = str(json_data) + def json(self): + return self.json_data + + if '/decause' in args[0]: + data = { + "current": 0, + "decrements": 0, + "increments": 0, + "release": "f24", + "total": 0, + "username": "decause" + } + return MockResponse(json_data=data, status_code=200) + + return MockResponse({}, 404) + +def mocked_requests_post(*args, **kwargs): + class MockResponse: + def __init__(self, json_data, status_code): + self.json_data = json_data + self.status_code = status_code + self.text = str(json_data) + + def json(self): + return self.json_data + if '/decause' in kwargs['url']: + data = { + "current": 1, + "decrements": 0, + "increments": 1, + "release": "f24", + "total": 1, + "username": "decause" + } + return MockResponse(json_data=data, status_code=200) + + return MockResponse({}, 404) + + +class ContactsTest(APPTest): + + plugin = "contact" + + def setUp(self): + super(ContactsTest, self).setUp() + hub = Hub.by_name('ralph') + widget = Widget( + plugin='contact', + index=1, + ) + hub.widgets.append(widget) + self.session.commit() + self.widget_idx = widget.idx + + @mock.patch('requests.get', side_effect=mocked_requests_get) + def test_plus_plus_get_valid(self, mock_get): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/status" % ( + self.widget_idx, "decause") + result = self.app.get(url) + expected = { + "current": 0, + "decrements": 0, + "increments": 0, + "release": "f24", + "total": 0, + "username": "decause" + } + self.assertEqual(result.status_code, 200) + self.assertEqual( + json.loads(result.get_data(as_text=True)), + expected) + + @mock.patch('requests.post', side_effect=mocked_requests_post) + def test_plus_plus_post_increment_valid(self, mock_post): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/update" % ( + self.widget_idx, "decause") + user = FakeAuthorization('ralph') + with auth_set(hubs.app.app, user): + result = self.app.post(url, data={'increment': True}) + expected = { + "current": 1, + "decrements": 0, + "increments": 1, + "release": "f24", + "total": 1, + "username": "decause" + } + self.assertEqual(result.status_code, 200) + self.assertEqual( + json.loads(result.get_data(as_text=True)), + expected) + + @mock.patch('requests.post', side_effect=mocked_requests_post) + def test_plus_plus_post_increment_myself_error(self, mock_post): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/update" % ( + self.widget_idx, "ralph") + user = FakeAuthorization('ralph') + with auth_set(hubs.app.app, user): + result = self.app.post(url, data={'increment': True}) + self.assertEqual(result.status_code, 403) + self.assertEqual( + result.get_data(as_text=True), + 'You may not modify your own karma.') + + @mock.patch('requests.post', side_effect=mocked_requests_post) + def test_plus_plus_post_increment_user_does_not_exist(self, mock_post): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/update" % ( + self.widget_idx, "doesnotexist") + user = FakeAuthorization('ralph') + with auth_set(hubs.app.app, user): + result = self.app.post(url, data={'increment': True}) + self.assertEqual(result.status_code, 403) + self.assertEqual( + result.get_data(as_text=True), + 'User does not exist') + + @mock.patch('requests.post', side_effect=mocked_requests_post) + def test_plus_plus_post_increment_no_data_error(self, mock_post): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/update" % ( + self.widget_idx, "decause") + user = FakeAuthorization('ralph') + with auth_set(hubs.app.app, user): + result = self.app.post(url, data={}) + self.assertEqual(result.status_code, 403) + exp_str = "You must set 'decrement' or 'increment' " \ + "with a boolean value in the body" + self.assertEqual(result.get_data(as_text=True), exp_str) + + def test_plus_plus_receiver_does_not_exist(self): + widget = self.widget_instance("ralph", self.plugin) + url = "/ralph/w/contact/%d/plus-plus/%s/status" % ( + self.widget_idx, "doesnotexist") + result = self.app.get(url) + self.assertEqual(result.status_code, 403) + self.assertEqual(result.get_data(as_text=True), 'User does not exist') diff --git a/hubs/views/__init__.py b/hubs/views/__init__.py index 1c97db4..f3d8a0c 100644 --- a/hubs/views/__init__.py +++ b/hubs/views/__init__.py @@ -5,4 +5,3 @@ from .hub import * from .widget import * from .user import * from .api import * -from .plus_plus import * diff --git a/hubs/views/plus_plus.py b/hubs/views/plus_plus.py deleted file mode 100644 index 4e25746..0000000 --- a/hubs/views/plus_plus.py +++ /dev/null @@ -1,77 +0,0 @@ -from __future__ import unicode_literals, absolute_import - -import flask -import requests -import six -import hubs.models - -from hubs.app import app, session -from .utils import login_required - - -# These views should be converted to widget-specific views, now that we can do -# that. - - -@app.route('/plus_plus//status', methods=['GET']) -def plus_plus_status(user): - receiver = hubs.models.User.by_username(user) - if not receiver: - return 'User does not exist', 403 - pp_url = app.config['PLUS_PLUS_URL'] + str(receiver.username) - req = None - try: - req = requests.get(pp_url) - except: - flask.abort(500) - if req.status_code == 200: - return flask.jsonify(req.json()) - else: - return req.text, req.status_code - - -def plus_plus_update_bool_helper(val): - if isinstance(val, bool): - return val - elif isinstance(val, six.string_types): - fmt_str = str(val).replace("'", "").replace('"', '').lower() - return fmt_str in ("yes", "true", "t", "1") - else: - raise ValueError - - -@app.route('/plus_plus//update', methods=['POST']) -@login_required -def plus_plus_update(user): - receiver = hubs.models.User.by_username(user) - - if not receiver: - return 'User does not exist', 403 - - if user == flask.g.auth.nickname: - return 'You may not modify your own karma.', 403 - - if 'decrement' not in flask.request.form \ - and 'increment' not in flask.request.form: - return "You must set 'decrement' or 'increment' " \ - "with a boolean value in the body", 403 - - update = 'increment' if 'increment' in flask.request.form else 'decrement' - - update_bool_val = plus_plus_update_bool_helper(flask.request.form[update]) - pp_url = app.config['PLUS_PLUS_URL'] + str(receiver.username) - sender = hubs.models.User.by_username(flask.g.auth.nickname) - pp_token = app.config['PLUS_PLUS_TOKEN'] - auth_header = {'Authorization': 'token {}'.format(pp_token)} - data = {'sender': sender.username, update: update_bool_val} - req = None - try: - req = requests.post(url=pp_url, headers=auth_header, data=data) - except: - flask.abort(500) - - if req.status_code == 200: - return flask.jsonify(req.json()) - else: - return req.text, req.status_code - diff --git a/hubs/widgets/contact/__init__.py b/hubs/widgets/contact/__init__.py index f5e5722..43df3b4 100644 --- a/hubs/widgets/contact/__init__.py +++ b/hubs/widgets/contact/__init__.py @@ -1,12 +1,14 @@ from __future__ import unicode_literals -import flask - import fedmsg.config import fedmsg.meta +import flask import hubs.models +import requests +import six from hubs.widgets.base import Widget, WidgetView +from hubs.views.utils import login_required config = fedmsg.config.load_config() @@ -35,7 +37,9 @@ class BaseView(WidgetView): usergroup = True user = hubs.models.User.by_username(hub.name) email = user.username + '@fedoraproject.org' - karma_url = flask.url_for('plus_plus_status', user=user.username) + karma_url = flask.url_for( + 'contact_plus_plus_status', + hub=hub.name, idx=instance.idx, user=user.username) fas_info = { 'usergroup': usergroup, 'location': 'United States', @@ -71,3 +75,86 @@ class BaseView(WidgetView): 'wikilink': wikilink, } return fas_info + + +def _get_pp_url(username): + pp_url = flask.current_app.config['PLUS_PLUS_URL'] + if not pp_url.endswith("/"): + pp_url += "/" + pp_url += username + return pp_url + + +class PlusPlusStatus(WidgetView): + + name = "plus_plus_status" + url_rules = ["/plus-plus//status"] + + def dispatch_request(self, *args, **kwargs): + username = kwargs["user"] + receiver = hubs.models.User.by_username(username) + if not receiver: + return 'User does not exist', 403 + pp_url = _get_pp_url(username) + req = None + try: + req = requests.get(pp_url) + except: + flask.abort(500) + if req.status_code == 200: + return flask.jsonify(req.json()) + else: + return req.text, req.status_code + + +def _pp_update_bool_helper(val): + if isinstance(val, bool): + return val + elif isinstance(val, six.string_types): + fmt_str = str(val).replace("'", "").replace('"', '').lower() + return fmt_str in ("yes", "true", "t", "1") + else: + raise ValueError + + +class PlusPlusUpdate(WidgetView): + + name = "plus_plus_update" + url_rules = ["/plus-plus//update"] + methods = ['POST'] + decorators = [login_required] + + def dispatch_request(self, *args, **kwargs): + username = kwargs["user"] + receiver = hubs.models.User.by_username(username) + if not receiver: + return 'User does not exist', 403 + + if username == flask.g.auth.nickname: + return 'You may not modify your own karma.', 403 + + if 'decrement' not in flask.request.form \ + and 'increment' not in flask.request.form: + return "You must set 'decrement' or 'increment' " \ + "with a boolean value in the body", 403 + + update = ('increment' if 'increment' in flask.request.form + else 'decrement') + + update_bool_val = _pp_update_bool_helper( + flask.request.form[update]) + pp_url = _get_pp_url(username) + sender = hubs.models.User.by_username(flask.g.auth.nickname) + pp_token = flask.current_app.config['PLUS_PLUS_TOKEN'] + auth_header = {'Authorization': 'token {}'.format(pp_token)} + data = {'sender': sender.username, update: update_bool_val} + req = None + try: + req = requests.post(url=pp_url, headers=auth_header, data=data) + except: + flask.abort(500) + + if req.status_code == 200: + return flask.jsonify(req.json()) + else: + return req.text, req.status_code From 6b50a07bd5326b37cc9ded125b5b669fb1353903 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Mar 04 2017 08:45:28 +0000 Subject: [PATCH 2/4] Add a simple test for the contact widget base view --- diff --git a/hubs/tests/test_widgets/test_contact.py b/hubs/tests/test_widgets/test_contact.py index d338e18..ae92f87 100644 --- a/hubs/tests/test_widgets/test_contact.py +++ b/hubs/tests/test_widgets/test_contact.py @@ -70,6 +70,22 @@ class ContactsTest(APPTest): self.session.commit() self.widget_idx = widget.idx + def test_data_simple(self): + widget = self.widget_instance('ralph', self.plugin) + response = self.app.get('/ralph/%i/json' % widget.idx) + self.assertEqual(response.status_code, 200) + data = json.loads(response.get_data(as_text=True)) + self.assertDictEqual(data['data'], { + 'account_age': 'Oct 2010', + 'email': 'ralph@fedoraproject.org', + 'ircnick': 'ralph', + 'karma_url': '/ralph/w/contact/%i/plus-plus/ralph/status' + % widget.idx, + 'location': 'United States', + 'timezone': 'UTC', + 'usergroup': True, + }) + @mock.patch('requests.get', side_effect=mocked_requests_get) def test_plus_plus_get_valid(self, mock_get): widget = self.widget_instance("ralph", self.plugin) From 3fa6eddfd8839d3df8751d1e139cc8b12971ff79 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Mar 07 2017 17:27:01 +0000 Subject: [PATCH 3/4] Implement review suggestions by @jcline --- diff --git a/hubs/tests/test_widgets/test_contact.py b/hubs/tests/test_widgets/test_contact.py index ae92f87..a934c38 100644 --- a/hubs/tests/test_widgets/test_contact.py +++ b/hubs/tests/test_widgets/test_contact.py @@ -1,13 +1,14 @@ from __future__ import unicode_literals -import flask -import hubs import json +import flask +import mock + +import hubs from hubs.models import Hub, Widget from hubs.tests import APPTest, FakeAuthorization, auth_set from hubs.tests.test_widgets import WidgetTest -from mock import mock def mocked_requests_get(*args, **kwargs): @@ -147,7 +148,7 @@ class ContactsTest(APPTest): user = FakeAuthorization('ralph') with auth_set(hubs.app.app, user): result = self.app.post(url, data={'increment': True}) - self.assertEqual(result.status_code, 403) + self.assertEqual(result.status_code, 404) self.assertEqual( result.get_data(as_text=True), 'User does not exist') @@ -160,7 +161,7 @@ class ContactsTest(APPTest): user = FakeAuthorization('ralph') with auth_set(hubs.app.app, user): result = self.app.post(url, data={}) - self.assertEqual(result.status_code, 403) + self.assertEqual(result.status_code, 400) exp_str = "You must set 'decrement' or 'increment' " \ "with a boolean value in the body" self.assertEqual(result.get_data(as_text=True), exp_str) @@ -170,5 +171,5 @@ class ContactsTest(APPTest): url = "/ralph/w/contact/%d/plus-plus/%s/status" % ( self.widget_idx, "doesnotexist") result = self.app.get(url) - self.assertEqual(result.status_code, 403) + self.assertEqual(result.status_code, 404) self.assertEqual(result.get_data(as_text=True), 'User does not exist') diff --git a/hubs/widgets/contact/__init__.py b/hubs/widgets/contact/__init__.py index 43df3b4..90a9e0c 100644 --- a/hubs/widgets/contact/__init__.py +++ b/hubs/widgets/contact/__init__.py @@ -94,13 +94,9 @@ class PlusPlusStatus(WidgetView): username = kwargs["user"] receiver = hubs.models.User.by_username(username) if not receiver: - return 'User does not exist', 403 + return 'User does not exist', 404 pp_url = _get_pp_url(username) - req = None - try: - req = requests.get(pp_url) - except: - flask.abort(500) + req = requests.get(pp_url, timeout=5) if req.status_code == 200: return flask.jsonify(req.json()) else: @@ -128,7 +124,7 @@ class PlusPlusUpdate(WidgetView): username = kwargs["user"] receiver = hubs.models.User.by_username(username) if not receiver: - return 'User does not exist', 403 + return 'User does not exist', 404 if username == flask.g.auth.nickname: return 'You may not modify your own karma.', 403 @@ -136,7 +132,7 @@ class PlusPlusUpdate(WidgetView): if 'decrement' not in flask.request.form \ and 'increment' not in flask.request.form: return "You must set 'decrement' or 'increment' " \ - "with a boolean value in the body", 403 + "with a boolean value in the body", 400 update = ('increment' if 'increment' in flask.request.form else 'decrement') @@ -148,12 +144,8 @@ class PlusPlusUpdate(WidgetView): pp_token = flask.current_app.config['PLUS_PLUS_TOKEN'] auth_header = {'Authorization': 'token {}'.format(pp_token)} data = {'sender': sender.username, update: update_bool_val} - req = None - try: - req = requests.post(url=pp_url, headers=auth_header, data=data) - except: - flask.abort(500) - + req = requests.post( + url=pp_url, headers=auth_header, data=data, timeout=5) if req.status_code == 200: return flask.jsonify(req.json()) else: From 8471f0efaa5fc14a8cea2a04771723c58884ae52 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Mar 08 2017 13:44:24 +0000 Subject: [PATCH 4/4] Contact: return a specific error on timeouts --- diff --git a/hubs/widgets/contact/__init__.py b/hubs/widgets/contact/__init__.py index 90a9e0c..37b4b1c 100644 --- a/hubs/widgets/contact/__init__.py +++ b/hubs/widgets/contact/__init__.py @@ -96,7 +96,10 @@ class PlusPlusStatus(WidgetView): if not receiver: return 'User does not exist', 404 pp_url = _get_pp_url(username) - req = requests.get(pp_url, timeout=5) + try: + req = requests.get(pp_url, timeout=5) + except requests.Timeout: + return 'The request to {url} timed out'.format(url=pp_url), 504 if req.status_code == 200: return flask.jsonify(req.json()) else: @@ -144,8 +147,11 @@ class PlusPlusUpdate(WidgetView): pp_token = flask.current_app.config['PLUS_PLUS_TOKEN'] auth_header = {'Authorization': 'token {}'.format(pp_token)} data = {'sender': sender.username, update: update_bool_val} - req = requests.post( - url=pp_url, headers=auth_header, data=data, timeout=5) + try: + req = requests.post( + url=pp_url, headers=auth_header, data=data, timeout=5) + except requests.Timeout: + return 'The request to {url} timed out'.format(url=pp_url), 504 if req.status_code == 200: return flask.jsonify(req.json()) else: