Merge pull request #1468 from diox/ratings-for-admins-app-owners
Allow app owners and admin to see ratings from non-public apps (bug 943093)
This commit is contained in:
Mathieu Pillard
Коммит
d4bf5e24a7
|
|
@ -8,7 +8,7 @@ from reviews.models import Review, ReviewFlag
|
|||
from mkt.account.serializers import AccountSerializer
|
||||
from mkt.api.fields import SlugOrPrimaryKeyRelatedField, SplitField
|
||||
from mkt.api.exceptions import Conflict
|
||||
from mkt.regions import get_region, REGIONS_DICT
|
||||
from mkt.regions import REGIONS_DICT, get_region
|
||||
from mkt.versions.api import SimpleVersionSerializer
|
||||
from mkt.webapps.models import Webapp
|
||||
|
||||
|
|
@ -60,25 +60,16 @@ class RatingSerializer(serializers.ModelSerializer):
|
|||
return (not self.get_is_author(obj) and
|
||||
obj.reviewflag_set.filter(user=self.request.amo_user).exists())
|
||||
|
||||
@classmethod
|
||||
def get_app_from_value(cls, value):
|
||||
try:
|
||||
app = Webapp.objects.valid().get(id=value)
|
||||
except (Webapp.DoesNotExist, ValueError):
|
||||
try:
|
||||
app = Webapp.objects.valid().get(app_slug=value)
|
||||
except Webapp.DoesNotExist:
|
||||
raise serializers.ValidationError('Invalid app')
|
||||
if not app.listed_in(region=REGIONS_DICT[get_region()]):
|
||||
raise serializers.ValidationError(
|
||||
'App not available in this region')
|
||||
return app
|
||||
|
||||
def validate(self, attrs):
|
||||
attrs['user'] = self.request.amo_user
|
||||
attrs['ip_address'] = self.request.META.get('REMOTE_ADDR', '')
|
||||
|
||||
if not getattr(self, 'object'):
|
||||
# If we are creating a rating, then we need to do various checks on
|
||||
# the app. Because these checks need the version as well, we have
|
||||
# to do them here and not in validate_app().
|
||||
|
||||
# If the app is packaged, add in the current version.
|
||||
if attrs['addon'].is_packaged:
|
||||
attrs['version'] = attrs['addon'].current_version
|
||||
|
||||
|
|
@ -88,11 +79,14 @@ class RatingSerializer(serializers.ModelSerializer):
|
|||
qs = self.context['view'].queryset.filter(addon=app, user=amo_user)
|
||||
if app.is_packaged:
|
||||
qs = qs.filter(version=attrs['version'])
|
||||
|
||||
if qs.exists():
|
||||
raise Conflict('You have already reviewed this app.')
|
||||
|
||||
# Return 403 if the user is attempting to review their own app:
|
||||
# Return 403 is the app is not public.
|
||||
if not app.is_public():
|
||||
raise PermissionDenied('The app requested is not public.')
|
||||
|
||||
# Return 403 if the user is attempting to review their own app.
|
||||
if app.has_author(amo_user):
|
||||
raise PermissionDenied('You may not review your own app.')
|
||||
|
||||
|
|
@ -100,13 +94,18 @@ class RatingSerializer(serializers.ModelSerializer):
|
|||
if app.is_premium() and not app.is_purchased(amo_user):
|
||||
raise PermissionDenied("You may not review paid apps you "
|
||||
"haven't purchased.")
|
||||
|
||||
# Return 403 if the app is not available in the current region.
|
||||
current_region = get_region()
|
||||
if not app.listed_in(region=REGIONS_DICT[current_region]):
|
||||
raise PermissionDenied('App not available in region "%s".' %
|
||||
current_region)
|
||||
|
||||
return attrs
|
||||
|
||||
def validate_app(self, attrs, source):
|
||||
if not getattr(self, 'object'):
|
||||
app = attrs[source]
|
||||
attrs[source] = RatingSerializer.get_app_from_value(app.pk)
|
||||
else:
|
||||
# Don't allow users to change the app on an existing rating.
|
||||
if getattr(self, 'object'):
|
||||
attrs[source] = self.object.addon
|
||||
return attrs
|
||||
|
||||
|
|
|
|||
|
|
@ -161,21 +161,59 @@ class TestRatingResource(RestOAuth, amo.tests.AMOPaths):
|
|||
eq_(data['info']['slug'], self.app.app_slug)
|
||||
eq_(data['info']['current_version'], self.app.current_version.version)
|
||||
|
||||
def test_filter_by_invalid_app_slug(self):
|
||||
def test_filter_by_invalid_app(self):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
self._get_filter(app='wrongslug', expected_status=400)
|
||||
self._get_filter(app='wrongslug', expected_status=404)
|
||||
self._get_filter(app=2465478, expected_status=404)
|
||||
|
||||
def test_filter_by_nonpublic_app(self):
|
||||
@patch('mkt.ratings.views.get_region')
|
||||
def test_filter_by_nonpublic_app(self, get_region_mock):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
self.app.update(status=amo.STATUS_PENDING)
|
||||
self._get_filter(app=self.app.app_slug, expected_status=400)
|
||||
get_region_mock.return_value = 'us'
|
||||
res, data = self._get_filter(app=self.app.app_slug, expected_status=403)
|
||||
eq_(data['detail'], 'The app requested is not public or not available '
|
||||
'in region "us".')
|
||||
|
||||
@patch('mkt.ratings.serializers.get_region')
|
||||
def test_filter_by_nonpublic_app_admin(self):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
self.grant_permission(self.user, 'Addons:Edit')
|
||||
self.app.update(status=amo.STATUS_PENDING)
|
||||
self._get_filter(app=self.app.app_slug)
|
||||
|
||||
def test_filter_by_nonpublic_app_owner(self):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
AddonUser.objects.create(user=self.user, addon=self.app)
|
||||
self.app.update(status=amo.STATUS_PENDING)
|
||||
self._get_filter(app=self.app.app_slug)
|
||||
|
||||
@patch('mkt.ratings.views.get_region')
|
||||
def test_filter_by_app_excluded_in_region(self, get_region_mock):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
AddonExcludedRegion.objects.create(addon=self.app,
|
||||
region=mkt.regions.BR.id)
|
||||
get_region_mock.return_value = 'br'
|
||||
r, data = self._get_filter(app=self.app.app_slug, expected_status=400)
|
||||
eq_(data['detail'], 'App not available in this region')
|
||||
res, data = self._get_filter(app=self.app.app_slug, expected_status=403)
|
||||
eq_(data['detail'], 'The app requested is not public or not available '
|
||||
'in region "br".')
|
||||
|
||||
@patch('mkt.ratings.views.get_region')
|
||||
def test_filter_by_app_excluded_in_region_admin(self, get_region_mock):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
self.grant_permission(self.user, 'Addons:Edit')
|
||||
AddonExcludedRegion.objects.create(addon=self.app,
|
||||
region=mkt.regions.BR.id)
|
||||
get_region_mock.return_value = 'br'
|
||||
self._get_filter(app=self.app.app_slug)
|
||||
|
||||
@patch('mkt.ratings.views.get_region')
|
||||
def test_filter_by_app_excluded_in_region_owner(self, get_region_mock):
|
||||
Review.objects.create(addon=self.app, user=self.user, body='yes')
|
||||
AddonUser.objects.create(user=self.user, addon=self.app)
|
||||
AddonExcludedRegion.objects.create(addon=self.app,
|
||||
region=mkt.regions.BR.id)
|
||||
get_region_mock.return_value = 'br'
|
||||
self._get_filter(app=self.app.app_slug)
|
||||
|
||||
def test_anonymous_get_list_without_app(self):
|
||||
res, data = self._get_url(self.list_url, client=self.anon)
|
||||
|
|
@ -302,12 +340,12 @@ class TestRatingResource(RestOAuth, amo.tests.AMOPaths):
|
|||
region=mkt.regions.BR.id)
|
||||
get_region_mock.return_value = 'br'
|
||||
res, data = self._create()
|
||||
eq_(400, res.status_code)
|
||||
eq_(403, res.status_code)
|
||||
|
||||
def test_create_for_nonpublic(self):
|
||||
self.app.update(status=amo.STATUS_PENDING)
|
||||
res, data = self._create()
|
||||
eq_(400, res.status_code)
|
||||
eq_(403, res.status_code)
|
||||
|
||||
def test_create_duplicate_rating(self):
|
||||
self._create()
|
||||
|
|
|
|||
|
|
@ -1,15 +1,16 @@
|
|||
from django.core.paginator import Paginator
|
||||
from django.http import Http404
|
||||
|
||||
import commonware.log
|
||||
from rest_framework.decorators import action
|
||||
from rest_framework.exceptions import (MethodNotAllowed, NotAuthenticated,
|
||||
ParseError)
|
||||
PermissionDenied)
|
||||
from rest_framework.mixins import CreateModelMixin
|
||||
from rest_framework.permissions import AllowAny, IsAuthenticated
|
||||
from rest_framework.serializers import ValidationError
|
||||
from rest_framework.viewsets import GenericViewSet, ModelViewSet
|
||||
|
||||
import amo
|
||||
from access.acl import check_addon_ownership
|
||||
from lib.metrics import record_action
|
||||
from reviews.models import Review, ReviewFlag
|
||||
|
||||
|
|
@ -20,6 +21,8 @@ from mkt.api.authorization import (AnyOf, AllowOwner, AllowRelatedAppOwner,
|
|||
ByHttpMethod, GroupPermission)
|
||||
from mkt.api.base import CORSMixin, MarketplaceView
|
||||
from mkt.ratings.serializers import RatingFlagSerializer, RatingSerializer
|
||||
from mkt.regions import REGIONS_DICT, get_region
|
||||
from mkt.webapps.models import Webapp
|
||||
|
||||
|
||||
log = commonware.log.getLogger('z.api')
|
||||
|
|
@ -99,9 +102,19 @@ class RatingViewSet(CORSMixin, MarketplaceView, ModelViewSet):
|
|||
|
||||
def get_app(self, ident):
|
||||
try:
|
||||
return self.serializer_class.get_app_from_value(ident)
|
||||
except ValidationError as e:
|
||||
raise ParseError(detail=e.messages[0])
|
||||
app = Webapp.objects.by_identifier(ident)
|
||||
except Webapp.DoesNotExist:
|
||||
raise Http404
|
||||
current_region = get_region()
|
||||
if ((not app.is_public()
|
||||
or not app.listed_in(region=REGIONS_DICT[current_region]))
|
||||
and not check_addon_ownership(self.request, app)):
|
||||
# App owners and admin can see the app even if it's not public
|
||||
# or not available in the current region. Regular users or
|
||||
# anonymous users can't.
|
||||
raise PermissionDenied('The app requested is not public or not '
|
||||
'available in region "%s".' % current_region)
|
||||
return app
|
||||
|
||||
def list(self, request, *args, **kwargs):
|
||||
response = super(RatingViewSet, self).list(request, *args, **kwargs)
|
||||
|
|
@ -132,7 +145,7 @@ class RatingViewSet(CORSMixin, MarketplaceView, ModelViewSet):
|
|||
(obj.pk, self.request.amo_user.id))
|
||||
|
||||
def partial_update(self, *args, **kwargs):
|
||||
# We don't need/want PATCH.
|
||||
# We don't need/want PATCH for now.
|
||||
raise MethodNotAllowed('PATCH is not supported for this endpoint.')
|
||||
|
||||
def get_extra_data(self, app, amo_user):
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче