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

humitos · 2019-07-17T15:29:56Z

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

readthedocs/api/v3/tests/test_projects.py

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.

…ct-update-endpoint

humitos · 2019-09-10T16:50:45Z

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

…humitos/apiv3/project-update-endpoint

ericholscher

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

ericholscher · 2019-10-01T12:43:08Z

docs/api/v3.rst

+            "default_version": "v0.27.0"
+        }
+
+    :statuscode 204: Updated successfully


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

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?

Seems reasonable 👍

ericholscher · 2019-10-01T12:43:32Z

readthedocs/api/v3/mixins.py

@@ -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 is a super confusing name. Why does it have Update in the name twice?

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

ericholscher · 2019-10-01T12:44:36Z

readthedocs/api/v3/permissions.py

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


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

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)

ericholscher · 2019-10-01T12:45:11Z

readthedocs/api/v3/serializers.py

+
+    """Serializer used to modify a Project once imported."""
+
+    repository = RepositorySerializer(source='*')


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

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

ericholscher · 2019-10-01T12:47:00Z

readthedocs/api/v3/views.py

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


Does this also allow us to PATCH to Versions?

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

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

Co-Authored-By: Eric Holscher <[email protected]>

humitos · 2019-10-01T14:26:07Z

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.

…adthedocs/readthedocs.org into humitos/apiv3/project-update-endpoint

ericholscher

Seems good to me 👍

Allow to modify a Project once it's imported

c3044fc

humitos commented Jul 17, 2019

View reviewed changes

readthedocs/api/v3/tests/test_projects.py Show resolved Hide resolved

humitos added Status: blocked Issue is blocked on another issue PR: work in progress Pull request is not ready for full review labels Jul 17, 2019

humitos added 3 commits September 10, 2019 18:36

Get "project_slug" from view kwargs if it's the root endpoint

0601c2d

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.

Limit detail permissions to list/retrieve/superproject

6629c00

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.

Merge remote-tracking branch 'origin/master' into humitos/apiv3/proje…

2a393f5

…ct-update-endpoint

humitos removed PR: work in progress Pull request is not ready for full review Status: blocked Issue is blocked on another issue labels Sep 10, 2019

humitos requested a review from a team September 10, 2019 16:50

Lint

e591184

humitos mentioned this pull request Oct 1, 2019

Create subproject relationship via APIv3 endpoint #6176

Merged

humitos added 2 commits October 1, 2019 13:17

Merge branch 'master' of github.com:readthedocs/readthedocs.org into …

0bb5528

…humitos/apiv3/project-update-endpoint

Docs for Project update endpoint

cdad40f

ericholscher reviewed Oct 1, 2019

View reviewed changes

humitos and others added 3 commits October 1, 2019 15:52

Update docs/api/v3.rst

be0ccb1

Co-Authored-By: Eric Holscher <[email protected]>

Note about permission action string names

61dc78b

Rename class to UpdateMixin and improve its docstring

ca2a1c6

humitos added 2 commits October 1, 2019 16:27

Merge branch 'humitos/apiv3/project-update-endpoint' of github.com:re…

ad12f1f

…adthedocs/readthedocs.org into humitos/apiv3/project-update-endpoint

Verb consistency

13c86e5

ericholscher approved these changes Oct 7, 2019

View reviewed changes

humitos merged commit 7fcdf5c into master Oct 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

humitos commented Jul 17, 2019

humitos commented Sep 10, 2019 •

edited

Loading

ericholscher left a comment

ericholscher Oct 1, 2019

humitos Oct 1, 2019 •

edited

Loading

ericholscher Oct 7, 2019

ericholscher Oct 1, 2019

humitos Oct 1, 2019

ericholscher Oct 1, 2019

humitos Oct 1, 2019

ericholscher Oct 1, 2019

humitos Oct 1, 2019

ericholscher Oct 1, 2019

humitos Oct 1, 2019 •

edited

Loading

humitos Oct 1, 2019

humitos commented Oct 1, 2019

ericholscher left a comment


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

		repository = RepositorySerializer(source='*')

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

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

Conversation

humitos commented Jul 17, 2019

humitos commented Sep 10, 2019 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Oct 1, 2019

ericholscher left a comment

Choose a reason for hiding this comment

humitos commented Sep 10, 2019 •

edited

Loading

humitos Oct 1, 2019 •

edited

Loading

humitos Oct 1, 2019 •

edited

Loading