Merge pull request #3357 from diox/add-ratings-api

Add API to list add-on reviews for an add-on
This commit is contained in:
Mathieu Pillard 2016-08-30 13:50:01 +02:00 коммит произвёл GitHub
Родитель 46ddaff913 f86a1c8bc8
Коммит f9395189c3
18 изменённых файлов: 500 добавлений и 53 удалений

Просмотреть файл

@ -235,6 +235,7 @@ nitpick_ignore = [
('http:obj', 'int'),
('http:obj', 'float'),
('http:obj', 'object'),
('http:obj', 'object|null'),
('http:obj', 'string'),
('http:obj', 'string|object|null'),
('http:obj', 'string|null'),

Просмотреть файл

@ -38,5 +38,6 @@ using the API.
discovery
download_sources
internal
reviews
signing
stats

Просмотреть файл

@ -0,0 +1,46 @@
=======
Reviews
=======
.. note::
These APIs are experimental and are currently being worked on. Endpoints
may change without warning. The only authentication method available at
the moment is :ref:`the internal one<api-auth-internal>`.
----
List
----
.. review-list:
This endpoint allows you to fetch user reviews for a given add-on.
.. http:get:: /api/v3/addons/addon/(int:id|string:slug|string:guid)/reviews/
:>json int count: The number of results for this query.
:>json string next: The URL of the next page of results.
:>json string previous: The URL of the previous page of results.
:>json array results: An array of :ref:`reviews <review-detail-object>`.
------
Detail
------
.. review-detail:
This endpoint allows you to fetch a user review by id.
.. http:get:: /api/v3/addons/addon/(int:id|string:slug|string:guid)/reviews/(int:id)/
.. _review-detail-object:
:>json int id: The review id.
:>json string|null body: The text of the review.
:>json string|null: The title of the review.
:>json int rating: The rating the user gave as part of the review.
:>json object|null reply: The review object containing the developer reply to this review, if any (The fields ``rating`` and ``reply`` are omitted).
:>json string version: The add-on version string the review applies to.
:>json object user: Object holding information about the user who posted the review.
:>json string user.url: The user profile URL.
:>json string user.name: The user name.

Просмотреть файл

@ -3,25 +3,24 @@ from django.conf.urls import include, patterns, url
from rest_framework.routers import SimpleRouter
from rest_framework_nested.routers import NestedSimpleRouter
from . import views
from olympia.reviews.views import ReviewViewSet
from .views import (
AddonFeaturedView, AddonSearchView, AddonVersionViewSet, AddonViewSet)
addons = SimpleRouter()
addons.register(r'addon', views.AddonViewSet)
addons.register(r'addon', AddonViewSet)
# Router for children of /addons/addon/{addon_pk}/.
sub_addons = NestedSimpleRouter(addons, r'addon', lookup='addon')
sub_addons.register(r'versions', views.AddonVersionViewSet,
base_name='addon-version')
sub_addons.register('versions', AddonVersionViewSet, base_name='addon-version')
sub_addons.register('reviews', ReviewViewSet, base_name='addon-review')
urlpatterns = patterns(
'',
url(r'', include(addons.urls)),
url(r'', include(sub_addons.urls)),
url(r'^search/$',
views.AddonSearchView.as_view(),
name='addon-search'),
url(r'^featured/$',
views.AddonFeaturedView.as_view(),
name='addon-featured'),
url(r'^search/$', AddonSearchView.as_view(), name='addon-search'),
url(r'^featured/$', AddonFeaturedView.as_view(), name='addon-featured'),
)

Просмотреть файл

@ -12,6 +12,7 @@ from olympia.constants.applications import APPS_ALL
from olympia.constants.categories import CATEGORIES_BY_ID
from olympia.files.models import File
from olympia.users.models import UserProfile
from olympia.users.serializers import BaseUserSerializer
from olympia.versions.models import ApplicationsVersions, License, Version
@ -24,17 +25,6 @@ class AddonFeatureCompatibilitySerializer(serializers.ModelSerializer):
fields = ('e10s', )
class AddonAuthorSerializer(serializers.ModelSerializer):
url = serializers.SerializerMethodField()
class Meta:
model = UserProfile
fields = ('name', 'url')
def get_url(self, obj):
return absolutify(obj.get_url_path())
class FileSerializer(serializers.ModelSerializer):
url = serializers.SerializerMethodField()
platform = ReverseChoiceField(choices=amo.PLATFORM_CHOICES_API.items())
@ -143,7 +133,7 @@ class AddonEulaPolicySerializer(serializers.ModelSerializer):
class AddonSerializer(serializers.ModelSerializer):
authors = AddonAuthorSerializer(many=True, source='listed_authors')
authors = BaseUserSerializer(many=True, source='listed_authors')
categories = serializers.SerializerMethodField()
current_beta_version = SimpleVersionSerializer()
current_version = SimpleVersionSerializer()

Просмотреть файл

@ -1548,10 +1548,10 @@ class AddonAndVersionViewSetDetailMixin(object):
"""Tests that play with addon state and permissions. Shared between addon
and version viewset detail tests since both need to react the same way."""
def _test_url(self):
raise NotImplemented
raise NotImplementedError
def _set_tested_url(self, param):
raise NotImplemented
raise NotImplementedError
def test_get_by_id(self):
self._test_url()

Просмотреть файл

@ -634,20 +634,22 @@ class AddonViewSet(RetrieveModelMixin, GenericViewSet):
return Addon.unfiltered.all()
return super(AddonViewSet, self).get_queryset()
def get_object(self):
value = self.kwargs.get('pk')
if value and not value.isdigit():
# If the value contains anything other than a digit, it's either
# a slug or a guid. guids need to contain either {} or @, which are
# invalid in a slug.
if self.addon_id_pattern.match(value):
self.lookup_field = 'guid'
def get_lookup_field(self, identifier):
lookup_field = 'pk'
if identifier and not identifier.isdigit():
# If the identifier contains anything other than a digit, it's
# either a slug or a guid. guids need to contain either {} or @,
# which are invalid in a slug.
if self.addon_id_pattern.match(identifier):
lookup_field = 'guid'
else:
self.lookup_field = 'slug'
self.kwargs.update({
'pk': None,
self.lookup_field: value
})
lookup_field = 'slug'
return lookup_field
def get_object(self):
identifier = self.kwargs.get('pk')
self.lookup_field = self.get_lookup_field(identifier)
self.kwargs[self.lookup_field] = identifier
return super(AddonViewSet, self).get_object()
@detail_route()

Просмотреть файл

@ -53,8 +53,11 @@ class ESPaginator(Paginator):
return page
class ESPageNumberPagination(PageNumberPagination):
"""Custom pagination implementation to hook in our `ESPaginator`."""
class CustomPageNumberPagination(PageNumberPagination):
page_size_query_param = 'page_size'
django_paginator_class = ESPaginator
max_page_size = 50
class ESPageNumberPagination(CustomPageNumberPagination):
"""Custom pagination implementation to hook in our `ESPaginator`."""
django_paginator_class = ESPaginator

Просмотреть файл

@ -1,3 +1,5 @@
from collections import defaultdict
from rest_framework.permissions import BasePermission, SAFE_METHODS
from olympia.access import acl
@ -56,6 +58,15 @@ class AnyOf(BasePermission):
return self
class AllowNone(BasePermission):
def has_permission(self, request, view):
return False
def has_object_permission(self, request, view, obj):
return False
class AllowAddonAuthor(BasePermission):
"""Allow access if the user is in the object authors."""
def has_permission(self, request, view):
@ -115,3 +126,34 @@ class AllowReadOnlyIfReviewedAndListed(BasePermission):
def has_object_permission(self, request, view, obj):
return (obj.is_reviewed() and not obj.disabled_by_user and
obj.is_listed and self.has_permission(request, view))
class ByHttpMethod(BasePermission):
"""
Permission class allowing you to define different permissions depending on
the HTTP method used.
method_permission is a dict with the lowercase http method names as keys,
permission classes (not instantiated, like DRF expects them) as values.
Warning: you probably want to define AllowAny for 'options' if you are
using a CORS-enabled endpoint.
"""
def __init__(self, method_permissions, default=None):
if default is None:
default = AllowNone()
self.method_permissions = defaultdict(lambda: default)
for method, perm in method_permissions.items():
# Initialize the permissions by calling them like DRF does.
self.method_permissions[method] = perm()
def has_permission(self, request, view):
perm = self.method_permissions[request.method.lower()]
return perm.has_permission(request, view)
def has_object_permission(self, request, view, obj):
perm = self.method_permissions[request.method.lower()]
return perm.has_object_permission(request, view, obj)
def __call__(self):
return self

Просмотреть файл

@ -10,8 +10,8 @@ from rest_framework.views import APIView
from olympia.access.models import Group, GroupUser
from olympia.addons.models import Addon
from olympia.api.permissions import (
AllowAddonAuthor, AllowReadOnlyIfReviewedAndListed, AllowReviewer,
AllowReviewerUnlisted, AnyOf, GroupPermission)
AllowAddonAuthor, AllowNone, AllowReadOnlyIfReviewedAndListed,
AllowReviewer, AllowReviewerUnlisted, AnyOf, ByHttpMethod, GroupPermission)
from olympia.amo.tests import TestCase, WithDynamicEndpoints
from olympia.users.models import UserProfile
@ -26,15 +26,6 @@ class ProtectedView(APIView):
return Response('ok')
class AllowNone(BasePermission):
"""A permission class that never allows access, for testing."""
def has_permission(self, request, view):
return False
def has_object_permission(self, request, view, obj):
return False
def myview(*args, **kwargs):
pass
@ -81,6 +72,16 @@ class TestGroupPermission(TestCase):
assert not perm.has_permission(request, view)
class TestAllowNone(TestCase):
def test_has_permission(self):
request = RequestFactory().get('/')
assert not AllowNone().has_permission(request, myview)
def test_has_object_permission(self):
request = RequestFactory().get('/')
assert not AllowNone().has_object_permission(request, myview, None)
class TestAnyOf(TestCase):
def test_has_permission(self):
request = RequestFactory().get('/')
@ -343,3 +344,52 @@ class TestAllowReadOnlyIfReviewedAndListed(TestCase):
for verb in self.unsafe_methods + self.safe_methods:
assert not self.permission.has_object_permission(
self.request(verb), myview, obj)
class TestByHttpMethod(TestCase):
def setUp(self):
self.get_permission = Mock
self.patch_permission = Mock
self.post_permission = Mock
self.put_permission = Mock
self.permission = ByHttpMethod({
'get': self.get_permission,
})
self.set_permission_mock('get', True)
def set_permission_mock(self, method, value):
mock = self.permission.method_permissions[method]
mock.has_permission.return_value = value
def set_object_permission_mock(self, method, value):
mock = self.permission.method_permissions[method]
mock.has_object_permission.return_value = value
def test_get(self):
self.request = RequestFactory().get('/')
assert self.permission.has_permission(self.request, 'myview') is True
self.set_permission_mock('get', False)
assert self.permission.has_permission(self.request, 'myview') is False
def test_get_obj(self):
obj = Mock()
self.request = RequestFactory().get('/')
self.set_object_permission_mock('get', True)
assert self.permission.has_object_permission(
self.request, 'myview', obj) is True
self.set_object_permission_mock('get', False)
assert self.permission.has_object_permission(
self.request, 'myview', obj) is False
def test_missing_method(self):
self.request = RequestFactory().post('/')
assert self.permission.has_permission(self.request, 'myview') is False
obj = Mock()
self.request = RequestFactory().post('/')
assert self.permission.has_object_permission(
self.request, 'myview', obj) is False
self.request = RequestFactory().options('/')
assert self.permission.has_permission(self.request, 'myview') is False

Просмотреть файл

@ -1629,6 +1629,11 @@ REST_FRAMEWORK = {
# Enable pagination
'PAGE_SIZE': 25,
# Use our pagination class by default, which allows clients to request a
# different page size.
'DEFAULT_PAGINATION_CLASS': (
'olympia.api.paginator.CustomPageNumberPagination')
}
# This is the DSN to the local Sentry service. It might be overridden in

Просмотреть файл

@ -12,6 +12,7 @@ from olympia.amo.models import ManagerBase, ModelBase
from olympia.amo import helpers
from olympia.amo.celery import task
from olympia.translations.fields import save_signal, TranslatedField
from olympia.translations.helpers import truncate
from olympia.users.models import UserProfile
log = logging.getLogger('z.review')
@ -93,6 +94,11 @@ class Review(ModelBase):
db_table = 'reviews'
ordering = ('-created',)
def __unicode__(self):
if self.title:
return unicode(self.title)
return truncate(unicode(self.body), 10)
def get_url_path(self):
return helpers.url('addons.reviews.detail', self.addon.slug, self.id)

Просмотреть файл

@ -0,0 +1,33 @@
from rest_framework import serializers
from olympia.reviews.models import Review
from olympia.users.serializers import BaseUserSerializer
class BaseReviewSerializer(serializers.ModelSerializer):
# title and body are TranslatedFields, but there is never more than one
# translation for each review - it's essentially useless. Because of that
# we use a simple CharField in the API, hiding the fact that it's a
# TranslatedField underneath.
body = serializers.CharField()
title = serializers.CharField()
user = BaseUserSerializer()
class Meta:
model = Review
fields = ('id', 'body', 'title', 'version', 'user')
def to_representation(self, obj):
data = super(BaseReviewSerializer, self).to_representation(obj)
# For the version, we want to accept PKs for input, but use the version
# string for output.
data['version'] = unicode(obj.version.version) if obj.version else None
return data
class ReviewSerializer(BaseReviewSerializer):
reply = BaseReviewSerializer(read_only=True)
class Meta:
model = Review
fields = BaseReviewSerializer.Meta.fields + ('rating', 'reply')

Просмотреть файл

@ -0,0 +1,64 @@
# -*- coding: utf-8 -*-
from rest_framework.test import APIRequestFactory
from olympia.amo.helpers import absolutify
from olympia.amo.tests import addon_factory, TestCase, user_factory
from olympia.reviews.models import Review
from olympia.reviews.serializers import ReviewSerializer
class TestBaseReviewSerializer(TestCase):
def setUp(self):
self.request = APIRequestFactory().get('/')
self.user = user_factory()
def serialize(self):
serializer = ReviewSerializer(context={'request': self.request})
return serializer.to_representation(self.review)
def test_basic(self):
addon = addon_factory()
self.review = Review.objects.create(
addon=addon, user=self.user, rating=4,
version=addon.current_version, body=u'This is my rëview. Like ît?',
title=u'My Review Titlé')
result = self.serialize()
assert result['id'] == self.review.pk
assert result['body'] == unicode(self.review.body)
assert result['title'] == unicode(self.review.title)
assert result['rating'] == int(self.review.rating)
assert result['reply'] is None
assert result['user'] == {
'name': unicode(self.user.name),
'url': absolutify(self.user.get_url_path()),
}
assert result['version'] == self.review.version.version
self.review.update(version=None)
result = self.serialize()
assert result['version'] is None
def test_with_reply(self):
addon = addon_factory()
reply_user = user_factory()
self.review = Review.objects.create(
addon=addon, user=self.user, version=addon.current_version,
body=u'This is my rëview. Like ît ?', title=u'My Review Titlé')
reply = Review.objects.create(
addon=addon, user=reply_user, version=addon.current_version,
body=u'Thîs is a reply.', title=u'My rèply', reply_to=self.review)
result = self.serialize()
assert result['reply']
assert 'rating' not in result['reply']
assert 'reply' not in result['reply']
assert result['reply']['id'] == reply.pk
assert result['reply']['body'] == unicode(reply.body)
assert result['reply']['title'] == unicode(reply.title)
assert result['reply']['user'] == {
'name': unicode(reply_user.name),
'url': absolutify(reply_user.get_url_path()),
}
assert result['reply']['version'] == reply.version.version

Просмотреть файл

@ -1,14 +1,19 @@
# -*- coding: utf-8 -*-
import json
from pyquery import PyQuery as pq
from django.core.urlresolvers import reverse
import mock
from pyquery import PyQuery as pq
from rest_framework.exceptions import ParseError
from olympia.amo.tests import TestCase, MobileTest
from olympia.addons.utils import generate_addon_guid
from olympia.amo import helpers
from olympia.amo.tests import addon_factory, TestCase, MobileTest, user_factory
from olympia.access.models import Group, GroupUser
from olympia.addons.models import Addon, AddonUser
from olympia.devhub.models import ActivityLog
from olympia.reviews.views import ReviewViewSet
from olympia.reviews.models import Review, ReviewFlag
from olympia.users.models import UserProfile
@ -585,3 +590,114 @@ class TestMobileReviews(MobileTest, TestCase):
self.mobile_init()
r = self.client.get(helpers.url('addons.reviews.add', self.addon.slug))
assert r.status_code == 302
class TestReviewViewSetGet(TestCase):
def setUp(self):
self.addon = addon_factory(
guid=generate_addon_guid(), name=u'My Addôn', slug='my-addon')
self.url = reverse(
'addon-review-list', kwargs={'addon_pk': self.addon.pk})
def test_list(self):
review1 = Review.objects.create(
addon=self.addon, body='review 1', user=user_factory())
review2 = Review.objects.create(
addon=self.addon, body='review 2', user=user_factory())
review1.update(created=self.days_ago(1))
# Add a review belonging to a different add-on, a reply and a deleted
# review. They should not be present in the list.
review_deleted = Review.objects.create(
addon=self.addon, body='review deleted', user=review1.user)
review_deleted.delete()
Review.objects.create(
addon=self.addon, body='reply to review 1', reply_to=review1,
user=user_factory())
Review.objects.create(
addon=addon_factory(), body='review other addon',
user=review1.user)
assert Review.unfiltered.count() == 5
response = self.client.get(self.url)
assert response.status_code == 200
data = json.loads(response.content)
assert data['count'] == 2
assert data['results']
assert len(data['results']) == 2
assert data['results'][0]['id'] == review2.pk
assert data['results'][1]['id'] == review1.pk
def test_list_unknown_addon(self):
self.url = reverse(
'addon-review-list', kwargs={'addon_pk': self.addon.pk + 42})
# We don't bother checking for the add-on existence, so this should
# just result in an empty list.
response = self.client.get(self.url)
assert response.status_code == 200
data = json.loads(response.content)
assert data['count'] == 0
assert data['results'] == []
def test_list_addon_guid(self):
self.url = reverse(
'addon-review-list', kwargs={'addon_pk': self.addon.guid})
self.test_list()
def test_list_addon_slug(self):
self.url = reverse(
'addon-review-list', kwargs={'addon_pk': self.addon.slug})
self.test_list()
def test_list_user(self):
# Change this test to do a reverse() and expect a list of results
# instead of an exception when we implement listing reviews for a
# particular user.
with self.assertRaises(NotImplementedError):
ReviewViewSet(action='list', kwargs={'user_pk': 1}).get_queryset()
def test_list_no_user_or_addon(self):
# We have a fallback in get_queryset() to avoid listing all reviews on
# the website if somehow we messed up the if conditions. It should not
# be possible to reach it, but test it by forcing the instantiation of
# the viewset with no kwargs other than action='list'.
with self.assertRaises(ParseError):
ReviewViewSet(action='list', kwargs={}).get_queryset()
def test_detail(self):
review = Review.objects.create(
addon=self.addon, body='review 1', user=user_factory())
self.url = reverse(
'addon-review-detail',
kwargs={'addon_pk': self.addon.pk, 'pk': review.pk})
response = self.client.get(self.url)
assert response.status_code == 200
data = json.loads(response.content)
assert data['id'] == review.pk
def test_detail_reply(self):
review = Review.objects.create(
addon=self.addon, body='review', user=user_factory())
reply = Review.objects.create(
addon=self.addon, body='reply to review', user=user_factory(),
reply_to=review)
self.url = reverse(
'addon-review-detail',
kwargs={'addon_pk': self.addon.pk, 'pk': reply.pk})
response = self.client.get(self.url)
assert response.status_code == 200
data = json.loads(response.content)
assert data['id'] == reply.pk
def test_detail_deleted(self):
review = Review.objects.create(
addon=self.addon, body='review 1', user=user_factory())
self.url = reverse(
'addon-review-detail',
kwargs={'addon_pk': self.addon.pk, 'pk': review.pk})
review.delete()
response = self.client.get(self.url)
assert response.status_code == 404

Просмотреть файл

@ -13,6 +13,10 @@ from django.utils.translation import ugettext as _
import commonware.log
from mobility.decorators import mobile_template
from rest_framework.exceptions import ParseError
from rest_framework.mixins import ListModelMixin, RetrieveModelMixin
from rest_framework.permissions import AllowAny
from rest_framework.viewsets import GenericViewSet
from waffle.decorators import waffle_switch
from olympia import amo
@ -23,9 +27,12 @@ from olympia.amo import helpers, utils as amo_utils
from olympia.access import acl
from olympia.addons.decorators import addon_view_factory
from olympia.addons.models import Addon
from olympia.addons.views import AddonViewSet
from olympia.api.permissions import ByHttpMethod
from .helpers import user_can_delete_review
from .models import Review, ReviewFlag, GroupedRating, Spam
from .serializers import ReviewSerializer
from . import forms
@ -311,3 +318,47 @@ def spam(request):
reviews[addon.id].addon = addon
return render(request, 'reviews/spam.html',
dict(buckets=buckets, review_perms=dict(is_admin=True)))
class ReviewViewSet(ListModelMixin, RetrieveModelMixin, GenericViewSet):
permission_classes = [
ByHttpMethod({
'get': AllowAny,
'head': AllowAny,
'options': AllowAny, # Needed for CORS.
}),
]
serializer_class = ReviewSerializer
queryset = Review.objects.all()
def filter_queryset(self, qs):
addon_identifier = self.kwargs.get('addon_pk')
if self.action == 'list' and addon_identifier:
# No need to load the actual add-on. Just filter the queryset
# according to what the addon_pk parameter we got look like.
lookup_field = AddonViewSet(
request=self.request, kwargs={'pk': self.kwargs['addon_pk']}
).get_lookup_field(addon_identifier)
qs = qs.filter(**{'addon__%s' % lookup_field: addon_identifier})
return qs
def get_queryset(self):
if self.action == 'list':
if self.kwargs.get('addon_pk'):
# When listing add-on reviews, exclude replies, they'll be
# included during serialization as children of the relevant
# reviews instead.
self.queryset = Review.without_replies.all()
elif self.kwargs.get('user_pk'):
raise NotImplementedError
else:
# Don't allow listing reviews without filtering by add-on or
# user.
raise ParseError('Need an addon or user identifier')
qs = super(ReviewViewSet, self).get_queryset()
# The serializer needs user, reply and version, so use
# prefetch_related() to avoid extra queries (avoid select_related() as
# we need crazy joins already)
return qs.prefetch_related('reply', 'user', 'version')

Просмотреть файл

@ -0,0 +1,15 @@
from rest_framework import serializers
from olympia.amo.helpers import absolutify
from olympia.users.models import UserProfile
class BaseUserSerializer(serializers.ModelSerializer):
url = serializers.SerializerMethodField()
class Meta:
model = UserProfile
fields = ('name', 'url')
def get_url(self, obj):
return absolutify(obj.get_url_path())

Просмотреть файл

@ -0,0 +1,23 @@
from rest_framework.test import APIRequestFactory
from olympia.amo.helpers import absolutify
from olympia.amo.tests import TestCase, user_factory
from olympia.users.models import UserProfile
from olympia.users.serializers import BaseUserSerializer
class TestBaseUserSerializer(TestCase):
def setUp(self):
self.request = APIRequestFactory().get('/')
self.user = user_factory()
def serialize(self):
# Manually reload the user first to clear any cached properties.
self.user = UserProfile.objects.get(pk=self.user.pk)
serializer = BaseUserSerializer(context={'request': self.request})
return serializer.to_representation(self.user)
def test_basic(self):
result = self.serialize()
assert result['name'] == self.user.name
assert result['url'] == absolutify(self.user.get_url_path())