Address code review issues

core/controllers/profile.py

@@ -281,7 +281,7 @@ def post(self):
         if feconf.CAN_SEND_EMAILS_TO_USERS and not has_ever_registered:
             email_manager.send_post_signup_email(self.user_id)
 
-        user_services.generate_profile_picture(self.user_id)
+        user_services.generate_initial_profile_picture(self.user_id)
 
         self.render_json({})
 

core/domain/user_services.py

@@ -33,8 +33,9 @@
 
 MAX_USERNAME_LENGTH = 50
 # Size (in px) of the gravatar being retrieved.
-GRAVATAR_SIZE = 150
+GRAVATAR_SIZE_PX = 150
 # Data url for images/avatar/user_blue_72px.png
+# Generated using utils.convert_png_to_data_url
 DEFAULT_IDENTICON_DATA_URL = (
     'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEwAAABMCAYAAADHl1ErAAAAAXNSR0IArs4c6QAADhtJREFUeAHt%0AXHlwVdUZ/859jyxmIQESyCaglC0iAgkJIntrIpvKphSwY2ttxbFOp9R/cGGqdhykLaMVO2OtoyRS%0ACEKNEpYKyBIVQ1iNkBhNMCtb8shiQpJ3b7/fTW7m5uUlecu9L4nTM5Pce8895zvf93vnnPud833f%0AEdQLKXb5jsC6%2BuZERZbHKaSMYRbGKERxgpQQUkSIIigEbAmFavlfrUKiVhCVcFa%2BIJEvJOlCcNCA%0AnNKMFQ0o58vEfPgmhS5Mn0ot8n2KIs8lIZJJUfy8almIJqbxhRDSIbJKe2s%2BXvWlV/RcrGwqYGGp%0A20bI1LyaeVmjKMrodp4EycGBAy6MjgsrSxozqG7O5GgxcVREeEigNDAwwBpmsUiRKGu3y1caGlts%0AtQ3yjbOFV6sPnypXTuRXBReU2GLqGprHkUKSRlMIUcD3WyUakGbbt7JYyzf6agpgYfe9O8kui/U8%0AnB7UhJIkUTljwrBTTz449mZKUlyCEBTnjTCKQiX7T5ScfGP3Rf9j5ysny7IyTKXHPwYP690WSXnZ%0AtvcXp71pw1ldQwELm59%2BlyzbX%2BbeNL%2Btscb4EYOyNz2ZWD99wtAFnGdxxoQBefbs85f3rHsjJyiv%0AuGo60wsATe51WZJkWW/LWnXGgDZUEoYAFr58x0B7beOLPHGv5XnFIpGoS0mKOfze%2Bpmj/f2smNR9%0Alm42teQ/8vLRgv0nyuZwVwtm1Ows5BZLSMBz1RkrbnjLiNeAhaWmPWgn%2BxYeejwkRMu9idH7tm%2BY%0AE8/z0EhvmfOmPs9/RQ9tOJx3IKc8lUixkqBKC1nW2vat3u0NXY8Bi1%2B%2Bw6%2BktnETD7%2BnwEB4iP/p%0AL/5xf03U4IBZ3jBkdN2K641Hkn/7YWh17c1JoM3D9PW4kIB1eRkrmjxpyyPAeK4aLttbPuAhOIU5%0AaHpm1cTMZ1ffuRT8eMKED%2BooL6Wd%2B2Bj%2BtnFUGeYyVzJYl3Kc9sld9t2W8Dw%2BWkTWuz2fdxQ9ACr%0A9P3Jfy7%2BZuSw0HnuNtwb5Ysqaw4mPJb5k%2BYW%2BVZuv9xqsaRWZ60%2B7w4vbgEWnrJ1hp3kTO5ZYUPC%0AAnK%2B3bYiitWDWHca7O2yrI6U3r5yR8U1W2MiC2%2BzkLS4ev%2BaY67y1a749VQBYLUIZT/AGhUTduS7%0Af68Y39/AgozgGbxDBsgCmSBbT/Jr710CDMMQPYvHf2DC2Mj9p95efA8TCNKI9MNrEGSALJAJskFG%0AV%2BTocUhigrfbWz5jYtH4VdrAMksBdYVnI8vYJ/8q83hhmW0WEy23WKx39/Qh6LaHQXXA1xBgYc5i%0AsBL4/scCFoC3QCbIBhkhK2TGi65St4CpeharDvgaYoJnIv15GHaFQRBkg4w8p02BzF0VRH6XgEGD%0AV5VS1rOgOvTHCb47wfXvIBtkhE4JmSG7/r3%2B3ilg6toQyx1OUEr7i56lF8zde8gIWVEPSz1g4IyG%0AU8CwkMbaEMudNg3eWd0fXR5khcyQXcXAiYSdAMMWDY/ltVhIY23IdXr8kjqh21%2BzRKvMogUYAAtH%0AQToBhv0sbNFg16GvLaQdmTfjGTJDdmCgYuHQSIfe07pTSqewn3V9z6qrvb1F48Crzx6xNTR4QXoE%0A9tN4c2%2ByfufWqudC3VbmAYzNPwZrkf6dL%2B4LSm5Q9vkrVH79B6qs%2BoH8B1goatAtNCIqmOZOiabw%0A4G5VJMNYREdhDD7ae6J0USsmtEwj3t7DYLCwK83f8WbbzauZP7/kq53SxiY7vfmfC5R24Fv6prTr%0ADVEWgqbfEUlPLY2nlKkxGv%2BmXbFzG7H4/eE8g/tZyO92zbDSPoe1WncUgT14X4G189Nimvjobnrh%0AX6e6BQuo8DCho2crafnzB2n%2BMwe4PL5H5iVgACx4wEltli%2B1sXbA%2BGkNcmCwUN%2BY%2BI%2B3WOjZt3Lp%0Al68cpQoefu6m4%2Bcqae7TWfTfk%2BXuVnWrvA4LFRtUVockjKxKc8sJmMJsWWsiON/U9eJvNmXTtk%2B%2B%0AdYt5Z4WZX0p/bjYtmBbn7LURefaw%2BVuvwoQnBliTYCxu7WFskQb1WROjcvliKlibM/IMAQv8siD0%0A643H6etiGx7NSBbYUlXCbRipgKnme859Ysl4jwwDrnKaV2SjDe%2B0tu9qnZ7KsQWch/YxVpt6KunZ%0AexieUVPDSIJjCC86k3lwyikJ0di%2BMS09/3au2iuMbuDr4mpKN2CIO%2BMLVnpgA4yAlVRX1ziV4fOD%0ArwOv2k2bDM4UVvEkXeaMJ0PyXn3/nCF0HIkAE2ADjICVpChiLArBMcSxsJHPmdmXjCTXiVZRRS19%0AVVTdKd%2BIDA0bYCW1%2BWcRvGiMIN4Vjb1flHb1yrD8rM9LDKOlJ6RhA6ww6au%2BD3A50hcy%2Bt5sRRP8%0AFpSYo8zqsBnDPax13oJ/ltEgafSqam5SU7NdezTtWsHrTzOShg2wYtWP3SQ5wZnNjMZA80Z9s1mk%0AO9CtMakdDRtgJcGnFK3C869D6wY%2BRISp7loGUnROKtKkdtqxYawkzQGXdwNUN0nnrHiXGxxoJf40%0Ae0fEhdpRg29xoZT7RTRsgJV%2B8e0%2BJTdqJIwd4kZpz4pOGWN%2BG5Lq2s38wQHXMzZdq2XiAlllgP2%2B%0AaH6yOX4xGjbAinejlVq0CG9l10T3rNT99wwnf96KMyvNuHMoDR0UaAr5dmwYK1YrhAoYXLtNaa2N%0A6DAW5vFF6qLClGZeeHSyKXRBVMMGWLFaoUZYEPzgTWuxjfC6lROI/RgMb2bZ7JGUaOIcqWEDrDDp%0A50MCBA0YLokDQRgx0p%2BdTezH4PDG88dxI8LotaeneU7AhZo6bPK5hwkVMERYuFDX6yLT2JDx99/f%0ATVY2anibYiOCaPuGuayydDB%2BeUu2U30NG2AlCaFcRAmEo3QqaVLGynm30a6X5sHz2uMWksZH0pHX%0AF9CIYeb/zho2CAqTgoMDvoTXCmJ3EI7isQRuVpw9KYqytyykhxk8qASuJoD84mNTKGvjveSLFQQw%0AUeOaGCNE0Flqvs5o8b/9gZ8xwyMmj404NComZJyrzHtbLjTIjxZNv1X9C/S30pXqRrLVdd4lh7Ej%0AOX4oPfHAOHrzD9Np9l1RZMHnygeJ45kOZXxaPJ6byr6WueotdfAjhI73rGdu2ZXnn5oY7QM2OjZx%0Ax8hw%2BvPjCepf2bUfqJz/Llc1qHpb1OBAiosMpoFB5i%2BtOnLV%2BoTgL9ypYYZ8bZ0tOd6QmuUNbCiF%0AMoN9GPM0TCbeXYoZcgvhr48kOyLlVF6AESf1UwV7G88jBbC/ISqsjzDb62wAC9UmydhoAaz6b/tW%0AcIgQul7ntI8woMNCxQZstQOGSFYeqQriDeGI0Ud47jU2gIEae8kmtlZsWllpB6zNO2UXZwcg3rDX%0AOO0jDbdhEIDoXs1zB6y1A4YHhP3iiuBMOJXh3tfJzuZ/qBbfX65nR5UGqmto8TUL2OoqAgZoWMNE%0AY6KTMhOa%2Bt4ehCDfmxjz8c4X5y3UChp5hVk/j63Vpwuu0zdlNVTIrkuFfC1hkOobO%2B//Qw8LD/an%0A26JDaFRsKI2KCWU76kCaOi6CoHYYnZY9d/DjAzllC/lDmFWz75EFevqdFmGIkbbL9hREsiI40yg/%0A11wGhxex9PlXV%2BjEhatUU99ZQdUzpr%2BH08n1mkb1L%2BfiVf0rGs5Lo2nxkXT3HUPZ0S7WawAhsxrF%0Ay6HPwKJDY/zQqYehAPey1%2BDgDxfsSxkPwZPYaTmU7S7BPWDXkWLafayYLlWaaidW2cASK5nBWzJz%0AOD3AG5YebCgqw5dvP4PoXab1Oveu3znK5xQIOPW31DZchL/6M6vv2sn%2B68scK3b1jDlo%2B6Hv6G87%0A8ij/e1M3cbtiQc3HML4vKZbWrbyTpowe3G1Z7SVH7e7cmHZmGXePSmtI4FhnQfVOAQMBNfhdse/C%0AwvzsO/cf6ykapKlZpq0HCmlzxlc%2B6U2akK5c2XJNf3x4At3D29hdJUTrTnz0wxlwOrEIy5Kugum7%0ABAyEtaGJwKVrH63mrSDn0besEdNTmz9XJ%2B6uGOoL%2BbAr/OXJJIoM77jryx%2Bh0iGL0mSENnc1FDX%2B%0AO6gVWqZ2RfQ9I5oLQgj75fxO/q%2BvpJ9TnXTxlevr6cPjlyj5iUx2bb%2BsZ7UesqlgsayQWf/S8b7b%0AHobC3QWYrv3rZ%2BwuXuhIs88/Y4v8vfWz4BvrdoBpj4BBejWE2W4/yupTGMJ%2BD21O/emf3j1t2bTN%0ArYD8PgWkv7/FflvUwE8uFFelMAg2i8Uy05UTBlwCTAWtLUieJ8XA2MiQIxXX6xNYI%2B6XC3Wep%2Br5%0Axz/Jsszij1qDVREprp4s4DJgGmjaMQzcUA5bgaNkRTbH3GxSf5SEVMoxRBUMlrnHMIB//Arounxb%0AjgZZuWWtSzlokmyGkwWv4Bm8QwZ1GLpxZgUYcquHaRLgQ6A/SobJ4IiGpeyc7RE9ja55V/aKEOID%0A5s/3R8loQjkeVsTzwmmeF2oYuFlamT5xFeII/4qh3LMmgR/oWT4/rEgPhONxWEKifUJW4mWikfpy%0Avr5nBbNIkUQeD8BU7lm9fxyWHgDHA9fYQlzHg/0w/6qjuZzqdKwvb/J9PveiAl4Hz%2BE5q%2B8duKYX%0AHjHSjkf6sXkqWyEZK4QFLIQ51iihWrr2CJKCeE6fzm2pax8Grm8e6acHDffth0YSLdF9CCoZvFye%0A55okRU7gIetV1AkPuRJZSCfZUdefezJMYf3v0MhOwHVzLKlQxAWSRJlQlDr%2BzrPcUjjbGwbyBB2m%0ACKH62/K7KwywjWM8b5CQq%2BH9x%2B%2BCSVZiFKH8eI4ldQQOz4jJ/P/Bt86QcSFPPVqZA50Qu4NwFK7i%0A3tHK7HEEJ5reOFr5fwkK97jkk8ywAAAAAElFTkSuQmCC%0A') #pylint: disable=line-too-long
 
@@ -210,23 +211,28 @@ def get_users_settings(user_ids):
     return result
 
 
-def generate_profile_picture(user_id):
+def generate_initial_profile_picture(user_id):
     """Generates a profile picture for a new user."""
     user_email = get_email_from_user_id(user_id)
     user_gravatar = fetch_gravatar(user_email)
     update_profile_picture_data_url(user_id, user_gravatar)
 
 
+def get_gravatar_url(email):
+    """Returns the gravatar url for the specified email."""
+    return (
+        'https://www.gravatar.com/avatar/%s?d=identicon&s=%s' %
+        (hashlib.md5(email).hexdigest(), GRAVATAR_SIZE_PX))
+
+
 def fetch_gravatar(email):
     """Returns the gravatar corresponding to the user's email,
     or an identicon generated from the email if the gravatar doesn't exist.
 
     Note: if the call to the gravatar service fails,
     this returns DEFAULT_IDENTICON_DATA_URL and logs an error.
     """
-    gravatar_url = (
-        'http://www.gravatar.com/avatar/%s?d=identicon&s=%s' %
-        (hashlib.md5(email).hexdigest(), GRAVATAR_SIZE))
+    gravatar_url = get_gravatar_url(email)
     try:
         result = urlfetch.fetch(
             gravatar_url,

core/domain/user_services_test.py

@@ -15,7 +15,6 @@
 # limitations under the License.
 
 import logging
-from contextlib import nested
 import os
 
 from core.domain import collection_services
@@ -139,39 +138,57 @@ def test_get_user_id_from_username(self):
 
     def test_fetch_gravatar_success(self):
         user_email = '[email protected]'
-        gravatar_filepath = os.path.join(
+        expected_gravatar_filepath = os.path.join(
             'static', 'images', 'avatar', 'gravatar_example.png')
-        with open(gravatar_filepath, 'r') as f:
+        with open(expected_gravatar_filepath, 'r') as f:
             gravatar = f.read()
         with self.urlfetch_mock(content=gravatar):
             profile_picture = user_services.fetch_gravatar(user_email)
-            gravatar_data_url = utils.convert_png_to_data_url(gravatar_filepath)
+            gravatar_data_url = utils.convert_png_to_data_url(
+                expected_gravatar_filepath)
             self.assertEqual(profile_picture, gravatar_data_url)
 
     def test_fetch_gravatar_failure_404(self):
         user_email = '[email protected]'
-        logging_error_mock = test_utils.CallCounter(lambda x: x)
+        error_messages = []
+        def log_mock(message):
+            error_messages.append(message)
+
+        gravatar_url = user_services.get_gravatar_url(user_email)
+        expected_error_message = (
+            '[Status 404] Failed to fetch Gravatar from %s' % gravatar_url)
+        logging_error_mock = test_utils.CallCounter(log_mock)
         urlfetch_counter = test_utils.CallCounter(urlfetch.fetch)
-        with nested(
-            self.urlfetch_mock(status_code=404),
-            self.swap(logging, 'error', logging_error_mock),
-            self.swap(urlfetch, 'fetch', urlfetch_counter)):
+        urlfetch_mock_ctx = self.urlfetch_mock(status_code=404)
+        log_swap_ctx = self.swap(logging, 'error', logging_error_mock)
+        fetch_swap_ctx = self.swap(urlfetch, 'fetch', urlfetch_counter)
+        with urlfetch_mock_ctx, log_swap_ctx, fetch_swap_ctx:
             profile_picture = user_services.fetch_gravatar(user_email)
             self.assertEqual(urlfetch_counter.times_called, 1)
             self.assertEqual(logging_error_mock.times_called, 1)
+            self.assertEqual(expected_error_message, error_messages[0])
             self.assertEqual(
                 profile_picture, user_services.DEFAULT_IDENTICON_DATA_URL)
 
     def test_fetch_gravatar_failure_exception(self):
         user_email = '[email protected]'
-        logging_error_mock = test_utils.CallCounter(lambda x: x)
+        error_messages = []
+        def log_mock(message):
+            error_messages.append(message)
+
+        gravatar_url = user_services.get_gravatar_url(user_email)
+        expected_error_message = (
+            'Failed to fetch Gravatar from %s' % gravatar_url)
+        logging_error_mock = test_utils.CallCounter(log_mock)
         urlfetch_fail_mock = test_utils.FailingFunction(
-            urlfetch.fetch, urlfetch.InvalidURLError, 'infinity')
-        with nested(
-            self.swap(logging, 'error', logging_error_mock),
-            self.swap(urlfetch, 'fetch', urlfetch_fail_mock)):
+            urlfetch.fetch, urlfetch.InvalidURLError,
+            test_utils.FailingFunction.INFINITY)
+        log_swap_ctx = self.swap(logging, 'error', logging_error_mock)
+        fetch_swap_ctx = self.swap(urlfetch, 'fetch', urlfetch_fail_mock)
+        with log_swap_ctx, fetch_swap_ctx:
             profile_picture = user_services.fetch_gravatar(user_email)
             self.assertEqual(logging_error_mock.times_called, 1)
+            self.assertEqual(expected_error_message, error_messages[0])
             self.assertEqual(
                 profile_picture, user_services.DEFAULT_IDENTICON_DATA_URL)
 

core/tests/test_utils.py

@@ -71,8 +71,10 @@ def __init__(self, service_name='urlfetch'):
         self.response = None
         self.request = None
 
-    def set_return_values(self, **kwargs):
-        self.return_values = kwargs
+    def set_return_values(self, content='', status_code=200, headers=None):
+        self.return_values['content'] = content
+        self.return_values['status_code'] = status_code
+        self.return_values['headers'] = headers
 
     def _Dynamic_Fetch(self, request, response): # pylint: disable=invalid-name
         return_values = self.return_values
@@ -280,7 +282,8 @@ def signup(self, email, username):
         self.login(email)
         # Signup uses a custom urlfetch mock (URLFetchServiceMock), instead
         # of the stub provided by testbed. This custom mock is disabled
-        # immediately once the signup is complete.
+        # immediately once the signup is complete. This is done to avoid external 
+        # calls being made to Gravatar when running the backend tests.
         with self.urlfetch_mock():
             response = self.testapp.get(feconf.SIGNUP_URL)
             self.assertEqual(response.status_int, 200)
@@ -601,14 +604,15 @@ def urlfetch_mock(
         context of a 'with' statement.
 
         This mock is currently used for signup to prevent external HTTP
-        requests to fetch the Gravatar profile picture for new users.
+        requests to fetch the Gravatar profile picture for new users while the
+        backend tests are being run.
 
         args:
           - content: Response content or body.
           - status_code: Response status code.
           - headers: Response headers.
         """
-        if not headers:
+        if headers is None:
             response_headers = {}
         else:
             response_headers = headers

utils.py

@@ -229,7 +229,7 @@ def get_random_choice(alist):
 
 
 def convert_png_binary_to_data_url(content):
-    """Converts a png image string(content) to a data URL."""
+    """Converts a png image string (represented by 'content') to a data URL."""
     if imghdr.what(None, content) == 'png':
         return 'data:image/png;base64,%s' % urllib.quote(
             content.encode('base64'))