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

Improvements to ViewSet extra actions #5605

Merged
merged 6 commits into from Jul 6, 2018

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 17, 2017

Resolves #2062, #2934, #3271, #5755
Fixes #4920

This will obviously require more extensive testing and documentation updates, but I'd like to hammer out the implementation details first. But first, here's a preview of the changes to the browsable API:

change-password

The above changes include:

  • Breadcrumb and page title support custom names/titles for actions instead of just "User instance".
  • The method docstring is now used as the action's description.
  • A dropdown of "Extra Actions" (appropriately filtered to detail/non-detail actions).
  • Also, note that custom serializer classes were already supported.

To implement this, it was necessary to change how routers and viewsets interact. At a higher level:

  • Views more explicitly customize their display name and description.
  • ViewSets are provided more context/configuration from the router. For example, it can now determine if the current request is operation on a list or detail action.

In more detail detail:

  • Note that these changes build off of Add '.basename' and '.reverse_action()' to ViewSet #5648 and Rework dynamic list/detail actions #5705, which were spun out of this PR.
  • Views now accept name and description via the initkwargs.
    • This provides a general hook for customizing their display on the browsable API.
    • This is necessary to allow action methods to specify they're own name/description.
  • get_view_name and get_view_description now expect the View instance instead of a class.
  • get_view_name and get_view_description respect the new View name and description.
  • ViewSets consider name and suffix to be mutually exclusive.
  • The action decorator now allows users to specify the name, uses the docstring as the description.
  • Adds ViewSet.get_extra_action_url_map for rendering extra actions.
  • Adds renderer/template code for both the browsable and admin APIs.
  • Adds method mapping to ViewSet actions. Multiple ViewSet methods can handle different HTTP methods of an action. ex:
    class MyViewSet(ViewSet):
    
        @action(detail=False)
        def example(self, request, **kwargs):
            ...
    
        @example.mapping.post
        def create_example(self, request, **kwargs):
            ...

TODO:

@rpkilby rpkilby added this to the v3.8 milestone Nov 17, 2017
@rpkilby rpkilby force-pushed the viewset-actions branch 2 times, most recently from 27a2e93 to 7bac555 Compare Nov 18, 2017
Copy link
Contributor

@auvipy auvipy left a comment

I like the naming of action rater then detail/list route.

how many routable actions will be possible on a single viewset?

@rpkilby
Copy link
Member Author

rpkilby commented Nov 18, 2017

I like the naming of action rater then detail/list route.

Agreed @auvipy. It's not so much that I'm in favor of action, more that I've never particularly liked list_route/detail_route.

how many routable actions will be possible on a single viewset?

There isn't any limit, as there wasn't any limit to list/detail routes.

SIde note: more commits incoming

@auvipy
Copy link
Contributor

auvipy commented Nov 18, 2017

It would be great to see some good example as part of this pr. thanks for the great work!

@rpkilby
Copy link
Member Author

rpkilby commented Nov 18, 2017

@auvipy, right now, there's a screenshot in the original comment, which should demonstrate the practical changes to the browsable API. As far as code goes, it's pretty much just:

class MyViewSet(...):
    @action(methods=['get'], detail=True, name='Custom name'
            serializer_class=ThingySerializer)
    def do_a_thing(self, request, pk, *args, **kwargs):
        """
        # Docstrings are now used as descriptions!
        """

@auvipy
Copy link
Contributor

auvipy commented Nov 18, 2017

How this change would affect this #5551?

@rpkilby
Copy link
Member Author

rpkilby commented Nov 18, 2017

These changes aren't directly related to #5551.

warnings.warn(
"`detail_route` has been deprecated in favor of `action`, which accepts a "
"`detail` boolean. Use `@action(['GET'], detail=True)` instead.",
PendingDeprecationWarning
Copy link
Contributor

@merwok merwok Nov 18, 2017

Choose a reason for hiding this comment

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

I think this needs a stacklevel parameter so that the source code in the traceback points to the file using detail_route, not this file.

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. Interesting.

Initial thought is 😱 breaking change but with the deprecation it's not a big issue I think.

I need to sit down with the implementation details but cool. 👍

Used to mark a method on a ViewSet that should be routed for detail requests.
"""
warnings.warn(
"`detail_route` has been deprecated in favor of `action`, which accepts a "
Copy link
Collaborator

@carltongibson carltongibson Nov 20, 2017

Choose a reason for hiding this comment

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

This is quite a big change for users. I think I'd like to see the message here saying, "pending deprecation" and "will be removed in 3.10" so that it's mega-clear what sort of timescale users have.

  • This will require updating these messages for 3.9 (unless we're clever with the wording) but I think we need to be extra helpful here.
  • I think this also merits a beginning on the release notes.

Copy link
Member Author

@rpkilby rpkilby Nov 21, 2017

Choose a reason for hiding this comment

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

Agreed. My main consideration right now is to determine if this is the path we ultimately want to move toward, and then work backwards from that w/ deprecations and whatnot.

Right now, the state of the PR is more exploratory. For example, I also want to try and fix #4920, which will require more modifications to the dynamic route generation.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Hey @rpkilby. This is pay off from #5705. Fancy re-doing from that as a base? (If it's not too much faff.)

@carltongibson
Copy link
Collaborator

carltongibson commented Mar 26, 2018

Hey @rpkilby. Any appetite for pushing this over the line? It looks like it just needs docs and a few test cases

@carltongibson carltongibson removed this from the 3.8 Release milestone Apr 3, 2018
@rpkilby rpkilby force-pushed the viewset-actions branch 2 times, most recently from 144c55d to b4d3eab Compare May 16, 2018
@rpkilby rpkilby changed the title [wip] Viewset extra actions Improvements to ViewSet extra actions May 16, 2018
@rpkilby rpkilby added this to the 3.9 Release milestone May 16, 2018
@rpkilby
Copy link
Member Author

rpkilby commented May 16, 2018

Hi all - the PR is largely done and ready for review (documentation not withstanding). In addition to tests, the updates implements #4920 and copies the "Extra Actions" dropdown from the Browsable API to the Admin API template.

@rpkilby rpkilby force-pushed the viewset-actions branch 3 times, most recently from 597415d to c604380 Compare Jun 25, 2018
rpkilby added 2 commits Jun 25, 2018
Removed old test logic around link/action decorators from `v2.3`. Also
simplified the test by making the results explicit instead of computed.
@rpkilby
Copy link
Member Author

rpkilby commented Jun 25, 2018

Okay, added some documentation. This includes:

  • Updated the VIEW_NAME_FUNCTION/VIEW_DESCRIPTION_FUNCTION docs to the new signature, and documented the relevant ViewSet optional arguments (name, suffix, detail, description).
  • Document the new method mapping enhancement for the action decorator.

@carltongibson carltongibson merged commit 0148a9f into encode:master Jul 6, 2018
1 check passed
@rpkilby rpkilby deleted the viewset-actions branch Jul 6, 2018
@noisy
Copy link

noisy commented Jul 14, 2018

I just found this pull request, because I tried to make my actions visible in browsable api. Thank you for this work.

Right now I am developing my project installing DRF directly from git. @carltongibson - do you know when we can expect a new version of DRF with this feature included?

@rpkilby
Copy link
Member Author

rpkilby commented Jul 14, 2018

Brief aside: I know there is a little more work that needs to be done before a 3.9 release can be shipped, but one minor blocker is #6081, which fixes a flaw in this pull request. I should have time to work on it this weekend.

@carltongibson
Copy link
Collaborator

carltongibson commented Jul 15, 2018

@noisy we’re working on the 3.9 release now. I would think a few weeks. Pip’s VCS support is first class. There’s no reason why you can’t use the version from master.

soerface added a commit to soerface/django-rest-framework that referenced this pull request Jan 18, 2019
The documentation does currently not describe how to use a custom
`serializer_class` with extra actions. The example uses a custom
serializer without using the seralizer_class attribute, resulting in the
generated documentation page presenting the wrong form.

The author of the feature described the usage of the `serializer_class`
kwarg in his pr encode#5605:
encode#5605 (comment)
soerface added a commit to soerface/django-rest-framework that referenced this pull request Jan 18, 2019
The documentation does currently not describe how to use a custom
`serializer_class` with extra actions. The example uses a custom
serializer without using the seralizer_class attribute, resulting in the
generated documentation page presenting the wrong form.

The author of the feature described the usage of the `serializer_class`
kwarg in his pr encode#5605:
encode#5605 (comment)
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* View suffix already set by initializer

* Add 'name' and 'description' attributes to ViewSet

ViewSets may now provide their `name` and `description` attributes
directly, instead of relying on view introspection to derive them.
These attributes may also be provided with the view's initkwargs.

The ViewSet `name` and `suffix` initkwargs are mutually exclusive.

The `action` decorator now provides the `name` and `description` to
the view's initkwargs. By default, these values are derived from the
method name and its docstring. The `name` may be overridden by providing
it as an argument to the decorator.

The `get_view_name` and `get_view_description` hooks now provide the
view instance to the handler, instead of the view class. The default
implementations of these handlers now respect the `name`/`description`.

* Add 'extra actions' to ViewSet & browsable APIs

* Update simple router tests

Removed old test logic around link/action decorators from `v2.3`. Also
simplified the test by making the results explicit instead of computed.

* Add method mapping to ViewSet actions

* Document extra action method mapping
@gabn88
Copy link
Contributor

gabn88 commented Mar 22, 2021

@carltongibson @rpkilby
Great work with this addition!
I was wondering for almost a year why I couldn't get the "extra actions" button on my production site. In tests it worked, but on my live site I just couldn't get it working.
Until I saw: #7500 which fixed it right away.
Would it be possible to incorporate this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants