diff --git a/docs/topics/api/overview.rst b/docs/topics/api/overview.rst index 91ae868131..f416235444 100644 --- a/docs/topics/api/overview.rst +++ b/docs/topics/api/overview.rst @@ -289,3 +289,4 @@ v4 API changelog * 2018-10-25: added ``fxa_edit_email_url`` parameter on accounts API to return the full URL for editing the user's email on FxA. https://github.com/mozilla/addons-server/issues/8674 * 2018-10-31: added ``id`` to discovery API ``addons.current_version`` object. This change was also backported to the `v3` API. https://github.com/mozilla/addons-server/issues/9855 * 2018-11-15: added ``is_custom`` to the license object in version detail output in the addons API. +* 2018-11-22: added ``flags`` to the rating object in the ratings API when ``show_flags_for`` parameter supplied. diff --git a/docs/topics/api/ratings.rst b/docs/topics/api/ratings.rst index 317b584595..bb81cc66ff 100644 --- a/docs/topics/api/ratings.rst +++ b/docs/topics/api/ratings.rst @@ -27,6 +27,7 @@ user has already posted a rating for the current version of an add-on. .. http:get:: /api/v4/ratings/rating/ :query string addon: The :ref:`add-on ` id, slug, or guid to fetch ratings from. When passed, the ratings shown will always be the latest posted by each user on this particular add-on (which means there should only be one rating per user in the results), unless the ``version`` parameter is also passed. + :query string show_flags_for: The user id to show flags for. If the request is made with an authenticated user matching this parameter value, a ``flags`` property will be added to the response as described below in :ref:`ratings `. :query string show_permissions_for: The user id to show permissions for. If the request is made with an authenticated user matching this parameter value, and the ``addon`` parameter is also present, a ``can_reply`` property will be added to the response as described below. :query string filter: The :ref:`filter(s) ` to apply. :query int user: The user id to fetch ratings from. @@ -78,12 +79,16 @@ This endpoint allows you to fetch a rating by its id. .. _rating-detail-object: + :query string show_flags_for: The user id to show flags for. If the request is made with an authenticated user matching this parameter value, a ``flags`` property will be added to the response as described below. :>json int id: The rating id. :>json object addon: A simplified :ref:`add-on ` object that contains only a few properties: ``id``, ``name``, ``icon_url`` and ``slug``. :>json string|null body: The text of the rating. :>json boolean is_deleted: Boolean indicating whether the rating has been deleted or not. :>json boolean is_latest: Boolean indicating whether the rating is the latest posted by the user on the same add-on. :>json int previous_count: The number of ratings posted by the user on the same add-on before this one. + :>json object flags[]: A list of flags the user requesting has previously applied to this rating (that haven't been processed by moderators already). Only present if ``show_flags_for`` parameter sent. + :>json string flags.flag: A :ref:`constant` describing the reason behind the flagging. + :>json string|null flags.note: A note to explain further the reason behind the flagging if ``flag`` was ``rating_flag_reason_other``; null otherwise. :>json int score: The score the user gave as part of the rating. :>json object|null reply: The rating object containing the developer reply to this rating, if any (The fields ``rating``, ``reply`` and ``version`` are omitted). :>json int version.id: The add-on version id the rating applies to. diff --git a/src/olympia/lib/settings_base.py b/src/olympia/lib/settings_base.py index 6f77dca10d..7fc7216a23 100644 --- a/src/olympia/lib/settings_base.py +++ b/src/olympia/lib/settings_base.py @@ -1775,6 +1775,7 @@ DRF_API_GATES = { 'del-addons-created-field', 'del-accounts-fxa-edit-email-url', 'del-version-license-is-custom', + 'del-ratings-flags', ), 'v4': ( 'l10n_flat_input_output', diff --git a/src/olympia/ratings/serializers.py b/src/olympia/ratings/serializers.py index 10365c5b2f..f58c69a807 100644 --- a/src/olympia/ratings/serializers.py +++ b/src/olympia/ratings/serializers.py @@ -33,10 +33,11 @@ class BaseRatingSerializer(serializers.ModelSerializer): is_latest = serializers.BooleanField(read_only=True) previous_count = serializers.IntegerField(read_only=True) user = BaseUserSerializer(read_only=True) + flags = serializers.SerializerMethodField() class Meta: model = Rating - fields = ('id', 'addon', 'body', 'created', 'is_deleted', + fields = ('id', 'addon', 'body', 'created', 'flags', 'is_deleted', 'is_developer_reply', 'is_latest', 'previous_count', 'user') def __init__(self, *args, **kwargs): @@ -90,10 +91,22 @@ class BaseRatingSerializer(serializers.ModelSerializer): return data + def get_flags(self, obj): + if self.context['view'].should_include_flags(): + # should be maximum one RatingFlag per rating+user anyway. + rating_flags = obj.ratingflag_set.filter( + user=self.context['request'].user) + return [ + {'flag': flag.flag, 'note': flag.note or None} + for flag in rating_flags] + return None + def to_representation(self, instance): out = super(BaseRatingSerializer, self).to_representation(instance) if self.request and is_gate_active(self.request, 'ratings-title-shim'): out['title'] = None + if not self.context['view'].should_include_flags(): + out.pop('flags', None) return out @@ -160,7 +173,7 @@ class RatingSerializer(BaseRatingSerializer): is_gate_active(self.request, 'ratings-rating-shim')) if score_to_rating: score_field = self.fields.pop('score') - score_field.source = None # drf complains if we specifiy source. + score_field.source = None # drf complains if we specify source. self.fields['rating'] = score_field def validate_version(self, version): diff --git a/src/olympia/ratings/tests/test_serializers.py b/src/olympia/ratings/tests/test_serializers.py index c9980c049f..dabbecd93f 100644 --- a/src/olympia/ratings/tests/test_serializers.py +++ b/src/olympia/ratings/tests/test_serializers.py @@ -6,15 +6,16 @@ from rest_framework.test import APIRequestFactory from olympia.amo.templatetags.jinja_helpers import absolutify from olympia.amo.tests import TestCase, addon_factory, user_factory -from olympia.ratings.models import Rating +from olympia.ratings.models import Rating, RatingFlag from olympia.ratings.serializers import RatingSerializer class TestBaseRatingSerializer(TestCase): def setUp(self): self.request = APIRequestFactory().get('/') - self.view = Mock(spec=['get_addon_object']) + self.view = Mock(spec=['get_addon_object', 'should_include_flags']) self.view.get_addon_object.return_value = None + self.view.should_include_flags.return_value = False self.user = user_factory() def serialize(self, **extra_context): @@ -67,6 +68,8 @@ class TestBaseRatingSerializer(TestCase): assert result['version'] is None # Check the default, when DRF_API_GATES['ratings-title-shim'] isn't set assert 'title' not in result + # Check the default, when `show_flags_for=...` isn't sent. + assert 'flags' not in result def test_deleted_rating_but_view_allowing_it_to_be_shown(self): # We don't need to change self.view.should_access_deleted_ratings @@ -293,3 +296,23 @@ class TestBaseRatingSerializer(TestCase): assert serializer.fields['id'].read_only is True assert serializer.fields['reply'].read_only is True assert serializer.fields['user'].read_only is True + + def test_include_flags(self): + addon = addon_factory() + self.request.user = user_factory() + self.view.get_addon_object.return_value = addon + self.view.should_include_flags.return_value = True + self.rating = Rating.objects.create( + addon=addon, user=self.user, rating=4, + version=addon.current_version, body=u'This is my rëview. Like ît?') + + result = self.serialize() + assert 'flags' in result + assert result['flags'] == [] + + RatingFlag.objects.create( + rating=self.rating, user=self.request.user, flag=RatingFlag.OTHER, + note=u'foo') + result = self.serialize() + assert 'flags' in result + assert result['flags'] == [{'flag': RatingFlag.OTHER, 'note': 'foo'}] diff --git a/src/olympia/ratings/tests/test_views.py b/src/olympia/ratings/tests/test_views.py index fec1465bd4..93b235d59e 100644 --- a/src/olympia/ratings/tests/test_views.py +++ b/src/olympia/ratings/tests/test_views.py @@ -823,6 +823,11 @@ class TestRatingViewSetGet(TestCase): if 'show_grouped_ratings' not in kwargs: assert 'grouped_ratings' not in data + + if 'show_for' not in kwargs: + assert 'flags' not in data['results'][0] + assert 'flags' not in data['results'][1] + return data def test_list_show_permission_for_anonymous(self): @@ -1301,6 +1306,92 @@ class TestRatingViewSetGet(TestCase): data = json.loads(response.content) assert data['detail'] == 'Need an addon or user parameter' + def test_list_show_flags_for_anonymous(self): + response = self.client.get( + self.url, {'addon': self.addon.pk, 'show_flags_for': 666}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_list_show_flags_for_not_int(self): + response = self.client.get( + self.url, {'addon': self.addon.pk, 'show_flags_for': 'nope'}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_list_show_flags_for_not_right_user(self): + self.user = user_factory() + self.client.login_api(self.user) + response = self.client.get( + self.url, {'addon': self.addon.pk, + 'show_flags_for': self.user.pk + 42}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_list_rating_flags(self): + self.user = user_factory() + self.client.login_api(self.user) + rating1 = Rating.objects.create( + addon=self.addon, body='review 1', user=user_factory(), + rating=2) + rating0 = Rating.objects.create( + addon=self.addon, body='review 0', user=user_factory(), + rating=1) + reply_to_0 = Rating.objects.create( + addon=self.addon, body='reply to review 0', reply_to=rating0, + user=user_factory()) + params = {'addon': self.addon.pk, 'show_flags_for': self.user.pk} + + # First, not flagged + response = self.client.get(self.url, params) + assert response.status_code == 200 + data = json.loads(response.content) + assert data['results'][0]['flags'] == [] + assert data['results'][0]['reply']['flags'] == [] + assert data['results'][1]['flags'] == [] + + # then add some RatingFlag - one for a rating, the other a reply + RatingFlag.objects.create( + rating=rating1, user=self.user, flag=RatingFlag.LANGUAGE) + RatingFlag.objects.create( + rating=reply_to_0, user=self.user, flag=RatingFlag.OTHER, + note=u'foo') + + response = self.client.get(self.url, params) + assert response.status_code == 200 + data = json.loads(response.content) + rating0 = data['results'][0] + rating1 = data['results'][1] + assert 'flags' in rating0 + assert 'flags' in rating1 + assert 'flags' in rating0['reply'] + assert rating0['flags'] == [] + assert rating0['reply']['flags'] == [ + {'flag': RatingFlag.OTHER, 'note': 'foo'}] + assert rating1['flags'] == [ + {'flag': RatingFlag.LANGUAGE, 'note': None}] + + def test_list_rating_flags_absent_in_v3(self): + self.user = user_factory() + self.client.login_api(self.user) + rating = Rating.objects.create( + addon=self.addon, body='review', user=user_factory(), + rating=1) + RatingFlag.objects.create( + rating=rating, user=self.user, flag=RatingFlag.OTHER, + note=u'foo') + params = {'addon': self.addon.pk, 'show_flags_for': self.user.pk} + response = self.client.get( + reverse_ns('rating-list', api_version='v3'), params) + assert response.status_code == 200 + data = json.loads(response.content) + assert 'flags' not in data['results'][0] + def test_detail(self): review = Rating.objects.create( addon=self.addon, body='review 1', user=user_factory()) @@ -1368,6 +1459,83 @@ class TestRatingViewSetGet(TestCase): assert data['reply'] assert data['reply']['id'] == reply.pk + def test_detail_show_flags_for_anonymous(self): + rating = Rating.objects.create( + addon=self.addon, body='review', user=user_factory()) + detail_url = reverse_ns(self.detail_url_name, kwargs={'pk': rating.pk}) + response = self.client.get(detail_url, {'show_flags_for': 666}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_detail_show_flags_for_not_int(self): + rating = Rating.objects.create( + addon=self.addon, body='review', user=user_factory()) + detail_url = reverse_ns(self.detail_url_name, kwargs={'pk': rating.pk}) + response = self.client.get(detail_url, {'show_flags_for': 'nope'}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_detail_show_flags_for_not_right_user(self): + self.user = user_factory() + self.client.login_api(self.user) + rating = Rating.objects.create( + addon=self.addon, body='review', user=user_factory()) + detail_url = reverse_ns(self.detail_url_name, kwargs={'pk': rating.pk}) + response = self.client.get( + detail_url, {'show_flags_for': self.user.pk + 42}) + assert response.status_code == 400 + assert response.data['detail'] == ( + 'show_flags_for parameter value should be equal to the user ' + 'id of the authenticated user') + + def test_detail_rating_flags(self): + self.user = user_factory() + self.client.login_api(self.user) + rating = Rating.objects.create( + addon=self.addon, body='review 1', user=user_factory(), + rating=2) + + detail_url = reverse_ns(self.detail_url_name, kwargs={'pk': rating.pk}) + params = {'show_flags_for': self.user.pk} + + # First, not flagged + response = self.client.get(detail_url, params) + assert response.status_code == 200 + data = json.loads(response.content) + assert data['flags'] == [] + + # then add some RatingFlag - one for a rating, the other a reply + RatingFlag.objects.create( + rating=rating, user=self.user, flag=RatingFlag.LANGUAGE) + + response = self.client.get(detail_url, params) + assert response.status_code == 200 + data = json.loads(response.content) + assert 'flags' in data + assert data['flags'] == [ + {'flag': RatingFlag.LANGUAGE, 'note': None}] + + def test_detail_rating_flags_absent_in_v3(self): + self.user = user_factory() + self.client.login_api(self.user) + rating = Rating.objects.create( + addon=self.addon, body='review', user=user_factory(), + rating=1) + RatingFlag.objects.create( + rating=rating, user=self.user, flag=RatingFlag.OTHER, + note=u'foo') + detail_url = reverse_ns( + self.detail_url_name, kwargs={'pk': rating.pk}, api_version='v3') + params = {'show_flags_for': self.user.pk} + response = self.client.get(detail_url, params) + assert response.status_code == 200 + data = json.loads(response.content) + assert 'flags' not in data + def test_list_by_admin_does_not_show_deleted_by_default(self): self.user = user_factory() self.grant_permission(self.user, 'Addons:Edit') diff --git a/src/olympia/ratings/views.py b/src/olympia/ratings/views.py index 34af21b5e0..d7b56fac29 100644 --- a/src/olympia/ratings/views.py +++ b/src/olympia/ratings/views.py @@ -313,6 +313,27 @@ class RatingViewSet(AddonChildMixin, ModelViewSet): return super(RatingViewSet, self).get_addon_object( permission_classes=[AllowIfPublic]) + def should_include_flags(self): + if not hasattr(self, '_should_include_flags'): + request = self.request + self._should_include_flags = ( + 'show_flags_for' in request.GET and + not is_gate_active(request, 'del-ratings-flags') + ) + if self._should_include_flags: + # Check the parameter was sent correctly + try: + show_flags_for = ( + serializers.IntegerField().to_internal_value( + request.GET['show_flags_for'])) + if show_flags_for != request.user.pk: + raise serializers.ValidationError + except serializers.ValidationError: + raise ParseError( + 'show_flags_for parameter value should be equal to ' + 'the user id of the authenticated user') + return self._should_include_flags + def check_permissions(self, request): """Perform permission checks. @@ -412,6 +433,9 @@ class RatingViewSet(AddonChildMixin, ModelViewSet): 'the user id of the authenticated user') extra_data['can_reply'] = ( self.check_can_reply_permission_for_ratings_list()) + # Call this here so the validation checks on the `show_flags_for` are + # carried out even when there are no results to serialize. + self.should_include_flags() response = super(RatingViewSet, self).get_paginated_response(data) if extra_data: response.data.update(extra_data)