-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
APIv3 "Import Project" endpoint #5857
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
fd6b96c
f6073e7
f344d82
9081474
6aa7eda
44d0f2f
ff5b088
a5b0015
2c88524
58bc94e
92c0e8a
3d70928
1896e33
9a19a0e
d550922
f153864
28b7606
b29b3b5
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 |
---|---|---|
|
@@ -9,7 +9,7 @@ | |
from rest_framework import serializers | ||
|
||
from readthedocs.builds.models import Build, Version | ||
from readthedocs.projects.constants import LANGUAGES, PROGRAMMING_LANGUAGES | ||
from readthedocs.projects.constants import LANGUAGES, PROGRAMMING_LANGUAGES, REPO_CHOICES | ||
from readthedocs.projects.models import Project | ||
from readthedocs.redirects.models import Redirect, TYPE_CHOICES as REDIRECT_TYPE_CHOICES | ||
|
||
|
@@ -313,7 +313,10 @@ def get_project_homepage(self, obj): | |
class RepositorySerializer(serializers.Serializer): | ||
|
||
url = serializers.CharField(source='repo') | ||
type = serializers.CharField(source='repo_type') | ||
type = serializers.ChoiceField( | ||
source='repo_type', | ||
choices=REPO_CHOICES, | ||
) | ||
|
||
|
||
class ProjectLinksSerializer(BaseLinksSerializer): | ||
|
@@ -386,6 +389,24 @@ def get_translations(self, obj): | |
return self._absolute_url(path) | ||
|
||
|
||
class ProjectCreateSerializer(FlexFieldsModelSerializer): | ||
|
||
"""Serializer used to Import a Project.""" | ||
|
||
repository = RepositorySerializer(source='*') | ||
homepage = serializers.URLField(source='project_url', required=False) | ||
|
||
class Meta: | ||
model = Project | ||
fields = ( | ||
'name', | ||
'language', | ||
'programming_language', | ||
'repository', | ||
'homepage', | ||
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 can add more fields here. Maybe these are also useful:
|
||
) | ||
|
||
|
||
class ProjectSerializer(FlexFieldsModelSerializer): | ||
|
||
language = LanguageSerializer() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
{ | ||
"_links": { | ||
"_self": "https://readthedocs.org/api/v3/projects/test-project/", | ||
"builds": "https://readthedocs.org/api/v3/projects/test-project/builds/", | ||
"redirects": "https://readthedocs.org/api/v3/projects/test-project/redirects/", | ||
"subprojects": "https://readthedocs.org/api/v3/projects/test-project/subprojects/", | ||
"superproject": "https://readthedocs.org/api/v3/projects/test-project/superproject/", | ||
"translations": "https://readthedocs.org/api/v3/projects/test-project/translations/", | ||
"versions": "https://readthedocs.org/api/v3/projects/test-project/versions/" | ||
}, | ||
"created": "2019-04-29T10:00:00Z", | ||
"default_branch": "master", | ||
"default_version": "latest", | ||
"description": null, | ||
"id": 4, | ||
"language": { | ||
"code": "en", | ||
"name": "English" | ||
}, | ||
"modified": "2019-04-29T12:00:00Z", | ||
"name": "Test Project", | ||
"privacy_level": { | ||
"code": "public", | ||
"name": "Public" | ||
}, | ||
"programming_language": { | ||
"code": "py", | ||
"name": "Python" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "https://github.com/rtfd/template" | ||
}, | ||
"slug": "test-project", | ||
"subproject_of": null, | ||
"tags": [], | ||
"translation_of": null, | ||
"urls": { | ||
"documentation": "http://readthedocs.org/docs/test-project/en/latest/", | ||
"project_homepage": "http://template.readthedocs.io/" | ||
}, | ||
"users": [ | ||
{ | ||
"created": "2019-04-29T10:00:00Z", | ||
"username": "testuser" | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import django_filters.rest_framework as filters | ||
from django.utils.safestring import mark_safe | ||
from rest_flex_fields.views import FlexFieldsMixin | ||
from rest_framework import status | ||
from rest_framework.authentication import TokenAuthentication | ||
from rest_framework.decorators import action | ||
from rest_framework.metadata import SimpleMetadata | ||
|
@@ -20,8 +21,10 @@ | |
from readthedocs.builds.models import Build, Version | ||
from readthedocs.core.utils import trigger_build | ||
from readthedocs.projects.models import Project | ||
from readthedocs.projects.views.mixins import ProjectImportMixin | ||
from readthedocs.redirects.models import Redirect | ||
|
||
|
||
from .filters import BuildFilter, ProjectFilter, VersionFilter | ||
from .mixins import ProjectQuerySetMixin | ||
from .permissions import PublicDetailPrivateListing, IsProjectAdmin | ||
|
@@ -30,6 +33,7 @@ | |
BuildCreateSerializer, | ||
BuildSerializer, | ||
ProjectSerializer, | ||
ProjectCreateSerializer, | ||
RedirectCreateSerializer, | ||
RedirectDetailSerializer, | ||
VersionSerializer, | ||
|
@@ -66,7 +70,8 @@ class APIv3Settings: | |
|
||
|
||
class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
FlexFieldsMixin, ReadOnlyModelViewSet): | ||
FlexFieldsMixin, ProjectImportMixin, CreateModelMixin, | ||
ReadOnlyModelViewSet): | ||
|
||
# Markdown docstring is automatically rendered by BrowsableAPIRenderer. | ||
|
||
|
@@ -114,14 +119,13 @@ class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | |
* Subprojects of a project: ``/api/v3/projects/{project_slug}/subprojects/`` | ||
* Superproject of a project: ``/api/v3/projects/{project_slug}/superproject/`` | ||
|
||
Go to [https://docs.readthedocs.io/en/stable/api/v3.html](https://docs.readthedocs.io/en/stable/api/v3.html) | ||
Go to [https://docs.readthedocs.io/page/api/v3.html](https://docs.readthedocs.io/page/api/v3.html) | ||
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. I think this should auto-link, no? Or you can put 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's rendered as Markdown using "some implementation" and |
||
for a complete documentation of the APIv3. | ||
""" # noqa | ||
|
||
model = Project | ||
lookup_field = 'slug' | ||
lookup_url_kwarg = 'project_slug' | ||
serializer_class = ProjectSerializer | ||
filterset_class = ProjectFilter | ||
queryset = Project.objects.all() | ||
permit_list_expands = [ | ||
|
@@ -130,6 +134,21 @@ class ProjectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | |
'active_versions.last_build.config', | ||
] | ||
|
||
def get_serializer_class(self): | ||
""" | ||
Return correct serializer depending on the action. | ||
|
||
For GET it returns a serializer with many fields and on PUT/PATCH/POST, | ||
it return a serializer to validate just a few fields. | ||
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 not allow us to update the entire project via API? 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. No, in theory only the fields from We may want to add more, though. 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. Seems like a good test case, actually. 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. Added. |
||
""" | ||
if self.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. 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.
I'm not sure if it makes sense to make it a constant. Although, maybe it's good to add a comment in this line to avoid confusions. 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. Yea, we should add a comment at least, since it's not like the others. |
||
# NOTE: ``superproject`` is the @action defined in the | ||
# ProjectViewSet that returns the superproject of a project. | ||
return ProjectSerializer | ||
|
||
if self.action == 'create': | ||
return ProjectCreateSerializer | ||
|
||
def get_queryset(self): | ||
# Allow hitting ``/api/v3/projects/`` to list their own projects | ||
if self.basename == 'projects' and self.action == 'list': | ||
|
@@ -165,6 +184,32 @@ def get_view_description(self, *args, **kwargs): # pylint: disable=arguments-di | |
return mark_safe(description.format(project_slug=project.slug)) | ||
return description | ||
|
||
def create(self, request, *args, **kwargs): | ||
""" | ||
Import Project. | ||
|
||
Override to use a different serializer in the response. | ||
""" | ||
serializer = self.get_serializer(data=request.data) | ||
serializer.is_valid(raise_exception=True) | ||
self.perform_create(serializer) | ||
headers = self.get_success_headers(serializer.data) | ||
|
||
# Use serializer that fully render a Project | ||
serializer = ProjectSerializer(instance=serializer.instance) | ||
|
||
return Response(serializer.data, status=status.HTTP_201_CREATED, headers=headers) | ||
|
||
def perform_create(self, serializer): | ||
""" | ||
Import Project. | ||
|
||
Trigger our internal mechanism to import a project after it's saved in | ||
the database. | ||
""" | ||
project = serializer.save() | ||
self.finish_import_project(self.request, project) | ||
|
||
@action(detail=True, methods=['get']) | ||
def superproject(self, request, project_slug): | ||
"""Return the superproject of a ``Project``.""" | ||
|
@@ -174,7 +219,7 @@ def superproject(self, request, project_slug): | |
data = self.get_serializer(superproject).data | ||
return Response(data) | ||
except Exception: | ||
return Response(status=404) | ||
return Response(status=status.HTTP_404_NOT_FOUND) | ||
|
||
|
||
class SubprojectRelationshipViewSet(APIv3Settings, NestedViewSetMixin, | ||
|
@@ -263,7 +308,7 @@ def update(self, request, *args, **kwargs): | |
# ``httpOnly`` on our cookies and the ``PUT/PATCH`` method are triggered | ||
# via Javascript | ||
super().update(request, *args, **kwargs) | ||
return Response(status=204) | ||
return Response(status=status.HTTP_204_NO_CONTENT) | ||
|
||
|
||
class BuildsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
|
@@ -303,11 +348,11 @@ def create(self, request, **kwargs): # pylint: disable=arguments-differ | |
|
||
if build: | ||
data.update({'triggered': True}) | ||
status = 202 | ||
code = status.HTTP_202_ACCEPTED | ||
else: | ||
data.update({'triggered': False}) | ||
status = 400 | ||
return Response(data=data, status=status) | ||
code = status.HTTP_400_BAD_REQUEST | ||
return Response(data=data, status=code) | ||
|
||
|
||
class RedirectsViewSet(APIv3Settings, NestedViewSetMixin, ProjectQuerySetMixin, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
|
||
"""Mixin classes for project views.""" | ||
|
||
from celery import chain | ||
from django.shortcuts import get_object_or_404 | ||
|
||
from readthedocs.core.utils import prepare_build | ||
from readthedocs.projects.models import Project | ||
from readthedocs.projects.signals import project_import | ||
|
||
|
||
class ProjectRelationMixin: | ||
|
@@ -44,3 +47,55 @@ def get_context_data(self, **kwargs): | |
context = super().get_context_data(**kwargs) | ||
context[self.project_context_object_name] = self.get_project() | ||
return context | ||
|
||
|
||
class ProjectImportMixin: | ||
|
||
"""Helpers to import a Project.""" | ||
|
||
def finish_import_project(self, request, project, tags=None): | ||
""" | ||
Perform last steps to import a project into Read the Docs. | ||
|
||
- Add the user from request as maintainer | ||
- Set all the tags to the project | ||
- Send Django Signal | ||
- Trigger initial build | ||
|
||
It requires the Project was already saved into the DB. | ||
|
||
:param request: Django Request object | ||
:param project: Project instance just imported (already saved) | ||
:param tags: tags to add to the project | ||
""" | ||
if not tags: | ||
tags = [] | ||
|
||
project.users.add(request.user) | ||
for tag in tags: | ||
project.tags.add(tag) | ||
|
||
# TODO: this signal could be removed, or used for sync task | ||
project_import.send(sender=project, request=request) | ||
|
||
self.trigger_initial_build(project, request.user) | ||
|
||
def trigger_initial_build(self, project, user): | ||
""" | ||
Trigger initial build after project is imported. | ||
|
||
:param project: project's documentation to be built | ||
:returns: Celery AsyncResult promise | ||
""" | ||
|
||
update_docs, build = prepare_build(project) | ||
if (update_docs, build) == (None, None): | ||
return None | ||
|
||
from readthedocs.oauth.tasks import attach_webhook | ||
task_promise = chain( | ||
attach_webhook.si(project.pk, user.pk), | ||
update_docs, | ||
) | ||
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. Do we need to chain these? Seems like we still want to update the docs even if webhook setup fails, and they don't depend on each other in any real way. 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 do a chain just to follow an order. If Anyways, this logic is not something I want to change/refactor in this PR because it's something bigger than this. |
||
async_result = task_promise.apply_async() | ||
return async_result |
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.
Do we want to support that? I feel like creating a project via the web form is probably not going to be the best UX.
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've been thinking on making the browsable API consistent with what the API offers. Besides consistency, it helps to explore it and have a better understanding of what's available and what can be done: not "self-documented" but in that direction.
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 other hand, removing the
create
from this check it fails with 404 when accessing to/api/v3/projects/
because of an internal DRF check. Actually, that's how I realized that I needed it :)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.
Overall I'm in favor of making the browsable API be consistent with what the API can do. We still can restrict creating with additional permissions or even turn off the browsable API in prod completely if needed (hopefully not as I do believe the browsable API makes the API more approachable). Generally though I hope regular users are not creating projects in production through the DRF UI.