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

Conversation

@humitos
Copy link
Member

commented Jul 17, 2019

  • allow users to modify existing projects using the API
  • only allow to change fields that are not possible to change via the YAML
  • users should only be able to change projects where they are maintainers
humitos added 3 commits Sep 10, 2019
We are nesting all the endpoints inside `/projects/`. When hitting,
`/project/<slug>` this endpoint does not have any "parent project", so
we need to get the "project_slug" from the view kwargs instead of from
the URL.
Do not allow any other action that's not list, retrieve or
superproject when the detail=True (accessing an object directly)

`/subprojects/` and `/translations/` are considered `list` actions
with detail=True views.
@humitos humitos requested a review from readthedocs/core Sep 10, 2019
@humitos

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2019

This PR should be ready to review and all the permissions should be fixed and working as we need.

humitos added 2 commits Oct 1, 2019
…humitos/apiv3/project-update-endpoint
Copy link
Member

left a comment

This seems like a good change, but I don't fully understand it -- where is the logic that actually makes it so that PATCH works now? Is it the overridden update method? Is it the change to the serializer?

docs/api/v3.rst Outdated Show resolved Hide resolved
"default_version": "v0.27.0"
}

:statuscode 204: Updated successfully

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 1, 2019

Member

I assume it will also raise an error in some cases?

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

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?

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 7, 2019

Member

Seems reasonable 👍

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

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 1, 2019

Member

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

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 1, 2019

Member

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

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

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)


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

repository = RepositorySerializer(source='*')

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 1, 2019

Member

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

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@ericholscher

ericholscher Oct 1, 2019

Member

Does this also allow us to PATCH to Versions?

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

Yes. It makes the PUT and PATCH to behave in the same way (because of UpdatePartialUpdateMixin: returns 204 on both verbs).

This comment has been minimized.

Copy link
@humitos

humitos Oct 1, 2019

Author Member

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

humitos and others added 3 commits Oct 1, 2019
Co-Authored-By: Eric Holscher <25510+ericholscher@users.noreply.github.com>
@humitos

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2019

where is the logic that actually makes it so that PATCH works now?Is it the overridden update method?

Yes.

Basically, this is the line that allows the PATCH/PUT: https://github.com/readthedocs/readthedocs.org/pull/5952/files#diff-e7d0914abe4a0641d49078f90f75e695R77

UpdateModelMixin defines the update method (PUT) and partial_update (PATCH) and makes DRF to "listen" to these verbs as well. The rest of the PR are permission handling, refactor, tests and docs.

humitos added 2 commits Oct 1, 2019
…adthedocs/readthedocs.org into humitos/apiv3/project-update-endpoint
Copy link
Member

left a comment

Seems good to me 👍

@humitos humitos merged commit 7fcdf5c into master Oct 8, 2019
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.