From 6a7d25cec7d4d5efe54d786e616cf15cd48a95ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Wed, 27 Jul 2016 17:18:58 +0200 Subject: [PATCH] Using PATCH to do comment rating. --- .../modules/nodes/custom/__init__.py | 4 + .../modules/nodes/custom/comment.py | 118 ++++++++++++++ pillar/application/utils/__init__.py | 5 +- pillar/application/utils/authorization.py | 2 +- tests/test_patch.py | 147 ++++++++++++++++++ 5 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 pillar/application/modules/nodes/custom/comment.py create mode 100644 tests/test_patch.py diff --git a/pillar/application/modules/nodes/custom/__init__.py b/pillar/application/modules/nodes/custom/__init__.py index d2f28595..49678b56 100644 --- a/pillar/application/modules/nodes/custom/__init__.py +++ b/pillar/application/modules/nodes/custom/__init__.py @@ -18,3 +18,7 @@ def register_patch_handler(node_type): return func return wrapper + + +# Import sub-modules so they can register themselves. +from . import comment diff --git a/pillar/application/modules/nodes/custom/comment.py b/pillar/application/modules/nodes/custom/comment.py new file mode 100644 index 00000000..211bd9d3 --- /dev/null +++ b/pillar/application/modules/nodes/custom/comment.py @@ -0,0 +1,118 @@ +"""PATCH support for comment nodes.""" +import logging + +from flask import current_app +import werkzeug.exceptions as wz_exceptions + +from application.utils import authorization, authentication, jsonify +from . import register_patch_handler + +log = logging.getLogger(__name__) +ROLES_FOR_COMMENT_VOTING = {u'subscriber', u'demo'} +VALID_COMMENT_OPERATIONS = {u'upvote', u'downvote', u'revoke'} + + +@register_patch_handler(u'comment') +def patch_comment(node_id, patch): + assert_is_valid_patch(node_id, patch) + user_id = authentication.current_user_id() + + # Find the node + nodes_coll = current_app.data.driver.db['nodes'] + node_query = {'_id': node_id, + '$or': [{'properties.ratings.$.user': {'$exists': False}}, + {'properties.ratings.$.user': user_id}]} + node = nodes_coll.find_one(node_query, + projection={'properties': 1}) + if node is None: + log.warning('How can the node not be found?') + raise wz_exceptions.NotFound('Node %s not found' % node_id) + + props = node['properties'] + + # Find the current rating (if any) + rating = next((rating for rating in props.get('ratings', ()) + if rating.get('user') == user_id), None) + + def revoke(): + if not rating: + # No rating, this is a no-op. + return + + label = 'positive' if rating.get('is_positive') else 'negative' + update = {'$pull': {'properties.ratings': rating}, + '$inc': {'properties.rating_%s' % label: -1}} + return update + + def upvote(): + if rating and rating.get('is_positive'): + # There already was a positive rating, so this is a no-op. + return + + update = {'$inc': {'properties.rating_positive': 1}} + if rating: + update['$inc']['properties.rating_negative'] = -1 + update['$set'] = {'properties.ratings.$.is_positive': True} + else: + update['$push'] = {'properties.ratings': { + 'user': user_id, 'is_positive': True, + }} + return update + + def downvote(): + if rating and not rating.get('is_positive'): + # There already was a negative rating, so this is a no-op. + return + + update = {'$inc': {'properties.rating_negative': 1}} + if rating: + update['$inc']['properties.rating_positive'] = -1 + update['$set'] = {'properties.ratings.$.is_positive': False} + else: + update['$push'] = {'properties.ratings': { + 'user': user_id, 'is_positive': False, + }} + return update + + actions = { + u'upvote': upvote, + u'downvote': downvote, + u'revoke': revoke, + } + action = actions[patch['op']] + mongo_update = action() + + if not mongo_update: + return jsonify({'_status': 'OK', 'result': 'no-op'}) + + log.info('Running %s', mongo_update) + if rating: + result = nodes_coll.update_one({'_id': node_id, 'properties.ratings.user': user_id}, + mongo_update) + else: + result = nodes_coll.update_one({'_id': node_id}, mongo_update) + + return jsonify({'_status': 'OK', 'result': result}) + + +def assert_is_valid_patch(node_id, patch): + """Raises an exception when the patch isn't valid.""" + + try: + op = patch['op'] + except KeyError: + raise wz_exceptions.BadRequest("PATCH should have a key 'op' indicating the operation.") + + if op not in VALID_COMMENT_OPERATIONS: + raise wz_exceptions.BadRequest('Operation should be one of %s', + ', '.join(VALID_COMMENT_OPERATIONS)) + + # See whether the user is allowed to patch + if authorization.user_matches_roles(ROLES_FOR_COMMENT_VOTING): + log.debug('User is allowed to upvote/downvote comment') + return + + # Access denied. + log.info('User %s wants to PATCH comment node %s, but is not allowed.', + authentication.current_user_id(), node_id) + raise wz_exceptions.Forbidden() diff --git a/pillar/application/utils/__init__.py b/pillar/application/utils/__init__.py index e723c431..65400912 100644 --- a/pillar/application/utils/__init__.py +++ b/pillar/application/utils/__init__.py @@ -8,7 +8,7 @@ import bson.objectid from eve import RFC1123_DATE_FORMAT from flask import current_app from werkzeug import exceptions as wz_exceptions - +import pymongo.results __all__ = ('remove_private_keys', 'PillarJSONEncoder') log = logging.getLogger(__name__) @@ -41,6 +41,9 @@ class PillarJSONEncoder(json.JSONEncoder): if isinstance(obj, bson.ObjectId): return str(obj) + if isinstance(obj, pymongo.results.UpdateResult): + return obj.raw_result + # Let the base class default method raise the TypeError return json.JSONEncoder.default(self, obj) diff --git a/pillar/application/utils/authorization.py b/pillar/application/utils/authorization.py index bf2afdf7..2b64fc4c 100644 --- a/pillar/application/utils/authorization.py +++ b/pillar/application/utils/authorization.py @@ -123,7 +123,7 @@ def has_permissions(collection_name, resource, method, append_allowed_methods=Fa return False -def compute_aggr_permissions(collection_name, resource, check_node_type): +def compute_aggr_permissions(collection_name, resource, check_node_type=None): """Returns a permissions dict.""" # We always need the know the project. diff --git a/tests/test_patch.py b/tests/test_patch.py new file mode 100644 index 00000000..feb2ab76 --- /dev/null +++ b/tests/test_patch.py @@ -0,0 +1,147 @@ +from common_test_class import AbstractPillarTest +import common_test_data as ctd + + +class PatchCommentTest(AbstractPillarTest): + def setUp(self, **kwargs): + AbstractPillarTest.setUp(self, **kwargs) + + self.user_id = self.create_user(user_id=24 * 'a') + self.owner_id = self.create_user(user_id=24 * 'b') + self.create_valid_auth_token(self.user_id, 'token') + self.create_valid_auth_token(self.owner_id, 'owner-token') + self.project_id, _ = self.ensure_project_exists() + + comment = {'description': '', + 'project': self.project_id, + 'node_type': 'comment', + 'user': self.owner_id, + 'properties': {'rating_positive': 0, + 'rating_negative': 0, + 'status': 'published', + 'confidence': 0, + 'content': 'Purrrr kittycat', + }, + 'name': 'Test comment'} + + # Use MongoDB to insert the node (comments aren't allowed to be parentless, + # so Eve would bark). + with self.app.test_request_context(): + nodes_coll = self.app.data.driver.db['nodes'] + result = nodes_coll.insert_one(comment) + self.assertIsNotNone(result.inserted_id) + comment_id = result.inserted_id + + self.node_url = '/nodes/%s' % comment_id + + def test_upvote_other_comment(self): + # Patch the node + self.patch(self.node_url, + json={'op': 'upvote'}, + auth_token='token') + + # Get the node again, to inspect its changed state. + patched_node = self.get(self.node_url, auth_token='token').json() + self.assertEqual(1, patched_node['properties']['rating_positive']) + self.assertEqual(0, patched_node['properties']['rating_negative']) + self.assertEqual({u'user': str(self.user_id), u'is_positive': True}, + patched_node['properties']['ratings'][0]) + self.assertEqual(1, len(patched_node['properties']['ratings'])) + + def test_upvote_twice(self): + # Both tests check for rating_positive=1 + self.test_upvote_other_comment() + self.test_upvote_other_comment() + + def test_downvote_other_comment(self): + # Patch the node + self.patch(self.node_url, + json={'op': 'downvote'}, + auth_token='token').json() + + # Get the node again, to inspect its changed state. + patched_node = self.get(self.node_url, auth_token='token').json() + self.assertEqual(0, patched_node['properties']['rating_positive']) + self.assertEqual(1, patched_node['properties']['rating_negative']) + self.assertEqual({u'user': str(self.user_id), u'is_positive': False}, + patched_node['properties']['ratings'][0]) + self.assertEqual(1, len(patched_node['properties']['ratings'])) + + def test_downvote_twice(self): + # Both tests check for rating_negative=1 + self.test_downvote_other_comment() + self.test_downvote_other_comment() + + def test_up_then_downvote(self): + self.test_upvote_other_comment() + self.test_downvote_other_comment() + + def test_down_then_upvote(self): + self.test_downvote_other_comment() + self.test_upvote_other_comment() + + def test_down_then_up_then_downvote(self): + self.test_downvote_other_comment() + self.test_upvote_other_comment() + self.test_downvote_other_comment() + + def test_revoke_noop(self): + # Patch the node + self.patch(self.node_url, + json={'op': 'revoke'}, + auth_token='token') + + # Get the node again, to inspect its changed state. + patched_node = self.get(self.node_url, auth_token='token').json() + self.assertEqual(0, patched_node['properties']['rating_positive']) + self.assertEqual(0, patched_node['properties']['rating_negative']) + self.assertEqual([], patched_node['properties'].get('ratings', [])) + + def test_revoke_upvote(self): + self.test_upvote_other_comment() + self.test_revoke_noop() + + def test_revoke_downvote(self): + self.test_downvote_other_comment() + self.test_revoke_noop() + + def test_with_other_users(self): + # Generate a bunch of users + other_user_ids = [] + for idx in range(5): + uid = self.create_user(user_id=24 * str(idx)) + other_user_ids.append(uid) + self.create_valid_auth_token(uid, 'other-token-%i' % idx) + + # Let them all vote positive. + for idx in range(5): + self.patch(self.node_url, + json={'op': 'upvote'}, + auth_token='other-token-%i' % idx) + + # Use our standard user to downvote (the negative nancy) + self.patch(self.node_url, + json={'op': 'downvote'}, + auth_token='token') + + # Let one of the other users revoke + self.patch(self.node_url, + json={'op': 'revoke'}, + auth_token='other-token-2') + + # And another user downvotes to override their previous upvote + self.patch(self.node_url, + json={'op': 'downvote'}, + auth_token='other-token-4') + + # Inspect the result + patched_node = self.get(self.node_url, auth_token='token').json() + self.assertEqual(3, patched_node['properties']['rating_positive']) + self.assertEqual(2, patched_node['properties']['rating_negative']) + self.assertEqual([ + {u'user': unicode(other_user_ids[0]), u'is_positive': True}, + {u'user': unicode(other_user_ids[1]), u'is_positive': True}, + {u'user': unicode(other_user_ids[3]), u'is_positive': True}, + {u'user': unicode(other_user_ids[4]), u'is_positive': False}, + {u'user': unicode(self.user_id), u'is_positive': False}, + ], patched_node['properties'].get('ratings', []))