Merge pull request #454 from edx-solutions/rc/2015-06-15

Rc/2015 06 15

lms/djangoapps/api_manager/users/tests.py

@@ -418,6 +418,40 @@ class UsersApiTests(ModuleStoreTestCase):
         self.assertEqual(response.data['is_active'], False)
         self.assertIsNotNone(response.data['created'])
 
+    def test_user_detail_invalid_email(self):
+        test_uri = '{}/{}'.format(self.users_base_uri, self.user.id)
+        data = {
+            'email': 'fail'
+        }
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 400)
+        self.assertIn('Invalid email address', response.content)
+
+    def test_user_detail_duplicate_email(self):
+        user2 = UserFactory()
+        test_uri = '{}/{}'.format(self.users_base_uri, self.user.id)
+        test_uri2 = '{}/{}'.format(self.users_base_uri, user2.id)
+        data = {
+            'email': self.test_email
+        }
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 200)
+        response = self.do_post(test_uri2, data)
+        self.assertEqual(response.status_code, 400)
+        self.assertIn('A user with that email address already exists.', response.content)
+
+    def test_user_detail_email_updated(self):
+        test_uri = '{}/{}'.format(self.users_base_uri, self.user.id)
+        new_email = 'test@example.com'
+        data = {
+            'email': new_email
+        }
+        self.assertNotEqual(self.user.email, new_email)
+        response = self.do_post(test_uri, data)
+        self.assertEqual(response.status_code, 200)
+        self.user = User.objects.get(id=self.user.id)
+        self.assertEqual(self.user.email, new_email)
+
     def test_user_detail_post_duplicate_username(self):
         """
         Create two users, then pass the same first username in request in order to update username of second user.
@@ -1435,6 +1469,8 @@ class UsersApiTests(ModuleStoreTestCase):
             response.data['level_of_education'], data["level_of_education"])
         self.assertEqual(
             str(response.data['year_of_birth']), data["year_of_birth"])
+        # This one's technically on the user model itself, but can be updated.
+        self.assertEqual(response.data['email'], data['email'])
 
     def test_user_organizations_list(self):
         user_id = self.user.id

lms/djangoapps/api_manager/users/views.py

@@ -469,6 +469,25 @@ class UsersDetail(SecureAPIView):
         if is_staff is not None:
             existing_user.is_staff = is_staff
             response_data['is_staff'] = existing_user.is_staff
+        email = request.DATA.get('email')
+        if email is not None:
+            email_fail = False
+            try:
+                validate_email(email)
+            except ValidationError:
+                email_fail = True
+                response_data['message'] = _('Invalid email address {}.').format(repr(email))
+            if email != existing_user.email:
+                try:
+                    # Email addresses need to be unique in the LMS, though Django doesn't enforce it directly.
+                    User.objects.get(email=email)
+                    email_fail = True
+                    response_data['message'] = _('A user with that email address already exists.')
+                except ObjectDoesNotExist:
+                    pass
+            if email_fail:
+                return Response(response_data, status=status.HTTP_400_BAD_REQUEST)
+            existing_user.email = email
         existing_user.save()
 
         username = request.DATA.get('username', None)

lms/djangoapps/django_comment_client/management/commands/export_discussion_participation.py

@@ -5,6 +5,8 @@ from datetime import datetime
 from optparse import make_option
 
 from django.core.management.base import BaseCommand, CommandError
+import os
+from path import path
 
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
@@ -15,6 +17,7 @@ from student.models import CourseEnrollment
 
 from lms.lib.comment_client.user import User
 import django_comment_client.utils as utils
+from xmodule.modulestore.django import modulestore
 
 
 class DiscussionExportFields(object):
@@ -39,9 +42,11 @@ class Command(BaseCommand):
 
     Usage:
         ./manage.py lms export_discussion_participation course_key [dest_file] [OPTIONS]
+        ./manage.py lms export_discussion_participation [dest_directory] --all [OPTIONS]
 
         * course_key - target course key (e.g. edX/DemoX/T1)
         * dest_file - location of destination file (created if missing, overwritten if exists)
+        * dest_directory - location to store all dumped files to. Will dump into the current directory otherwise.
 
     OPTIONS:
 
@@ -49,6 +54,10 @@ class Command(BaseCommand):
         * end-date - date time in iso8601 format (YYYY-MM-DD hh:mm:ss). Filters discussion participation stats
           by creation date: no threads/comments/replies created *after* this date is included in calculation
 
+    FLAGS:
+        * cohorted_only - only dump cohorted inline discussion threads
+        * all - Dump all social stats at once into a particular directory.
+
     Examples:
 
     * `./manage.py lms export_discussion_participation <course_key>` - exports entire discussion participation stats for
@@ -62,9 +71,13 @@ class Command(BaseCommand):
     * `./manage.py lms export_discussion_participation <course_key> --end-date=<iso8601 datetime>
         --thread-type=[discussion|question]` - exports discussion participation stats for a course for
         threads/comments/replies created before specified date, including only threads of specified type
+    * `./manage.py lms export_discussion_participation <course_key> --cohorted_only` - exports only cohorted discussion
+        participation stats for a course; output is written to default location (same folder, auto-generated file name)
     """
     THREAD_TYPE_PARAMETER = 'thread_type'
     END_DATE_PARAMETER = 'end_date'
+    ALL_PARAMETER = 'all'
+    COHORTED_ONLY_PARAMETER = 'cohorted_only'
 
     args = "<course_id> <output_file_location>"
 
@@ -86,6 +99,18 @@ class Command(BaseCommand):
             default=None,
             help='Include threads, comments and replies created before the supplied date (iso8601 format)'
         ),
+        make_option(
+            '--all',
+            action='store_true',
+            dest=ALL_PARAMETER,
+            default=False,
+        ),
+        make_option(
+            '--cohorted_only',
+            action='store_true',
+            dest=COHORTED_ONLY_PARAMETER,
+            default=False,
+        )
     )
 
     def _get_filter_string_representation(self, options):
@@ -103,8 +128,34 @@ class Command(BaseCommand):
             "social_stats_{course}_{date:%Y_%m_%d_%H_%M_%S}.csv".format(course=course_key, date=datetime.utcnow())
         )
 
-    def handle(self, *args, **options):
-        """ Executes command """
+    @staticmethod
+    def get_all_courses():
+        """
+        Gets all courses. Made into a separate function because patch isn't cooperating.
+        """
+        return modulestore().get_courses()
+
+    def dump_all(self, *args, **options):
+        if len(args) > 1:
+            raise CommandError("May not specify course and destination root directory with the --all option.")
+        args = list(args)
+        try:
+            dir_name = path(args.pop())
+        except IndexError:
+            dir_name = path('social_stats')
+
+        if not os.path.exists(dir_name):
+            os.makedirs(dir_name)
+
+        for course in self.get_all_courses():
+            raw_course_key = unicode(course.location.course_key)
+            args = [
+                raw_course_key,
+                dir_name / self.get_default_file_location(raw_course_key)
+            ]
+            self.dump_one(*args, **options)
+
+    def dump_one(self, *args, **options):
         if not args:
             raise CommandError("Course id not specified")
         if len(args) > 2:
@@ -125,12 +176,22 @@ class Command(BaseCommand):
         if not course:
             raise CommandError("Invalid course id: {}".format(course_key))
 
+        target_discussion_ids = None
+        if options.get(self.COHORTED_ONLY_PARAMETER, False):
+            cohorted_discussions = course.cohort_config.get('cohorted_inline_discussions', None)
+            if not cohorted_discussions:
+                raise CommandError("Only cohorted discussions are marked for export, "
+                                   "but no cohorted discussions found for the course")
+            else:
+                target_discussion_ids = cohorted_discussions
+
         raw_end_date = options.get(self.END_DATE_PARAMETER, None)
         end_date = dateutil.parser.parse(raw_end_date) if raw_end_date else None
         data = Extractor().extract(
             course_key,
             end_date=end_date,
-            thread_type=(options.get(self.THREAD_TYPE_PARAMETER, None))
+            thread_type=(options.get(self.THREAD_TYPE_PARAMETER, None)),
+            thread_ids=target_discussion_ids,
         )
 
         filter_str = self._get_filter_string_representation(options)
@@ -139,8 +200,14 @@ class Command(BaseCommand):
         with open(output_file_location, 'wb') as output_stream:
             Exporter(output_stream).export(data)
 
-        self.stdout.write("Success!\n")
+    def handle(self, *args, **options):
+        """ Executes command """
+        if options.get(self.ALL_PARAMETER, False):
+            self.dump_all(*args, **options)
+        else:
+            self.dump_one(*args, **options)
 
+        self.stdout.write("Success!\n")
 
 class Extractor(object):
     """ Extracts discussion participation data from db and cs_comments_service """
@@ -165,11 +232,13 @@ class Extractor(object):
         users = CourseEnrollment.users_enrolled_in(course_key)
         return {user.id: user for user in users}
 
-    def _get_social_stats(self, course_key, end_date=None, thread_type=None):
+    def _get_social_stats(self, course_key, end_date=None, thread_type=None, thread_ids=None):
         """ Gets social stats for course with specified filter parameters """
         return {
             int(user_id): data for user_id, data
-            in User.all_social_stats(str(course_key), end_date=end_date, thread_type=thread_type).iteritems()
+            in User.all_social_stats(
+                str(course_key), end_date=end_date, thread_type=thread_type, thread_ids=thread_ids
+            ).iteritems()
         }
 
     def _merge_user_data_and_social_stats(self, userdata, social_stats):
@@ -187,13 +256,14 @@ class Extractor(object):
             result.append(utils.merge_dict(user_record, stats))
         return result
 
-    def extract(self, course_key, end_date=None, thread_type=None):
+    def extract(self, course_key, end_date=None, thread_type=None, thread_ids=None):
         """ Extracts and merges data according to course key and filter parameters """
         users = self._get_users(course_key)
         social_stats = self._get_social_stats(
             course_key,
             end_date=end_date,
-            thread_type=thread_type
+            thread_type=thread_type,
+            thread_ids=thread_ids
         )
         return self._merge_user_data_and_social_stats(users, social_stats)
 

lms/djangoapps/django_comment_client/tests/management/test_export_discussion_participation.py

@@ -19,11 +19,13 @@ from datetime import datetime
 _target_module = "django_comment_client.management.commands.export_discussion_participation"
 
 _std_parameters_list = (
-    (CourseLocator(org="edX", course="demoX", run="now"), None, None),
-    (CourseLocator(org="otherX", course="courseX", run="later"), datetime(2015, 2, 12), None),
-    (CourseLocator(org="NotAX", course="NotADemo", run="anyyear"), None, 'discussion'),
-    (CourseLocator(org="YeaAX", course="YesADemo", run="anyday"), None, 'question'),
-    (CourseLocator(org="WhatX", course="WhatADemo", run="last_year"), datetime(2014, 3, 17), 'question')
+    (CourseLocator(org="edX", course="demoX", run="now"), None, None, None),
+    (CourseLocator(org="otherX", course="courseX", run="later"), datetime(2015, 2, 12), None, None),
+    (CourseLocator(org="NotAX", course="NotADemo", run="anyyear"), None, 'discussion', None),
+    (CourseLocator(org="YeaAX", course="YesADemo", run="anyday"), None, 'question', None),
+    (CourseLocator(org="WhatX", course="WhatADemo", run="last_year"), datetime(2014, 3, 17), 'question', None),
+    (CourseLocator(org="WhatX", course="WhatADemo", run="last_year"), datetime(2014, 3, 17), None, ['123']),
+    (CourseLocator(org="WhatX", course="WhatADemo", run="last_year"), datetime(2014, 3, 17), 'question', ['1', '2']),
 )
 # pylint: enable=invalid-name
 
@@ -42,38 +44,62 @@ class CommandTest(TestCase):
         self.command.stdout = mock.Mock()
         self.command.stderr = mock.Mock()
 
-    def set_up_default_mocks(self, patched_get_courses):
+    def set_up_default_mocks(self, patched_get_course):
         """ Sets up default mocks passed via class decorator """
-        patched_get_courses.return_value = CourseLocator("edX", "demoX", "now")
+        patched_get_course.return_value = mock.Mock(spec=CourseLocator)
 
     # pylint:disable=unused-argument
-    def test_handle_given_no_arguments_raises_command_error(self, patched_get_courses):
+    def test_handle_given_no_arguments_raises_command_error(self, patched_get_course):
         """ Tests that raises error if invoked with no arguments """
         with self.assertRaises(CommandError):
             self.command.handle()
 
     # pylint:disable=unused-argument
-    def test_handle_given_more_than_two_args_raises_command_error(self, patched_get_courses):
+    def test_handle_given_more_than_two_args_raises_command_error(self, patched_get_course):
         """ Tests that raises error if invoked with too many arguments """
         with self.assertRaises(CommandError):
             self.command.handle(1, 2, 3)
 
-    def test_handle_given_invalid_course_key_raises_invalid_key_error(self, patched_get_courses):
+    def test_handle_given_invalid_course_key_raises_invalid_key_error(self, patched_get_course):
         """ Tests that invalid key errors are propagated """
-        patched_get_courses.return_value = None
+        patched_get_course.return_value = None
         with self.assertRaises(InvalidKeyError):
             self.command.handle("I'm invalid key")
 
-    def test_handle_given_missing_course_raises_command_error(self, patched_get_courses):
+    def test_handle_given_missing_course_raises_command_error(self, patched_get_course):
         """ Tests that raises command error if missing course key was provided """
-        patched_get_courses.return_value = None
+        patched_get_course.return_value = None
         with self.assertRaises(CommandError):
             self.command.handle("edX/demoX/now")
 
+    # pylint: disable=unused-argument
+    def test_all_option(self, patched_get_course):
+        """ Tests that the 'all' option does run the dump command for all courses """
+        self.command.dump_one = mock.Mock()
+        self.command.get_all_courses = mock.Mock()
+        course_list = [mock.Mock() for __ in range(0, 3)]
+        locator_list = [
+            CourseLocator(org="edX", course="demoX", run="now"),
+            CourseLocator(org="Sandbox", course="Sandbox", run="Sandbox"),
+            CourseLocator(org="Test", course="Testy", run="Testify"),
+        ]
+        for index, course in enumerate(course_list):
+            course.location.course_key = locator_list[index]
+        self.command.get_all_courses.return_value = course_list
+        self.command.handle("test_dir", all=True, dummy='test')
+        calls = self.command.dump_one.call_args_list
+        self.assertEqual(len(calls), 3)
+        self.assertEqual(calls[0][0][0], 'course-v1:edX+demoX+now')
+        self.assertEqual(calls[1][0][0], 'course-v1:Sandbox+Sandbox+Sandbox')
+        self.assertEqual(calls[2][0][0], 'course-v1:Test+Testy+Testify')
+        self.assertIn('test_dir/social_stats_course-v1edXdemoXnow', calls[0][0][1])
+        self.assertIn('test_dir/social_stats_course-v1SandboxSandboxSandbox', calls[1][0][1])
+        self.assertIn('test_dir/social_stats_course-v1TestTestyTestify', calls[2][0][1])
+
     @ddt.data("edX/demoX/now", "otherX/CourseX/later")
-    def test_handle_writes_to_correct_location_when_output_file_not_specified(self, course_key, patched_get_courses):
+    def test_handle_writes_to_correct_location_when_output_file_not_specified(self, course_key, patched_get_course):
         """ Tests that when no explicit filename is given data is exported to default location """
-        self.set_up_default_mocks(patched_get_courses)
+        self.set_up_default_mocks(patched_get_course)
         expected_filename = utils.format_filename(
             "social_stats_{course}_{date:%Y_%m_%d_%H_%M_%S}.csv".format(course=course_key, date=datetime.utcnow())
         )
@@ -85,9 +111,9 @@ class CommandTest(TestCase):
             patched_open.assert_called_with(expected_filename, 'wb')
 
     @ddt.data("test.csv", "other_file.csv")
-    def test_handle_writes_to_correct_location_when_output_file_is_specified(self, location, patched_get_courses):
+    def test_handle_writes_to_correct_location_when_output_file_is_specified(self, location, patched_get_course):
         """ Tests that when explicit filename is given data is exported to chosen location """
-        self.set_up_default_mocks(patched_get_courses)
+        self.set_up_default_mocks(patched_get_course)
         patched_open = mock.mock_open()
         with mock.patch("{}.open".format(_target_module), patched_open, create=True), \
                 mock.patch(_target_module + ".Extractor.extract") as patched_extractor:
@@ -95,9 +121,9 @@ class CommandTest(TestCase):
             self.command.handle("irrelevant/course/key", location)
             patched_open.assert_called_with(location, 'wb')
 
-    def test_handle_creates_correct_exporter(self, patched_get_courses):
+    def test_handle_creates_correct_exporter(self, patched_get_course):
         """ Tests that creates correct exporter """
-        self.set_up_default_mocks(patched_get_courses)
+        self.set_up_default_mocks(patched_get_course)
         patched_open = mock.mock_open()
         with mock.patch("{}.open".format(_target_module), patched_open, create=True), \
                 mock.patch(_target_module + ".Extractor.extract") as patched_extractor, \
@@ -112,9 +138,9 @@ class CommandTest(TestCase):
         {"1": {"num_threads": 12}},
         {"1": {"num_threads": 14, "num_comments": 7}}
     )
-    def test_handle_exports_correct_data(self, extracted, patched_get_courses):
+    def test_handle_exports_correct_data(self, extracted, patched_get_course):
         """ Tests that invokes export with correct data """
-        self.set_up_default_mocks(patched_get_courses)
+        self.set_up_default_mocks(patched_get_course)
         patched_open = mock.mock_open()
         with mock.patch("{}.open".format(_target_module), patched_open, create=True), \
                 mock.patch(_target_module + ".Extractor.extract") as patched_extractor, \
@@ -126,10 +152,14 @@ class CommandTest(TestCase):
     @ddt.unpack
     @ddt.data(*_std_parameters_list)
     def test_handle_passes_correct_parameters_to_extractor(
-        self, course_key, end_date, thread_type, patched_get_courses
+        self, course_key, end_date, thread_type, cohorted_thread_ids, patched_get_course
     ):
         """ Tests that when no explicit filename is given data is exported to default location """
-        self.set_up_default_mocks(patched_get_courses)
+        self.set_up_default_mocks(patched_get_course)
+        if cohorted_thread_ids:
+            type(patched_get_course.return_value).cohort_config = mock.PropertyMock(
+                return_value={'cohorted_inline_discussions': cohorted_thread_ids}
+            )
         patched_open = mock.mock_open()
         with mock.patch("{}.open".format(_target_module), patched_open, create=True), \
                 mock.patch(_target_module + ".Extractor.extract") as patched_extractor:
@@ -137,9 +167,12 @@ class CommandTest(TestCase):
             self.command.handle(
                 str(course_key),
                 end_date=end_date.isoformat() if end_date else end_date,
-                thread_type=thread_type
+                thread_type=thread_type,
+                cohorted_only=True if cohorted_thread_ids else False
+            )
+            patched_extractor.assert_called_with(
+                course_key, end_date=end_date, thread_type=thread_type, thread_ids=cohorted_thread_ids
             )
-            patched_extractor.assert_called_with(course_key, end_date=end_date, thread_type=thread_type)
 
 
 def _make_user_mock(user_id, username="", email="", first_name="", last_name=""):
@@ -197,15 +230,17 @@ class ExtractorTest(TestCase):
 
     @ddt.unpack
     @ddt.data(*_std_parameters_list)
-    def test_extract_invokes_correct_data_extraction_methods(self, course_key, end_date, thread_type):
+    def test_extract_invokes_correct_data_extraction_methods(self, course_key, end_date, thread_type, thread_ids):
         """ Tests that correct underlying extractors are called with proper arguments """
         with mock.patch(_target_module + '.CourseEnrollment.users_enrolled_in') as patched_users_enrolled_in, \
                 mock.patch(_target_module + ".User.all_social_stats") as patched_all_social_stats:
-            self.extractor.extract(course_key, end_date=end_date, thread_type=thread_type)
+            self.extractor.extract(course_key, end_date=end_date, thread_type=thread_type, thread_ids=thread_ids)
             patched_users_enrolled_in.return_value = []
             patched_users_enrolled_in.patched_all_social_stats = {}
             patched_users_enrolled_in.assert_called_with(course_key)
-            patched_all_social_stats.assert_called_with(str(course_key), end_date=end_date, thread_type=thread_type)
+            patched_all_social_stats.assert_called_with(
+                str(course_key), end_date=end_date, thread_type=thread_type, thread_ids=thread_ids
+            )
 
     @ddt.unpack
     @ddt.data(

lms/djangoapps/static_template_view/views.py

@@ -62,8 +62,8 @@ def render_press_release(request, slug):
 
 
 def render_404(request):
-    return HttpResponseNotFound(render_to_string('static_templates/404.html', {}))
+    return HttpResponseNotFound(render_to_string('static_templates/404-plain.html', {}))
 
 
 def render_500(request):
-    return HttpResponseServerError(render_to_string('static_templates/server-error.html', {}))
+    return HttpResponseServerError(render_to_string('static_templates/server-error-plain.html', {}))

lms/lib/comment_client/user.py

@@ -119,9 +119,9 @@ class User(models.Model):
         return get_user_social_stats(self.id, self.course_id, end_date=end_date)
 
     @classmethod
-    def all_social_stats(cls, course_id, end_date=None, thread_type=None):
+    def all_social_stats(cls, course_id, end_date=None, thread_type=None, thread_ids=None):
         """ Get social stats for all users participating in a course """
-        return get_user_social_stats('*', course_id, end_date=end_date, thread_type=thread_type)
+        return get_user_social_stats('*', course_id, end_date=end_date, thread_type=thread_type, thread_ids=thread_ids)
 
     def _retrieve(self, *args, **kwargs):
         url = self.url(action='get', params=self.attributes)
@@ -156,7 +156,7 @@ class User(models.Model):
         self._update_from_response(response)
 
 
-def get_user_social_stats(user_id, course_id, end_date=None, thread_type=None):
+def get_user_social_stats(user_id, course_id, end_date=None, thread_type=None, thread_ids=None):
     """ Queries cs_comments_service for social_stats """
     if not course_id:
         raise CommentClientRequestError("Must provide course_id when retrieving social stats for the user")
@@ -167,9 +167,11 @@ def get_user_social_stats(user_id, course_id, end_date=None, thread_type=None):
         params.update({'end_date': end_date.isoformat()})
     if thread_type:
         params.update({'thread_type': thread_type})
+    if thread_ids:
+        params.update({'thread_ids': ",".join(thread_ids)})
 
     response = perform_request(
-        'get',
+        'post',
         url,
         params
     )

lms/templates/static_templates/404-plain.html

+<html>
+<body>
+<h1>Page not found</h1>
+</body>
+</html>

lms/templates/static_templates/server-error-plain.html

+<html>
+<body>
+<h1>
+There has been a 500 error on the servers
+</h1>
+</body>
+</html>

requirements/edx/custom.txt

@@ -4,7 +4,7 @@
 -e git+https://github.com/edx-solutions/xblock-mentoring.git@82a4219b865d12db80ac57bda43fef9e30bec3f1#egg=xblock-mentoring
 -e git+https://github.com/edx-solutions/xblock-image-explorer.git@21b9bcc4f2c7917463ab18a596161ac6c58c9c4a#egg=xblock-image-explorer
 -e git+https://github.com/edx-solutions/xblock-drag-and-drop.git@92ee2055a16899090a073e1df81e35d5293ad767#egg=xblock-drag-and-drop
--e git+https://github.com/edx-solutions/xblock-drag-and-drop-v2.git@ed24dbef753e411f2c9b17339ae21f3a89a8531c#egg=xblock-drag-and-drop-v2
+-e git+https://github.com/edx-solutions/xblock-drag-and-drop-v2.git@5736ed8774b92c8b8396b5bd455f8a8fb80295fb#egg=xblock-drag-and-drop-v2
 -e git+https://github.com/edx-solutions/xblock-ooyala.git@ac49b30452aff0cc34cace6a34b788e100490f24#egg=xblock-ooyala
 -e git+https://github.com/edx-solutions/xblock-group-project.git@dd8eaf16b3bc7b7be3fb392d588328dadef56c00#egg=xblock-group-project
 -e git+https://github.com/edx-solutions/xblock-adventure.git@effa22006bb6528bc6d3788787466eb4e74e1161#egg=xblock-adventure