Merge pull request #9958 from edx/cdyer/MA-1280-restful-conventions

Make the profile_image api more restful.

openedx/core/djangoapps/profile_images/tests/test_views.py

@@ -8,6 +8,8 @@ import unittest
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
+from django.http import HttpResponse
+
 import mock
 from mock import patch
 from PIL import Image
@@ -64,7 +66,7 @@ class PatchedClient(APIClient):
         return super(PatchedClient, self).request(*args, **kwargs)
 
 
-class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase):
+class ProfileImageEndpointMixin(UserSettingsEventTestMixin):
     """
     Base class / shared infrastructure for tests of profile_image "upload" and
     "remove" endpoints.
@@ -72,9 +74,10 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase):
     # subclasses should override this with the name of the view under test, as
     # per the urls.py configuration.
     _view_name = None
+    client_class = PatchedClient
 
     def setUp(self):
-        super(ProfileImageEndpointTestCase, self).setUp()
+        super(ProfileImageEndpointMixin, self).setUp()
         self.user = UserFactory.create(password=TEST_PASSWORD)
         # Ensure that parental controls don't apply to this user
         self.user.profile.year_of_birth = 1980
@@ -92,7 +95,7 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase):
         self.reset_tracker()
 
     def tearDown(self):
-        super(ProfileImageEndpointTestCase, self).tearDown()
+        super(ProfileImageEndpointMixin, self).tearDown()
         for name in get_profile_image_names(self.user.username).values():
             self.storage.delete(name)
 
@@ -136,18 +139,46 @@ class ProfileImageEndpointTestCase(UserSettingsEventTestMixin, APITestCase):
         profile = self.user.profile.__class__.objects.get(user=self.user)
         self.assertEqual(profile.has_profile_image, has_profile_image)
 
+    def check_anonymous_request_rejected(self, method):
+        """
+        Make sure that the specified method rejects access by unauthorized users.
+        """
+        anonymous_client = APIClient()
+        request_method = getattr(anonymous_client, method)
+        response = request_method(self.url)
+        self.check_response(response, 401)
+        self.assert_no_events_were_emitted()
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+@mock.patch('openedx.core.djangoapps.profile_images.views.log')
+class ProfileImageViewGeneralTestCase(ProfileImageEndpointMixin, APITestCase):
+    """
+    Tests for the profile image endpoint
+    """
+    _view_name = "accounts_profile_image_api"
+
+    def test_unsupported_methods(self, mock_log):
+        """
+        Test that GET, PUT, and PATCH are not supported.
+        """
+        self.assertEqual(405, self.client.get(self.url).status_code)
+        self.assertEqual(405, self.client.put(self.url).status_code)
+        self.assertEqual(405, self.client.patch(self.url).status_code)  # pylint: disable=no-member
+        self.assertFalse(mock_log.info.called)
+        self.assert_no_events_were_emitted()
+
 
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
 @mock.patch('openedx.core.djangoapps.profile_images.views.log')
-class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
+class ProfileImageViewPostTestCase(ProfileImageEndpointMixin, APITestCase):
     """
-    Tests for the profile_image upload endpoint.
+    Tests for the POST method of the profile_image api endpoint.
     """
-    _view_name = "profile_image_upload"
+    _view_name = "accounts_profile_image_api"
 
     # Use the patched version of the API client to workaround a unicode issue
     # with DRF 3.1 and Django 1.4.  Remove this after we upgrade Django past 1.4!
-    client_class = PatchedClient
 
     def check_upload_event_emitted(self, old=None, new=TEST_UPLOAD_DT):
         """
@@ -158,26 +189,12 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
             setting='profile_image_uploaded_at', old=old, new=new
         )
 
-    def test_unsupported_methods(self, mock_log):
-        """
-        Test that GET, PUT, PATCH, and DELETE are not supported.
-        """
-        self.assertEqual(405, self.client.get(self.url).status_code)
-        self.assertEqual(405, self.client.put(self.url).status_code)
-        self.assertEqual(405, self.client.patch(self.url).status_code)
-        self.assertEqual(405, self.client.delete(self.url).status_code)
-        self.assertFalse(mock_log.info.called)
-        self.assert_no_events_were_emitted()
-
     def test_anonymous_access(self, mock_log):
         """
-        Test that an anonymous client (not logged in) cannot POST.
+        Test that an anonymous client (not logged in) cannot call POST.
         """
-        anonymous_client = APIClient()
-        response = anonymous_client.post(self.url)
-        self.assertEqual(401, response.status_code)
+        self.check_anonymous_request_rejected('post')
         self.assertFalse(mock_log.info.called)
-        self.assert_no_events_were_emitted()
 
     @patch('openedx.core.djangoapps.profile_images.views._make_upload_dt', side_effect=[TEST_UPLOAD_DT, TEST_UPLOAD_DT2])
     def test_upload_self(self, mock_make_image_version, mock_log):  # pylint: disable=unused-argument
@@ -204,7 +221,8 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
 
     def test_upload_other(self, mock_log):
         """
-        Test that an authenticated user cannot POST to another user's upload endpoint.
+        Test that an authenticated user cannot POST to another user's upload
+        endpoint.
         """
         different_user = UserFactory.create(password=TEST_PASSWORD)
         # Ignore UserProfileFactory creation events.
@@ -221,7 +239,8 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
 
     def test_upload_staff(self, mock_log):
         """
-        Test that an authenticated staff cannot POST to another user's upload endpoint.
+        Test that an authenticated staff cannot POST to another user's upload
+        endpoint.
         """
         staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD)
         # Ignore UserProfileFactory creation events.
@@ -306,14 +325,14 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
 
 @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
 @mock.patch('openedx.core.djangoapps.profile_images.views.log')
-class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
+class ProfileImageViewDeleteTestCase(ProfileImageEndpointMixin, APITestCase):
     """
-    Tests for the profile_image remove endpoint.
+    Tests for the DELETE method of the profile_image endpoint.
     """
-    _view_name = "profile_image_remove"
+    _view_name = "accounts_profile_image_api"
 
     def setUp(self):
-        super(ProfileImageRemoveTestCase, self).setUp()
+        super(ProfileImageViewDeleteTestCase, self).setUp()
         with make_image_file() as image_file:
             create_profile_images(image_file, get_profile_image_names(self.user.username))
             self.check_images()
@@ -330,34 +349,19 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
             setting='profile_image_uploaded_at', old=TEST_UPLOAD_DT, new=None
         )
 
-    def test_unsupported_methods(self, mock_log):
-        """
-        Test that GET, PUT, PATCH, and DELETE are not supported.
-        """
-        self.assertEqual(405, self.client.get(self.url).status_code)
-        self.assertEqual(405, self.client.put(self.url).status_code)
-        self.assertEqual(405, self.client.patch(self.url).status_code)
-        self.assertEqual(405, self.client.delete(self.url).status_code)
-        self.assertFalse(mock_log.info.called)
-        self.assert_no_events_were_emitted()
-
     def test_anonymous_access(self, mock_log):
         """
-        Test that an anonymous client (not logged in) cannot call GET or POST.
+        Test that an anonymous client (not logged in) cannot call DELETE.
         """
-        anonymous_client = APIClient()
-        for request in (anonymous_client.get, anonymous_client.post):
-            response = request(self.url)
-            self.assertEqual(401, response.status_code)
+        self.check_anonymous_request_rejected('delete')
         self.assertFalse(mock_log.info.called)
-        self.assert_no_events_were_emitted()
 
     def test_remove_self(self, mock_log):
         """
-        Test that an authenticated user can POST to remove their own profile
+        Test that an authenticated user can DELETE to remove their own profile
         images.
         """
-        response = self.client.post(self.url)
+        response = self.client.delete(self.url)
         self.check_response(response, 204)
         self.check_images(False)
         self.check_has_profile_image(False)
@@ -369,7 +373,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
 
     def test_remove_other(self, mock_log):
         """
-        Test that an authenticated user cannot POST to remove another user's
+        Test that an authenticated user cannot DELETE to remove another user's
         profile images.
         """
         different_user = UserFactory.create(password=TEST_PASSWORD)
@@ -377,7 +381,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         self.reset_tracker()
         different_client = APIClient()
         different_client.login(username=different_user.username, password=TEST_PASSWORD)
-        response = different_client.post(self.url)
+        response = different_client.delete(self.url)
         self.check_response(response, 404)
         self.check_images(True)  # thumbnails should remain intact.
         self.check_has_profile_image(True)
@@ -386,13 +390,13 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
 
     def test_remove_staff(self, mock_log):
         """
-        Test that an authenticated staff user can POST to remove another user's
+        Test that an authenticated staff user can DELETE to remove another user's
         profile images.
         """
         staff_user = UserFactory(is_staff=True, password=TEST_PASSWORD)
         staff_client = APIClient()
         staff_client.login(username=staff_user.username, password=TEST_PASSWORD)
-        response = self.client.post(self.url)
+        response = self.client.delete(self.url)
         self.check_response(response, 204)
         self.check_images(False)
         self.check_has_profile_image(False)
@@ -405,13 +409,69 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
     @patch('student.models.UserProfile.save')
     def test_remove_failure(self, user_profile_save, mock_log):
         """
-        Test that when upload validation fails, the proper HTTP response and
+        Test that when remove validation fails, the proper HTTP response and
         messages are returned.
         """
         user_profile_save.side_effect = [Exception(u"whoops"), None]
         with self.assertRaises(Exception):
-            self.client.post(self.url)
+            self.client.delete(self.url)
         self.check_images(True)  # thumbnails should remain intact.
         self.check_has_profile_image(True)
         self.assertFalse(mock_log.info.called)
         self.assert_no_events_were_emitted()
+
+
+class DeprecatedProfileImageTestMixin(ProfileImageEndpointMixin):
+    """
+    Actual tests for DeprecatedProfileImage.*TestCase classes defined here.
+
+    Requires:
+        self._view_name
+        self._replacement_method
+    """
+
+    def test_unsupported_methods(self, mock_log):
+        """
+        Test that GET, PUT, PATCH, and DELETE are not supported.
+        """
+        self.assertEqual(405, self.client.get(self.url).status_code)
+        self.assertEqual(405, self.client.put(self.url).status_code)
+        self.assertEqual(405, self.client.patch(self.url).status_code)
+        self.assertEqual(405, self.client.delete(self.url).status_code)
+        self.assertFalse(mock_log.info.called)
+        self.assert_no_events_were_emitted()
+
+    def test_post_calls_replacement_view_method(self, mock_log):
+        """
+        Test that calls to this view pass through the the new view.
+        """
+        with patch(self._replacement_method) as mock_method:
+            mock_method.return_value = HttpResponse()
+            self.client.post(self.url)
+            assert mock_method.called
+        self.assertFalse(mock_log.info.called)
+        self.assert_no_events_were_emitted()
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+@mock.patch('openedx.core.djangoapps.profile_images.views.log')
+class DeprecatedProfileImageUploadTestCase(DeprecatedProfileImageTestMixin, APITestCase):
+    """
+    Tests for the deprecated profile_image upload endpoint.
+
+    Actual tests defined on DeprecatedProfileImageTestMixin
+    """
+    _view_name = 'profile_image_upload'
+    _replacement_method = 'openedx.core.djangoapps.profile_images.views.ProfileImageView.post'
+
+
+@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Profile Image API is only supported in LMS')
+@mock.patch('openedx.core.djangoapps.profile_images.views.log')
+class DeprecatedProfileImageRemoveTestCase(DeprecatedProfileImageTestMixin, APITestCase):
+    """
+    Tests for the deprecated profile_image remove endpoint.
+
+    Actual tests defined on DeprecatedProfileImageTestMixin
+    """
+    _view_name = "profile_image_remove"
+    _replacement_method = 'openedx.core.djangoapps.profile_images.views.ProfileImageView.delete'

openedx/core/djangoapps/profile_images/urls.py

 """
 Defines the URL routes for this app.
+
+NOTE: These views are deprecated.  These routes are superseded by
+``/api/user/v1/accounts/{username}/image``, found in
+``openedx.core.djangoapps.user_api.urls``.
 """
-from .views import ProfileImageUploadView, ProfileImageRemoveView
 
 from django.conf.urls import patterns, url
 
+from .views import ProfileImageUploadView, ProfileImageRemoveView
+
 USERNAME_PATTERN = r'(?P<username>[\w.+-]+)'
 
 urlpatterns = patterns(

openedx/core/djangoapps/profile_images/views.py

@@ -35,49 +35,85 @@ def _make_upload_dt():
     return datetime.datetime.utcnow().replace(tzinfo=utc)
 
 
-class ProfileImageUploadView(APIView):
+class ProfileImageView(APIView):
     """
-    **Use Case**
+    **Use Cases**
 
-        * Upload an image for the user's profile.
+        Add or remove profile images associated with user accounts.
 
-          The requesting user must be signed in. The signed in user can only
-          upload his or her own profile image.
+        The requesting user must be signed in.  Users can only add profile
+        images to their own account.  Users with staff access can remove
+        profile images for other user accounts.  All other users can remove
+        only their own profile images.
 
-    **Example Request**
+    **Example Requests**
 
-        POST /api/profile_images/v1/{username}/upload
+        POST /api/user/v1/accounts/{username}/image
+
+        DELETE /api/user/v1/accounts/{username}/image
+
+    **Example POST Responses**
+
+        When the requesting user attempts to upload an image for their own
+        account, the request returns one of the following responses:
 
-    **Example Responses**
+        * If the upload could not be performed, the request returns an HTTP 400
+          "Bad Request" response with information about why the request failed.
 
-        When the requesting user tries to upload the image for a different user, the
-        request returns one of the following responses.
+        * If the upload is successful, the request returns an HTTP 204 "No
+          Content" response with no additional content.
 
-        * If the requesting user has staff access, the request returns an HTTP 403
-          "Forbidden" response.
+        If the requesting user tries to upload an image for a different
+        user, the request returns one of the following responses:
 
-        * If the requesting user does not have staff access, the request returns
-          an HTTP 404 "Not Found" response.
+        * If no user matches the "username" parameter, the request returns an
+          HTTP 404 "Not Found" response.
 
-        * If no user matches the "username" parameter, the request returns an HTTP
-          404 "Not Found" response.
+        * If the user whose profile image is being uploaded exists, but the
+          requesting user does not have staff access, the request returns an
+          HTTP 404 "Not Found" response.
 
-        * If the upload could not be performed, the request returns an HTTP 400 "Bad
-          Request" response with more information.
+        * If the specified user exists, and the requesting user has staff
+          access, the request returns an HTTP 403 "Forbidden" response.
 
-        * If the upload is successful, the request returns an HTTP 204 "No Content"
-          response with no additional content.
+    **Example DELETE Responses**
 
+        When the requesting user attempts to remove the profile image for
+        their own account, the request returns one of the following
+        responses:
+
+        * If the image could not be removed, the request returns an HTTP 400
+          "Bad Request" response with information about why the request failed.
+
+        * If the request successfully removes the image, the request returns
+          an HTTP 204 "No Content" response with no additional content.
+
+        When the requesting user tries to remove the profile image for a
+        different user, the view will return one of the following responses:
+
+        * If the requesting user has staff access, and the "username" parameter
+          matches a user, the profile image for the specified user is deleted,
+          and the request returns an HTTP 204 "No Content" response with no
+          additional content.
+
+        * If the requesting user has staff access, but no user is matched by
+          the "username" parameter, the request returns an HTTP 404 "Not Found"
+          response.
+
+        * If the requesting user does not have staff access, the request
+          returns an HTTP 404 "Not Found" response, regardless of whether
+          the user exists or not.
     """
-    parser_classes = (MultiPartParser, FormParser,)
 
+    parser_classes = (MultiPartParser, FormParser)
     authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
     permission_classes = (permissions.IsAuthenticated, IsUserInUrl)
 
     def post(self, request, username):
         """
-        POST /api/profile_images/v1/{username}/upload
+        POST /api/user/v1/accounts/{username}/image
         """
+
         # validate request:
         # verify that the user's
         # ensure any file was sent
@@ -121,50 +157,11 @@ class ProfileImageUploadView(APIView):
         # send client response.
         return Response(status=status.HTTP_204_NO_CONTENT)
 
-
-class ProfileImageRemoveView(APIView):
-    """
-    **Use Case**
-
-        * Remove all of the profile images associated with the user's account.
-
-          The requesting user must be signed in.
-
-          Users with staff access can remove profile images for other user
-          accounts.
-
-          Users without staff access can only remove their own profile images.
-
-    **Example Request**
-
-        POST /api/profile_images/v1/{username}/remove
-
-    **Example Responses**
-
-        When the requesting user tries to remove the profile image for a
-        different user, the request returns one of the following responses.
-
-        * If the user does not have staff access, the request returns an HTTP
-          404 "Not Found" response.
-
-        * If no user matches the "username" parameter, the request returns an
-          HTTP 404 "Not Found" response.
-
-        * If the image could not be removed, the request returns an HTTP 400
-          "Bad Request" response with more information.
-
-        * If the request successfully removes the image, the request returns
-          an HTTP 204 "No Content" response with no additional content.
-
-
-    """
-    authentication_classes = (OAuth2AuthenticationAllowInactiveUser, SessionAuthenticationAllowInactiveUser)
-    permission_classes = (permissions.IsAuthenticated, IsUserInUrlOrStaff)
-
-    def post(self, request, username):  # pylint: disable=unused-argument
+    def delete(self, request, username):  # pylint: disable=unused-argument
         """
-        POST /api/profile_images/v1/{username}/remove
+        DELETE /api/user/v1/accounts/{username}/image
         """
+
         try:
             # update the user account to reflect that the images were removed.
             set_has_profile_image(username, False)
@@ -182,3 +179,42 @@ class ProfileImageRemoveView(APIView):
 
         # send client response.
         return Response(status=status.HTTP_204_NO_CONTENT)
+
+
+class ProfileImageUploadView(APIView):
+    """
+    **DEPRECATION WARNING**
+
+        /api/profile_images/v1/{username}/upload is deprecated.
+        All requests should now be sent to
+        /api/user/v1/accounts/{username}/image
+    """
+
+    parser_classes = ProfileImageView.parser_classes
+    authentication_classes = ProfileImageView.authentication_classes
+    permission_classes = ProfileImageView.permission_classes
+
+    def post(self, request, username):
+        """
+        POST /api/profile_images/v1/{username}/upload
+        """
+        return ProfileImageView().post(request, username)
+
+
+class ProfileImageRemoveView(APIView):
+    """
+    **DEPRECATION WARNING**
+
+        /api/profile_images/v1/{username}/remove is deprecated.
+        This endpoint's POST is replaced by the DELETE method at
+        /api/user/v1/accounts/{username}/image.
+    """
+
+    authentication_classes = ProfileImageView.authentication_classes
+    permission_classes = ProfileImageView.permission_classes
+
+    def post(self, request, username):
+        """
+        POST /api/profile_images/v1/{username}/remove
+        """
+        return ProfileImageView().delete(request, username)

openedx/core/djangoapps/user_api/urls.py

@@ -2,27 +2,33 @@
 Defines the URL routes for this app.
 """
 
+from django.conf.urls import patterns, url
+
+from ..profile_images.views import ProfileImageView
 from .accounts.views import AccountView
 from .preferences.views import PreferencesView, PreferencesDetailView
 
-from django.conf.urls import patterns, url
-
 USERNAME_PATTERN = r'(?P<username>[\w.+-]+)'
 
 urlpatterns = patterns(
     '',
     url(
-        r'^v1/accounts/' + USERNAME_PATTERN + '$',
+        r'^v1/accounts/{}$'.format(USERNAME_PATTERN),
         AccountView.as_view(),
         name="accounts_api"
     ),
     url(
-        r'^v1/preferences/' + USERNAME_PATTERN + '$',
+        r'^v1/accounts/{}/image$'.format(USERNAME_PATTERN),
+        ProfileImageView.as_view(),
+        name="accounts_profile_image_api"
+    ),
+    url(
+        r'^v1/preferences/{}$'.format(USERNAME_PATTERN),
         PreferencesView.as_view(),
         name="preferences_api"
     ),
     url(
-        r'^v1/preferences/' + USERNAME_PATTERN + '/(?P<preference_key>[a-zA-Z0-9_]+)$',
+        r'^v1/preferences/{}/(?P<preference_key>[a-zA-Z0-9_]+)$'.format(USERNAME_PATTERN),
         PreferencesDetailView.as_view(),
         name="preferences_detail_api"
     ),