From bb2599cecdf3a9e3d5ef1d17940c5dd924719e3f Mon Sep 17 00:00:00 2001 From: jsa Date: Mon, 30 Mar 2015 10:17:09 -0400 Subject: [PATCH] Include profile image version in URLs. TNL-1860 --- ...n.py => 0048_add_profile_image_version.py} | 12 ++--- ...add_unique_languageproficiency_code_use.py | 2 +- common/djangoapps/student/models.py | 12 ++++- .../student/tests/test_parental_controls.py | 4 +- .../profile_images/tests/test_views.py | 11 +++-- .../core/djangoapps/profile_images/views.py | 13 +++++- .../user_api/accounts/image_helpers.py | 46 +++++++++++++++---- .../accounts/tests/test_image_helpers.py | 25 ++++++---- .../user_api/accounts/tests/test_views.py | 16 ++++--- 9 files changed, 97 insertions(+), 44 deletions(-) rename common/djangoapps/student/migrations/{0048_add_has_profile_image_boolean.py => 0048_add_profile_image_version.py} (97%) diff --git a/common/djangoapps/student/migrations/0048_add_has_profile_image_boolean.py b/common/djangoapps/student/migrations/0048_add_profile_image_version.py similarity index 97% rename from common/djangoapps/student/migrations/0048_add_has_profile_image_boolean.py rename to common/djangoapps/student/migrations/0048_add_profile_image_version.py index a5f3ebf4a1..e842a06bfc 100644 --- a/common/djangoapps/student/migrations/0048_add_has_profile_image_boolean.py +++ b/common/djangoapps/student/migrations/0048_add_profile_image_version.py @@ -8,15 +8,15 @@ from django.db import models class Migration(SchemaMigration): def forwards(self, orm): - # Adding field 'UserProfile.has_profile_image' - db.add_column('auth_userprofile', 'has_profile_image', - self.gf('django.db.models.fields.BooleanField')(default=False), + # Adding field 'UserProfile.profile_image_uploaded_at' + db.add_column('auth_userprofile', 'profile_image_uploaded_at', + self.gf('django.db.models.fields.DateTimeField')(null=True), keep_default=False) def backwards(self, orm): - # Deleting field 'UserProfile.has_profile_image' - db.delete_column('auth_userprofile', 'has_profile_image') + # Deleting field 'UserProfile.profile_image_uploaded_at' + db.delete_column('auth_userprofile', 'profile_image_uploaded_at') models = { @@ -192,4 +192,4 @@ class Migration(SchemaMigration): } } - complete_apps = ['student'] \ No newline at end of file + complete_apps = ['student'] diff --git a/common/djangoapps/student/migrations/0049_auto__add_languageproficiency__add_unique_languageproficiency_code_use.py b/common/djangoapps/student/migrations/0049_auto__add_languageproficiency__add_unique_languageproficiency_code_use.py index e2625bda0e..47c536afe3 100644 --- a/common/djangoapps/student/migrations/0049_auto__add_languageproficiency__add_unique_languageproficiency_code_use.py +++ b/common/djangoapps/student/migrations/0049_auto__add_languageproficiency__add_unique_languageproficiency_code_use.py @@ -173,7 +173,7 @@ class Migration(SchemaMigration): 'courseware': ('django.db.models.fields.CharField', [], {'default': "'course.xml'", 'max_length': '255', 'blank': 'True'}), 'gender': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), 'goals': ('django.db.models.fields.TextField', [], {'null': 'True', 'blank': 'True'}), - 'has_profile_image': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'profile_image_uploaded_at': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}), 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), 'language': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '255', 'blank': 'True'}), 'level_of_education': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '6', 'null': 'True', 'blank': 'True'}), diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 3eb82fe028..e82b8eeb99 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -251,7 +251,15 @@ class UserProfile(models.Model): goals = models.TextField(blank=True, null=True) allow_certificate = models.BooleanField(default=1) bio = models.TextField(blank=True, null=True) - has_profile_image = models.BooleanField(default=0) + profile_image_uploaded_at = models.DateTimeField(null=True) + + @property + def has_profile_image(self): + """ + Convenience method that returns a boolean indicating whether or not + this user has uploaded a profile image. + """ + return self.profile_image_uploaded_at is not None def get_meta(self): # pylint: disable=missing-docstring js_str = self.meta @@ -321,7 +329,7 @@ def user_profile_pre_save_callback(sender, **kwargs): # Remove profile images for users who require parental consent if user_profile.requires_parental_consent() and user_profile.has_profile_image: - user_profile.has_profile_image = False + user_profile.profile_image_uploaded_at = None class UserSignupSource(models.Model): diff --git a/common/djangoapps/student/tests/test_parental_controls.py b/common/djangoapps/student/tests/test_parental_controls.py index 3a3623cf6c..d37bd96729 100644 --- a/common/djangoapps/student/tests/test_parental_controls.py +++ b/common/djangoapps/student/tests/test_parental_controls.py @@ -69,14 +69,14 @@ class ProfileParentalControlsTest(TestCase): """Verify that a profile's image obeys parental controls.""" # Verify that an image cannot be set for a user with no year of birth set - self.profile.has_profile_image = True + self.profile.profile_image_uploaded_at = datetime.datetime.now() self.profile.save() self.assertFalse(self.profile.has_profile_image) # Verify that an image can be set for an adult user current_year = datetime.datetime.now().year self.set_year_of_birth(current_year - 20) - self.profile.has_profile_image = True + self.profile.profile_image_uploaded_at = datetime.datetime.now() self.profile.save() self.assertTrue(self.profile.has_profile_image) diff --git a/openedx/core/djangoapps/profile_images/tests/test_views.py b/openedx/core/djangoapps/profile_images/tests/test_views.py index 327f606218..57871422ab 100644 --- a/openedx/core/djangoapps/profile_images/tests/test_views.py +++ b/openedx/core/djangoapps/profile_images/tests/test_views.py @@ -2,13 +2,14 @@ Test cases for the HTTP endpoints of the profile image api. """ from contextlib import closing +import datetime +from pytz import UTC import unittest from django.conf import settings from django.core.urlresolvers import reverse import mock from mock import patch - from PIL import Image from rest_framework.test import APITestCase, APIClient @@ -17,13 +18,14 @@ from student.tests.factories import UserFactory from ...user_api.accounts.image_helpers import ( set_has_profile_image, get_profile_image_names, - get_profile_image_storage + 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" +TEST_UPLOAD_DT = datetime.datetime(2002, 1, 9, 15, 43, 01, tzinfo=UTC) class ProfileImageEndpointTestCase(APITestCase): @@ -121,7 +123,8 @@ class ProfileImageUploadTestCase(ProfileImageEndpointTestCase): self.assertEqual(401, response.status_code) self.assertFalse(mock_log.info.called) - def test_upload_self(self, mock_log): + @patch('openedx.core.djangoapps.profile_images.views._make_upload_dt', return_value=TEST_UPLOAD_DT) + def test_upload_self(self, mock_make_image_version, mock_log): # pylint: disable=unused-argument """ Test that an authenticated user can POST to their own upload endpoint. """ @@ -244,7 +247,7 @@ class ProfileImageRemoveTestCase(ProfileImageEndpointTestCase): with make_image_file() as image_file: create_profile_images(image_file, get_profile_image_names(self.user.username)) self.check_images() - set_has_profile_image(self.user.username, True) + set_has_profile_image(self.user.username, True, TEST_UPLOAD_DT) def test_unsupported_methods(self, mock_log): """ diff --git a/openedx/core/djangoapps/profile_images/views.py b/openedx/core/djangoapps/profile_images/views.py index 8968cd459b..21211af669 100644 --- a/openedx/core/djangoapps/profile_images/views.py +++ b/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 datetime import logging from django.utils.translation import ugettext as _ @@ -16,7 +17,7 @@ from openedx.core.lib.api.authentication import ( SessionAuthenticationAllowInactiveUser, ) 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 openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_names, set_has_profile_image from .images import validate_uploaded_image, create_profile_images, remove_profile_images, ImageValidationError log = logging.getLogger(__name__) @@ -25,6 +26,14 @@ LOG_MESSAGE_CREATE = 'Generated and uploaded images %(image_names)s for user %(u LOG_MESSAGE_DELETE = 'Deleted images %(image_names)s for user %(user_id)s' +def _make_upload_dt(): + """ + Generate a server-side timestamp for the upload. This is in a separate + function so its behavior can be overridden in tests. + """ + return datetime.datetime.utcnow() + + class ProfileImageUploadView(APIView): """ **Use Cases** @@ -92,7 +101,7 @@ class ProfileImageUploadView(APIView): 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) + set_has_profile_image(username, True, _make_upload_dt()) log.info( LOG_MESSAGE_CREATE, diff --git a/openedx/core/djangoapps/user_api/accounts/image_helpers.py b/openedx/core/djangoapps/user_api/accounts/image_helpers.py index 227ae4a0ef..8c38dd4f3f 100644 --- a/openedx/core/djangoapps/user_api/accounts/image_helpers.py +++ b/openedx/core/djangoapps/user_api/accounts/image_helpers.py @@ -47,15 +47,18 @@ def _get_profile_image_filename(name, size, file_extension=PROFILE_IMAGE_FILE_EX return '{name}_{size}.{file_extension}'.format(name=name, size=size, file_extension=file_extension) -def _get_profile_image_urls(name, storage, file_extension=PROFILE_IMAGE_FILE_EXTENSION): +def _get_profile_image_urls(name, storage, file_extension=PROFILE_IMAGE_FILE_EXTENSION, version=None): """ Returns a dict containing the urls for a complete set of profile images, keyed by "friendly" name (e.g. "full", "large", "medium", "small"). """ - return { - size_display_name: storage.url(_get_profile_image_filename(name, size, file_extension=file_extension)) - for size_display_name, size in PROFILE_IMAGE_SIZES_MAP.items() - } + def _make_url(size): # pylint: disable=missing-docstring + url = storage.url( + _get_profile_image_filename(name, size, file_extension=file_extension) + ) + return '{}?v={}'.format(url, version) if version is not None else url + + return {size_display_name: _make_url(size) for size_display_name, size in PROFILE_IMAGE_SIZES_MAP.items()} def get_profile_image_names(username): @@ -88,7 +91,11 @@ def get_profile_image_urls_for_user(user): """ if user.profile.has_profile_image: - return _get_profile_image_urls(_make_profile_image_name(user.username), get_profile_image_storage()) + return _get_profile_image_urls( + _make_profile_image_name(user.username), + get_profile_image_storage(), + version=user.profile.profile_image_uploaded_at.strftime("%s"), + ) else: return _get_default_profile_image_urls() @@ -103,19 +110,38 @@ def _get_default_profile_image_urls(): return _get_profile_image_urls( settings.PROFILE_IMAGE_DEFAULT_FILENAME, staticfiles_storage, - file_extension=settings.PROFILE_IMAGE_DEFAULT_FILE_EXTENSION + file_extension=settings.PROFILE_IMAGE_DEFAULT_FILE_EXTENSION, ) -def set_has_profile_image(username, has_profile_image=True): +def set_has_profile_image(username, is_uploaded, upload_dt=None): """ System (not user-facing) API call used to store whether the user has - uploaded a profile image. Used by profile_image API. + uploaded a profile image, and if so, when. Used by profile_image API. + + Arguments: + username (django.contrib.auth.User.username): references the user who + uploaded an image. + + is_uploaded (bool): whether or not the user has an uploaded profile + image. + + upload_dt (datetime.datetime): If `is_uploaded` is True, this should + contain the server-side date+time of the upload. If `is_uploaded` + is False, the parameter is optional and will be ignored. + + Raises: + ValueError: is_uploaded was True, but no upload datetime was supplied. + UserNotFound: no user with username `username` exists. """ + if is_uploaded and upload_dt is None: + raise ValueError("No upload datetime was supplied.") + elif not is_uploaded: + upload_dt = None try: profile = UserProfile.objects.get(user__username=username) except ObjectDoesNotExist: raise UserNotFound() - profile.has_profile_image = has_profile_image + profile.profile_image_uploaded_at = upload_dt profile.save() diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py b/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py index c1cb633c30..2a40476279 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_image_helpers.py @@ -1,8 +1,10 @@ """ Tests for helpers.py """ +import datetime import hashlib from mock import patch +from pytz import UTC from unittest import skipUnless from django.conf import settings @@ -12,6 +14,7 @@ from ..image_helpers import get_profile_image_urls_for_user from student.tests.factories import UserFactory TEST_SIZES = {'full': 50, 'small': 10} +TEST_PROFILE_IMAGE_UPLOAD_DT = datetime.datetime(2002, 1, 9, 15, 43, 01, tzinfo=UTC) @patch.dict('openedx.core.djangoapps.user_api.accounts.image_helpers.PROFILE_IMAGE_SIZES_MAP', TEST_SIZES, clear=True) @@ -25,17 +28,17 @@ class ProfileImageUrlTestCase(TestCase): self.user = UserFactory() # Ensure that parental controls don't apply to this user self.user.profile.year_of_birth = 1980 - self.user.profile.has_profile_image = False + self.user.profile.profile_image_uploaded_at = TEST_PROFILE_IMAGE_UPLOAD_DT self.user.profile.save() - def verify_url(self, actual_url, expected_name, expected_pixels): + def verify_url(self, actual_url, expected_name, expected_pixels, expected_version): """ Verify correct url structure. """ self.assertEqual( actual_url, - 'http://example-storage.com/profile-images/{name}_{size}.jpg'.format( - name=expected_name, size=expected_pixels + 'http://example-storage.com/profile-images/{name}_{size}.jpg?v={version}'.format( + name=expected_name, size=expected_pixels, version=expected_version ) ) @@ -48,7 +51,7 @@ class ProfileImageUrlTestCase(TestCase): '/static/default_{size}.png'.format(size=expected_pixels) ) - def verify_urls(self, expected_name, actual_urls, is_default=False): + def verify_urls(self, actual_urls, expected_name, is_default=False): """ Verify correct url dictionary structure. """ @@ -57,18 +60,20 @@ class ProfileImageUrlTestCase(TestCase): if is_default: self.verify_default_url(url, TEST_SIZES[size_display_name]) else: - self.verify_url(url, expected_name, TEST_SIZES[size_display_name]) + self.verify_url( + url, expected_name, TEST_SIZES[size_display_name], TEST_PROFILE_IMAGE_UPLOAD_DT.strftime("%s") + ) def test_get_profile_image_urls(self): """ Tests `get_profile_image_urls_for_user` """ - self.user.profile.has_profile_image = True + self.user.profile.profile_image_uploaded_at = TEST_PROFILE_IMAGE_UPLOAD_DT self.user.profile.save() expected_name = hashlib.md5('secret' + self.user.username).hexdigest() actual_urls = get_profile_image_urls_for_user(self.user) - self.verify_urls(expected_name, actual_urls) + self.verify_urls(actual_urls, expected_name, is_default=False) - self.user.profile.has_profile_image = False + self.user.profile.profile_image_uploaded_at = None self.user.profile.save() - self.verify_urls('default', get_profile_image_urls_for_user(self.user), is_default=True) + self.verify_urls(get_profile_image_urls_for_user(self.user), 'default', is_default=True) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 72d5af5bfd..72ce24d11b 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -5,6 +5,7 @@ import ddt import hashlib import json from mock import patch +from pytz import UTC import unittest from django.conf import settings @@ -19,6 +20,8 @@ from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KE from openedx.core.djangoapps.user_api.preferences.api import set_user_preference from .. import PRIVATE_VISIBILITY, ALL_USERS_VISIBILITY +TEST_PROFILE_IMAGE_UPLOADED_AT = datetime.datetime(2002, 1, 9, 15, 43, 01, tzinfo=UTC) + # this is used in one test to check the behavior of profile image url # generation with a relative url in the config. @@ -97,7 +100,7 @@ class UserAPITestCase(APITestCase): legacy_profile.mailing_address = "Park Ave" legacy_profile.gender = "f" legacy_profile.bio = "Tired mother of twins" - legacy_profile.has_profile_image = True + legacy_profile.profile_image_uploaded_at = TEST_PROFILE_IMAGE_UPLOADED_AT legacy_profile.language_proficiencies.add(LanguageProficiency(code='en')) legacy_profile.save() @@ -123,24 +126,23 @@ class TestAccountAPI(UserAPITestCase): corresponds to whether the user has or hasn't set a profile image. """ + template = '{root}/{filename}_{{size}}.{extension}' if has_profile_image: url_root = 'http://example-storage.com/profile-images' filename = hashlib.md5('secret' + self.user.username).hexdigest() file_extension = 'jpg' + template += '?v={}'.format(TEST_PROFILE_IMAGE_UPLOADED_AT.strftime("%s")) else: url_root = 'http://testserver/static' filename = 'default' file_extension = 'png' + template = template.format(root=url_root, filename=filename, extension=file_extension) self.assertEqual( data['profile_image'], { 'has_image': has_profile_image, - 'image_url_full': '{root}/{filename}_50.{extension}'.format( - root=url_root, filename=filename, extension=file_extension, - ), - 'image_url_small': '{root}/{filename}_10.{extension}'.format( - root=url_root, filename=filename, extension=file_extension, - ) + 'image_url_full': template.format(size=50), + 'image_url_small': template.format(size=10), } )