PR#250: Plus-plus service backend integration - fedora-hubs

fedora-hubs

#250 Plus-plus service backend integration

Merged 7 years ago by skrzepto. Opened 7 years ago by skrzepto.

skrzepto/fedora-hubs plus-plus into develop

Plus-Plus service backend integration

skrzepto • 7 years ago

1edbef4

hubs/app.py

file modified

+67

		`@@ -10,6 +10,7 @@`
		`import munch`

		`from flask.ext.oidc import OpenIDConnect`
		`+ from requests import request`

		`import hubs.models`
		`import hubs.widgets`
		`@@ -732,3 +733,69 @@`
		`return flask.abort(400)`
		`session.commit()`
		`return flask.redirect(flask.url_for('hub', name=hub.name))`
		`+`
		`+`
		`+ @app.route('/plus_plus/<user>/status', methods=['GET'])`
		`+ def plus_plus_status(user):`
		`+ receiver = hubs.models.User.by_username(session, user)`
		`+`
		`+ if not receiver:`
		`+ return 'User does not exist', 403`
		`+`
		`+ pp_url = app.config['PLUS_PLUS_URL'] + str(receiver.username)`
		`+ req = None`
		`+ try:`
		`+ req = request('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, (str, unicode)):`
		`+ fmt_str = str(val).replace("'", "").replace('"', '').lower()`
		`+ return fmt_str in ("yes", "true", "t", "1")`
		`+ else:`
		`+ raise ValueError`
		`+`
		`+`
		`+ @app.route('/plus_plus/<user>/update', methods=['POST'])`
		`+ @login_required`
		`+ def plus_plus_update(user):`
		`+ receiver = hubs.models.User.by_username(session, 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:`
sayanchowdhury commented 7 years ago `all(e not in flask.request.form for e in ('increment', 'decrement'))`?
skrzepto commented 7 years ago I think the way it is now compared to the suggestion is simpler and easier to read. So I think I'll stick with how it is.
sayanchowdhury commented 7 years ago Agree.
		`+ 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(session, 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 = request('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`

hubs/default_config.py

file modified

		`@@ -10,6 +10,8 @@`
		`HUB_OF_THE_MONTH = 'commops'`

		`SSE_URL = 'http://localhost:8080/user/'`
		`+ PLUS_PLUS_URL = 'http://localhost:5001/user/'`
		`+ PLUS_PLUS_TOKEN = 'thisismytoken'`
skrzepto commented 7 years ago This must match the plus-plus token

		`OIDC_CLIENT_SECRETS = os.path.join(os.path.dirname(`
		`os.path.abspath(__file__)), '..', 'client_secrets.json')`

hubs/tests/test_fedora_hubs_flask_api.py

file modified

+112

		`@@ -4,6 +4,7 @@`
		`from flask import json`
		`from os.path import dirname`
		`import vcr`
		`+ from mock import mock`
		`from werkzeug.datastructures import ImmutableMultiDict`

		`import hubs`
		`@@ -357,6 +358,117 @@`
		`result = self.app.get(url)`
		`self.assertEqual(result.status_code, 404)`

		`+ def mocked_requests_get(args, *kwargs):`
		`+ class MockResponse:`
		`+ def __init__(self, json_data, status_code):`
		`+ self.json_data = json_data`
		`+ self.status_code = status_code`
		`+`
		`+ def json(self):`
		`+ return self.json_data`
		`+`
		`+ if '/plus_plus/decause/status' in args:`
		`+ 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`
		`+`
		`+ def json(self):`
		`+ return self.json_data`
		`+`
		`+ if '/plus_plus/decause/update' in args:`
		`+ 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.data), 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.data), 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.data, '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.data, '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.data, 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.data, 'User does not exist')`
		`+`
		`+`
		`+`

		`if __name__ == '__main__':`
		`unittest.main()`

skrzepto commented 7 years ago

This will relay a plus-plus action to the plus-plus service.

skrzepto commented on line 6 of hubs/default_config.py 7 years ago

This must match the plus-plus token

skrzepto commented on line 37 of hubs/app.py 7 years ago

This name is a little ambiguous any suggestions to alter it?

skrzepto commented 7 years ago

Tests? I'll look into adding some. I'll need to look into mocking requests

3 new commits added

cleaning up mock request classes
adding case where trying to plus_plus a user who doesnt exist
adding plus_plus tests

7 years ago

pingou commented on line 25 of hubs/app.py 7 years ago

Might be cleaner to have 2 separate endpoints, one for GET, one for POST

pingou commented on line 40 of hubs/app.py 7 years ago

Small note about bool()

>>> bool('False')
True

skrzepto commented on line 25 of hubs/app.py 7 years ago

Would you recommend for GET to have the end point /plus_plus/<user>/status and for POST `/plus_plus/<user>/update ?

sayanchowdhury commented on line 25 of hubs/app.py 7 years ago

makes sense.

skrzepto commented on line 40 of hubs/app.py 7 years ago

ooh good catch let me think about this one

1 new commit added

seperating the status and update routes of plus_plus, adding more tests and made a bool helper function for updating karma

7 years ago

sayanchowdhury commented on line 57 of hubs/app.py 7 years ago

all(e not in flask.request.form for e in ('increment', 'decrement'))?

sayanchowdhury commented 7 years ago

Looks good to me.

skrzepto commented on line 57 of hubs/app.py 7 years ago

I think the way it is now compared to the suggestion is simpler and easier to read. So I think I'll stick with how it is.

sayanchowdhury commented on line 57 of hubs/app.py 7 years ago

Agree.

rebased

7 years ago

skrzepto commented 7 years ago

I just squashed the commits

Pull-Request has been merged by skrzepto

7 years ago

skrzepto commented 7 years ago

hmm just thought of one edge case right after merging. Should we force the plus_plus update/status to be for user hubs only or can groups use plus_plus as well?