Expose version list for diffing, rework reviewers api to be more concise (#10684)
* Expose version list for diffing, rework reviewers api to be more concise We're now mounting any listing/browsing/future diffing APIs under the already existing addon/<addon_id>/ endpoint which makes the whole reviewer APIs behave more consistently. Also implements the listing of versions that can be used for browsing and diffing. Fixes #10432 * Simplify view, don't rely on @action decorator but just make it the detail view * Smaller cleanups, fix permissions * More smaller cleanups, split list/retreive better * Fix link in docs * Improve tests, remove 'should_show_channel' and 'url' * Smaller performance optimization * Fix flake8 * Improve docs
This commit is contained in:
Christopher Grebs
GitHub
Родитель
600fe38b5a
Коммит
1352908bcf
|
|
@ -84,6 +84,28 @@ add-on.
|
|||
:>json boolean needs_admin_content_review: Boolean indicating whether the add-on needs its content to be reviewed by an admin or not.
|
||||
|
||||
|
||||
-------------
|
||||
List Versions
|
||||
-------------
|
||||
|
||||
This endpoint allows you to list versions that can be used either for :ref:`browsing <reviewers-versions-browse>` or diffing versions.
|
||||
|
||||
.. note::
|
||||
Requires authentication and the current user to have ``ReviewerTools:View``
|
||||
permission for listed add-ons as well as ``Addons:ReviewUnlisted`` for
|
||||
unlisted add-ons. Additionally the current user can also be the owner
|
||||
of the add-on.
|
||||
|
||||
If the user doesn't have ``AddonsReviewUnlisted`` permissions only listed versions are shown. Otherwise it can contain mixed listed and unlisted versions.
|
||||
|
||||
.. http:get:: /api/v4/reviewers/addon/(int:addon_id)/versions/
|
||||
|
||||
:>json int id: The version id.
|
||||
:>json string channel: The version channel, which determines its visibility on the site. Can be either ``unlisted`` or ``listed``.
|
||||
:>json string version: The version number string for the version.
|
||||
|
||||
.. _reviewers-versions-browse:
|
||||
|
||||
------
|
||||
Browse
|
||||
------
|
||||
|
|
@ -96,7 +118,7 @@ This endpoint allows you to browse through the contents of an Add-on version.
|
|||
unlisted add-ons. Additionally the current user can also be the owner
|
||||
of the add-on.
|
||||
|
||||
.. http:get:: /api/v4/reviewers/browse/(int:version_id)/
|
||||
.. http:get:: /api/v4/reviewers/addon/(int:addon_id)/versions/(int:version_id)/
|
||||
|
||||
Inherits most properties from :ref:`version detail <version-detail-object>` except ``files``.
|
||||
|
||||
|
|
|
|||
|
|
@ -1,14 +1,21 @@
|
|||
from django.conf.urls import include, url
|
||||
|
||||
from rest_framework.routers import SimpleRouter
|
||||
from rest_framework_nested.routers import NestedSimpleRouter
|
||||
|
||||
from .views import AddonReviewerViewSet, BrowseVersionViewSet
|
||||
from .views import (
|
||||
AddonReviewerViewSet, ReviewAddonVersionViewSet)
|
||||
|
||||
|
||||
addons = SimpleRouter()
|
||||
addons.register(r'addon', AddonReviewerViewSet, basename='reviewers-addon')
|
||||
addons.register(r'browse', BrowseVersionViewSet, basename='reviewers-browse')
|
||||
|
||||
versions = NestedSimpleRouter(addons, r'addon', lookup='addon')
|
||||
versions.register(
|
||||
r'versions', ReviewAddonVersionViewSet, basename='reviewers-versions')
|
||||
|
||||
|
||||
urlpatterns = [
|
||||
url(r'', include(addons.urls)),
|
||||
url(r'', include(versions.urls)),
|
||||
]
|
||||
|
|
|
|||
|
|
@ -203,3 +203,10 @@ class AddonBrowseVersionSerializer(VersionSerializer):
|
|||
|
||||
def get_has_been_validated(self, obj):
|
||||
return obj.current_file.has_been_validated
|
||||
|
||||
|
||||
class DiffableVersionSerializer(VersionSerializer):
|
||||
|
||||
class Meta:
|
||||
model = Version
|
||||
fields = ('id', 'channel', 'version')
|
||||
|
|
|
|||
|
|
@ -5163,11 +5163,11 @@ class TestAddonReviewerViewSet(TestCase):
|
|||
assert activity_log.arguments[0] == self.addon
|
||||
|
||||
|
||||
class TestBrowseViewSet(TestCase):
|
||||
class TestReviewAddonVersionViewSetDetail(TestCase):
|
||||
client_class = APITestClient
|
||||
|
||||
def setUp(self):
|
||||
super(TestBrowseViewSet, self).setUp()
|
||||
super(TestReviewAddonVersionViewSetDetail, self).setUp()
|
||||
|
||||
# TODO: Most of the initial setup could be moved to
|
||||
# setUpTestData but unfortunately paths are setup in pytest via a
|
||||
|
|
@ -5195,8 +5195,9 @@ class TestBrowseViewSet(TestCase):
|
|||
assert '"name": "Beastify"' in result['file']['content']
|
||||
|
||||
def _set_tested_url(self):
|
||||
self.url = reverse_ns('reviewers-browse-detail', kwargs={
|
||||
'pk': self.version.pk})
|
||||
self.url = reverse_ns('reviewers-versions-detail', kwargs={
|
||||
'addon_pk': self.addon.pk,
|
||||
'version_pk': self.version.pk})
|
||||
|
||||
def test_anonymous(self):
|
||||
response = self.client.get(self.url)
|
||||
|
|
@ -5217,8 +5218,9 @@ class TestBrowseViewSet(TestCase):
|
|||
user = UserProfile.objects.create(username='reviewer')
|
||||
self.grant_permission(user, 'Addons:Review')
|
||||
self.client.login_api(user)
|
||||
self.url = reverse_ns('reviewers-browse-detail', kwargs={
|
||||
'pk': self.version.current_file.pk + 42})
|
||||
self.url = reverse_ns('reviewers-versions-detail', kwargs={
|
||||
'addon_pk': self.addon.pk,
|
||||
'version_pk': self.version.current_file.pk + 42})
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 404
|
||||
|
||||
|
|
@ -5317,6 +5319,124 @@ class TestBrowseViewSet(TestCase):
|
|||
assert response.status_code == 403
|
||||
|
||||
|
||||
class TestReviewAddonVersionViewSetList(TestCase):
|
||||
client_class = APITestClient
|
||||
|
||||
def setUp(self):
|
||||
super(TestReviewAddonVersionViewSetList, self).setUp()
|
||||
|
||||
self.addon = addon_factory(
|
||||
name=u'My Addôn', slug='my-addon',
|
||||
file_kw={'filename': 'webextension_no_id.xpi'})
|
||||
|
||||
extract_version_to_git(self.addon.current_version.pk)
|
||||
|
||||
self.version = self.addon.current_version
|
||||
self.version.refresh_from_db()
|
||||
|
||||
self._set_tested_url()
|
||||
|
||||
def _test_url(self):
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 200
|
||||
result = json.loads(response.content)
|
||||
assert result == [{
|
||||
'version': self.version.version,
|
||||
'id': self.version.id,
|
||||
'channel': u'listed',
|
||||
}]
|
||||
|
||||
def _set_tested_url(self):
|
||||
self.url = reverse_ns('reviewers-versions-list', kwargs={
|
||||
'addon_pk': self.addon.pk})
|
||||
|
||||
def test_anonymous(self):
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 401
|
||||
|
||||
def test_permissions_reviewer(self):
|
||||
user = UserProfile.objects.create(username='reviewer')
|
||||
self.grant_permission(user, 'Addons:Review')
|
||||
self.client.login_api(user)
|
||||
self._test_url()
|
||||
|
||||
def test_permissions_disabled_version_author(self):
|
||||
user = UserProfile.objects.create(username='author')
|
||||
AddonUser.objects.create(user=user, addon=self.addon)
|
||||
self.client.login_api(user)
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
self._test_url()
|
||||
|
||||
def test_permissions_disabled_version_admin(self):
|
||||
user = UserProfile.objects.create(username='admin')
|
||||
self.grant_permission(user, '*:*')
|
||||
self.client.login_api(user)
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
self._test_url()
|
||||
|
||||
def test_permissions_disabled_version_user_but_not_author(self):
|
||||
user = UserProfile.objects.create(username='simpleuser')
|
||||
self.client.login_api(user)
|
||||
self.version.files.update(status=amo.STATUS_DISABLED)
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 403
|
||||
|
||||
def test_show_only_listed_without_unlisted_permission(self):
|
||||
user = UserProfile.objects.create(username='admin')
|
||||
|
||||
# User doesn't have ReviewUnlisted permission
|
||||
self.grant_permission(user, 'Addons:Review')
|
||||
|
||||
self.client.login_api(user)
|
||||
|
||||
version_factory(
|
||||
addon=self.addon, channel=amo.RELEASE_CHANNEL_UNLISTED)
|
||||
|
||||
response = self.client.get(self.url)
|
||||
assert response.status_code == 200
|
||||
result = json.loads(response.content)
|
||||
|
||||
assert result == [
|
||||
{
|
||||
'version': self.version.version,
|
||||
'id': self.version.id,
|
||||
'channel': u'listed'
|
||||
},
|
||||
]
|
||||
|
||||
def test_show_listed_and_unlisted_with_permissions(self):
|
||||
user = UserProfile.objects.create(username='admin')
|
||||
|
||||
# User doesn't have ReviewUnlisted permission
|
||||
self.grant_permission(user, 'Addons:ReviewUnlisted')
|
||||
|
||||
self.client.login_api(user)
|
||||
|
||||
unlisted_version = version_factory(
|
||||
addon=self.addon, channel=amo.RELEASE_CHANNEL_UNLISTED)
|
||||
|
||||
# We have a .only() and .no_transforms or .only_translations
|
||||
# querysets which reduces the amount of queries to "only" 11
|
||||
with self.assertNumQueries(11):
|
||||
response = self.client.get(self.url)
|
||||
|
||||
assert response.status_code == 200
|
||||
result = json.loads(response.content)
|
||||
|
||||
assert result == [
|
||||
{
|
||||
'version': unlisted_version.version,
|
||||
'id': unlisted_version.id,
|
||||
'channel': u'unlisted'
|
||||
},
|
||||
{
|
||||
'version': self.version.version,
|
||||
'id': self.version.id,
|
||||
'channel': u'listed'
|
||||
},
|
||||
]
|
||||
|
||||
|
||||
class TestThemeBackgroundImages(ReviewBase):
|
||||
|
||||
def setUp(self):
|
||||
|
|
|
|||
|
|
@ -20,7 +20,7 @@ from rest_framework import status
|
|||
from rest_framework.decorators import action
|
||||
from rest_framework.response import Response
|
||||
from rest_framework.viewsets import GenericViewSet
|
||||
from rest_framework.mixins import RetrieveModelMixin
|
||||
from rest_framework.mixins import ListModelMixin, RetrieveModelMixin
|
||||
|
||||
import olympia.core.logger
|
||||
|
||||
|
|
@ -52,7 +52,8 @@ from olympia.reviewers.models import (
|
|||
clear_reviewing_cache, get_flags, get_reviewing_cache,
|
||||
get_reviewing_cache_key, set_reviewing_cache)
|
||||
from olympia.reviewers.serializers import (
|
||||
AddonReviewerFlagsSerializer, AddonBrowseVersionSerializer)
|
||||
AddonReviewerFlagsSerializer, AddonBrowseVersionSerializer,
|
||||
DiffableVersionSerializer)
|
||||
from olympia.reviewers.utils import (
|
||||
AutoApprovedTable, ContentReviewTable, ExpiredInfoRequestsTable,
|
||||
ReviewHelper, ViewFullReviewQueueTable, ViewPendingQueueTable,
|
||||
|
|
@ -1240,43 +1241,86 @@ class AddonReviewerViewSet(GenericViewSet):
|
|||
return Response(serializer.data)
|
||||
|
||||
|
||||
class BrowseVersionViewSet(RetrieveModelMixin, GenericViewSet):
|
||||
class ReviewAddonVersionViewSet(ListModelMixin, RetrieveModelMixin,
|
||||
GenericViewSet):
|
||||
permission_classes = [AnyOf(
|
||||
AllowReviewer, AllowReviewerUnlisted, AllowAddonAuthor,
|
||||
)]
|
||||
|
||||
serializer_class = AddonBrowseVersionSerializer
|
||||
lookup_url_kwarg = 'version_pk'
|
||||
|
||||
def get_queryset(self):
|
||||
"""Return queryset to be used for the view."""
|
||||
# Permission classes disallow access to non-public/unlisted add-ons
|
||||
# unless logged in as a reviewer/addon owner/admin, so we don't have to
|
||||
# filter the base queryset here.
|
||||
return Version.unfiltered.all()
|
||||
return (
|
||||
Version.unfiltered
|
||||
.get_queryset()
|
||||
.only_translations()
|
||||
.filter(addon=self.get_addon_object())
|
||||
.order_by('-created'))
|
||||
|
||||
def get_serializer_context(self):
|
||||
context = super(BrowseVersionViewSet, self).get_serializer_context()
|
||||
context.update({
|
||||
'file': self.request.GET.get('file', None)
|
||||
})
|
||||
return context
|
||||
def get_addon_object(self):
|
||||
return get_object_or_404(
|
||||
Addon.objects.get_queryset().only_translations(),
|
||||
pk=self.kwargs.get('addon_pk'))
|
||||
|
||||
def check_object_permissions(self, request, obj):
|
||||
"""
|
||||
Check if the request should be permitted for a given version.
|
||||
def get_object(self):
|
||||
qset = self.filter_queryset(self.get_queryset())
|
||||
|
||||
filter_kwargs = {self.lookup_field: self.kwargs[self.lookup_url_kwarg]}
|
||||
|
||||
obj = get_object_or_404(qset, **filter_kwargs)
|
||||
|
||||
This calls the DRF implementation but forwards the respective Add-on
|
||||
as `obj` since most of our permission classes are based on an Add-on
|
||||
being passed around.
|
||||
"""
|
||||
# If the instance is marked as deleted and the client is not allowed to
|
||||
# see deleted instances, we want to return a 404, behaving as if it
|
||||
# does not exist.
|
||||
if obj.deleted and not (
|
||||
GroupPermission(amo.permissions.ADDONS_VIEW_DELETED).
|
||||
has_object_permission(request, self, obj.addon)):
|
||||
has_object_permission(self.request, self, obj.addon)):
|
||||
raise http.Http404
|
||||
|
||||
return (
|
||||
super(BrowseVersionViewSet, self)
|
||||
.check_object_permissions(request, obj.addon))
|
||||
# Now we can checking permissions
|
||||
self.check_object_permissions(self.request, obj)
|
||||
|
||||
return obj
|
||||
|
||||
def check_permissions(self, request):
|
||||
if self.action == u'list':
|
||||
# When listing DRF doesn't explicitly check for object permissions
|
||||
# but here we need to do that against the parent add-on.
|
||||
# So we're calling check_object_permission() ourselves,
|
||||
# which will pass down the addon object directly.
|
||||
return (
|
||||
super(ReviewAddonVersionViewSet, self)
|
||||
.check_object_permissions(request, self.get_addon_object()))
|
||||
|
||||
super(ReviewAddonVersionViewSet, self).check_permissions(request)
|
||||
|
||||
def check_object_permissions(self, request, obj):
|
||||
"""Check permissions against the parent add-on object."""
|
||||
return super(ReviewAddonVersionViewSet, self).check_object_permissions(
|
||||
request, obj.addon)
|
||||
|
||||
def list(self, request, *args, **kwargs):
|
||||
"""Return all (re)viewable versions for this add-on.
|
||||
|
||||
Full list, no pagination."""
|
||||
qset = self.filter_queryset(self.get_queryset())
|
||||
|
||||
if not acl.check_unlisted_addons_reviewer(self.request):
|
||||
qset = qset.filter(channel=amo.RELEASE_CHANNEL_LISTED)
|
||||
|
||||
# Smaller performance optimization, only list fields we actually
|
||||
# need.
|
||||
qset = qset.no_transforms().only(
|
||||
*DiffableVersionSerializer.Meta.fields)
|
||||
|
||||
serializer = DiffableVersionSerializer(qset, many=True)
|
||||
|
||||
return Response(serializer.data)
|
||||
|
||||
def retrieve(self, request, *args, **kwargs):
|
||||
serializer = AddonBrowseVersionSerializer(
|
||||
instance=self.get_object(),
|
||||
context={'file': self.request.GET.get('file', None)})
|
||||
return Response(serializer.data)
|
||||
|
|
|
|||
Загрузка…
Ссылка в новой задаче