-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
APIv3 endpoint: allow to modify a Project once it's imported #5952
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c3044fc
0601c2d
6629c00
2a393f5
e591184
0bb5528
cdad40f
be0ccb1
61dc78b
ca2a1c6
ad12f1f
13c86e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,14 @@ def has_permission(self, request, view): | |
# hitting ``/projects/``, allowing | ||
return True | ||
|
||
if view.detail: | ||
# NOTE: ``superproject`` is an action name, defined by the class | ||
# method under ``ProjectViewSet``. We should apply the same | ||
# permissions restrictions than for a detail action (since it only | ||
# returns one superproject if exists). ``list`` and ``retrieve`` are | ||
# DRF standard action names (same as ``update`` or ``partial_update``). | ||
if view.detail and view.action in ('list', 'retrieve', 'superproject'): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These feel like they should be constants? What is a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We already talked about this at #5857 (comment) (I'm adding this comment as a code comment) |
||
# detail view is only allowed on list/retrieve actions (not | ||
# ``update`` or ``partial_update``). | ||
return True | ||
|
||
project = view._get_parent_project() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,20 @@ | |
from django.conf import settings | ||
from django.contrib.auth.models import User | ||
from django.core.urlresolvers import reverse | ||
from django.utils.translation import ugettext as _ | ||
|
||
from rest_flex_fields import FlexFieldsModelSerializer | ||
from rest_flex_fields.serializers import FlexFieldsSerializerMixin | ||
from rest_framework import serializers | ||
|
||
from readthedocs.builds.models import Build, Version | ||
from readthedocs.projects.constants import LANGUAGES, PROGRAMMING_LANGUAGES, REPO_CHOICES | ||
from readthedocs.projects.constants import ( | ||
LANGUAGES, | ||
PROGRAMMING_LANGUAGES, | ||
REPO_CHOICES, | ||
PRIVACY_CHOICES, | ||
PROTECTED, | ||
) | ||
from readthedocs.projects.models import Project, EnvironmentVariable | ||
from readthedocs.redirects.models import Redirect, TYPE_CHOICES as REDIRECT_TYPE_CHOICES | ||
|
||
|
@@ -427,6 +435,41 @@ class Meta: | |
) | ||
|
||
|
||
class ProjectUpdateSerializer(FlexFieldsModelSerializer): | ||
|
||
"""Serializer used to modify a Project once imported.""" | ||
|
||
repository = RepositorySerializer(source='*') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
(see docs https://www.django-rest-framework.org/api-guide/fields/#source) |
||
homepage = serializers.URLField(source='project_url') | ||
|
||
# Exclude ``Protected`` as a possible value for Privacy Level | ||
privacy_level_choices = list(PRIVACY_CHOICES) | ||
privacy_level_choices.remove((PROTECTED, _('Protected'))) | ||
privacy_level = serializers.ChoiceField(choices=privacy_level_choices) | ||
|
||
class Meta: | ||
model = Project | ||
fields = ( | ||
# Settings | ||
'name', | ||
'repository', | ||
'language', | ||
'programming_language', | ||
'homepage', | ||
|
||
# Advanced Settings -> General Settings | ||
'default_version', | ||
'default_branch', | ||
'privacy_level', | ||
'analytics_code', | ||
'show_version_warning', | ||
'single_version', | ||
|
||
# NOTE: we do not allow to change any setting that can be set via | ||
# the YAML config file. | ||
) | ||
|
||
|
||
class ProjectSerializer(FlexFieldsModelSerializer): | ||
|
||
homepage = serializers.SerializerMethodField() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
|
||
|
||
from .filters import BuildFilter, ProjectFilter, VersionFilter | ||
from .mixins import ProjectQuerySetMixin | ||
from .mixins import ProjectQuerySetMixin, UpdateMixin | ||
from .permissions import PublicDetailPrivateListing, IsProjectAdmin | ||
from .renderers import AlphabeticalSortedJSONRenderer | ||
from .serializers import ( | ||
|
@@ -36,6 +36,7 @@ | |
EnvironmentVariableSerializer, | ||
ProjectSerializer, | ||
ProjectCreateSerializer, | ||
ProjectUpdateSerializer, | ||
RedirectCreateSerializer, | ||
RedirectDetailSerializer, | ||
VersionSerializer, | ||
|
@@ -73,6 +74,7 @@ class APIv3Settings: | |
|
||
class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
FlexFieldsMixin, ProjectImportMixin, CreateModelMixin, | ||
UpdateMixin, UpdateModelMixin, | ||
ReadOnlyModelViewSet): | ||
|
||
# Markdown docstring is automatically rendered by BrowsableAPIRenderer. | ||
|
@@ -151,6 +153,9 @@ def get_serializer_class(self): | |
if self.action == 'create': | ||
return ProjectCreateSerializer | ||
|
||
if self.action in ('update', 'partial_update'): | ||
return ProjectUpdateSerializer | ||
|
||
def get_queryset(self): | ||
# Allow hitting ``/api/v3/projects/`` to list their own projects | ||
if self.basename == 'projects' and self.action == 'list': | ||
|
@@ -271,7 +276,8 @@ class TranslationRelationshipViewSet(APIv3Settings, NestedViewSetMixin, | |
# of ``ProjectQuerySetMixin`` to make calling ``super().get_queryset()`` work | ||
# properly and filter nested dependencies | ||
class VersionsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
FlexFieldsMixin, UpdateModelMixin, ReadOnlyModelViewSet): | ||
FlexFieldsMixin, UpdateMixin, | ||
UpdateModelMixin, ReadOnlyModelViewSet): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this also allow us to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. It makes the PUT and PATCH to behave in the same way (because of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was already working like that, but I moved the |
||
|
||
model = Version | ||
lookup_field = 'slug' | ||
|
@@ -298,20 +304,6 @@ def get_serializer_class(self): | |
return VersionSerializer | ||
return VersionUpdateSerializer | ||
|
||
def update(self, request, *args, **kwargs): | ||
""" | ||
Make PUT/PATCH behaves in the same way. | ||
|
||
Force to return 204 is the update was good. | ||
""" | ||
|
||
# NOTE: ``Authorization:`` header is mandatory to use this method from | ||
# Browsable API since SessionAuthentication can't be used because we set | ||
# ``httpOnly`` on our cookies and the ``PUT/PATCH`` method are triggered | ||
# via Javascript | ||
super().update(request, *args, **kwargs) | ||
return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
||
|
||
class BuildsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
FlexFieldsMixin, ReadOnlyModelViewSet): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it will also raise an error in some cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the "Ship APIv3" PR I was thinking to remove all the redundant docs or un-useful list of fields: avoid things like
slug: the slug of the Project
and just document that ones that are useful.I applied the same concept for status codes, mentioning only the important ones (based on David's comment as well: #4863 (comment))
I did a commit for this in that PR: f7e168f
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable 👍