Skip to content
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

APIv3 endpoint: allow to modify a Project once it's imported #5952

Merged
merged 12 commits into from
Oct 8, 2019
29 changes: 29 additions & 0 deletions docs/api/v3.rst
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,35 @@ Project create
:statuscode 400: Some field is invalid


Project update
++++++++++++++

.. http:patch:: /api/v3/projects/(string:project_slug)/

Edit an existent project.
humitos marked this conversation as resolved.
Show resolved Hide resolved

**Example request**:

.. sourcecode:: bash

$ curl \
-X PATCH \
-H "Authorization: Token <token>" https://readthedocs.org/api/v3/projects/pip/ \
-H "Content-Type: application/json" \
-d @body.json

The content of ``body.json`` is like,

.. sourcecode:: json

{
"name": "New name for the project",
"default_version": "v0.27.0"
}

:statuscode 204: Updated successfully
Copy link
Member

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?

Copy link
Member Author

@humitos humitos Oct 1, 2019

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable 👍



Versions
~~~~~~~~

Expand Down
25 changes: 25 additions & 0 deletions readthedocs/api/v3/mixins.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
from django.shortcuts import get_object_or_404
from rest_framework import status
from rest_framework.response import Response

from readthedocs.builds.models import Version
from readthedocs.projects.models import Project
Expand Down Expand Up @@ -28,6 +30,12 @@ def _get_parent_object_lookup(self, lookup_names):

def _get_parent_project(self):
slug = self._get_parent_object_lookup(self.PROJECT_LOOKUP_NAMES)

# when hitting ``/projects/<slug>/`` we don't have a "parent" project
# because this endpoint is the base one, so we just get the project from
# ``project_slug`` kwargs
slug = slug or self.kwargs.get('project_slug')

return get_object_or_404(Project, slug=slug)

def _get_parent_version(self):
Expand Down Expand Up @@ -92,3 +100,20 @@ def get_queryset(self):

# List view are only allowed if user is owner of parent project
return self.listing_objects(queryset, self.request.user)


class UpdatePartialUpdateMixin:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a super confusing name. Why does it have Update in the name twice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably rename this to UpdateMixin only. This class makes PUT to return 204 on success as PATCH.


"""
Make PUT/PATCH behaves in the same way.

Force to return 204 if the update was good.
"""

def update(self, request, *args, **kwargs):
# 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)
4 changes: 3 additions & 1 deletion readthedocs/api/v3/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ def has_permission(self, request, view):
# hitting ``/projects/``, allowing
return True

if view.detail:
# detail view is only allowed on list/retrieve actions (not
# ``update`` or ``partial_update``)
if view.detail and view.action in ('list', 'retrieve', 'superproject'):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These feel like they should be constants? What is a superproject? It still feels weird that it is a method name to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

We already talked about this at #5857 (comment)

(I'm adding this comment as a code comment)

return True

project = view._get_parent_project()
Expand Down
45 changes: 44 additions & 1 deletion readthedocs/api/v3/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -427,6 +435,41 @@ class Meta:
)


class ProjectUpdateSerializer(FlexFieldsModelSerializer):

"""Serializer used to modify a Project once imported."""

repository = RepositorySerializer(source='*')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is source='*'? It should likely have a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* it's standard for DRF but has a special meaning. It means that the whole object will be passed to the RepositorySerializer, not just a specific field of it.

(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()
Expand Down
2 changes: 1 addition & 1 deletion readthedocs/api/v3/tests/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def setUp(self):
self.others_token = fixture.get(Token, key='other', user=self.other)
self.others_project = fixture.get(
Project,
slug='others_project',
slug='others-project',
related_projects=[],
main_language_project=None,
users=[self.other],
Expand Down
89 changes: 89 additions & 0 deletions readthedocs/api/v3/tests/test_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,3 +181,92 @@ def test_import_project_with_extra_fields(self):
self.assertEqual(project.repo, 'https://github.com/rtfd/template')
self.assertNotEqual(project.default_version, 'v1.0')
self.assertIn(self.me, project.users.all())

def test_update_project(self):
data = {
'name': 'Updated name',
'repository': {
'url': 'https://bitbucket.com/rtfd/updated-repository',
'type': 'hg',
},
'language': 'es',
'programming_language': 'js',
'homepage': 'https://updated-homepage.org',
'default_version': 'stable',
'default_branch': 'updated-default-branch',
'privacy_level': 'private',
'analytics_code': 'UA-XXXXXX',
'show_version_warning': False,
'single_version': True,
}

self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
response = self.client.put(
reverse(
'projects-detail',
kwargs={
'project_slug': self.project.slug,
},
),
data,
)
self.assertEqual(response.status_code, 204)

self.project.refresh_from_db()
self.assertEqual(self.project.name, 'Updated name')
self.assertEqual(self.project.slug, 'project')
self.assertEqual(self.project.repo, 'https://bitbucket.com/rtfd/updated-repository')
self.assertEqual(self.project.repo_type, 'hg')
self.assertEqual(self.project.language, 'es')
self.assertEqual(self.project.programming_language, 'js')
self.assertEqual(self.project.project_url, 'https://updated-homepage.org')
self.assertEqual(self.project.default_version, 'stable')
self.assertEqual(self.project.default_branch, 'updated-default-branch')
self.assertEqual(self.project.privacy_level, 'private')
self.assertEqual(self.project.analytics_code, 'UA-XXXXXX')
self.assertEqual(self.project.show_version_warning, False)
self.assertEqual(self.project.single_version, True)

def test_partial_update_project(self):
data = {
'name': 'Updated name',
'repository': {
'url': 'https://github.com/rtfd/updated-repository',
},
'default_branch': 'updated-default-branch',
}

self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
response = self.client.patch(
reverse(
'projects-detail',
kwargs={
'project_slug': self.project.slug,
},
),
data,
)
self.assertEqual(response.status_code, 204)

self.project.refresh_from_db()
self.assertEqual(self.project.name, 'Updated name')
self.assertEqual(self.project.slug, 'project')
self.assertEqual(self.project.repo, 'https://github.com/rtfd/updated-repository')
self.assertNotEqual(self.project.default_version, 'updated-default-branch')

def test_partial_update_others_project(self):
data = {
'name': 'Updated name',
}

self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
response = self.client.patch(
reverse(
'projects-detail',
kwargs={
'project_slug': self.others_project.slug,
},
),
data,
)
self.assertEqual(response.status_code, 403)
humitos marked this conversation as resolved.
Show resolved Hide resolved
24 changes: 8 additions & 16 deletions readthedocs/api/v3/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@


from .filters import BuildFilter, ProjectFilter, VersionFilter
from .mixins import ProjectQuerySetMixin
from .mixins import ProjectQuerySetMixin, UpdatePartialUpdateMixin
from .permissions import PublicDetailPrivateListing, IsProjectAdmin
from .renderers import AlphabeticalSortedJSONRenderer
from .serializers import (
Expand All @@ -36,6 +36,7 @@
EnvironmentVariableSerializer,
ProjectSerializer,
ProjectCreateSerializer,
ProjectUpdateSerializer,
RedirectCreateSerializer,
RedirectDetailSerializer,
VersionSerializer,
Expand Down Expand Up @@ -73,6 +74,7 @@ class APIv3Settings:

class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin,
FlexFieldsMixin, ProjectImportMixin, CreateModelMixin,
UpdatePartialUpdateMixin, UpdateModelMixin,
ReadOnlyModelViewSet):

# Markdown docstring is automatically rendered by BrowsableAPIRenderer.
Expand Down Expand Up @@ -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':
Expand Down Expand Up @@ -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, UpdatePartialUpdateMixin,
UpdateModelMixin, ReadOnlyModelViewSet):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also allow us to PATCH to Versions?

Copy link
Member Author

@humitos humitos Oct 1, 2019

Choose a reason for hiding this comment

The 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 UpdatePartialUpdateMixin: returns 204 on both verbs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was already working like that, but I moved the update method to a Mixin since it's shared with another class now.


model = Version
lookup_field = 'slug'
Expand All @@ -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):
Expand Down