From 8a4505e53f5c19638b014143d7c54c38be6f8817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Mon, 1 Apr 2019 01:25:48 +0200 Subject: [PATCH 1/6] WIP: ratings for studydocs --- amivapi/events/model.py | 3 +- amivapi/studydocs/__init__.py | 7 +++ amivapi/studydocs/model.py | 74 +++++++++++++++++++++++++- amivapi/studydocs/rating.py | 47 ++++++++++++++++ amivapi/tests/studydocs/test_rating.py | 54 +++++++++++++++++++ 5 files changed, 183 insertions(+), 2 deletions(-) create mode 100644 amivapi/studydocs/rating.py create mode 100644 amivapi/tests/studydocs/test_rating.py diff --git a/amivapi/events/model.py b/amivapi/events/model.py index b02ac554..b22f9c94 100644 --- a/amivapi/events/model.py +++ b/amivapi/events/model.py @@ -512,7 +512,8 @@ 'data_relation': { 'resource': 'events', - 'embeddable': True + 'embeddable': True, + 'cascade_delete': True, }, 'not_patchable': True, 'required': True, diff --git a/amivapi/studydocs/__init__.py b/amivapi/studydocs/__init__.py index 5f5ecfa7..5ee2d568 100644 --- a/amivapi/studydocs/__init__.py +++ b/amivapi/studydocs/__init__.py @@ -13,6 +13,7 @@ add_uploader_on_insert ) from amivapi.studydocs.summary import add_summary +from amivapi.studydocs.rating import add_rating, add_rating_collection from amivapi.studydocs.model import studydocdomain, StudyDocValidator from amivapi.utils import register_domain, register_validator @@ -22,7 +23,13 @@ def init_app(app): register_domain(app, studydocdomain) register_validator(app, StudyDocValidator) + # Uploader app.on_insert_item_studydocuments += add_uploader_on_insert app.on_insert_studydocuments += add_uploader_on_bulk_insert + # Rating + app.on_fetched_item_studydocuments += add_rating + app.on_fetched_resource_studydocuments += add_rating_collection + + # Meta summary app.on_fetched_resource_studydocuments += add_summary diff --git a/amivapi/studydocs/model.py b/amivapi/studydocs/model.py index 55c66079..279b2828 100644 --- a/amivapi/studydocs/model.py +++ b/amivapi/studydocs/model.py @@ -36,6 +36,12 @@
+## Ratings + +TODO! + +
+ ## Summary For more efficient searching of study documents, a *summary* of available @@ -196,7 +202,73 @@ def _validate_allow_summary(self, *args, **kwargs): 'description': 'The year in which the course *was taken*, ' 'to separate older from newer files.', 'allow_summary': True, - } + }, + + 'rating': { + 'title': 'Study Document Rating', + 'description': 'The study document rating as a fraction of ' + 'upvotes divided by all votes. Computed using ' + 'a confidence interval. Null if no votes have ' + 'been cast.', + 'example': '0.9', + + 'type': 'float', + 'readonly': True, + }, + }, + }, + + 'studydocratings': { + 'resource_title': "Study Document Rating", + 'item_title': "Study Document Rating", + + 'description': "Rating for Study documents (TODO)", + + 'resource_methods': ['GET', 'POST'], + 'item_methods': ['GET', 'PATCH', 'DELETE'], + + # TODO + 'authentication': StudydocsAuth, + + 'schema': { + 'user': { + 'description': 'The rating user.', + 'example': '679ff66720812cdc2da4fb4a', + + 'type': 'objectid', + 'data_relation': { + 'resource': 'users', + 'embeddable': True, + 'cascade_delete': True, + }, + 'not_patchable': True, + 'required': True, + 'unique_combination': ['studydoc'], + }, + + 'studydoc': { + 'title': 'Study Document', + 'description': 'The rated study doc.', + 'example': '10d8e50e303049ecb856ae9b', + + 'data_relation': { + 'resource': 'studydocuments', + 'embeddable': True, + 'cascade_delete': True, + }, + 'not_patchable': True, + 'required': True, + 'type': 'objectid', + }, + 'rating': { + 'description': 'The given rating, can be an up- or downvote.', + 'example': 'up', + + 'type': 'string', + 'allowed': ['up', 'down'], + 'required': True + }, }, } + } diff --git a/amivapi/studydocs/rating.py b/amivapi/studydocs/rating.py new file mode 100644 index 00000000..a6a53211 --- /dev/null +++ b/amivapi/studydocs/rating.py @@ -0,0 +1,47 @@ +# -*- coding: utf-8 -*- +# +# license: AGPLv3, see LICENSE for details. In addition we strongly encourage +# you to buy us beer if we meet and you like the software. +"""Studydoc rating computation.""" + +from bson import ObjectId +from flask import current_app +from math import sqrt + + +def lower_confidence_bound(upvotes, downvotes, z=1.28): + """Compute the lower bound of the wilson confidence interval is returned, + which takes the number of votes into account. [1] + + We use z = 1.28 by default, which corresponds to a 80% confidence interval + (As there are only a few votes, we do not want to be overly cautious). + + [1]: http://www.evanmiller.org/how-not-to-sort-by-average-rating.html + """ + total = upvotes + downvotes + if not total: + return + + p = upvotes / total + bound = ((p + (z**2)/(2*total) - + z * sqrt((p * (1-p) + (z**2)/(4*total)) / total)) / + (1 + (z**2)/total)) + + # Ensure that the bound is not below 0 + return max(bound, 0) + + +def add_rating(item): + """Computes the rating for a study document.""" + ratings = current_app.data.driver.db['studydocratings'] + lookup = {'studydoc': ObjectId(item['_id'])} + + upvotes = ratings.count_documents({'rating': 'up', **lookup}) + downvotes = ratings.count_documents({'rating': 'down', **lookup}) + + item['rating'] = lower_confidence_bound(upvotes, downvotes) + + +def add_rating_collection(response): + for item in response['_items']: + add_rating(item) diff --git a/amivapi/tests/studydocs/test_rating.py b/amivapi/tests/studydocs/test_rating.py new file mode 100644 index 00000000..5731bd17 --- /dev/null +++ b/amivapi/tests/studydocs/test_rating.py @@ -0,0 +1,54 @@ +# -*- coding: utf-8 -*- +# +# license: AGPLv3, see LICENSE for details. In addition we strongly encourage +# you to buy us beer if we meet and you like the software. +"""Tests for studydocuments rating.""" + +from amivapi.studydocs.rating import lower_confidence_bound +from amivapi.tests.utils import WebTestNoAuth + + +class StudydocsRatingTest(WebTestNoAuth): + """Test rating computation.""" + + def test_no_rating(self): + """Without any votes, the rating is None.""" + doc = self.new_object('studydocuments') + + response = self.api.get('/studydocuments/' + str(doc['_id']), + status_code=200) + + self.assertIsNone(response.json['rating']) + + def test_bound(self): + """Without any votes, the rating is None.""" + for upvotes, downvotes, rating in [ + (1, 0, 0.38), + (5, 0, 0.75), + (10, 0, 0.86), + (1, 1, 0.16), + (5, 2, 0.47), + (5, 5, 0.31), + (50, 50, 0.44), + ]: + bound = lower_confidence_bound(upvotes, downvotes) + self.assertAlmostEqual(rating, bound, 2) + + def test_rating(self): + """Test that the rating is correctly attached to the studydoc.""" + doc = self.new_object('studydocuments') + lookup = {'studydoc': str(doc['_id'])} + + response = self.api.get('/studydocuments/' + str(doc['_id']), + status_code=200) + # No ratings yet + self.assertIsNone(response.json['rating']) + + self.load_fixture({ + 'users': [{}], + 'studydocratings': [{'rating': 'up', **lookup}], + }) + + response = self.api.get('/studydocuments/' + str(doc['_id']), + status_code=200) + self.assertAlmostEqual(response.json['rating'], 0.38, 2) From a698146c1ab8362360cd2a39b2e0f377ab3c2e53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Tue, 2 Apr 2019 15:25:49 +0200 Subject: [PATCH 2/6] WIP rating tests --- amivapi/tests/studydocs/test_studydocs_auth.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/amivapi/tests/studydocs/test_studydocs_auth.py b/amivapi/tests/studydocs/test_studydocs_auth.py index 56b9b85e..c1d9669a 100644 --- a/amivapi/tests/studydocs/test_studydocs_auth.py +++ b/amivapi/tests/studydocs/test_studydocs_auth.py @@ -88,3 +88,16 @@ def test_can_see_all_docs(self): docs = self.api.get('/studydocuments', token=token, status_code=200) self.assertEqual(len(docs.json['_items']), 5) + + +class StudyDocRatingAuthTest(WebTest): + """Test for rating authentication.""" + + def test_own_ratings(self): + ... + + def test_other_ratings(self): + ... + + def test_admin_ratings(self): + ... From fc446edd791543cc18fe4abaf0ecfd30fd65533a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Mon, 3 Jun 2019 15:39:10 +0200 Subject: [PATCH 3/6] Finish implementation, docs missing --- amivapi/studydocs/__init__.py | 17 +++--- amivapi/studydocs/authorization.py | 9 +-- amivapi/studydocs/model.py | 10 +-- amivapi/studydocs/rating.py | 53 +++++++++++++--- amivapi/tests/studydocs/test_rating.py | 84 +++++++++++++++++++++++++- 5 files changed, 140 insertions(+), 33 deletions(-) diff --git a/amivapi/studydocs/__init__.py b/amivapi/studydocs/__init__.py index 5ee2d568..de161a59 100644 --- a/amivapi/studydocs/__init__.py +++ b/amivapi/studydocs/__init__.py @@ -8,12 +8,10 @@ Contains settings for eve resource, special validation and email_confirmation logic needed for signup of non members to events. """ -from amivapi.studydocs.authorization import ( - add_uploader_on_bulk_insert, - add_uploader_on_insert -) +from amivapi.studydocs.authorization import add_uploader_on_insert from amivapi.studydocs.summary import add_summary -from amivapi.studydocs.rating import add_rating, add_rating_collection +from amivapi.studydocs.rating import ( + init_rating, update_rating_post, update_rating_patch, update_rating_delete) from amivapi.studydocs.model import studydocdomain, StudyDocValidator from amivapi.utils import register_domain, register_validator @@ -24,12 +22,13 @@ def init_app(app): register_validator(app, StudyDocValidator) # Uploader - app.on_insert_item_studydocuments += add_uploader_on_insert - app.on_insert_studydocuments += add_uploader_on_bulk_insert + app.on_insert_studydocuments += add_uploader_on_insert # Rating - app.on_fetched_item_studydocuments += add_rating - app.on_fetched_resource_studydocuments += add_rating_collection + app.on_insert_studydocuments += init_rating + app.on_inserted_studydocumentratings += update_rating_post + app.on_updated_studydocumentratings += update_rating_patch + app.on_deleted_item_studydocumentratings += update_rating_delete # Meta summary app.on_fetched_resource_studydocuments += add_summary diff --git a/amivapi/studydocs/authorization.py b/amivapi/studydocs/authorization.py index 159dd1bc..b7cef767 100644 --- a/amivapi/studydocs/authorization.py +++ b/amivapi/studydocs/authorization.py @@ -20,12 +20,7 @@ def has_resource_write_permission(self, user_id): return True -def add_uploader_on_insert(item): - """Add the _author field before inserting studydocs""" - item['uploader'] = g.get('current_user') - - -def add_uploader_on_bulk_insert(items): +def add_uploader_on_insert(items): """Add the _author field before inserting studydocs""" for item in items: - add_uploader_on_insert(item) + item['uploader'] = g.get('current_user') diff --git a/amivapi/studydocs/model.py b/amivapi/studydocs/model.py index 279b2828..53e3e510 100644 --- a/amivapi/studydocs/model.py +++ b/amivapi/studydocs/model.py @@ -218,8 +218,8 @@ def _validate_allow_summary(self, *args, **kwargs): }, }, - 'studydocratings': { - 'resource_title': "Study Document Rating", + 'studydocumentratings': { + 'resource_title': "Study Document Ratings", 'item_title': "Study Document Rating", 'description': "Rating for Study documents (TODO)", @@ -227,7 +227,7 @@ def _validate_allow_summary(self, *args, **kwargs): 'resource_methods': ['GET', 'POST'], 'item_methods': ['GET', 'PATCH', 'DELETE'], - # TODO + # TODO 'authentication': StudydocsAuth, 'schema': { @@ -243,10 +243,10 @@ def _validate_allow_summary(self, *args, **kwargs): }, 'not_patchable': True, 'required': True, - 'unique_combination': ['studydoc'], + 'unique_combination': ['studydocument'], }, - 'studydoc': { + 'studydocument': { 'title': 'Study Document', 'description': 'The rated study doc.', 'example': '10d8e50e303049ecb856ae9b', diff --git a/amivapi/studydocs/rating.py b/amivapi/studydocs/rating.py index a6a53211..5119462e 100644 --- a/amivapi/studydocs/rating.py +++ b/amivapi/studydocs/rating.py @@ -2,12 +2,21 @@ # # license: AGPLv3, see LICENSE for details. In addition we strongly encourage # you to buy us beer if we meet and you like the software. -"""Studydoc rating computation.""" +"""Studydoc rating computation. + +For the rating, we use the lower bound of a confidence interval around +the 'naive' average vote, which accounts for the number of votes. + +The rating for a study document is updated each time a study doc rating is +for the respective document is POSTed or PATCHed. The value is then written +to the database to allow sorting of study documents by rating. +""" -from bson import ObjectId from flask import current_app from math import sqrt +from amivapi.utils import get_id + def lower_confidence_bound(upvotes, downvotes, z=1.28): """Compute the lower bound of the wilson confidence interval is returned, @@ -31,17 +40,43 @@ def lower_confidence_bound(upvotes, downvotes, z=1.28): return max(bound, 0) -def add_rating(item): +def _update_rating(studydoc_id): """Computes the rating for a study document.""" - ratings = current_app.data.driver.db['studydocratings'] - lookup = {'studydoc': ObjectId(item['_id'])} + docs = current_app.data.driver.db['studydocuments'] + ratings = current_app.data.driver.db['studydocumentratings'] + lookup = {'studydocument': studydoc_id} + # Check votes upvotes = ratings.count_documents({'rating': 'up', **lookup}) downvotes = ratings.count_documents({'rating': 'down', **lookup}) - item['rating'] = lower_confidence_bound(upvotes, downvotes) + # Compute rating and write to database + rating = lower_confidence_bound(upvotes, downvotes) + docs.update_one({'_id': studydoc_id}, {'$set': {'rating': rating}}) + + +def init_rating(items): + """On creating of a study-document, set the rating to None.""" + for item in items: + item['rating'] = None + # lookup = {'_id': {'$in': [get_id(item['_id']) for item in items]}} + # updates = {'$set': {'rating': None}} + # current_app.data.driver.db['studydocuments'].update_many(lookup, updates) + + +def update_rating_post(items): + """Rating update hook for POST requests.""" + for item in items: + _update_rating(get_id(item['studydocument'])) + + +def update_rating_patch(updates, original): + """Rating update hook for PATCH requests.""" + _update_rating(get_id(updates['studydocument']) + if 'studydocument' in updates else + get_id(original['studydocument'])) -def add_rating_collection(response): - for item in response['_items']: - add_rating(item) +def update_rating_delete(item): + """Rating update hook for DELETE requests.""" + _update_rating(get_id(item['studydocument'])) diff --git a/amivapi/tests/studydocs/test_rating.py b/amivapi/tests/studydocs/test_rating.py index 5731bd17..d8886124 100644 --- a/amivapi/tests/studydocs/test_rating.py +++ b/amivapi/tests/studydocs/test_rating.py @@ -12,7 +12,7 @@ class StudydocsRatingTest(WebTestNoAuth): """Test rating computation.""" def test_no_rating(self): - """Without any votes, the rating is None.""" + """Without any votes, the `rating` is in the document, and is None.""" doc = self.new_object('studydocuments') response = self.api.get('/studydocuments/' + str(doc['_id']), @@ -30,6 +30,9 @@ def test_bound(self): (5, 2, 0.47), (5, 5, 0.31), (50, 50, 0.44), + (0, 1, 0.0), + (0, 5, 0.0), + (0, 10, 0.0), ]: bound = lower_confidence_bound(upvotes, downvotes) self.assertAlmostEqual(rating, bound, 2) @@ -37,7 +40,7 @@ def test_bound(self): def test_rating(self): """Test that the rating is correctly attached to the studydoc.""" doc = self.new_object('studydocuments') - lookup = {'studydoc': str(doc['_id'])} + lookup = {'studydocument': str(doc['_id'])} response = self.api.get('/studydocuments/' + str(doc['_id']), status_code=200) @@ -46,9 +49,84 @@ def test_rating(self): self.load_fixture({ 'users': [{}], - 'studydocratings': [{'rating': 'up', **lookup}], + 'studydocumentratings': [{'rating': 'up', **lookup}], }) response = self.api.get('/studydocuments/' + str(doc['_id']), status_code=200) self.assertAlmostEqual(response.json['rating'], 0.38, 2) + + def test_sort_by_rating(self): + """Ensure that sorting by rating works.""" + ids = [24 * '0', 24 * '1', 24 * '2'] + self.load_fixture({ + 'users': [{}], + 'studydocuments': [ + {'_id': ids[0]}, {'_id': ids[1]}, {'_id': ids[2]} + ], + 'studydocumentratings': [ + # Rate such that: + # 0: No rating, 1: Low rating, 2: High rating + {'studydocument': ids[2], 'rating': 'up'}, + {'studydocument': ids[1], 'rating': 'down'}, + ], + }) + response = self.api.get('/studydocuments?sort=rating', status_code=200) + for (item, expected_id) in zip(response.json['_items'], ids): + self.assertEqual(item['_id'], expected_id) + + # Also test reverse order, to ensure that it was no accident + response = self.api.get('/studydocuments?sort=-rating', status_code=200) + for (item, expected_id) in zip(response.json['_items'], ids[::-1]): + self.assertEqual(item['_id'], expected_id) + + def _assert_rating(self, studydoc_id, rating): + response = self.api.get('/studydocuments/' + studydoc_id, + status_code=200) + self.assertAlmostEqual(response.json['rating'], rating, 2) + + def test_new_vote(self): + """POSTing a new vote updates the rating.""" + user = str(self.new_object('users')['_id']) + doc = str(self.new_object('studydocuments')['_id']) + self._assert_rating(doc, None) + + data = {'user': user, 'studydocument': doc, 'rating': 'up'} + self.api.post('/studydocumentratings', data=data, status_code=201) + self._assert_rating(doc, 0.38) + + def test_change_vote(self): + """PATCHing an existing vote updates the rating.""" + self.new_object('users') + doc = str(self.new_object('studydocuments')['_id']) + rating = self.new_object('studydocumentratings', rating='up') + self._assert_rating(doc, 0.38) + + headers = {'If-Match': rating['_etag']} + data = {'rating': 'down'} + self.api.patch('/studydocumentratings/' + str(rating['_id']), + data=data, headers=headers, status_code=200) + self._assert_rating(doc, 0.0) + + def test_delete_vote(self): + """DELETEing votes update the rating.""" + user_1, user_2 = self.load_fixture({'users': [{}, {}]}) + doc = str(self.new_object('studydocuments')['_id']) + rating_1, rating_2 = self.load_fixture({ + 'studydocumentratings': [ + {'user': str(user_1['_id']), 'rating': 'up'}, + {'user': str(user_2['_id']), 'rating': 'down'} + ] + }) + self._assert_rating(doc, 0.16) + + self.api.delete('/studydocumentratings/' + str(rating_2['_id']), + headers={'If-Match': rating_2['_etag']}, + status_code=204) + self._assert_rating(doc, 0.38) + + # After the last rating get's deleted, the rating is `None` again + self.api.delete('/studydocumentratings/' + str(rating_1['_id']), + headers={'If-Match': rating_1['_etag']}, + status_code=204) + self._assert_rating(doc, None) \ No newline at end of file From 8755537bd04cbb510f291b1adac740186353fc11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Mon, 3 Jun 2019 18:10:11 +0200 Subject: [PATCH 4/6] Improve security and add description. --- amivapi/studydocs/authorization.py | 15 ++++ amivapi/studydocs/model.py | 67 +++++++++++----- amivapi/tests/studydocs/test_rating.py | 101 ++++++++++++++++++++++++- 3 files changed, 163 insertions(+), 20 deletions(-) diff --git a/amivapi/studydocs/authorization.py b/amivapi/studydocs/authorization.py index b7cef767..41e50f3a 100644 --- a/amivapi/studydocs/authorization.py +++ b/amivapi/studydocs/authorization.py @@ -20,6 +20,21 @@ def has_resource_write_permission(self, user_id): return True +class StudydocratingsAuth(AmivTokenAuth): + def has_item_write_permission(self, user_id, item): + """Allow users to modify only their own ratings.""" + # item['user'] is Objectid, convert to str + return user_id == str(get_id(item['user'])) + + def create_user_lookup_filter(self, user_id): + """Allow users to only see their own ratings.""" + return {'user': user_id} + + def has_resource_write_permission(self, user_id): + # All users can rate studydocs + return True + + def add_uploader_on_insert(items): """Add the _author field before inserting studydocs""" for item in items: diff --git a/amivapi/studydocs/model.py b/amivapi/studydocs/model.py index 53e3e510..3d135313 100644 --- a/amivapi/studydocs/model.py +++ b/amivapi/studydocs/model.py @@ -4,9 +4,10 @@ # you to buy us beer if we meet and you like the software. """Model for studydocuments.""" -from amivapi.settings import DEPARTMENT_LIST +from flask import g -from .authorization import StudydocsAuth +from amivapi.settings import DEPARTMENT_LIST +from .authorization import StudydocsAuth, StudydocratingsAuth description = (""" @@ -23,22 +24,18 @@
-## Security +## Ratings -In addition to the usual -[permissions](#section/Authentication-and-Authorization/Authorization), -file uploaders have additional permissions: +Every **User** can rate study documents by either up- or downvoting, using the +[Study Document Ratings](#tag/Study-Document-Rating) endpoint. -- **Users** can modify all items they uploaded themselves (identified by the - user id in the `uploader` field). +Ratings are not simply averages, but take the number of votes into account. +Concretely, the rating is the lower bound of the wilson confidence interval. -- **Admins** can modify items for all users. +[You can read more here][1] if you are interested! -
+[1]: http://www.evanmiller.org/how-not-to-sort-by-average-rating.html -## Ratings - -TODO!
@@ -64,6 +61,26 @@ The summary is only computed for documents matching the current `where` query, e.g. when searching for ITET documents, only professors related to ITET documents will show up in the summary. + +
+ +## Security + +In addition to the usual +[permissions](#section/Authentication-and-Authorization/Authorization), +file uploaders have additional permissions, and rating access is restricted: + +- **Users** can modify all items they uploaded themselves (identified by the + user id in the `uploader` field). They can give ratings (only for themselves, + not for other users) and see their own ratings. + +- **Admins** can see and modify items and ratings for all users. + +""") + + +description_rating = (""" + """) @@ -73,6 +90,20 @@ class StudyDocValidator(object): def _validate_allow_summary(self, *args, **kwargs): """{'type': 'boolean'}""" + def _validate_only_self(self, enabled, field, value): + """Validate if the id can be used for a rating. + + Users can only sign up themselves + Moderators and admins can sign up everyone + + The rule's arguments are validated against this schema: + {'type': 'boolean'} + """ + user = g.get('current_user') + if enabled and not g.get('resource_admin') and (str(value) != user): + self._error(field, "You can only rate with your own id." + "(Your id: %s)" % (user)) + studydocdomain = { 'studydocuments': { @@ -222,17 +253,16 @@ def _validate_allow_summary(self, *args, **kwargs): 'resource_title': "Study Document Ratings", 'item_title': "Study Document Rating", - 'description': "Rating for Study documents (TODO)", + 'description': "A rating for a [Study Document](#tag/Study-Document).", 'resource_methods': ['GET', 'POST'], 'item_methods': ['GET', 'PATCH', 'DELETE'], - # TODO - 'authentication': StudydocsAuth, + 'authentication': StudydocratingsAuth, 'schema': { 'user': { - 'description': 'The rating user.', + 'description': 'The rating user. You can only use your own id.', 'example': '679ff66720812cdc2da4fb4a', 'type': 'objectid', @@ -243,12 +273,13 @@ def _validate_allow_summary(self, *args, **kwargs): }, 'not_patchable': True, 'required': True, + 'only_self': True, 'unique_combination': ['studydocument'], }, 'studydocument': { 'title': 'Study Document', - 'description': 'The rated study doc.', + 'description': 'The rated study document.', 'example': '10d8e50e303049ecb856ae9b', 'data_relation': { diff --git a/amivapi/tests/studydocs/test_rating.py b/amivapi/tests/studydocs/test_rating.py index d8886124..3da61e45 100644 --- a/amivapi/tests/studydocs/test_rating.py +++ b/amivapi/tests/studydocs/test_rating.py @@ -5,7 +5,7 @@ """Tests for studydocuments rating.""" from amivapi.studydocs.rating import lower_confidence_bound -from amivapi.tests.utils import WebTestNoAuth +from amivapi.tests.utils import WebTest, WebTestNoAuth class StudydocsRatingTest(WebTestNoAuth): @@ -129,4 +129,101 @@ def test_delete_vote(self): self.api.delete('/studydocumentratings/' + str(rating_1['_id']), headers={'If-Match': rating_1['_etag']}, status_code=204) - self._assert_rating(doc, None) \ No newline at end of file + self._assert_rating(doc, None) + + def test_rate_once(self): + """Every user can rate a document only once.""" + user = str(self.new_object('users')['_id']) + doc = str(self.new_object('studydocuments')['_id']) + + data = {'user': user, 'studydocument': doc, 'rating': 'up'} + self.api.post('/studydocumentratings', data=data, status_code=201) + self.api.post('/studydocumentratings', data=data, status_code=422) + + +class StudydocsRatingAuthTest(WebTest): + """Test rating permissions and visibility.""" + + def test_see_own(self): + """Users can only see their own ratings.""" + user_1, user_2 = self.load_fixture({'users': [{}, {}]}) + self.new_object('studydocuments') + rating_1, rating_2 = self.load_fixture({ + 'studydocumentratings': [ + {'user': str(user_1['_id']), 'rating': 'up'}, + {'user': str(user_2['_id']), 'rating': 'down'} + ] + }) + + token = self.get_user_token(rating_1['user']) + + # Resource + response = self.api.get('/studydocumentratings', + token=token, status_code=200) + ids = [item['_id'] for item in response.json['_items']] + self.assertIn(str(rating_1['_id']), ids) + self.assertNotIn(str(rating_2['_id']), ids) + + # Item + self.api.get('/studydocumentratings/' + str(rating_1['_id']), + token=token, status_code=200) + self.api.get('/studydocumentratings/' + str(rating_2['_id']), + token=token, status_code=404) + + def test_create(self): + """Users can only create ratings for themselves.""" + user_1, user_2 = self.load_fixture({'users': [{}, {}]}) + doc = str(self.new_object('studydocuments')['_id']) + + data_1 = {'studydocument': doc, 'user': str(user_1['_id']), + 'rating': 'up'} + data_2 = {'studydocument': doc, 'user': str(user_2['_id']), + 'rating': 'up'} + token = self.get_user_token(user_1['_id']) + self.api.post('/studydocumentratings', data=data_1, token=token, + status_code=201) + self.api.post('/studydocumentratings', data=data_2, token=token, + status_code=422) + + def test_modify_own(self): + """Users can only patch their own ratings.""" + user_1, user_2 = self.load_fixture({'users': [{}, {}]}) + self.new_object('studydocuments') + rating_1, rating_2 = self.load_fixture({ + 'studydocumentratings': [ + {'user': str(user_1['_id']), 'rating': 'down'}, + {'user': str(user_2['_id']), 'rating': 'down'} + ] + }) + + token = self.get_user_token(rating_1['user']) + updates = {'rating': 'up'} + + self.api.patch('/studydocumentratings/' + str(rating_1['_id']), + data=updates, token=token, status_code=200, + headers={'If-Match': rating_1['_etag']}) + # Cannot even see other rating, so a 404 is returned + self.api.patch('/studydocumentratings/' + str(rating_2['_id']), + data=updates, token=token, status_code=404, + headers={'If-Match': rating_2['_etag']}) + + def test_delete_own(self): + """Users can only delete their own ratings.""" + user_1, user_2 = self.load_fixture({'users': [{}, {}]}) + self.new_object('studydocuments') + rating_1, rating_2 = self.load_fixture({ + 'studydocumentratings': [ + {'user': str(user_1['_id']), 'rating': 'down'}, + {'user': str(user_2['_id']), 'rating': 'down'} + ] + }) + + token = self.get_user_token(rating_1['user']) + + self.api.delete('/studydocumentratings/' + str(rating_1['_id']), + token=token, status_code=204, + headers={'If-Match': rating_1['_etag']}) + # Cannot even see other rating, so a 404 is returned + self.api.delete('/studydocumentratings/' + str(rating_2['_id']), + token=token, status_code=404, + headers={'If-Match': rating_2['_etag']}) \ No newline at end of file From 35a460fe4a234f5e9c2e1aaa6a641a7765489855 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Mon, 3 Jun 2019 18:23:41 +0200 Subject: [PATCH 5/6] Polishing. --- amivapi/studydocs/rating.py | 11 +++++------ amivapi/tests/studydocs/test_rating.py | 6 +++--- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/amivapi/studydocs/rating.py b/amivapi/studydocs/rating.py index 5119462e..54fff3c0 100644 --- a/amivapi/studydocs/rating.py +++ b/amivapi/studydocs/rating.py @@ -18,8 +18,10 @@ from amivapi.utils import get_id -def lower_confidence_bound(upvotes, downvotes, z=1.28): - """Compute the lower bound of the wilson confidence interval is returned, +def compute_rating(upvotes, downvotes, z=1.28): + """Compute the rating. + + Concretely, the lower bound of the wilson confidence interval is returned, which takes the number of votes into account. [1] We use z = 1.28 by default, which corresponds to a 80% confidence interval @@ -51,7 +53,7 @@ def _update_rating(studydoc_id): downvotes = ratings.count_documents({'rating': 'down', **lookup}) # Compute rating and write to database - rating = lower_confidence_bound(upvotes, downvotes) + rating = compute_rating(upvotes, downvotes) docs.update_one({'_id': studydoc_id}, {'$set': {'rating': rating}}) @@ -59,9 +61,6 @@ def init_rating(items): """On creating of a study-document, set the rating to None.""" for item in items: item['rating'] = None - # lookup = {'_id': {'$in': [get_id(item['_id']) for item in items]}} - # updates = {'$set': {'rating': None}} - # current_app.data.driver.db['studydocuments'].update_many(lookup, updates) def update_rating_post(items): diff --git a/amivapi/tests/studydocs/test_rating.py b/amivapi/tests/studydocs/test_rating.py index 3da61e45..df11763a 100644 --- a/amivapi/tests/studydocs/test_rating.py +++ b/amivapi/tests/studydocs/test_rating.py @@ -4,7 +4,7 @@ # you to buy us beer if we meet and you like the software. """Tests for studydocuments rating.""" -from amivapi.studydocs.rating import lower_confidence_bound +from amivapi.studydocs.rating import compute_rating from amivapi.tests.utils import WebTest, WebTestNoAuth @@ -34,7 +34,7 @@ def test_bound(self): (0, 5, 0.0), (0, 10, 0.0), ]: - bound = lower_confidence_bound(upvotes, downvotes) + bound = compute_rating(upvotes, downvotes) self.assertAlmostEqual(rating, bound, 2) def test_rating(self): @@ -226,4 +226,4 @@ def test_delete_own(self): # Cannot even see other rating, so a 404 is returned self.api.delete('/studydocumentratings/' + str(rating_2['_id']), token=token, status_code=404, - headers={'If-Match': rating_2['_etag']}) \ No newline at end of file + headers={'If-Match': rating_2['_etag']}) From f23a688ce56321f3fd6cedfdd744d3c331b94f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Dietm=C3=BCller?= Date: Wed, 5 Jun 2019 14:52:35 +0200 Subject: [PATCH 6/6] Remove test stubs (they are part of the rating tests now). --- amivapi/tests/studydocs/test_studydocs_auth.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/amivapi/tests/studydocs/test_studydocs_auth.py b/amivapi/tests/studydocs/test_studydocs_auth.py index c1d9669a..56b9b85e 100644 --- a/amivapi/tests/studydocs/test_studydocs_auth.py +++ b/amivapi/tests/studydocs/test_studydocs_auth.py @@ -88,16 +88,3 @@ def test_can_see_all_docs(self): docs = self.api.get('/studydocuments', token=token, status_code=200) self.assertEqual(len(docs.json['_items']), 5) - - -class StudyDocRatingAuthTest(WebTest): - """Test for rating authentication.""" - - def test_own_ratings(self): - ... - - def test_other_ratings(self): - ... - - def test_admin_ratings(self): - ...