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 "Import Project" endpoint #5857

Merged
merged 18 commits into from
Jul 9, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions readthedocs/api/v3/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ def has_permission(self, request, view):
if is_authenticated:
if view.basename == 'projects' and any([
view.action == 'list',
view.action == 'create', # used to create Form in BrowsableAPIRenderer
Copy link
Member

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.

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

Copy link
Member Author

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

Copy link
Contributor

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.

view.action is None, # needed for BrowsableAPIRenderer
]):
# hitting ``/projects/``, allowing
Expand Down
25 changes: 23 additions & 2 deletions readthedocs/api/v3/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

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

We can add more fields here. Maybe these are also useful:

  • default_branch
  • default_version
  • tags
  • analytics_code
  • show_version_warning

)


class ProjectSerializer(FlexFieldsModelSerializer):

language = LanguageSerializer()
Expand Down
48 changes: 48 additions & 0 deletions readthedocs/api/v3/tests/responses/projects-list_POST.json
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"
}
]
}
64 changes: 64 additions & 0 deletions readthedocs/api/v3/tests/test_projects.py
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ def test_projects_versions_detail_unique(self):
)
self.assertEqual(response.status_code, 200)


def test_unauthed_projects_redirects_list(self):
response = self.client.get(
reverse(
Expand Down Expand Up @@ -529,3 +530,66 @@ def test_projects_redirects_detail_delete(self):
)
self.assertEqual(response.status_code, 204)
self.assertEqual(self.project.redirects.count(), 0)


def test_import_project(self):
data = {
'name': 'Test Project',
'repository': {
'url': 'https://github.com/rtfd/template',
'type': 'git',
},
'homepage': 'http://template.readthedocs.io/',
'programming_language': 'py',
}

self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
response = self.client.post(reverse('projects-list'), data)
self.assertEqual(response.status_code, 201)

query = Project.objects.filter(slug='test-project')
self.assertTrue(query.exists())

project = query.first()
self.assertEqual(project.name, 'Test Project')
self.assertEqual(project.slug, 'test-project')
self.assertEqual(project.repo, 'https://github.com/rtfd/template')
self.assertEqual(project.language, 'en')
self.assertEqual(project.programming_language, 'py')
self.assertEqual(project.privacy_level, 'public')
self.assertEqual(project.project_url, 'http://template.readthedocs.io/')
self.assertIn(self.me, project.users.all())
self.assertEqual(project.builds.count(), 1)

response_json = response.json()
response_json['created'] = '2019-04-29T10:00:00Z'
response_json['modified'] = '2019-04-29T12:00:00Z'

self.assertDictEqual(
response_json,
self._get_response_dict('projects-list_POST'),
)

def test_import_project_with_extra_fields(self):
data = {
'name': 'Test Project',
'repository': {
'url': 'https://github.com/rtfd/template',
'type': 'git',
},
'default_version': 'v1.0', # ignored: field not allowed
}

self.client.credentials(HTTP_AUTHORIZATION=f'Token {self.token.key}')
response = self.client.post(reverse('projects-list'), data)
self.assertEqual(response.status_code, 201)

query = Project.objects.filter(slug='test-project')
self.assertTrue(query.exists())

project = query.first()
self.assertEqual(project.name, 'Test Project')
self.assertEqual(project.slug, 'test-project')
self.assertEqual(project.repo, 'https://github.com/rtfd/template')
self.assertNotEqual(project.default_version, 'v1.0')
self.assertIn(self.me, project.users.all())
61 changes: 53 additions & 8 deletions readthedocs/api/v3/views.py
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
Expand All @@ -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
Expand All @@ -30,6 +33,7 @@
BuildCreateSerializer,
BuildSerializer,
ProjectSerializer,
ProjectCreateSerializer,
RedirectCreateSerializer,
RedirectDetailSerializer,
VersionSerializer,
Expand Down Expand Up @@ -66,7 +70,8 @@ class APIv3Settings:


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

# Markdown docstring is automatically rendered by BrowsableAPIRenderer.

Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

I think this should auto-link, no? Or you can put <> around it I believe (depending on markdown implementation, 😦)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It's rendered as Markdown using "some implementation" and [Text](Link) is the format that works.

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 = [
Expand All @@ -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.
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 not allow us to update the entire project via API?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, in theory only the fields from ProjectCreateSerializer.Meta.fields are allowed (name, language, repository and project_url). If there are extra fields passed, they are ignored.

We may want to add more, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good test case, actually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

"""
if self.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.

what is superproject here? They should probably be constants.

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 the @action defined in the ProjectViewSet that returns the superproject of a project.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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':
Expand Down Expand Up @@ -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``."""
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
55 changes: 55 additions & 0 deletions readthedocs/projects/views/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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,
)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We do a chain just to follow an order. If attach_webhook fails, it handles it by itself, set a message to the user and the build process continue. It does not block the update of the docs.

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
Loading