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

APIv3 "Import Project" endpoint #5857

merged 18 commits into from Jul 9, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jun 27, 2019

This PR allows POST at /api/v3/projects to Import a Project.

It follows the same idea that importing from the Dashboard:

  1. Create the Project object
  2. Assign the user who makes the request as maintainer
  3. Send project_import Django signal
  4. Trigger an initial build

Note that I had to refactor the function that trigger the initial build to be able to call the same logic from two different places.

@humitos humitos requested a review from Jun 27, 2019
@humitos humitos force-pushed the humitos/apiv3-import-project branch from d9f08a9 to 6302256 Jun 27, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This looks good, but I do think it might be useful to keep all the logic together from the view and code. This does it for the trigger_initial_build logic, but it would be useful for the logic around adding the user to the Project, since this varies between sites.

@@ -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

@ericholscher ericholscher Jul 1, 2019

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

@humitos humitos Jul 2, 2019

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

@humitos humitos Jul 2, 2019

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

@davidfischer davidfischer Jul 3, 2019

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.

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

@ericholscher ericholscher Jul 1, 2019

Does this not allow us to update the entire project via API?

Copy link
Member Author

@humitos humitos Jul 2, 2019

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

@davidfischer davidfischer Jul 3, 2019

Seems like a good test case, actually.

Copy link
Member Author

@humitos humitos Jul 8, 2019

Added.

For GET it returns a serializer with many fields and on PUT/PATCH/POST,
it return a serializer to validate just a few fields.
"""
if self.action in ('list', 'retrieve', 'superproject'):
Copy link
Member

@ericholscher ericholscher Jul 1, 2019

what is superproject here? They should probably be constants.

Copy link
Member Author

@humitos humitos Jul 2, 2019

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

@ericholscher ericholscher Jul 8, 2019

Yea, we should add a comment at least, since it's not like the others.

# TODO: these lines need to be adapted for Corporate
project.users.add(request.user)
project_import.send(sender=project, request=request)
trigger_initial_build(project, request.user)
Copy link
Member

@ericholscher ericholscher Jul 1, 2019

I wonder if this should all be abstracted into a single function called trigger_build(initial) and used in all the builds, so we only have to change this logic in one place.

headers = self.get_success_headers(serializer.data)

# TODO: these lines need to be adapted for Corporate
project.users.add(request.user)
Copy link
Member

@ericholscher ericholscher Jul 1, 2019

Where does this happen in the normal import workflow? I can't find it in the code.

Copy link
Member

@ericholscher ericholscher Jul 1, 2019

Copy link
Member

@ericholscher ericholscher Jul 1, 2019

So perhaps we should be inheriting form this form for the Form we're using?

Copy link
Member Author

@humitos humitos Jul 2, 2019

I'm not sure I like to inherit a Serializer from a Wizard Form. Although, maybe these common methods could be extracted into a Mixin that it's usage by both. Actually, in that case, there is nothing to change in corporate since it's already done the override.

Copy link
Member Author

@humitos humitos Jul 2, 2019

This involves a bigger refactor than I thought. All the process to import a project is very tied to forms methods. ProjectBasicForm.save links the remote repository.

ImportWizardView manipulates the Basic and the Extra form to make it all fit in one project before saving it plus triggering a new build (done method in the form)

@humitos
Copy link
Member Author

@humitos humitos commented Jul 2, 2019

This looks good, but I do think it might be useful to keep all the logic together from the view and code

Like a import_project(validated_data, request) helper that, create the project, assign the user, trigger the initial build, etc? I think this can be handy, I need to think a little on the implementation since we probably will need a class so it can be override from corporate (that has some differencies).

@humitos
Copy link
Member Author

@humitos humitos commented Jul 2, 2019

@ericholscher I refactored the Import Project code by extracting the shared bits into a Mixin. Looks better than the first option that I proposed, but still it's not perfect.

All the manipulation of the fields (from the Form or Serializer) happen on each view and once those things are done, the data is passed to the common method that will finish the import.


"""Helpers to import a Project."""

def import_project(self, project, tags, request):
Copy link
Member Author

@humitos humitos Jul 2, 2019

Maybe finish_import_project is a better name here, what do you think @ericholscher? Considering that the Project object is created outside this function and this one does the final steps.

Copy link
Member

@ericholscher ericholscher Jul 8, 2019

That seems better.

'language',
'programming_language',
'repository',
'homepage',
Copy link
Member Author

@humitos humitos Jul 8, 2019

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

  • default_branch
  • default_version
  • tags
  • analytics_code
  • show_version_warning

@humitos humitos force-pushed the humitos/apiv3-import-project branch from e81802b to 83d21ac Jul 8, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Lots of good stuff here, just a few comments.

We also need to add public-facing docs here, that is the biggest missing piece for me.

@@ -110,14 +115,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

@ericholscher ericholscher Jul 8, 2019

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

Copy link
Member Author

@humitos humitos Jul 9, 2019

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

For GET it returns a serializer with many fields and on PUT/PATCH/POST,
it return a serializer to validate just a few fields.
"""
if self.action in ('list', 'retrieve', 'superproject'):
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

Yea, we should add a comment at least, since it's not like the others.

if self.action in ('list', 'retrieve', 'superproject'):
return ProjectSerializer

if self.action in ('create',):
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

any reason not to just use a list here? Or check for the value explicitly?

Copy link
Member Author

@humitos humitos Jul 9, 2019

Just for consistency.

the database.
"""
project = serializer.save()
self.import_project(project, [], self.request)
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

What is the empty list for? We should use kwargs to make it more explicit.

Copy link
Member Author

@humitos humitos Jul 9, 2019

I put request as first argument and make tags an optional with None as default. I think it's better now.


"""Helpers to import a Project."""

def import_project(self, project, tags, request):
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

That seems better.


def import_project(self, project, tags, request):
"""
Import a Project into Read the Docs.
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

As you noted in the PR comment above, we should note what work it assumes has already been done in the code comment as well.


"""Helpers to import a Project."""

def import_project(self, project, tags, request):
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

I might put the request first, as that is the normal order in Django in my experience.

task_promise = chain(
attach_webhook.si(project.pk, user.pk),
update_docs,
)
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

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

@humitos humitos Jul 9, 2019

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.

for field, value in list(form_data.items()):
if field in extra_fields:
setattr(project, field, value)
project.save()

# TODO: this signal could be removed, or used for sync task
project_import.send(sender=project, request=self.request)
self.import_project(project, tags, self.request)
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

kwargs here too would be good.

@humitos
Copy link
Member Author

@humitos humitos commented Jul 9, 2019

We also need to add public-facing docs here, that is the biggest missing piece for me.

I was thinking on finish all the docs in another PR (#5895) so we can deploy this without blocking on docs, which are deployed just with a merge. It's on my list to write them.

@humitos humitos force-pushed the humitos/apiv3-import-project branch from c44f039 to b29b3b5 Jul 9, 2019
@humitos humitos requested a review from ericholscher Jul 9, 2019
@humitos
Copy link
Member Author

@humitos humitos commented Jul 9, 2019

@ericholscher I think I made all the changes needed from your comments. I had some issues when rebasing with master, hopefully I didn't break anything :)

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 9, 2019

Why not just merge master into the branch?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 9, 2019

now I can't really see what commits have changed since my review :/

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 9, 2019

Looking at it though, the changes look good to me 👍

@humitos
Copy link
Member Author

@humitos humitos commented Jul 9, 2019

@ericholscher I'm sorry about that. I usually do rebase against master but, I may need to change that flow because of this problem

@humitos humitos merged commit e574168 into master Jul 9, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/apiv3-import-project branch Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants