Add logging to image uploads/deletes.

Conflicts:
	openedx/core/djangoapps/profile_images/views.py

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

@@ -4,7 +4,6 @@ Test cases for the HTTP endpoints of the profile image api.
 from contextlib import closing
 import unittest
 
-import ddt
 from django.conf import settings
 from django.core.urlresolvers import reverse
 import mock
@@ -21,6 +20,7 @@ from ...user_api.accounts.image_helpers import (
     get_profile_image_storage
 )
 from ..images import create_profile_images, ImageValidationError
+from ..views import LOG_MESSAGE_CREATE, LOG_MESSAGE_DELETE
 from .helpers import make_image_file
 
 TEST_PASSWORD = "test"
@@ -94,15 +94,15 @@ class ProfileImageEndpointTestCase(APITestCase):
         self.assertEqual(profile.has_profile_image, has_profile_image)
 
 
-@ddt.ddt
 @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):
     """
     Tests for the profile_image upload endpoint.
     """
     _view_name = "profile_image_upload"
 
-    def test_unsupported_methods(self):
+    def test_unsupported_methods(self, mock_log):
         """
         Test that GET, PUT, PATCH, and DELETE are not supported.
         """
@@ -110,16 +110,18 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
         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)
 
-    def test_anonymous_access(self):
+    def test_anonymous_access(self, mock_log):
         """
         Test that an anonymous client (not logged in) cannot POST.
         """
         anonymous_client = APIClient()
         response = anonymous_client.post(self.url)
         self.assertEqual(401, response.status_code)
+        self.assertFalse(mock_log.info.called)
 
-    def test_upload_self(self):
+    def test_upload_self(self, mock_log):
         """
         Test that an authenticated user can POST to their own upload endpoint.
         """
@@ -128,8 +130,12 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
             self.check_response(response, 204)
             self.check_images()
             self.check_has_profile_image()
+        mock_log.info.assert_called_once_with(
+            LOG_MESSAGE_CREATE,
+            {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id}
+        )
 
-    def test_upload_other(self):
+    def test_upload_other(self, mock_log):
         """
         Test that an authenticated user cannot POST to another user's upload endpoint.
         """
@@ -141,8 +147,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
             self.check_response(response, 404)
             self.check_images(False)
             self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
-    def test_upload_staff(self):
+    def test_upload_staff(self, mock_log):
         """
         Test that an authenticated staff cannot POST to another user's upload endpoint.
         """
@@ -154,8 +161,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
             self.check_response(response, 403)
             self.check_images(False)
             self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
-    def test_upload_missing_file(self):
+    def test_upload_missing_file(self, mock_log):
         """
         Test that omitting the file entirely from the POST results in HTTP 400.
         """
@@ -167,8 +175,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
         )
         self.check_images(False)
         self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
-    def test_upload_not_a_file(self):
+    def test_upload_not_a_file(self, mock_log):
         """
         Test that sending unexpected data that isn't a file results in HTTP
         400.
@@ -181,8 +190,9 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
         )
         self.check_images(False)
         self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
-    def test_upload_validation(self):
+    def test_upload_validation(self, mock_log):
         """
         Test that when upload validation fails, the proper HTTP response and
         messages are returned.
@@ -200,9 +210,10 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
                 )
                 self.check_images(False)
                 self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
     @patch('PIL.Image.open')
-    def test_upload_failure(self, image_open):
+    def test_upload_failure(self, image_open, mock_log):
         """
         Test that when upload validation fails, the proper HTTP response and
         messages are returned.
@@ -217,9 +228,11 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase):
             )
             self.check_images(False)
             self.check_has_profile_image(False)
+        self.assertFalse(mock_log.info.called)
 
 
 @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):
     """
     Tests for the profile_image remove endpoint.
@@ -233,7 +246,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
             self.check_images()
             set_has_profile_image(self.user.username, True)
 
-    def test_unsupported_methods(self):
+    def test_unsupported_methods(self, mock_log):
         """
         Test that GET, PUT, PATCH, and DELETE are not supported.
         """
@@ -241,8 +254,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         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)
 
-    def test_anonymous_access(self):
+    def test_anonymous_access(self, mock_log):
         """
         Test that an anonymous client (not logged in) cannot call GET or POST.
         """
@@ -250,8 +264,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         for request in (anonymous_client.get, anonymous_client.post):
             response = request(self.url)
             self.assertEqual(401, response.status_code)
+        self.assertFalse(mock_log.info.called)
 
-    def test_remove_self(self):
+    def test_remove_self(self, mock_log):
         """
         Test that an authenticated user can POST to remove their own profile
         images.
@@ -260,8 +275,12 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         self.check_response(response, 204)
         self.check_images(False)
         self.check_has_profile_image(False)
+        mock_log.info.assert_called_once_with(
+            LOG_MESSAGE_DELETE,
+            {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id}
+        )
 
-    def test_remove_other(self):
+    def test_remove_other(self, mock_log):
         """
         Test that an authenticated user cannot POST to remove another user's
         profile images.
@@ -273,8 +292,9 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         self.check_response(response, 404)
         self.check_images(True)  # thumbnails should remain intact.
         self.check_has_profile_image(True)
+        self.assertFalse(mock_log.info.called)
 
-    def test_remove_staff(self):
+    def test_remove_staff(self, mock_log):
         """
         Test that an authenticated staff user can POST to remove another user's
         profile images.
@@ -286,9 +306,13 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         self.check_response(response, 204)
         self.check_images(False)
         self.check_has_profile_image(False)
+        mock_log.info.assert_called_once_with(
+            LOG_MESSAGE_DELETE,
+            {'image_names': get_profile_image_names(self.user.username).values(), 'user_id': self.user.id}
+        )
 
     @patch('student.models.UserProfile.save')
-    def test_remove_failure(self, user_profile_save):
+    def test_remove_failure(self, user_profile_save, mock_log):
         """
         Test that when upload validation fails, the proper HTTP response and
         messages are returned.
@@ -302,3 +326,4 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase):
         )
         self.check_images(True)  # thumbnails should remain intact.
         self.check_has_profile_image(True)
+        self.assertFalse(mock_log.info.called)

openedx/core/djangoapps/profile_images/views.py

@@ -2,6 +2,7 @@
 This module implements the upload and remove endpoints of the profile image api.
 """
 from contextlib import closing
+import logging
 
 from django.utils.translation import ugettext as _
 from rest_framework import permissions, status
@@ -18,6 +19,11 @@ from openedx.core.lib.api.permissions import IsUserInUrl, IsUserInUrlOrStaff
 from openedx.core.djangoapps.user_api.accounts.image_helpers import set_has_profile_image, get_profile_image_names
 from .images import validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError
 
+log = logging.getLogger(__name__)
+
+LOG_MESSAGE_CREATE = 'Generated and uploaded images %(image_names)s for user %(user_id)s'
+LOG_MESSAGE_DELETE = 'Deleted images %(image_names)s for user %(user_id)s'
+
 
 class ProfileImageUploadView(APIView):
     """
@@ -82,10 +88,16 @@ class ProfileImageUploadView(APIView):
                     )
 
                 # generate profile pic and thumbnails and store them
-                create_profile_images(uploaded_file, get_profile_image_names(username))
+                profile_image_names = get_profile_image_names(username)
+                create_profile_images(uploaded_file, profile_image_names)
 
                 # update the user account to reflect that a profile image is available.
                 set_has_profile_image(username, True)
+
+                log.info(
+                    LOG_MESSAGE_CREATE,
+                    {'image_names': profile_image_names.values(), 'user_id': request.user.id}
+                )
         except Exception as error:
             return Response(
                 {
@@ -135,7 +147,13 @@ class ProfileImageRemoveView(APIView):
             set_has_profile_image(username, False)
 
             # remove physical files from storage.
-            remove_profile_images(get_profile_image_names(username))
+            profile_image_names = get_profile_image_names(username)
+            remove_profile_images(profile_image_names)
+
+            log.info(
+                LOG_MESSAGE_DELETE,
+                {'image_names': profile_image_names.values(), 'user_id': request.user.id}
+            )
         except UserNotFound:
             return Response(status=status.HTTP_404_NOT_FOUND)
         except Exception as error: