address review feedback

common/djangoapps/student/management/commands/force_send_notification_digest.py

 """
 Django management command to force-send the daily/weekly digest emails
 """
-
+import sys
+import datetime
+import pytz
 import logging
 import logging.config
-import sys
 
+# This is specifially placed at the top
+# to act as a loggic configuration override for the rest of the
+# code
 # Have all logging go to stdout with management commands
 # this must be up at the top otherwise the
 # configuration does not appear to take affect
-import datetime
-import pytz
-from django.conf import settings
-from edx_notifications import const
-from edx_notifications.digests import send_notifications_digest, send_notifications_namespace_digest
-
-LOGGING = {
+logging.config.dictConfig({
     'version': 1,
     'handlers': {
         'console': {
@@ -27,16 +25,20 @@ LOGGING = {
         'handlers': ['console'],
         'level': 'INFO'
     }
-}
-logging.config.dictConfig(LOGGING)
+})
 
+from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError
 
-log = logging.getLogger(__file__)
+from edx_notifications import const
+from edx_notifications.digests import send_notifications_digest, send_notifications_namespace_digest
 
 from optparse import make_option, OptionParser
 
 
+log = logging.getLogger(__file__)
+
+
 class Command(BaseCommand):
     """
     Django management command to force-send the daily/weekly digest emails
@@ -73,18 +75,16 @@ class Command(BaseCommand):
         ),
     )
 
-    def send_daily_digest(self, namespace='All'):
+    def _send_digest(self, subject, preference_name, day_delta, namespace):
         """
-        Sends the daily digest.
+        Sends a digest
         """
         if const.NOTIFICATION_DIGEST_SEND_TIMEFILTERED:
-            from_timestamp = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=1)
+            from_timestamp = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=day_delta)
         else:
             from_timestamp = None
 
         to_timestamp = datetime.datetime.now(pytz.UTC)
-        preference_name = const.NOTIFICATION_DAILY_DIGEST_PREFERENCE_NAME
-        subject = const.NOTIFICATION_DAILY_DIGEST_SUBJECT
         from_email = const.NOTIFICATION_EMAIL_FROM_ADDRESS
 
         if namespace == "All":
@@ -95,28 +95,29 @@ class Command(BaseCommand):
             )
         return digests_sent
 
-    def send_weekly_digest(self, namespace='All'):
+    def send_daily_digest(self, namespace='All'):
         """
-        Sends the weekly digest.
+        Sends the daily digest.
         """
 
-        if const.NOTIFICATION_DIGEST_SEND_TIMEFILTERED:
-            from_timestamp = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=7)
-        else:
-            from_timestamp = None
+        return self._send_digest(
+            const.NOTIFICATION_DAILY_DIGEST_SUBJECT,
+            const.NOTIFICATION_DAILY_DIGEST_PREFERENCE_NAME,
+            1,
+            namespace
+        )
 
-        to_timestamp = datetime.datetime.now(pytz.UTC)
-        preference_name = const.NOTIFICATION_WEEKLY_DIGEST_PREFERENCE_NAME
-        subject = const.NOTIFICATION_WEEKLY_DIGEST_SUBJECT
-        from_email = const.NOTIFICATION_EMAIL_FROM_ADDRESS
+    def send_weekly_digest(self, namespace='All'):
+        """
+        Sends the weekly digest.
+        """
 
-        if namespace == "All":
-            digests_sent = send_notifications_digest(from_timestamp, to_timestamp, preference_name, subject, from_email)
-        else:
-            digests_sent = send_notifications_namespace_digest(
-                namespace, from_timestamp, to_timestamp, preference_name, subject, from_email
-            )
-        return digests_sent
+        return self._send_digest(
+            const.NOTIFICATION_WEEKLY_DIGEST_SUBJECT,
+            const.NOTIFICATION_WEEKLY_DIGEST_PREFERENCE_NAME,
+            7,
+            namespace
+        )
 
     def handle(self, *args, **options):
         """
@@ -128,7 +129,6 @@ class Command(BaseCommand):
          --ns=NAMESPACE : Sends the notifications for the particular NAMESPACE.
         """
 
-
         if not settings.FEATURES.get('ENABLE_NOTIFICATIONS', False):
             print 'ENABLE_NOTIFICATIONS not set to "true". Stopping...'
             return
@@ -140,16 +140,16 @@ class Command(BaseCommand):
 
         if options['send_daily_digest']:
             log.info("Sending the daily digest with namespace=%s...", options['namespace'])
-            weekly_digests_sent = self.send_daily_digest(options['namespace'])
-            log.info("Successfully sent %s digests...", weekly_digests_sent)
+            daily_digests_sent = self.send_daily_digest(options['namespace'])
+            log.info("Successfully sent %s digests...", daily_digests_sent)
 
         if options['send_weekly_digest']:
             log.info("Sending the weekly digest with namespace=%s...", options['namespace'])
-            daily_digests_sent = self.send_weekly_digest(options['namespace'])
-            log.info("Successfully sent %s digests...", daily_digests_sent)
+            weekly_digests_sent = self.send_weekly_digest(options['namespace'])
+            log.info("Successfully sent %s digests...", weekly_digests_sent)
 
         if not options['send_weekly_digest'] and not options['send_daily_digest']:
             parser.print_help()
             raise CommandError("Neither Daily, nor Weekly digest specified.")
 
-        log.info("Completed .")
+        log.info("Completed.")

common/djangoapps/student/management/commands/tests/test_force_send_notification_digest.py

@@ -10,8 +10,10 @@ from student.management.commands import force_send_notification_digest
 
 @mock.patch.dict(settings.FEATURES, {'ENABLE_NOTIFICATIONS': True})
 class ForceSendDigestCommandTest(TestCase):
-    def test_command_check(self):
-        # run the management command for purging notifications.
+    def test_command_all(self):
+        # run the management command for sending notification digests.
         force_send_notification_digest.Command().handle(**{'send_daily_digest': True, 'send_weekly_digest': True, 'namespace': 'All'})
+
+    def test_command_namespaced(self):
+        # run the management command for sending notification digests.
         force_send_notification_digest.Command().handle(**{'send_daily_digest': True, 'send_weekly_digest': True, 'namespace': 'ABC'})
-        # force_send_digest.Command().handle(**{'send_daily_digest': False, 'send_weekly_digest': False, 'namespace': 'ABC'})

common/djangoapps/student/scope_resolver.py

@@ -15,6 +15,23 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 log = logging.getLogger(__name__)
 
 
+def _get_course_key_from_string(course_id):
+    """
+    Helper method to convert a string formatted
+    course_id into a CourseKey
+    """
+
+    if not isinstance(course_id, CourseKey):
+        try:
+            course_key = CourseKey.from_string(course_id)
+        except InvalidKeyError:
+            course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+    else:
+        course_key = course_id
+
+    return course_key
+
+
 class CourseEnrollmentsScopeResolver(NotificationUserScopeResolver):
     """
     Implementation of the NotificationUserScopeResolver abstract
@@ -35,21 +52,16 @@ class CourseEnrollmentsScopeResolver(NotificationUserScopeResolver):
 
         if scope_name != 'course_enrollments':
             # we can't resolve any other scopes
+            # The API expects a None (not an exception) if this
+            # particular resolver is not able to resolve a scope_name
+            # which it does not know about.
             return None
 
         if 'course_id' not in scope_context:
             # did not receive expected parameters
-            return None
+            raise KeyError('Missing course_id in scope_context')
 
-        course_id = scope_context['course_id']
-
-        if not isinstance(course_id , CourseKey):
-            try:
-                course_key = CourseKey.from_string(course_id)
-            except InvalidKeyError:
-               course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
-        else:
-            course_key = course_id
+        course_key = _get_course_key_from_string(scope_context['course_id'])
 
         return CourseEnrollment.objects.values_list('user_id', flat=True).filter(
             is_active=1,
@@ -70,25 +82,23 @@ class NamespaceEnrollmentsScopeResolver(NotificationUserScopeResolver):
     def resolve(self, scope_name, scope_context, instance_context):
         """
         The entry point to resolve a scope_name with a given scope_context
+
+        scope_context must include a 'namespace' key/value pair to indicate
+        what course_id needs to be resolved
         """
 
         if scope_name != 'namespace_scope':
             # we can't resolve any other scopes
+            # The API expects a None (not an exception) if this
+            # particular resolver is not able to resolve a scope_name
+            # which it does not know about.
             return None
 
         if 'namespace' not in scope_context:
             # did not receive expected parameters
-            return None
-
-        course_id = scope_context['namespace']
+            raise KeyError('Missing course_id in scope_context')
 
-        if not isinstance(course_id , CourseKey):
-            try:
-                course_key = CourseKey.from_string(course_id)
-            except InvalidKeyError:
-               course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
-        else:
-            course_key = course_id
+        course_key = _get_course_key_from_string(scope_context['namespace'])
 
         query = User.objects.select_related('courseenrollment')
 
@@ -110,7 +120,7 @@ class NamespaceEnrollmentsScopeResolver(NotificationUserScopeResolver):
 
         query = query.values(*fields)
         query = query.filter(
-            courseenrollment__is_active=1,
+            courseenrollment__is_active=True,
             courseenrollment__course_id=course_key
         )
         return query
@@ -129,6 +139,9 @@ class StudentEmailScopeResolver(NotificationUserScopeResolver):
 
         if scope_name != 'user_email_resolver':
             # we can't resolve any other scopes
+            # The API expects a None (not an exception) if this
+            # particular resolver is not able to resolve a scope_name
+            # which it does not know about.
             return None
 
         user_id = scope_context.get('user_id')

common/djangoapps/student/tests/test_scope_resolver.py

@@ -62,7 +62,9 @@ class StudentTasksTestCase(ModuleStoreTestCase):
         resolver = CourseEnrollmentsScopeResolver()
 
         self.assertIsNone(resolver.resolve('bad', {'course_id': 'foo'}, None))
-        self.assertIsNone(resolver.resolve('course_enrollments', {'bad': 'foo'}, None))
+
+        with self.assertRaises(KeyError):
+            self.assertIsNone(resolver.resolve('course_enrollments', {'bad': 'foo'}, None))
 
     def test_namespace_scope(self):
         """
@@ -139,7 +141,7 @@ class StudentTasksTestCase(ModuleStoreTestCase):
 
         resolver = StudentEmailScopeResolver()
 
-        emails_resultset = resolver.resolve(
+        resolved_scopes = resolver.resolve(
             'user_email_resolver',
             {
                 'user_id': test_user_1.id,
@@ -147,7 +149,7 @@ class StudentTasksTestCase(ModuleStoreTestCase):
             None
         )
 
-        emails = [email['email'] for email in emails_resultset]
+        emails = [resolved_scope['email'] for resolved_scope in resolved_scopes]
 
         self.assertTrue(test_user_1.email in emails)
 

lms/djangoapps/api_manager/users/tests.py

@@ -1904,7 +1904,7 @@ class UsersApiTests(ModuleStoreTestCase):
         # mark as read
         test_uri = '{}/{}/notifications/{}/'.format(self.users_base_uri, user_id, sent_user_msg.msg.id)
         response = self.do_post(test_uri, {"read": True})
-        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.status_code, 201)
 
         # then verify unread count, which should be 0
         self.assertEqual(get_notifications_count_for_user(user_id, filters={'read': False}), 0)

lms/djangoapps/api_manager/users/views.py

@@ -1391,16 +1391,22 @@ class UsersRolesCoursesDetail(SecureAPIView):
 
 class UsersNotificationsDetail(SecureAPIView):
     """
-    Allows for a caller to delete a user's notification, passed in by msg_id. Note that the
-    user_msg_id must belong to the user_id passed in
+    Allows for a caller to mark a user's notification as read,
+    passed in by msg_id. Note that the user_msg_id must belong
+    to the user_id passed in
     """
     def post(self, request, user_id, msg_id):
         """
         POST /api/users/{user_id}/notifications/{msg_id}
+
+        payload:
+            {
+                'read': 'True' or 'False'
+            }
         """
 
         read = bool(request.DATA['read'])
 
         mark_notification_read(int(user_id), int(msg_id), read=read)
 
-        return Response({}, status=status.HTTP_200_OK)
+        return Response({}, status=status.HTTP_201_CREATED)

lms/envs/devstack.py

@@ -27,7 +27,7 @@ for pkg_name in ['track.contexts', 'track.middleware', 'dd.dogapi']:
 
 ################################ EMAIL ########################################
 
-#EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'
+EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'
 FEATURES['ENABLE_INSTRUCTOR_EMAIL'] = True     # Enable email for all Studio courses
 FEATURES['REQUIRE_COURSE_EMAIL_AUTH'] = False  # Give all courses email (don't require django-admin perms)
 
@@ -49,7 +49,6 @@ if FEATURES.get('PROFILER'):
 ANALYTICS_DASHBOARD_URL = None
 
 
-
 ################################ DEBUG TOOLBAR ################################
 FEATURES['DEBUG_TOOLBAR'] = True
 if FEATURES.get('DEBUG_TOOLBAR'):