Move end nomination operation to PATCH endpoint

2 files changed, 79 insertions, 81 deletions

diff --git a/pydis_site/apps/api/tests/test_nominations.py b/pydis_site/apps/api/tests/test_nominations.py
index 206dc65b..add5a7e4 100644
--- a/pydis_site/apps/api/tests/test_nominations.py
+++ b/pydis_site/apps/api/tests/test_nominations.py
@@ -240,7 +240,7 @@ class NominationTests(APISubdomainTestCase):
         response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json(), {
-            'end_reason': ["An active nomination can't have an unnominate reason."]
+            'end_reason': ["An active nomination can't have an end reason."]
         })
 
     def test_returns_200_update_reason_on_inactive(self):
@@ -274,9 +274,10 @@ class NominationTests(APISubdomainTestCase):
             host='api'
         )
         data = {
+            'active': False,
             'end_reason': 'He started playing jazz'
         }
-        response = self.client.put(url, data=data)
+        response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 200)
 
         nomination = Nomination.objects.get(id=response.json()['id'])
@@ -296,12 +297,13 @@ class NominationTests(APISubdomainTestCase):
             host='api'
         )
         data = {
-            'reason': 'Why does a whale have feet?'
+            'active': False,
+            'reason': 'Why does a whale have feet?',
         }
-        response = self.client.put(url, data=data)
+        response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json(), {
-            'reason': ['This field cannot be set at end_nomination.']
+            'reason': ['This field cannot be set when ending a nomination.']
         })
 
     def test_returns_400_on_missing_end_reason_end_nomination(self):
@@ -310,28 +312,30 @@ class NominationTests(APISubdomainTestCase):
             args=(self.active_nomination.id,),
             host='api'
         )
-        data = {}
+        data = {
+            'active': False,
+        }
 
-        response = self.client.put(url, data=data)
+        response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json(), {
             'end_reason': ['This field is required when ending a nomination.']
         })
 
-    def test_returns_400_on_ending_inactive_nomination(self):
+    def test_returns_400_on_invalid_use_of_active(self):
         url = reverse(
             'bot:nomination-detail',
             args=(self.inactive_nomination.id,),
             host='api'
         )
         data = {
-            'end_reason': 'He started playing jazz'
+            'active': False,
         }
 
-        response = self.client.put(url, data=data)
+        response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 400)
         self.assertEqual(response.json(), {
-            'active': ['A nomination must be active to be ended.']
+            'active': ['This field can only be used to end a nomination']
         })
 
     def test_returns_404_on_get_unknown_nomination(self):
@@ -360,23 +364,6 @@ class NominationTests(APISubdomainTestCase):
             "detail": "Not found."
         })
 
-    def test_returns_404_on_end_unknown_nomination(self):
-        url = reverse(
-            'bot:nomination-detail',
-            args=(9999,),
-            host='api'
-        )
-
-        data = {
-            'end_reason': 'He started playing jazz'
-        }
-
-        response = self.client.put(url, data=data)
-        self.assertEqual(response.status_code, 404)
-        self.assertEqual(response.json(), {
-            "detail": "Not found."
-        })
-
     def test_returns_405_on_list_put(self):
         url = reverse('bot:nomination-list', host='api')
 
@@ -422,6 +409,15 @@ class NominationTests(APISubdomainTestCase):
             "detail": "Method \"DELETE\" not allowed."
         })
 
+    def test_returns_405_on_detail_put(self):
+        url = reverse('bot:nomination-detail', args=(self.active_nomination.id,), host='api')
+
+        response = self.client.put(url, data={})
+        self.assertEqual(response.status_code, 405)
+        self.assertEqual(response.json(), {
+            "detail": "Method \"PUT\" not allowed."
+        })
+
     def test_filter_returns_0_objects_unknown_user__id(self):
         url = reverse('bot:nomination-list', host='api')
 
diff --git a/pydis_site/apps/api/viewsets/bot/nomination.py b/pydis_site/apps/api/viewsets/bot/nomination.py
index 2eec6275..fad16aa5 100644
--- a/pydis_site/apps/api/viewsets/bot/nomination.py
+++ b/pydis_site/apps/api/viewsets/bot/nomination.py
@@ -67,7 +67,7 @@ class NominationViewSet(CreateModelMixin, RetrieveModelMixin, ListModelMixin, Ge
     ... }
 
     ### Status codes
-    - 200: returned on succes
+    - 200: returned on success
     - 404: returned if a nomination with the given `id` could not be found
 
     ### POST /bot/nominations
@@ -84,7 +84,7 @@ class NominationViewSet(CreateModelMixin, RetrieveModelMixin, ListModelMixin, Ge
     ...     'user': 409107086526644234
     ... }
 
-    ### Response format
+    #### Response format
     See `GET /bot/nominations/<id:int>`
 
     #### Status codes
@@ -95,53 +95,59 @@ class NominationViewSet(CreateModelMixin, RetrieveModelMixin, ListModelMixin, Ge
         - The request contained a field that cannot be set at creation.
 
     ### PATCH /bot/nominations/<id:int>
-    Update the nomination with the given `id` and return the updated nomination.
-    For active nominations, only the `reason` may be updated; for inactive
-    nominations, both the `reason` and `end_reason` may be updated.
+    Update or end the nomination with the given `id` and return the updated nomination.
+
+    The PATCH route can be used for three distinct operations:
+    1. Updating the `reason` of `active` nomination;
+    2. Ending an `active` nomination;
+    3. Updating the `end_reason` or `reason` field of an `inactive` nomination.
+
+    While the response format and status codes are the same for all three operations (see
+    below), the request bodies vary depending on the operation. For all operations holds
+    that providing other valid fields is not allowed and invalid fields are ignored.
+
+    ### 1. Updating the `reason` of `active` nomination
 
     #### Request body
     >>> {
     ...     'reason': 'He would make a great helper',
-    ...     'end_reason': 'He needs some time to mature his Python knowledge'
     ... }
 
-    ### Response format
+    #### Response format
     See `GET /bot/nominations/<id:int>`
 
-    ## Status codes
+    #### Status codes
     - 200: returned on success
     - 400: if a field in the request body is invalid or disallowed
     - 404: if an infraction with the given `id` could not be found
 
-    ### PATCH /bot/nominations/<id:int>/end_nomination
-    Ends an active nomination and returns the updated nomination.
+    ### 2. Ending an `active` nomination
 
-    The `end_reason` field is the only allowed and required field
-    for this operation. The nomination will automatically be marked as
-    `active = false` and the datetime of this operation will be added to
-    the `ended_at` field.
+    #### Request body
+    >>> {
+    ...     'active': False
+    ...     'end_reason': 'They've been added to the Helpers team',
+    ... }
+
+    See operation 1 for the response format and status codes.
+
+    ### 3. Updating the `end_reason` or `reason` field of an `inactive` nomination.
 
     #### Request body
     >>> {
-    ...     'end_reason': 'He needs some time to mature his Python knowledge'
+    ...     'reason': 'Updated reason for this nomination',
+    ...     'end_reason': 'Updated end_reason for this nomination',
     ... }
 
-    ### Response format
-    See `GET /bot/nominations/<id:int>`
+    Note: The request body may contain either or both fields.
 
-    #### Status codes
-    - 200: returned on success
-    - 400: returned on failure for the following reasons:
-        - `end_reason` is missing from the request body;
-        - Any other field is present in the request body;
-        - The nomination was already inactiive.
-    - 404: if an infraction with the given `id` could not be found
+    See operation 1 for the response format and status codes.
     """
     serializer_class = NominationSerializer
     queryset = Nomination.objects.all()
     filter_backends = (DjangoFilterBackend, SearchFilter, OrderingFilter)
     filter_fields = ('user__id', 'actor__id', 'active')
-    frozen_fields = ('id', 'actor', 'inserted_at', 'user', 'ended_at', 'active')
+    frozen_fields = ('id', 'actor', 'inserted_at', 'user', 'ended_at')
     frozen_on_create = ('ended_at', 'end_reason', 'active', 'inserted_at')
 
     def create(self, request, *args, **kwargs):
@@ -180,43 +186,39 @@ class NominationViewSet(CreateModelMixin, RetrieveModelMixin, ListModelMixin, Ge
                 raise ValidationError({field: ['This field cannot be updated.']})
 
         instance = self.get_object()
-
-        if instance.active and request.data.get('end_reason'):
-            raise ValidationError(
-                {'end_reason': ["An active nomination can't have an unnominate reason."]}
-            )
-
         serializer = self.get_serializer(instance, data=request.data, partial=True)
         serializer.is_valid(raise_exception=True)
-        serializer.save()
 
-        return Response(serializer.data)
+        data = serializer.validated_data
 
-    def update(self, request, *args, **kwargs):
-        """
-        DRF action for ending an active nomination.
+        # There are three distinct PATCH scenarios we need to validate.
+        if instance.active and 'active' not in data:
+            # 1. We're updating an active nomination without ending it.
+            if request.data.get('end_reason'):
+                raise ValidationError(
+                    {'end_reason': ["An active nomination can't have an end reason."]}
+                )
 
-        Creates an API endpoint /bot/nominations/{id}/end_nomination to end a nomination. See
-        the class docstring for documentation.
-        """
-        for field in request.data:
-            if field != "end_reason":
-                raise ValidationError({field: ['This field cannot be set at end_nomination.']})
+        elif instance.active and not data.get('active'):
+            # 2. We're ending an active nomination.
+            if 'reason' in data:
+                raise ValidationError(
+                    {'reason': ['This field cannot be set when ending a nomination.']}
+                )
+
+            if 'end_reason' not in request.data:
+                raise ValidationError(
+                    {'end_reason': ['This field is required when ending a nomination.']}
+                )
+
+            instance.ended_at = timezone.now()
 
-        if "end_reason" not in request.data:
+        elif 'active' in data:
+            # 3. The `active` field is only allowed when ending a nomination.
             raise ValidationError(
-                {'end_reason': ['This field is required when ending a nomination.']}
+                {'active': ['This field can only be used to end a nomination']}
             )
 
-        instance = self.get_object()
-        if not instance.active:
-            raise ValidationError({'active': ['A nomination must be active to be ended.']})
-
-        serializer = self.get_serializer(instance, data=request.data, partial=True)
-        serializer.is_valid(raise_exception=True)
-        instance.active = False
-        instance.ended_at = timezone.now()
         serializer.save()
-        instance.save()
 
         return Response(serializer.data)