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

Rework dynamic list/detail actions #5705

Merged
merged 13 commits into from Jan 25, 2018

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Dec 22, 2017

Pulling a slightly more manageable chunk out of #5605 for review. The goal is to remove logic from the router in order to make the viewsets and list/detail actions more introspectable.

Changes:

  • Deprecated list_route & detail_route in favor of action decorator w/ detail boolean.
  • Deprecated dynamic list/detail route variants in favor of DynamicRoute w/ detail boolean.
  • Added detail boolean argument to regular Route
  • A route's detail is now provided as an initkwarg to the ViewSet, similar to suffix.
  • Added ViewSet.get_extra_actions() to formalize extra action access.
  • Added url_path and url_name to action signature. These values are no longer set/altered by the router, and can be more reliably inspected by the parent viewset.
  • Refactored dynamic route generation, replacing protected method w/ internal instance method.

For the release notes, I've added a section to discuss the changes as a larger feature.

TODO:

  • Add release notes
  • Replace deprecated functions in tests
  • Update docs w/ changes

@rpkilby rpkilby added this to the 3.8 Release milestone Dec 22, 2017
Copy link
Collaborator

@carltongibson carltongibson left a comment

I think this is nicely done. Good benefits. Worth having.

@rpkilby, again, thanks for the effort. If you can tick off the TODOs lets have it. 👍

* Merged `list_route` and `detail_route` into a single `action` decorator.
* Get all extra actions on a `ViewSet` with `.get_extra_actions()`.
* Extra actions now set the `url_name` and `url_path` on the decorated method.
* Enable action url reversing through `.reverse_action()` method (added in 3.6.4)
Copy link
Collaborator

@carltongibson carltongibson Jan 2, 2018

Choose a reason for hiding this comment

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

...3.7.4...

@rpkilby rpkilby force-pushed the action-decorator branch 2 times, most recently from 96b3531 to a591e16 Compare Jan 3, 2018
@rpkilby rpkilby force-pushed the action-decorator branch 2 times, most recently from 01f568f to 151c0c9 Compare Jan 3, 2018
@rpkilby
Copy link
Member Author

rpkilby commented Jan 4, 2018

Okay, this should be ready to go now. The docs changes are not completely trivial and feedback would be appreciated.


class UserViewSet(ModelViewSet):
...

@detail_route(methods=['post'], permission_classes=[IsAdminOrIsSelf])
@action(methods=['post'], detail=True, permission_classes=[IsAdminOrIsSelf])
Copy link
Collaborator

@carltongibson carltongibson Jan 4, 2018

Choose a reason for hiding this comment

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

At this point the detail parameter is mysterious. (Worth explaining it below the example.)

Copy link
Member Author

@rpkilby rpkilby Jan 4, 2018

Choose a reason for hiding this comment

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

Would it be sufficient to link to the viewset docs first? e.g., "go read this first, then come back here"

Copy link
Collaborator

@carltongibson carltongibson Jan 4, 2018

Choose a reason for hiding this comment

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

Yes. (I was thinking along those lines... 🙂)

Perhaps moving the note from ln124 up and into a Note: section may be worthwhile. (Who reads about Routers before ViewSets?)

Copy link
Member Author

@rpkilby rpkilby Jan 8, 2018

Choose a reason for hiding this comment

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

Updated the docs. Integrated line 124 into the start of the section.

@@ -130,29 +130,45 @@ def decorator(func):
return decorator


def detail_route(methods=None, **kwargs):
def action(methods=None, detail=True, url_path=None, url_name=None, **kwargs):
Copy link
Collaborator

@carltongibson carltongibson Jan 4, 2018

Choose a reason for hiding this comment

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

So, the detail parameter...:

  • Do we want an implied default? Or do we want to make it required, with a helpful assertion?
  • If implied which is the best default? i.e. Do we generate list or detail routes if you don't pass the parameter.
  • Do we want to document whichever we choose as not passing the parameter? @action(detail=True) and @action() in all the examples, say?

Not sure what I think there.

Copy link
Member Author

@rpkilby rpkilby Jan 8, 2018

Choose a reason for hiding this comment

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

Do we want an implied default? Or do we want to make it required, with a helpful assertion?

I don't think a default is beneficial here. Making it a required arg seems 👍 to me.

Copy link
Collaborator

@carltongibson carltongibson left a comment

OK. This looks good.

I'm just going to pause on this for now, since once it's in we're kind of committed to rolling a 3.8. (which is fine, but I just want to plan that first. 🙂) //cc @tomchristie

@carltongibson carltongibson merged commit 73203e6 into encode:master Jan 25, 2018
1 check passed
@rpkilby rpkilby deleted the action-decorator branch Jan 25, 2018
@xordoquy
Copy link
Collaborator

xordoquy commented Apr 4, 2018

It seems that there are some regression on list_route & detail_route.
If no url_name was provided, it used to default on the url_path:

url_name = initkwargs.pop("url_name", None) or url_path

With the new code, it default to the function name:

func.url_name = url_name or func.__name__.replace('_', '-')

which breaks a couple of tests here and there.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 4, 2018

Hey @xordoquy. Would you be able to put a regression test together for that?

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 4, 2018

need to investigate another issue and I'll do that.

@rpkilby
Copy link
Member Author

rpkilby commented Apr 4, 2018

Hi @xordoquy - this was an intentional change, as the url_path is not always a suitable default for the name. eg, when the path contains URL parameters.

Also, sorry for being afk lately - life has been pretty busy the last couple of months.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 4, 2018

that's ok, but this should be added to the breaking changes since it requires changes on detail_route and list_route.

@rpkilby
Copy link
Member Author

rpkilby commented Apr 4, 2018

Maybe the path forward is for action to have this new behavior, with list_route and detail_route maintaining the old behavior.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 4, 2018

I need to have a look at exactly what’s going on here but could we not handle the change in the shim?

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 4, 2018

@rpkilby you must not apologise for life. Contribution to Open Source needs to be scoped. There is no problem with that. Ever. The only problem is with the opposite. 🙂

@rpkilby
Copy link
Member Author

rpkilby commented Apr 4, 2018

Thanks - just letting you know I'm still around.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 4, 2018

Maybe the path forward is for action to have this new behavior, with list_route and detail_route maintaining the old behavior.

sounds sensible to me.

Also, sorry for being afk lately

Didn't react but seconding @carltongibson on that point, you don't need to apologize for that. You did a lot of work so far, and for that we owe you one.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 4, 2018

v.3.8.1 restores the old behaviour (via #5915). It's on PyPI now.

@jaap3
Copy link
Contributor

jaap3 commented Apr 5, 2018

Thanks you for the fix and the prompt release of v3.8.1

nijel added a commit to WeblateOrg/weblate that referenced this issue Apr 18, 2018
This is needed to avoid deprecation warnigs on recent versions,
see encode/django-rest-framework#5705

Signed-off-by: Michal Čihař <michal@cihar.com>
@Allan-Nava

This comment has been minimized.

@carltongibson
Copy link
Collaborator

carltongibson commented Apr 23, 2018

@Allan-Nava Please direct usage questions to Stack Overflow or the mailing list. (Again, the issue tracker is for bug reports.) Thanks.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this issue Nov 17, 2020
* Merge list/detail route decorators into 'action'

* Merge dynamic routes, add 'detail' attribute

* Add 'ViewSet.get_extra_actions()'

* Refactor dynamic route checking & collection

* Refactor dynamic route generation

* Add 'ViewSet.detail' initkwarg

* Fixup schema test

* Add release notes for dynamic action changes

* Replace list/detail route decorators in tests

* Convert tabs to spaces in router docs

* Update docs

* Make 'detail' a required argument of 'action'

* Improve router docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants