Fix action support for ViewSet suffixes (#6081)

* Add suffix support for actions

Removes the newly introduced `action.name` in favor of leveraging the
View's `.get_view_name()` method, which supports both name and suffix.

* Fix view description func docstrings

* Test action decorator name & suffix kwargs

* Adjust 'extra action' docs

Author: rpkilby

docs/api-guide/viewsets.md

@@ -127,7 +127,7 @@ You may inspect these attributes to adjust behaviour based on the current action
 
 ## Marking extra actions for routing
 
-If you have ad-hoc methods that should be routable, you can mark them as such with the `@action` decorator. Like regular actions, extra actions may be intended for either a list of objects, or a single instance. To indicate this, set the `detail` argument to `True` or `False`. The router will configure its URL patterns accordingly. e.g., the `DefaultRouter` will configure detail actions to contain `pk` in their URL patterns.
+If you have ad-hoc methods that should be routable, you can mark them as such with the `@action` decorator. Like regular actions, extra actions may be intended for either a single object, or an entire collection. To indicate this, set the `detail` argument to `True` or `False`. The router will configure its URL patterns accordingly. e.g., the `DefaultRouter` will configure detail actions to contain `pk` in their URL patterns.
 
 A more complete example of extra actions:
 
@@ -174,7 +174,7 @@ The decorator can additionally take extra arguments that will be set for the rou
         def set_password(self, request, pk=None):
            ...
 
-These decorator will route `GET` requests by default, but may also accept other HTTP methods by setting the `methods` argument.  For example:
+The `action` decorator will route `GET` requests by default, but may also accept other HTTP methods by setting the `methods` argument.  For example:
 
         @action(detail=True, methods=['post', 'delete'])
         def unset_password(self, request, pk=None):
@@ -186,7 +186,7 @@ To view all extra actions, call the `.get_extra_actions()` method.
 
 ### Routing additional HTTP methods for extra actions
 
-Extra actions can be mapped to different `ViewSet` methods. For example, the above password set/unset methods could be consolidated into a single route. Note that additional mappings do not accept arguments.
+Extra actions can map additional HTTP methods to separate `ViewSet` methods. For example, the above password set/unset methods could be consolidated into a single route. Note that additional mappings do not accept arguments.
 
 ```python
     @action(detail=True, methods=['put'], name='Change Password')

rest_framework/decorators.py

@@ -131,7 +131,7 @@ def decorator(func):
     return decorator
 
 
-def action(methods=None, detail=None, name=None, url_path=None, url_name=None, **kwargs):
+def action(methods=None, detail=None, url_path=None, url_name=None, **kwargs):
     """
     Mark a ViewSet method as a routable action.
 
@@ -145,18 +145,22 @@ def action(methods=None, detail=None, name=None, url_path=None, url_name=None, *
         "@action() missing required argument: 'detail'"
     )
 
+    # name and suffix are mutually exclusive
+    if 'name' in kwargs and 'suffix' in kwargs:
+        raise TypeError("`name` and `suffix` are mutually exclusive arguments.")
+
     def decorator(func):
         func.mapping = MethodMapper(func, methods)
 
         func.detail = detail
-        func.name = name if name else pretty_name(func.__name__)
         func.url_path = url_path if url_path else func.__name__
         func.url_name = url_name if url_name else func.__name__.replace('_', '-')
         func.kwargs = kwargs
-        func.kwargs.update({
-            'name': func.name,
-            'description': func.__doc__ or None
-        })
+
+        # Set descriptive arguments for viewsets
+        if 'name' not in kwargs and 'suffix' not in kwargs:
+            func.kwargs['name'] = pretty_name(func.__name__)
+        func.kwargs['description'] = func.__doc__ or None
 
         return func
     return decorator

rest_framework/views.py

@@ -23,7 +23,7 @@
 
 def get_view_name(view):
     """
-    Given a view class, return a textual name to represent the view.
+    Given a view instance, return a textual name to represent the view.
     This name is used in the browsable API, and in OPTIONS responses.
 
     This function is the default for the `VIEW_NAME_FUNCTION` setting.
@@ -48,7 +48,7 @@ def get_view_name(view):
 
 def get_view_description(view, html=False):
     """
-    Given a view class, return a textual description to represent the view.
+    Given a view instance, return a textual description to represent the view.
     This name is used in the browsable API, and in OPTIONS responses.
 
     This function is the default for the `VIEW_DESCRIPTION_FUNCTION` setting.

rest_framework/viewsets.py

@@ -183,7 +183,8 @@ def get_extra_action_url_map(self):
             try:
                 url_name = '%s-%s' % (self.basename, action.url_name)
                 url = reverse(url_name, self.args, self.kwargs, request=self.request)
-                action_urls[action.name] = url
+                view = self.__class__(**action.kwargs)
+                action_urls[view.get_view_name()] = url
             except NoReverseMatch:
                 pass  # URL requires additional arguments, ignore
 

tests/test_decorators.py

@@ -179,7 +179,6 @@ def test_action(request):
 
         assert test_action.mapping == {'get': 'test_action'}
         assert test_action.detail is True
-        assert test_action.name == 'Test action'
         assert test_action.url_path == 'test_action'
         assert test_action.url_name == 'test-action'
         assert test_action.kwargs == {
@@ -213,6 +212,47 @@ def method():
         for name in APIView.http_method_names:
             assert test_action.mapping[name] == name
 
+    def test_view_name_kwargs(self):
+        """
+        'name' and 'suffix' are mutually exclusive kwargs used for generating
+        a view's display name.
+        """
+        # by default, generate name from method
+        @action(detail=True)
+        def test_action(request):
+            raise NotImplementedError
+
+        assert test_action.kwargs == {
+            'description': None,
+            'name': 'Test action',
+        }
+
+        # name kwarg supersedes name generation
+        @action(detail=True, name='test name')
+        def test_action(request):
+            raise NotImplementedError
+
+        assert test_action.kwargs == {
+            'description': None,
+            'name': 'test name',
+        }
+
+        # suffix kwarg supersedes name generation
+        @action(detail=True, suffix='Suffix')
+        def test_action(request):
+            raise NotImplementedError
+
+        assert test_action.kwargs == {
+            'description': None,
+            'suffix': 'Suffix',
+        }
+
+        # name + suffix is a conflict.
+        with pytest.raises(TypeError) as excinfo:
+            action(detail=True, name='test name', suffix='Suffix')
+
+        assert str(excinfo.value) == "`name` and `suffix` are mutually exclusive arguments."
+
     def test_method_mapping(self):
         @action(detail=False)
         def test_action(request):
@@ -223,7 +263,7 @@ def test_action_post(request):
             raise NotImplementedError
 
         # The secondary handler methods should not have the action attributes
-        for name in ['mapping', 'detail', 'name', 'url_path', 'url_name', 'kwargs']:
+        for name in ['mapping', 'detail', 'url_path', 'url_name', 'kwargs']:
             assert hasattr(test_action, name) and not hasattr(test_action_post, name)
 
     def test_method_mapping_already_mapped(self):

tests/test_utils.py

@@ -52,6 +52,14 @@ def list_action(self, request, *args, **kwargs):
     def detail_action(self, request, *args, **kwargs):
         raise NotImplementedError
 
+    @action(detail=True, name='Custom Name')
+    def named_action(self, request, *args, **kwargs):
+        raise NotImplementedError
+
+    @action(detail=True, suffix='Custom Suffix')
+    def suffixed_action(self, request, *args, **kwargs):
+        raise NotImplementedError
+
 
 router = SimpleRouter()
 router.register(r'resources', ResourceViewSet)
@@ -145,6 +153,24 @@ def test_modelviewset_detail_action_breadcrumbs(self):
             ('Detail action', '/resources/1/detail_action/'),
         ]
 
+    def test_modelviewset_action_name_kwarg(self):
+        url = '/resources/1/named_action/'
+        assert get_breadcrumbs(url) == [
+            ('Root', '/'),
+            ('Resource List', '/resources/'),
+            ('Resource Instance', '/resources/1/'),
+            ('Custom Name', '/resources/1/named_action/'),
+        ]
+
+    def test_modelviewset_action_suffix_kwarg(self):
+        url = '/resources/1/suffixed_action/'
+        assert get_breadcrumbs(url) == [
+            ('Root', '/'),
+            ('Resource List', '/resources/'),
+            ('Resource Instance', '/resources/1/'),
+            ('Resource Custom Suffix', '/resources/1/suffixed_action/'),
+        ]
+
 
 class JsonFloatTests(TestCase):
     """

tests/test_viewsets.py

@@ -63,9 +63,28 @@ def unresolvable_detail_action(self, request, *args, **kwargs):
         raise NotImplementedError
 
 
+class ActionNamesViewSet(GenericViewSet):
+
+    def retrieve(self, request, *args, **kwargs):
+        return Response()
+
+    @action(detail=True)
+    def unnamed_action(self, request, *args, **kwargs):
+        raise NotImplementedError
+
+    @action(detail=True, name='Custom Name')
+    def named_action(self, request, *args, **kwargs):
+        raise NotImplementedError
+
+    @action(detail=True, suffix='Custom Suffix')
+    def suffixed_action(self, request, *args, **kwargs):
+        raise NotImplementedError
+
+
 router = SimpleRouter()
 router.register(r'actions', ActionViewSet)
 router.register(r'actions-alt', ActionViewSet, basename='actions-alt')
+router.register(r'names', ActionNamesViewSet, basename='names')
 
 
 urlpatterns = [
@@ -172,6 +191,19 @@ def test_detail_view(self):
     def test_uninitialized_view(self):
         self.assertEqual(ActionViewSet().get_extra_action_url_map(), OrderedDict())
 
+    def test_action_names(self):
+        # Action 'name' and 'suffix' kwargs should be respected
+        response = self.client.get('/api/names/1/')
+        view = response.renderer_context['view']
+
+        expected = OrderedDict([
+            ('Custom Name', 'http://testserver/api/names/1/named_action/'),
+            ('Action Names Custom Suffix', 'http://testserver/api/names/1/suffixed_action/'),
+            ('Unnamed action', 'http://testserver/api/names/1/unnamed_action/'),
+        ])
+
+        self.assertEqual(view.get_extra_action_url_map(), expected)
+
 
 @override_settings(ROOT_URLCONF='tests.test_viewsets')
 class ReverseActionTests(TestCase):