From 983eb962af48d9a0d7ffecbb065a4e460f078f4a Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Fri, 10 Jan 2020 16:55:13 -0500 Subject: [PATCH 1/7] Allow only one vote per user on resources --- app/api/routes/resource_modification.py | 46 +++++++++---- app/models.py | 20 ++++++ tests/unit/test_models.py | 35 +++++++++- .../unit/test_routes/test_resource_update.py | 66 ++++++++++++++----- 4 files changed, 136 insertions(+), 31 deletions(-) diff --git a/app/api/routes/resource_modification.py b/app/api/routes/resource_modification.py index e024651e..69251f6e 100644 --- a/app/api/routes/resource_modification.py +++ b/app/api/routes/resource_modification.py @@ -10,7 +10,7 @@ from app.api.routes.helpers import ( failures_counter, get_attributes, latency_summary, logger) from app.api.validations import requires_body, validate_resource, wrong_type -from app.models import Resource +from app.models import Resource, VoteInformation, Key @latency_summary.time() @@ -96,16 +96,12 @@ def update_resource(id, json, db): @latency_summary.time() @failures_counter.count_exceptions() -@bp.route('/resources//upvote', methods=['PUT']) -def upvote(id): - return update_votes(id, 'upvotes') - - -@latency_summary.time() -@failures_counter.count_exceptions() -@bp.route('/resources//downvote', methods=['PUT']) -def downvote(id): - return update_votes(id, 'downvotes') +@bp.route('/resources//', methods=['PUT']) +@authenticate +def change_votes(id, vote_direction): + if vote_direction not in ['upvote', 'downvote']: + redirect('/404') + return update_votes(id, f"{vote_direction}s") @latency_summary.time() @@ -122,7 +118,33 @@ def update_votes(id, vote_direction): return redirect('/404') initial_count = getattr(resource, vote_direction) - setattr(resource, vote_direction, initial_count + 1) + opposite_direction = 'downvotes' if vote_direction == 'upvotes' else 'upvotes' + opposite_count = getattr(resource, opposite_direction) + + api_key = request.headers.get('x-apikey') + vote_info = VoteInformation.query.get( + {'voter_apikey': api_key, 'resource_id': id} + ) + + if vote_info is None: + voter = Key.query.filter_by(apikey=api_key).first() + new_vote_info = VoteInformation( + voter_apikey=api_key, + resource_id=resource.id, + current_direction=vote_direction + ) + new_vote_info.voter = voter + resource.voters.append(new_vote_info) + setattr(resource, vote_direction, initial_count + 1) + else: + if vote_info.current_direction == vote_direction: + setattr(resource, vote_direction, initial_count - 1) + setattr(vote_info, 'current_direction', 'None') + else: + setattr(resource, opposite_direction, opposite_count - 1) \ + if vote_info.current_direction == opposite_direction else None + setattr(resource, vote_direction, initial_count + 1) + setattr(vote_info, 'current_direction', vote_direction) db.session.commit() return utils.standardize_response(payload=dict(data=resource.serialize)) diff --git a/app/models.py b/app/models.py index 780c3dcd..199646b5 100644 --- a/app/models.py +++ b/app/models.py @@ -34,6 +34,7 @@ class Resource(TimestampMixin, db.Model): upvotes = db.Column(db.INTEGER, default=0) downvotes = db.Column(db.INTEGER, default=0) times_clicked = db.Column(db.INTEGER, default=0) + voters = db.relationship('VoteInformation', back_populates='resource') @property def serialize(self): @@ -161,6 +162,7 @@ class Key(TimestampMixin, db.Model): apikey = db.Column(db.String, unique=True, nullable=False, index=True) email = db.Column(db.String, unique=True, nullable=False) blacklisted = db.Column(db.Boolean, default=False) + voted_resources = db.relationship('VoteInformation', back_populates='voter') @property def serialize(self): @@ -193,3 +195,21 @@ def __repr__(self): if self.blacklisted: tags = ' BLACKLISTED' return f"" + + +class VoteInformation(db.Model): + voter_apikey = db.Column(db.String, db.ForeignKey('key.apikey'), primary_key=True) + resource_id = db.Column(db.Integer, db.ForeignKey('resource.id'), primary_key=True) + current_direction = db.Column(db.String, nullable=False) + resource = db.relationship('Resource', back_populates='voters') + voter = db.relationship('Key', back_populates='voted_resources') + + @property + def serialize(self): + return { + 'vote_apikey': self.voter_apikey, + 'resource_id': self.resource_id, + 'current_direction': self.current_direction, + 'voter': self.voter, + 'resource': self.resource + } diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index 4fd29718..ab21484b 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -1,6 +1,6 @@ from datetime import datetime -from app.models import Category, Key, Language, Resource +from app.models import Category, Key, Language, Resource, VoteInformation def test_resource(): @@ -108,3 +108,36 @@ def test_key_blacklisted(): 'email': 'test@example.org', 'last_updated': time.strftime("%Y-%m-%d %H:%M:%S") }) + + +def test_vote_information(): + test_apikey = '1234abcd' + test_id = 1 + test_direction = 'upvote' + + resource = Resource( + id=test_id, + name='name', + url='https://resource.url', + category=Category(name='Category'), + languages=[Language(name='language')], + paid=False, + notes='Some notes' + ) + key = Key(email="test@example.org", apikey=test_apikey) + + vote_info = VoteInformation( + voter_apikey=key.apikey, + resource_id=resource.id, + current_direction=test_direction + ) + vote_info.voter = key + resource.voters.append(vote_info) + + assert (vote_info.serialize == { + 'vote_apikey': test_apikey, + 'resource_id': test_id, + 'current_direction': test_direction, + 'voter': key, + 'resource': resource + }) diff --git a/tests/unit/test_routes/test_resource_update.py b/tests/unit/test_routes/test_resource_update.py index 00c023eb..d3e157e3 100644 --- a/tests/unit/test_routes/test_resource_update.py +++ b/tests/unit/test_routes/test_resource_update.py @@ -4,43 +4,73 @@ ) -def test_update_votes(module_client, module_db, fake_algolia_save): +def test_update_votes(module_client, module_db, fake_auth_from_oc, fake_algolia_save): client = module_client - vote_direction = 'upvote' + UPVOTE = 'upvote' + DOWNVOTE = 'downvote' id = 1 + apikey = get_api_key(client) data = client.get(f"api/v1/resources/{id}").json['data'] response = client.put( - f"/api/v1/resources/{id}/{vote_direction}", follow_redirects=True) - initial_votes = data.get(f"{vote_direction}s") + f"/api/v1/resources/{id}/{UPVOTE}", + follow_redirects=True, + headers={'x-apikey': apikey}) + initial_upvotes = data.get(f"{UPVOTE}s") + initial_downvotes = data.get(f"{DOWNVOTE}s") + + assert (response.status_code == 200) + assert (response.json['data'].get(f"{UPVOTE}s") == initial_upvotes + 1) + response = client.put( + f"/api/v1/resources/{id}/{DOWNVOTE}", + follow_redirects=True, + headers={'x-apikey': apikey}) + # Simple limit vote per user test assert (response.status_code == 200) - assert (response.json['data'].get(f"{vote_direction}s") == initial_votes + 1) + assert (response.json['data'].get(f"{UPVOTE}s") == initial_upvotes) + assert (response.json['data'].get(f"{DOWNVOTE}s") == initial_downvotes + 1) - vote_direction = 'downvote' response = client.put( - f"/api/v1/resources/{id}/{vote_direction}", follow_redirects=True) + f"/api/v1/resources/{id}/{DOWNVOTE}", + follow_redirects=True, + headers={'x-apikey': apikey}) assert (response.status_code == 200) - assert (response.json['data'].get(f"{vote_direction}s") == initial_votes + 1) + assert (response.json['data'].get(f"{DOWNVOTE}s") == initial_downvotes) -def test_update_votes_invalid(module_client, module_db, fake_algolia_save): +def test_update_votes_invalid( + module_client, module_db, fake_auth_from_oc, fake_algolia_save): + client = module_client id = 'waffles' - response = module_client.put( - f"/api/v1/resources/{id}/upvote", follow_redirects=True) + apikey = get_api_key(client) + + response = client.put( + f"/api/v1/resources/{id}/upvote", + follow_redirects=True, + headers={'x-apikey': apikey}) assert_correct_response(response, 404) - response = module_client.put( - f"/api/v1/resources/{id}/downvote", follow_redirects=True) + response = client.put( + f"/api/v1/resources/{id}/downvote", + follow_redirects=True, + headers={'x-apikey': apikey}) assert_correct_response(response, 404) -def test_update_votes_out_of_bounds(module_client, module_db, fake_algolia_save): +def test_update_votes_out_of_bounds( + module_client, module_db, fake_auth_from_oc, fake_algolia_save): + client = module_client + apikey = get_api_key(client) too_high = 99999999 - response = module_client.put( - f"/api/v1/resources/{too_high}/upvote", follow_redirects=True) + response = client.put( + f"/api/v1/resources/{too_high}/upvote", + follow_redirects=True, + headers={'x-apikey': apikey}) assert_correct_response(response, 404) - response = module_client.put( - f"/api/v1/resources/{too_high}/downvote", follow_redirects=True) + response = client.put( + f"/api/v1/resources/{too_high}/downvote", + follow_redirects=True, + headers={'x-apikey': apikey}) assert_correct_response(response, 404) From d3eac046f9dfc4030f24424faa59135449c155da Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Sun, 8 Mar 2020 15:09:42 -0400 Subject: [PATCH 2/7] fixed an issue of not getting 404 when hitting routes other than upvote/downvote --- app/api/routes/resource_modification.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/api/routes/resource_modification.py b/app/api/routes/resource_modification.py index 69251f6e..745d8a7f 100644 --- a/app/api/routes/resource_modification.py +++ b/app/api/routes/resource_modification.py @@ -99,9 +99,8 @@ def update_resource(id, json, db): @bp.route('/resources//', methods=['PUT']) @authenticate def change_votes(id, vote_direction): - if vote_direction not in ['upvote', 'downvote']: - redirect('/404') - return update_votes(id, f"{vote_direction}s") + return update_votes(id, f"{vote_direction}s") \ + if vote_direction in ['upvote', 'downvote'] else redirect('/404') @latency_summary.time() From a195a155bb89a7d86e0c37a334032d271c28c6bd Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Thu, 12 Mar 2020 19:20:59 -0400 Subject: [PATCH 3/7] implemented error handling for parsing json payload --- app/api/auth.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/api/auth.py b/app/api/auth.py index aaa7d4cd..82e47645 100644 --- a/app/api/auth.py +++ b/app/api/auth.py @@ -5,6 +5,7 @@ from app.models import Key from app.utils import setup_logger, standardize_response from flask import g, request +from werkzeug.exceptions import HTTPException auth_logger = setup_logger('auth_logger') create_logger = setup_logger('create_auth_logger') @@ -116,6 +117,9 @@ def log_request(request, key): method = request.method path = request.path user = key.email - payload = request.json + try: + payload = request.get_json() + except HTTPException: + payload = None logger = create_logger if method == "POST" else update_logger logger.info(f"User: {user} Route: {path} Payload: {payload}") From 1a8b04c95b4f19b6d395b01ae4814184fc56cafe Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Thu, 12 Mar 2020 19:57:17 -0400 Subject: [PATCH 4/7] Updated openapi doc to include "upvote" and "downvote" --- app/static/openapi.yaml | 88 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/app/static/openapi.yaml b/app/static/openapi.yaml index d5066186..139697cc 100644 --- a/app/static/openapi.yaml +++ b/app/static/openapi.yaml @@ -517,6 +517,94 @@ paths: 404: $ref: '#/components/responses/NotFound' + /resources/{id}/upvote: + put: + summary: Upvote Resource - Increment Number of Upvotes + description: | + Increments the `upvotes` property of a resource. If the user has already upvoted the resource, upvotes property will decrement since a user is limited to only one vote per resource. + tags: + - Upvote Resource + parameters: + - name: id + in: path + description: Resource ID + required: true + style: simple + explode: false + schema: + type: integer + format: int64 + responses: + 200: + description: Upvoted resource + content: + application/json: + schema: + $ref: '#/components/schemas/Resource_data' + example: + apiVersion: '1.0' + data: + - category: 'Books' + created_at: '2020-01-07 22:06:45' + downvotes: 0 + id: 5 + languages: [] + last_updated: '2020-03-12 23:17:52' + name: 'Teach Your Kids to Code' + notes: null + paid: true + times_clicked: 0 + upvotes: 1 + url: 'http://teachyourkidstocode.com/' + status: 'ok' + status_code: 200 + 404: + $ref: '#/components/responses/NotFound' + + /resources/{id}/downvote: + put: + summary: Downvote Resource - Increment Number of Downvotes + description: | + Increments the `downvotes` property of a resource. If the user has already downvoted the resource, downvotes property will decrement since a user is limited to only one vote per resource. + tags: + - Update Resource + parameters: + - name: id + in: path + description: Resource ID + required: true + style: simple + explode: false + schema: + type: integer + format: int64 + responses: + 200: + description: Downvoted resource + content: + application/json: + schema: + $ref: '#/components/schemas/Resource_data' + example: + apiVersion: '1.0' + data: + - category: 'Books' + created_at: '2020-01-07 22:06:45' + downvotes: 1 + id: 5 + languages: [] + last_updated: '2020-03-12 23:17:52' + name: 'Teach Your Kids to Code' + notes: null + paid: true + times_clicked: 0 + upvotes: 0 + url: 'http://teachyourkidstocode.com/' + status: 'ok' + status_code: 200 + 404: + $ref: '#/components/responses/NotFound' + /resources: get: summary: List Resources From 5c2a6d4c9c7241d28b49ec8432a8dc31c7f93157 Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Thu, 12 Mar 2020 20:11:34 -0400 Subject: [PATCH 5/7] use "request.get_json()" instead of try/except when parsing payload --- app/api/auth.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/api/auth.py b/app/api/auth.py index 82e47645..175d997f 100644 --- a/app/api/auth.py +++ b/app/api/auth.py @@ -5,7 +5,6 @@ from app.models import Key from app.utils import setup_logger, standardize_response from flask import g, request -from werkzeug.exceptions import HTTPException auth_logger = setup_logger('auth_logger') create_logger = setup_logger('create_auth_logger') @@ -117,9 +116,6 @@ def log_request(request, key): method = request.method path = request.path user = key.email - try: - payload = request.get_json() - except HTTPException: - payload = None + payload = request.get_json(silent=True) logger = create_logger if method == "POST" else update_logger logger.info(f"User: {user} Route: {path} Payload: {payload}") From 17b22f75d07132999943354a4b750f2a52c5b5ee Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Thu, 12 Mar 2020 20:55:18 -0400 Subject: [PATCH 6/7] minor openapi doc fix --- app/static/openapi.yaml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/static/openapi.yaml b/app/static/openapi.yaml index 139697cc..5ea3547a 100644 --- a/app/static/openapi.yaml +++ b/app/static/openapi.yaml @@ -519,11 +519,11 @@ paths: /resources/{id}/upvote: put: - summary: Upvote Resource - Increment Number of Upvotes + summary: Upvote Resource description: | - Increments the `upvotes` property of a resource. If the user has already upvoted the resource, upvotes property will decrement since a user is limited to only one vote per resource. + Increments the `upvotes` property of a resource. If the user has already upvoted the resource, `upvotes` property will decrement since a user is limited to only one vote per resource. Also, if the user has already downvoted the resource, hitting this endpoint will increment `upvotes` property while decrementing `downvotes` property. tags: - - Upvote Resource + - Update Resource parameters: - name: id in: path @@ -563,9 +563,9 @@ paths: /resources/{id}/downvote: put: - summary: Downvote Resource - Increment Number of Downvotes + summary: Downvote Resource description: | - Increments the `downvotes` property of a resource. If the user has already downvoted the resource, downvotes property will decrement since a user is limited to only one vote per resource. + Increments the `downvotes` property of a resource. If the user has already downvoted the resource, `downvotes` property will decrement since a user is limited to only one vote per resource. Also, if the user has already upvoted the resource, hitting this endpoint will increment `downvotes` property while decrementing `upvotes` property. tags: - Update Resource parameters: From c9aa37cbee5827523efd3b99a3ac444255906f45 Mon Sep 17 00:00:00 2001 From: Chongyoon Nah Date: Thu, 12 Mar 2020 21:43:34 -0400 Subject: [PATCH 7/7] removed serialize function on "VoteInfomation", modified test accordingly --- app/models.py | 10 ---------- tests/unit/test_models.py | 12 +++++------- 2 files changed, 5 insertions(+), 17 deletions(-) diff --git a/app/models.py b/app/models.py index 199646b5..dbf1f9a8 100644 --- a/app/models.py +++ b/app/models.py @@ -203,13 +203,3 @@ class VoteInformation(db.Model): current_direction = db.Column(db.String, nullable=False) resource = db.relationship('Resource', back_populates='voters') voter = db.relationship('Key', back_populates='voted_resources') - - @property - def serialize(self): - return { - 'vote_apikey': self.voter_apikey, - 'resource_id': self.resource_id, - 'current_direction': self.current_direction, - 'voter': self.voter, - 'resource': self.resource - } diff --git a/tests/unit/test_models.py b/tests/unit/test_models.py index ab21484b..eb6aeb13 100644 --- a/tests/unit/test_models.py +++ b/tests/unit/test_models.py @@ -134,10 +134,8 @@ def test_vote_information(): vote_info.voter = key resource.voters.append(vote_info) - assert (vote_info.serialize == { - 'vote_apikey': test_apikey, - 'resource_id': test_id, - 'current_direction': test_direction, - 'voter': key, - 'resource': resource - }) + assert(vote_info.voter_apikey == test_apikey) + assert(vote_info.resource_id == test_id) + assert(vote_info.current_direction == test_direction) + assert(vote_info.voter == key) + assert(vote_info.resource == resource)