Merge branch 'release'

cms/djangoapps/contentstore/views/videos.py

@@ -46,7 +46,7 @@ class StatusDisplayStrings(object):
     # Translators: This is the status for a video that the servers are currently processing
     _IN_PROGRESS = ugettext_noop("In Progress")
     # Translators: This is the status for a video that the servers have successfully processed
-    _COMPLETE = ugettext_noop("Complete")
+    _COMPLETE = ugettext_noop("Ready")
     # Translators: This is the status for a video that the servers have failed to process
     _FAILED = ugettext_noop("Failed")
     # Translators: This is the status for a video for which an invalid

cms/envs/test.py

@@ -26,7 +26,17 @@ from uuid import uuid4
 
 # import settings from LMS for consistent behavior with CMS
 # pylint: disable=unused-import
-from lms.envs.test import (WIKI_ENABLED, PLATFORM_NAME, SITE_NAME, DEFAULT_FILE_STORAGE, MEDIA_ROOT, MEDIA_URL)
+from lms.envs.test import (
+    WIKI_ENABLED,
+    PLATFORM_NAME,
+    SITE_NAME,
+    DEFAULT_FILE_STORAGE,
+    MEDIA_ROOT,
+    MEDIA_URL,
+    # This is practically unused but needed by the oauth2_provider package, which
+    # some tests in common/ rely on.
+    OAUTH_OIDC_ISSUER,
+)
 
 # mongo connection settings
 MONGO_PORT_NUM = int(os.environ.get('EDXAPP_TEST_MONGO_PORT', '27017'))

cms/templates/videos_index.html

@@ -64,9 +64,10 @@
                 <h3 class="title-3">${_("Monitoring files as they upload")}</h3>
                 <p>${_("Each video file that you upload needs to reach the video processing servers successfully before additional work can begin. You can monitor the progress of files as they upload, and try again if the upload fails.")}</p>
                 <h3 class="title-3">${_("Managing uploaded files")}</h3>
-                <p>${_("After a file uploads successfully, automated processing begins. After automated processing begins for a file it is listed under Previous Uploads as {em_start}In Progress{em_end}. When the status is {em_start}Complete{em_end}, edX assigns a unique video ID to the video file and you can add it to your course. If something goes wrong, the {em_start}Failed{em_end} status message appears. Check for problems in the file and upload a replacement.").format(em_start='<strong>', em_end="</strong>")}</p>
+                <p>${_("After a file uploads successfully, automated processing begins. After automated processing begins for a file it is listed under Previous Uploads as {em_start}In Progress{em_end}. You can add the video to your course as soon as it has a unique video ID and the status is {em_start}Ready{em_end}. Allow 24 hours for file processing at the external video hosting sites to complete.").format(em_start='<strong>', em_end="</strong>")}</p>
+                <p>${_("If something goes wrong, the {em_start}Failed{em_end} status message appears. Check for problems in your original file and upload a replacement.").format(em_start='<strong>', em_end="</strong>")}</p>
                 <h3 class="title-3">${_("How do I get the videos into my course?")}</h3>
-                <p>${_("After processing is complete for the video file, you copy its unique video ID. On the Course Outline page, you create or locate a video component to play this video. Edit the video component to paste the ID into the Advanced {em_start}EdX Video ID{em_end} field.").format(em_start='<strong>', em_end="</strong>")}</p>
+                <p>${_("As soon as the status for a file is {em_start}Ready{em_end}, you can add that video to a component in your course. Copy the unique video ID. In another browser window, on the Course Outline page, create or locate a video component to play this video. Edit the video component to paste the ID into the Advanced {em_start}EdX Video ID{em_end} field. The video can play in the LMS as soon as its status is {em_start}Ready{em_end}, although processing may not be complete for all encodings and all video hosting sites.").format(em_start='<strong>', em_end="</strong>")}</p>
             </div>
         </aside>
     </section>

common/djangoapps/embargo/models.py

@@ -401,6 +401,8 @@ class CountryAccessRule(models.Model):
 
     CACHE_KEY = u"embargo.allowed_countries.{course_key}"
 
+    ALL_COUNTRIES = set(code[0] for code in list(countries))
+
     @classmethod
     def check_country_access(cls, course_id, country):
         """
@@ -415,6 +417,14 @@ class CountryAccessRule(models.Model):
             True if country found in allowed country
             otherwise check given country exists in list
         """
+        # If the country code is not in the list of all countries,
+        # we don't want to automatically exclude the user.
+        # This can happen, for example, when GeoIP falls back
+        # to using a continent code because it cannot determine
+        # the specific country.
+        if country not in cls.ALL_COUNTRIES:
+            return True
+
         cache_key = cls.CACHE_KEY.format(course_key=course_id)
         allowed_countries = cache.get(cache_key)
         if allowed_countries is None:
@@ -454,7 +464,7 @@ class CountryAccessRule(models.Model):
 
         # If there are no whitelist countries, default to all countries
         if not whitelist_countries:
-            whitelist_countries = set(code[0] for code in list(countries))
+            whitelist_countries = cls.ALL_COUNTRIES
 
         # Consolidate the rules into a single list of countries
         # that have access to the course.

common/djangoapps/embargo/tests/test_api.py

@@ -2,6 +2,7 @@
 Tests for EmbargoMiddleware
 """
 
+from contextlib import contextmanager
 import mock
 import unittest
 import pygeoip
@@ -85,9 +86,7 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
             self.user.profile.save()
 
         # Appear to make a request from an IP in a particular country
-        with mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr') as mock_ip:
-            mock_ip.return_value = ip_country
-
+        with self._mock_geoip(ip_country):
             # Call the API.  Note that the IP address we pass in doesn't
             # matter, since we're injecting a mock for geo-location
             result = embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
@@ -113,9 +112,7 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
             country=Country.objects.get(country='US')
         )
 
-        with mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr') as mock_ip:
-            mock_ip.return_value = 'US'
-
+        with self._mock_geoip('US'):
             # The user is set to None, because the user has not been authenticated.
             result = embargo_api.check_course_access(self.course.id, ip_address='0.0.0.0')
             self.assertFalse(result)
@@ -137,6 +134,14 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
         result = embargo_api.check_course_access(self.course.id, user=self.user, ip_address='FE80::0202:B3FF:FE1E:8329')
         self.assertTrue(result)
 
+    def test_country_access_fallback_to_continent_code(self):
+        # Simulate PyGeoIP falling back to a continent code
+        # instead of a country code.  In this case, we should
+        # allow the user access.
+        with self._mock_geoip('EU'):
+            result = embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
+            self.assertTrue(result)
+
     @mock.patch.dict(settings.FEATURES, {'EMBARGO': True})
     def test_profile_country_db_null(self):
         # Django country fields treat NULL values inconsistently.
@@ -156,15 +161,16 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
         self.assertTrue(result)
 
     def test_caching(self):
-        # Test the scenario that will go through every check
-        # (restricted course, but pass all the checks)
-        # This is the worst case, so it will hit all of the
-        # caching code.
-        with self.assertNumQueries(3):
-            embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
+        with self._mock_geoip('US'):
+            # Test the scenario that will go through every check
+            # (restricted course, but pass all the checks)
+            # This is the worst case, so it will hit all of the
+            # caching code.
+            with self.assertNumQueries(3):
+                embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
 
-        with self.assertNumQueries(0):
-            embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
+            with self.assertNumQueries(0):
+                embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
 
     def test_caching_no_restricted_courses(self):
         RestrictedCourse.objects.all().delete()
@@ -176,6 +182,12 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
         with self.assertNumQueries(0):
             embargo_api.check_course_access(self.course.id, user=self.user, ip_address='0.0.0.0')
 
+    @contextmanager
+    def _mock_geoip(self, country_code):
+        with mock.patch.object(pygeoip.GeoIP, 'country_code_by_addr') as mock_ip:
+            mock_ip.return_value = country_code
+            yield
+
 
 @ddt.ddt
 @override_settings(MODULESTORE=MODULESTORE_CONFIG)

common/djangoapps/oauth_exchange/forms.py

+"""
+Forms to support third-party to first-party OAuth 2.0 access token exchange
+"""
+from django.contrib.auth.models import User
+from django.forms import CharField
+from oauth2_provider.constants import SCOPE_NAMES
+import provider.constants
+from provider.forms import OAuthForm, OAuthValidationError
+from provider.oauth2.forms import ScopeChoiceField, ScopeMixin
+from provider.oauth2.models import Client
+from requests import HTTPError
+from social.backends import oauth as social_oauth
+
+from third_party_auth import pipeline
+
+
+class AccessTokenExchangeForm(ScopeMixin, OAuthForm):
+    """Form for access token exchange endpoint"""
+    access_token = CharField(required=False)
+    scope = ScopeChoiceField(choices=SCOPE_NAMES, required=False)
+    client_id = CharField(required=False)
+
+    def __init__(self, request, *args, **kwargs):
+        super(AccessTokenExchangeForm, self).__init__(*args, **kwargs)
+        self.request = request
+
+    def _require_oauth_field(self, field_name):
+        """
+        Raise an appropriate OAuthValidationError error if the field is missing
+        """
+        field_val = self.cleaned_data.get(field_name)
+        if not field_val:
+            raise OAuthValidationError(
+                {
+                    "error": "invalid_request",
+                    "error_description": "{} is required".format(field_name),
+                }
+            )
+        return field_val
+
+    def clean_access_token(self):
+        return self._require_oauth_field("access_token")
+
+    def clean_client_id(self):
+        return self._require_oauth_field("client_id")
+
+    def clean(self):
+        if self._errors:
+            return {}
+
+        backend = self.request.social_strategy.backend
+        if not isinstance(backend, social_oauth.BaseOAuth2):
+            raise OAuthValidationError(
+                {
+                    "error": "invalid_request",
+                    "error_description": "{} is not a supported provider".format(backend.name),
+                }
+            )
+
+        self.request.session[pipeline.AUTH_ENTRY_KEY] = pipeline.AUTH_ENTRY_API
+
+        client_id = self.cleaned_data["client_id"]
+        try:
+            client = Client.objects.get(client_id=client_id)
+        except Client.DoesNotExist:
+            raise OAuthValidationError(
+                {
+                    "error": "invalid_client",
+                    "error_description": "{} is not a valid client_id".format(client_id),
+                }
+            )
+        if client.client_type != provider.constants.PUBLIC:
+            raise OAuthValidationError(
+                {
+                    # invalid_client isn't really the right code, but this mirrors
+                    # https://github.com/edx/django-oauth2-provider/blob/edx/provider/oauth2/forms.py#L331
+                    "error": "invalid_client",
+                    "error_description": "{} is not a public client".format(client_id),
+                }
+            )
+        self.cleaned_data["client"] = client
+
+        user = None
+        try:
+            user = backend.do_auth(self.cleaned_data.get("access_token"))
+        except HTTPError:
+            pass
+        if user and isinstance(user, User):
+            self.cleaned_data["user"] = user
+        else:
+            # Ensure user does not re-enter the pipeline
+            self.request.social_strategy.clean_partial_pipeline()
+            raise OAuthValidationError(
+                {
+                    "error": "invalid_grant",
+                    "error_description": "access_token is not valid",
+                }
+            )
+
+        return self.cleaned_data

common/djangoapps/oauth_exchange/models.py

+"""
+A models.py is required to make this an app (until we move to Django 1.7)
+"""

common/djangoapps/oauth_exchange/tests/test_forms.py

+"""
+Tests for OAuth token exchange forms
+"""
+import unittest
+
+from django.conf import settings
+from django.contrib.sessions.middleware import SessionMiddleware
+from django.test import TestCase
+from django.test.client import RequestFactory
+import httpretty
+from provider import scope
+import social.apps.django_app.utils as social_utils
+
+from oauth_exchange.forms import AccessTokenExchangeForm
+from oauth_exchange.tests.utils import (
+    AccessTokenExchangeTestMixin,
+    AccessTokenExchangeMixinFacebook,
+    AccessTokenExchangeMixinGoogle
+)
+
+
+class AccessTokenExchangeFormTest(AccessTokenExchangeTestMixin):
+    """
+    Mixin that defines test cases for AccessTokenExchangeForm
+    """
+    def setUp(self):
+        super(AccessTokenExchangeFormTest, self).setUp()
+        self.request = RequestFactory().post("dummy_url")
+        SessionMiddleware().process_request(self.request)
+        self.request.social_strategy = social_utils.load_strategy(self.request, self.BACKEND)
+
+    def _assert_error(self, data, expected_error, expected_error_description):
+        form = AccessTokenExchangeForm(request=self.request, data=data)
+        self.assertEqual(
+            form.errors,
+            {"error": expected_error, "error_description": expected_error_description}
+        )
+        self.assertNotIn("partial_pipeline", self.request.session)
+
+    def _assert_success(self, data, expected_scopes):
+        form = AccessTokenExchangeForm(request=self.request, data=data)
+        self.assertTrue(form.is_valid())
+        self.assertEqual(form.cleaned_data["user"], self.user)
+        self.assertEqual(form.cleaned_data["client"], self.oauth_client)
+        self.assertEqual(scope.to_names(form.cleaned_data["scope"]), expected_scopes)
+
+
+# This is necessary because cms does not implement third party auth
+@unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled")
+@httpretty.activate
+class AccessTokenExchangeFormTestFacebook(
+        AccessTokenExchangeFormTest,
+        AccessTokenExchangeMixinFacebook,
+        TestCase
+):
+    """
+    Tests for AccessTokenExchangeForm used with Facebook
+    """
+    pass
+
+
+# This is necessary because cms does not implement third party auth
+@unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled")
+@httpretty.activate
+class AccessTokenExchangeFormTestGoogle(
+        AccessTokenExchangeFormTest,
+        AccessTokenExchangeMixinGoogle,
+        TestCase
+):
+    """
+    Tests for AccessTokenExchangeForm used with Google
+    """
+    pass

common/djangoapps/oauth_exchange/tests/test_views.py

+"""
+Tests for OAuth token exchange views
+"""
+from datetime import timedelta
+import json
+import mock
+import unittest
+
+from django.conf import settings
+from django.core.urlresolvers import reverse
+from django.test import TestCase
+import httpretty
+import provider.constants
+from provider import scope
+from provider.oauth2.models import AccessToken
+
+from oauth_exchange.tests.utils import (
+    AccessTokenExchangeTestMixin,
+    AccessTokenExchangeMixinFacebook,
+    AccessTokenExchangeMixinGoogle
+)
+
+
+class AccessTokenExchangeViewTest(AccessTokenExchangeTestMixin):
+    """
+    Mixin that defines test cases for AccessTokenExchangeView
+    """
+    def setUp(self):
+        super(AccessTokenExchangeViewTest, self).setUp()
+        self.url = reverse("exchange_access_token", kwargs={"backend": self.BACKEND})
+
+    def _assert_error(self, data, expected_error, expected_error_description):
+        response = self.client.post(self.url, data)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response["Content-Type"], "application/json")
+        self.assertEqual(
+            json.loads(response.content),
+            {"error": expected_error, "error_description": expected_error_description}
+        )
+        self.assertNotIn("partial_pipeline", self.client.session)
+
+    def _assert_success(self, data, expected_scopes):
+        response = self.client.post(self.url, data)
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response["Content-Type"], "application/json")
+        content = json.loads(response.content)
+        self.assertEqual(set(content.keys()), {"access_token", "token_type", "expires_in", "scope"})
+        self.assertEqual(content["token_type"], "Bearer")
+        self.assertLessEqual(
+            timedelta(seconds=int(content["expires_in"])),
+            provider.constants.EXPIRE_DELTA_PUBLIC
+        )
+        self.assertEqual(content["scope"], " ".join(expected_scopes))
+        token = AccessToken.objects.get(token=content["access_token"])
+        self.assertEqual(token.user, self.user)
+        self.assertEqual(token.client, self.oauth_client)
+        self.assertEqual(scope.to_names(token.scope), expected_scopes)
+
+    def test_single_access_token(self):
+        def extract_token(response):
+            return json.loads(response.content)["access_token"]
+
+        self._setup_provider_response(success=True)
+        for single_access_token in [True, False]:
+            with mock.patch(
+                "oauth_exchange.views.constants.SINGLE_ACCESS_TOKEN",
+                single_access_token
+            ):
+                first_response = self.client.post(self.url, self.data)
+                second_response = self.client.post(self.url, self.data)
+                self.assertEqual(
+                    extract_token(first_response) == extract_token(second_response),
+                    single_access_token
+                )
+
+    def test_get_method(self):
+        response = self.client.get(self.url, self.data)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            json.loads(response.content),
+            {
+                "error": "invalid_request",
+                "error_description": "Only POST requests allowed.",
+            }
+        )
+
+    def test_invalid_provider(self):
+        url = reverse("exchange_access_token", kwargs={"backend": "invalid"})
+        response = self.client.post(url, self.data)
+        self.assertEqual(response.status_code, 404)
+
+
+# This is necessary because cms does not implement third party auth
+@unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled")
+@httpretty.activate
+class AccessTokenExchangeViewTestFacebook(
+        AccessTokenExchangeViewTest,
+        AccessTokenExchangeMixinFacebook,
+        TestCase
+):
+    """
+    Tests for AccessTokenExchangeView used with Facebook
+    """
+    pass
+
+
+# This is necessary because cms does not implement third party auth
+@unittest.skipUnless(settings.FEATURES.get("ENABLE_THIRD_PARTY_AUTH"), "third party auth not enabled")
+@httpretty.activate
+class AccessTokenExchangeViewTestGoogle(
+        AccessTokenExchangeViewTest,
+        AccessTokenExchangeMixinGoogle,
+        TestCase
+):
+    """
+    Tests for AccessTokenExchangeView used with Google
+    """
+    pass

common/djangoapps/oauth_exchange/tests/utils.py

+"""
+Test utilities for OAuth access token exchange
+"""
+import json
+
+import httpretty
+import provider.constants
+from provider.oauth2.models import Client
+from social.apps.django_app.default.models import UserSocialAuth
+
+from student.tests.factories import UserFactory
+
+
+class AccessTokenExchangeTestMixin(object):
+    """
+    A mixin to define test cases for access token exchange. The following
+    methods must be implemented by subclasses:
+    * _assert_error(data, expected_error, expected_error_description)
+    * _assert_success(data, expected_scopes)
+    """
+    def setUp(self):
+        super(AccessTokenExchangeTestMixin, self).setUp()
+
+        self.client_id = "test_client_id"
+        self.oauth_client = Client.objects.create(
+            client_id=self.client_id,
+            client_type=provider.constants.PUBLIC
+        )
+        self.social_uid = "test_social_uid"
+        self.user = UserFactory()
+        UserSocialAuth.objects.create(user=self.user, provider=self.BACKEND, uid=self.social_uid)
+        self.access_token = "test_access_token"
+        # Initialize to minimal data
+        self.data = {
+            "access_token": self.access_token,
+            "client_id": self.client_id,
+        }
+
+    def _setup_provider_response(self, success):
+        """
+        Register a mock response for the third party user information endpoint;
+        success indicates whether the response status code should be 200 or 400
+        """
+        if success:
+            status = 200
+            body = json.dumps({self.UID_FIELD: self.social_uid})
+        else:
+            status = 400
+            body = json.dumps({})
+        httpretty.register_uri(
+            httpretty.GET,
+            self.USER_URL,
+            body=body,
+            status=status,
+            content_type="application/json"
+        )
+
+    def _assert_error(self, _data, _expected_error, _expected_error_description):
+        """
+        Given request data, execute a test and check that the expected error
+        was returned (along with any other appropriate assertions).
+        """
+        raise NotImplementedError()
+
+    def _assert_success(self, data, expected_scopes):
+        """
+        Given request data, execute a test and check that the expected scopes
+        were returned (along with any other appropriate assertions).
+        """
+        raise NotImplementedError()
+
+    def test_minimal(self):
+        self._setup_provider_response(success=True)
+        self._assert_success(self.data, expected_scopes=[])
+
+    def test_scopes(self):
+        self._setup_provider_response(success=True)
+        self.data["scope"] = "profile email"
+        self._assert_success(self.data, expected_scopes=["profile", "email"])
+
+    def test_missing_fields(self):
+        for field in ["access_token", "client_id"]:
+            data = dict(self.data)
+            del data[field]
+            self._assert_error(data, "invalid_request", "{} is required".format(field))
+
+    def test_invalid_client(self):
+        self.data["client_id"] = "nonexistent_client"
+        self._assert_error(
+            self.data,
+            "invalid_client",
+            "nonexistent_client is not a valid client_id"
+        )
+
+    def test_confidential_client(self):
+        self.oauth_client.client_type = provider.constants.CONFIDENTIAL
+        self.oauth_client.save()
+        self._assert_error(
+            self.data,
+            "invalid_client",
+            "test_client_id is not a public client"
+        )
+
+    def test_invalid_acess_token(self):
+        self._setup_provider_response(success=False)
+        self._assert_error(self.data, "invalid_grant", "access_token is not valid")
+
+    def test_no_linked_user(self):
+        UserSocialAuth.objects.all().delete()
+        self._setup_provider_response(success=True)
+        self._assert_error(self.data, "invalid_grant", "access_token is not valid")
+
+
+class AccessTokenExchangeMixinFacebook(object):
+    """Tests access token exchange with the Facebook backend"""
+    BACKEND = "facebook"
+    USER_URL = "https://graph.facebook.com/me"
+    # In facebook responses, the "id" field is used as the user's identifier
+    UID_FIELD = "id"
+
+
+class AccessTokenExchangeMixinGoogle(object):
+    """Tests access token exchange with the Google backend"""
+    BACKEND = "google-oauth2"
+    USER_URL = "https://www.googleapis.com/oauth2/v1/userinfo"
+    # In google-oauth2 responses, the "email" field is used as the user's identifier
+    UID_FIELD = "email"

common/djangoapps/oauth_exchange/views.py

+"""
+Views to support third-party to first-party OAuth 2.0 access token exchange
+"""
+from django.utils.decorators import method_decorator
+from django.views.decorators.csrf import csrf_exempt
+from provider import constants
+from provider.oauth2.views import AccessTokenView as AccessTokenView
+import social.apps.django_app.utils as social_utils
+
+from oauth_exchange.forms import AccessTokenExchangeForm
+
+
+class AccessTokenExchangeView(AccessTokenView):
+    """View for access token exchange"""
+    @method_decorator(csrf_exempt)
+    @method_decorator(social_utils.strategy("social:complete"))
+    def dispatch(self, *args, **kwargs):
+        return super(AccessTokenExchangeView, self).dispatch(*args, **kwargs)
+
+    def get(self, request, _backend):
+        return super(AccessTokenExchangeView, self).get(request)
+
+    def post(self, request, _backend):
+        form = AccessTokenExchangeForm(request=request, data=request.POST)
+        if not form.is_valid():
+            return self.error_response(form.errors)
+
+        user = form.cleaned_data["user"]
+        scope = form.cleaned_data["scope"]
+        client = form.cleaned_data["client"]
+
+        if constants.SINGLE_ACCESS_TOKEN:
+            edx_access_token = self.get_access_token(request, user, scope, client)
+        else:
+            edx_access_token = self.create_access_token(request, user, scope, client)
+
+        return self.access_token_response(edx_access_token)

common/djangoapps/track/shim.py

@@ -76,6 +76,8 @@ NAME_TO_EVENT_TYPE_MAP = {
     'edx.video.paused': 'pause_video',
     'edx.video.stopped': 'stop_video',
     'edx.video.loaded': 'load_video',
+    'edx.video.position.changed': 'seek_video',
+    'edx.video.seeked': 'seek_video',
     'edx.video.transcript.shown': 'show_transcript',
     'edx.video.transcript.hidden': 'hide_transcript',
 }
@@ -101,11 +103,14 @@ class VideoEventProcessor(object):
         if name not in NAME_TO_EVENT_TYPE_MAP:
             return
 
+        # Convert edx.video.seeked to edx.video.positiion.changed
+        if name == "edx.video.seeked":
+            event['name'] = "edx.video.position.changed"
+
         event['event_type'] = NAME_TO_EVENT_TYPE_MAP[name]
 
         if 'event' not in event:
             return
-
         payload = event['event']
 
         if 'module_id' in payload:
@@ -122,13 +127,38 @@ class VideoEventProcessor(object):
         if 'current_time' in payload:
             payload['currentTime'] = payload.pop('current_time')
 
-        event['event'] = json.dumps(payload)
-
-        if 'context' not in event:
-            return
-
-        context = event['context']
+        if 'context' in event:
+            context = event['context']
+
+            # Converts seek_type to seek and skip|slide to onSlideSeek|onSkipSeek
+            if 'seek_type' in payload:
+                seek_type = payload['seek_type']
+                if seek_type == 'slide':
+                    payload['type'] = "onSlideSeek"
+                elif seek_type == 'skip':
+                    payload['type'] = "onSkipSeek"
+                del payload['seek_type']
+
+            # For the iOS build that is returning a +30 for back skip 30
+            if (
+                context['application']['version'] == "1.0.02" and
+                context['application']['name'] == "edx.mobileapp.iOS"
+            ):
+                if 'requested_skip_interval' in payload and 'type' in payload:
+                    if (
+                        payload['requested_skip_interval'] == 30 and
+                        payload['type'] == "onSkipSeek"
+                    ):
+                        payload['requested_skip_interval'] = -30
+
+            # For the Android build that isn't distinguishing between skip/seek
+            if 'requested_skip_interval' in payload:
+                if abs(payload['requested_skip_interval']) != 30:
+                    if 'type' in payload:
+                        payload['type'] = 'onSlideSeek'
+
+            if 'open_in_browser_url' in context:
+                page, _sep, _tail = context.pop('open_in_browser_url').rpartition('/')
+                event['page'] = page
 
-        if 'open_in_browser_url' in context:
-            page, _sep, _tail = context.pop('open_in_browser_url').rpartition('/')
-            event['page'] = page
+        event['event'] = json.dumps(payload)

common/djangoapps/track/views/tests/test_segmentio.py

@@ -316,6 +316,7 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase):
         ('edx.video.paused', 'pause_video'),
         ('edx.video.stopped', 'stop_video'),
         ('edx.video.loaded', 'load_video'),
+        ('edx.video.position.changed', 'seek_video'),
         ('edx.video.transcript.shown', 'show_transcript'),
         ('edx.video.transcript.hidden', 'hide_transcript'),
     )
@@ -404,3 +405,127 @@ class SegmentIOTrackingTestCase(EventTrackingTestCase):
 
         self.assertEqualUnicode(actual_event, expected_event_without_payload)
         self.assertEqualUnicode(payload, expected_payload)
+
+    @data(
+        # Verify positive slide case. Verify slide to onSlideSeek. Verify edx.video.seeked emitted from iOS v1.0.02 is changed to edx.video.position.changed.
+        (1, 1, "seek_type", "slide", "onSlideSeek", "edx.video.seeked", "edx.video.position.changed", 'edx.mobileapp.iOS', '1.0.02'),
+        # Verify negative slide case. Verify slide to onSlideSeek. Verify edx.video.seeked to edx.video.position.changed.
+        (-2, -2, "seek_type", "slide", "onSlideSeek", "edx.video.seeked", "edx.video.position.changed", 'edx.mobileapp.iOS', '1.0.02'),
+        # Verify +30 is changed to -30 which is incorrectly emitted in iOS v1.0.02. Verify skip to onSkipSeek
+        (30, -30, "seek_type", "skip", "onSkipSeek", "edx.video.position.changed", "edx.video.position.changed", 'edx.mobileapp.iOS', '1.0.02'),
+        # Verify the correct case of -30 is also handled as well. Verify skip to onSkipSeek
+        (-30, -30, "seek_type", "skip", "onSkipSeek", "edx.video.position.changed", "edx.video.position.changed", 'edx.mobileapp.iOS', '1.0.02'),
+        # Verify positive slide case where onSkipSeek is changed to onSlideSkip. Verify edx.video.seeked emitted from Android v1.0.02 is changed to edx.video.position.changed.
+        (1, 1, "type", "onSkipSeek", "onSlideSeek", "edx.video.seeked", "edx.video.position.changed", 'edx.mobileapp.android', '1.0.02'),
+        # Verify positive slide case where onSkipSeek is changed to onSlideSkip. Verify edx.video.seeked emitted from Android v1.0.02 is changed to edx.video.position.changed.
+        (-2, -2, "type", "onSkipSeek", "onSlideSeek", "edx.video.seeked", "edx.video.position.changed", 'edx.mobileapp.android', '1.0.02'),
+        # Verify positive skip case where onSkipSeek is not changed and does not become negative.
+        (30, 30, "type", "onSkipSeek", "onSkipSeek", "edx.video.position.changed", "edx.video.position.changed", 'edx.mobileapp.android', '1.0.02'),
+        # Verify positive skip case where onSkipSeek is not changed.
+        (-30, -30, "type", "onSkipSeek", "onSkipSeek", "edx.video.position.changed", "edx.video.position.changed", 'edx.mobileapp.android', '1.0.02')
+    )
+    @unpack
+    def test_previous_builds(self,
+                             requested_skip_interval,
+                             expected_skip_interval,
+                             seek_type_key,
+                             seek_type,
+                             expected_seek_type,
+                             name,
+                             expected_name,
+                             platform,
+                             version,
+                             ):
+        """
+        Test backwards compatibility of previous app builds
+
+        iOS version 1.0.02: Incorrectly emits the skip back 30 seconds as +30
+        instead of -30.
+        Android version 1.0.02: Skip and slide were both being returned as a
+        skip. Skip or slide is determined by checking if the skip time is == -30
+        Additionally, for both of the above mentioned versions, edx.video.seeked
+        was sent instead of edx.video.position.changed
+        """
+        course_id = 'foo/bar/baz'
+        middleware = TrackMiddleware()
+        input_payload = {
+            "code": "mobile",
+            "new_time": 89.699177437,
+            "old_time": 119.699177437,
+            seek_type_key: seek_type,
+            "requested_skip_interval": requested_skip_interval,
+            'module_id': 'i4x://foo/bar/baz/some_module',
+        }
+        request = self.create_request(
+            data=self.create_segmentio_event_json(
+                name=name,
+                data=input_payload,
+                context={
+                    'open_in_browser_url': 'https://testserver/courses/foo/bar/baz/courseware/Week_1/Activity/2',
+                    'course_id': course_id,
+                    'application': {
+                        'name': platform,
+                        'version': version,
+                        'component': 'videoplayer'
+                    }
+                },
+            ),
+            content_type='application/json'
+        )
+        User.objects.create(pk=USER_ID, username=str(sentinel.username))
+
+        middleware.process_request(request)
+        try:
+            response = segmentio.segmentio_event(request)
+            self.assertEquals(response.status_code, 200)
+
+            expected_event_without_payload = {
+                'accept_language': '',
+                'referer': '',
+                'username': str(sentinel.username),
+                'ip': '',
+                'session': '',
+                'event_source': 'mobile',
+                'event_type': "seek_video",
+                'name': expected_name,
+                'agent': str(sentinel.user_agent),
+                'page': 'https://testserver/courses/foo/bar/baz/courseware/Week_1/Activity',
+                'time': datetime.strptime("2014-08-27T16:33:39.215Z", "%Y-%m-%dT%H:%M:%S.%fZ"),
+                'host': 'testserver',
+                'context': {
+                    'user_id': USER_ID,
+                    'course_id': course_id,
+                    'org_id': 'foo',
+                    'path': ENDPOINT,
+                    'client': {
+                        'library': {
+                            'name': 'test-app',
+                            'version': 'unknown'
+                        },
+                        'app': {
+                            'version': '1.0.1',
+                        },
+                    },
+                    'application': {
+                        'name': platform,
+                        'version': version,
+                        'component': 'videoplayer'
+                    },
+                    'received_at': datetime.strptime("2014-08-27T16:33:39.100Z", "%Y-%m-%dT%H:%M:%S.%fZ"),
+                },
+            }
+            expected_payload = {
+                "code": "mobile",
+                "new_time": 89.699177437,
+                "old_time": 119.699177437,
+                "type": expected_seek_type,
+                "requested_skip_interval": expected_skip_interval,
+                'id': 'i4x-foo-bar-baz-some_module',
+            }
+        finally:
+            middleware.process_response(request, None)
+
+        actual_event = dict(self.get_event())
+        payload = json.loads(actual_event.pop('event'))
+        self.assertEqualUnicode(actual_event, expected_event_without_payload)
+        self.assertEqualUnicode(payload, expected_payload)

common/test/acceptance/pages/lms/discussion.py

@@ -311,13 +311,15 @@ class DiscussionSortPreferencePage(CoursePage):
 
 
 class DiscussionTabSingleThreadPage(CoursePage):
-    def __init__(self, browser, course_id, thread_id):
+    def __init__(self, browser, course_id, discussion_id, thread_id):
         super(DiscussionTabSingleThreadPage, self).__init__(browser, course_id)
         self.thread_page = DiscussionThreadPage(
             browser,
             "body.discussion .discussion-article[data-id='{thread_id}']".format(thread_id=thread_id)
         )
-        self.url_path = "discussion/forum/dummy/threads/" + thread_id
+        self.url_path = "discussion/forum/{discussion_id}/threads/{thread_id}".format(
+            discussion_id=discussion_id, thread_id=thread_id
+        )
 
     def is_browser_on_page(self):
         return self.thread_page.is_browser_on_page()

common/test/acceptance/tests/discussion/helpers.py

@@ -5,12 +5,15 @@ Helper functions and classes for discussion tests.
 from uuid import uuid4
 import json
 
+from ...fixtures import LMS_BASE_URL
+from ...fixtures.course import CourseFixture
 from ...fixtures.discussion import (
     SingleThreadViewFixture,
     Thread,
     Response,
 )
-from ...fixtures import LMS_BASE_URL
+from ...pages.lms.discussion import DiscussionTabSingleThreadPage
+from ...tests.helpers import UniqueCourseTest
 
 
 class BaseDiscussionMixin(object):
@@ -83,3 +86,22 @@ class CohortTestMixin(object):
         data = {"users": username}
         response = course_fixture.session.post(url, data=data, headers=course_fixture.headers)
         self.assertTrue(response.ok, "Failed to add user to cohort")
+
+
+class BaseDiscussionTestCase(UniqueCourseTest):
+    def setUp(self):
+        super(BaseDiscussionTestCase, self).setUp()
+
+        self.discussion_id = "test_discussion_{}".format(uuid4().hex)
+        self.course_fixture = CourseFixture(**self.course_info)
+        self.course_fixture.add_advanced_settings(
+            {'discussion_topics': {'value': {'Test Discussion Topic': {'id': self.discussion_id}}}}
+        )
+        self.course_fixture.install()
+
+    def create_single_thread_page(self, thread_id):
+        """
+        Sets up a `DiscussionTabSingleThreadPage` for a given
+        `thread_id`.
+        """
+        return DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id)

common/test/acceptance/tests/discussion/test_cohorts.py

@@ -3,7 +3,7 @@ Tests related to the cohorting feature.
 """
 from uuid import uuid4
 
-from .helpers import BaseDiscussionMixin
+from .helpers import BaseDiscussionMixin, BaseDiscussionTestCase
 from .helpers import CohortTestMixin
 from ..helpers import UniqueCourseTest
 from ...pages.lms.auto_auth import AutoAuthPage
@@ -57,20 +57,17 @@ class CohortedDiscussionTestMixin(BaseDiscussionMixin, CohortTestMixin):
         self.assertEquals(self.thread_page.get_group_visibility_label(), "This post is visible to everyone.")
 
 
-class DiscussionTabSingleThreadTest(UniqueCourseTest):
+class DiscussionTabSingleThreadTest(BaseDiscussionTestCase):
     """
     Tests for the discussion page displaying a single thread.
     """
     def setUp(self):
         super(DiscussionTabSingleThreadTest, self).setUp()
-        self.discussion_id = "test_discussion_{}".format(uuid4().hex)
-        # Create a course to register for
-        self.course_fixture = CourseFixture(**self.course_info).install()
         self.setup_cohorts()
         AutoAuthPage(self.browser, course_id=self.course_id).visit()
 
     def setup_thread_page(self, thread_id):
-        self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id)  # pylint: disable=attribute-defined-outside-init
+        self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, self.discussion_id, thread_id)  # pylint: disable=attribute-defined-outside-init
         self.thread_page.visit()
 
     # pylint: disable=unused-argument

common/test/acceptance/tests/discussion/test_discussion.py

@@ -7,6 +7,7 @@ from pytz import UTC
 from uuid import uuid4
 from nose.plugins.attrib import attr
 
+from .helpers import BaseDiscussionTestCase
 from ..helpers import UniqueCourseTest
 from ...pages.lms.auto_auth import AutoAuthPage
 from ...pages.lms.courseware import CoursewarePage
@@ -139,22 +140,17 @@ class DiscussionHomePageTest(UniqueCourseTest):
 
 
 @attr('shard_1')
-class DiscussionTabSingleThreadTest(UniqueCourseTest, DiscussionResponsePaginationTestMixin):
+class DiscussionTabSingleThreadTest(BaseDiscussionTestCase, DiscussionResponsePaginationTestMixin):
     """
     Tests for the discussion page displaying a single thread
     """
 
     def setUp(self):
         super(DiscussionTabSingleThreadTest, self).setUp()
-        self.discussion_id = "test_discussion_{}".format(uuid4().hex)
-
-        # Create a course to register for
-        CourseFixture(**self.course_info).install()
-
         AutoAuthPage(self.browser, course_id=self.course_id).visit()
 
     def setup_thread_page(self, thread_id):
-        self.thread_page = DiscussionTabSingleThreadPage(self.browser, self.course_id, thread_id)  # pylint: disable=attribute-defined-outside-init
+        self.thread_page = self.create_single_thread_page(thread_id)  # pylint: disable=attribute-defined-outside-init
         self.thread_page.visit()
 
     def test_marked_answer_comments(self):
@@ -180,7 +176,7 @@ class DiscussionTabSingleThreadTest(UniqueCourseTest, DiscussionResponsePaginati
 
 
 @attr('shard_1')
-class DiscussionOpenClosedThreadTest(UniqueCourseTest):
+class DiscussionOpenClosedThreadTest(BaseDiscussionTestCase):
     """
     Tests for checking the display of attributes on open and closed threads
     """
@@ -188,8 +184,6 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest):
     def setUp(self):
         super(DiscussionOpenClosedThreadTest, self).setUp()
 
-        # Create a course to register for
-        CourseFixture(**self.course_info).install()
         self.thread_id = "test_thread_{}".format(uuid4().hex)
 
     def setup_user(self, roles=[]):
@@ -197,6 +191,7 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest):
         self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id()
 
     def setup_view(self, **thread_kwargs):
+        thread_kwargs.update({'commentable_id': self.discussion_id})
         view = SingleThreadViewFixture(
             Thread(id=self.thread_id, **thread_kwargs)
         )
@@ -209,7 +204,7 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest):
             self.setup_view(closed=True)
         else:
             self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, self.thread_id)
+        page = self.create_single_thread_page(self.thread_id)
         page.visit()
         page.close_open_thread()
         return page
@@ -230,23 +225,16 @@ class DiscussionOpenClosedThreadTest(UniqueCourseTest):
 
 
 @attr('shard_1')
-class DiscussionCommentDeletionTest(UniqueCourseTest):
+class DiscussionCommentDeletionTest(BaseDiscussionTestCase):
     """
     Tests for deleting comments displayed beneath responses in the single thread view.
     """
-
-    def setUp(self):
-        super(DiscussionCommentDeletionTest, self).setUp()
-
-        # Create a course to register for
-        CourseFixture(**self.course_info).install()
-
     def setup_user(self, roles=[]):
         roles_str = ','.join(roles)
         self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id()
 
     def setup_view(self):
-        view = SingleThreadViewFixture(Thread(id="comment_deletion_test_thread"))
+        view = SingleThreadViewFixture(Thread(id="comment_deletion_test_thread", commentable_id=self.discussion_id))
         view.addResponse(
             Response(id="response1"),
             [Comment(id="comment_other_author", user_id="other"), Comment(id="comment_self_author", user_id=self.user_id)])
@@ -255,7 +243,7 @@ class DiscussionCommentDeletionTest(UniqueCourseTest):
     def test_comment_deletion_as_student(self):
         self.setup_user()
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_deletion_test_thread")
+        page = self.create_single_thread_page("comment_deletion_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_deletable("comment_self_author"))
         self.assertTrue(page.is_comment_visible("comment_other_author"))
@@ -265,7 +253,7 @@ class DiscussionCommentDeletionTest(UniqueCourseTest):
     def test_comment_deletion_as_moderator(self):
         self.setup_user(roles=['Moderator'])
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_deletion_test_thread")
+        page = self.create_single_thread_page("comment_deletion_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_deletable("comment_self_author"))
         self.assertTrue(page.is_comment_deletable("comment_other_author"))
@@ -274,23 +262,16 @@ class DiscussionCommentDeletionTest(UniqueCourseTest):
 
 
 @attr('shard_1')
-class DiscussionResponseEditTest(UniqueCourseTest):
+class DiscussionResponseEditTest(BaseDiscussionTestCase):
     """
     Tests for editing responses displayed beneath thread in the single thread view.
     """
-
-    def setUp(self):
-        super(DiscussionResponseEditTest, self).setUp()
-
-        # Create a course to register for
-        CourseFixture(**self.course_info).install()
-
     def setup_user(self, roles=[]):
         roles_str = ','.join(roles)
         self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id()
 
     def setup_view(self):
-        view = SingleThreadViewFixture(Thread(id="response_edit_test_thread"))
+        view = SingleThreadViewFixture(Thread(id="response_edit_test_thread", commentable_id=self.discussion_id))
         view.addResponse(
             Response(id="response_other_author", user_id="other", thread_id="response_edit_test_thread"),
         )
@@ -317,7 +298,7 @@ class DiscussionResponseEditTest(UniqueCourseTest):
         """
         self.setup_user()
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread")
+        page = self.create_single_thread_page("response_edit_test_thread")
         page.visit()
         self.assertTrue(page.is_response_visible("response_other_author"))
         self.assertFalse(page.is_response_editable("response_other_author"))
@@ -334,7 +315,7 @@ class DiscussionResponseEditTest(UniqueCourseTest):
         """
         self.setup_user(roles=["Moderator"])
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread")
+        page = self.create_single_thread_page("response_edit_test_thread")
         page.visit()
         self.edit_response(page, "response_self_author")
         self.edit_response(page, "response_other_author")
@@ -362,7 +343,7 @@ class DiscussionResponseEditTest(UniqueCourseTest):
         """
         self.setup_user(roles=["Moderator"])
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "response_edit_test_thread")
+        page = self.create_single_thread_page("response_edit_test_thread")
         page.visit()
         self.edit_response(page, "response_self_author")
         self.edit_response(page, "response_other_author")
@@ -375,23 +356,16 @@ class DiscussionResponseEditTest(UniqueCourseTest):
 
 
 @attr('shard_1')
-class DiscussionCommentEditTest(UniqueCourseTest):
+class DiscussionCommentEditTest(BaseDiscussionTestCase):
     """
     Tests for editing comments displayed beneath responses in the single thread view.
     """
-
-    def setUp(self):
-        super(DiscussionCommentEditTest, self).setUp()
-
-        # Create a course to register for
-        CourseFixture(**self.course_info).install()
-
     def setup_user(self, roles=[]):
         roles_str = ','.join(roles)
         self.user_id = AutoAuthPage(self.browser, course_id=self.course_id, roles=roles_str).visit().get_user_id()
 
     def setup_view(self):
-        view = SingleThreadViewFixture(Thread(id="comment_edit_test_thread"))
+        view = SingleThreadViewFixture(Thread(id="comment_edit_test_thread", commentable_id=self.discussion_id))
         view.addResponse(
             Response(id="response1"),
             [Comment(id="comment_other_author", user_id="other"), Comment(id="comment_self_author", user_id=self.user_id)])
@@ -406,7 +380,7 @@ class DiscussionCommentEditTest(UniqueCourseTest):
     def test_edit_comment_as_student(self):
         self.setup_user()
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread")
+        page = self.create_single_thread_page("comment_edit_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_editable("comment_self_author"))
         self.assertTrue(page.is_comment_visible("comment_other_author"))
@@ -416,7 +390,7 @@ class DiscussionCommentEditTest(UniqueCourseTest):
     def test_edit_comment_as_moderator(self):
         self.setup_user(roles=["Moderator"])
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread")
+        page = self.create_single_thread_page("comment_edit_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_editable("comment_self_author"))
         self.assertTrue(page.is_comment_editable("comment_other_author"))
@@ -426,7 +400,7 @@ class DiscussionCommentEditTest(UniqueCourseTest):
     def test_cancel_comment_edit(self):
         self.setup_user()
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread")
+        page = self.create_single_thread_page("comment_edit_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_editable("comment_self_author"))
         original_body = page.get_comment_body("comment_self_author")
@@ -438,7 +412,7 @@ class DiscussionCommentEditTest(UniqueCourseTest):
         """Only one editor should be visible at a time within a single response"""
         self.setup_user(roles=["Moderator"])
         self.setup_view()
-        page = DiscussionTabSingleThreadPage(self.browser, self.course_id, "comment_edit_test_thread")
+        page = self.create_single_thread_page("comment_edit_test_thread")
         page.visit()
         self.assertTrue(page.is_comment_editable("comment_self_author"))
         self.assertTrue(page.is_comment_editable("comment_other_author"))

lms/djangoapps/course_structure_api/v0/tests.py

@@ -18,7 +18,8 @@ from xmodule.modulestore.xml import CourseLocationManager
 from xmodule.tests import get_test_system
 
 from courseware.tests.factories import GlobalStaffFactory, StaffFactory
-from openedx.core.djangoapps.content.course_structures.models import CourseStructure, update_course_structure
+from openedx.core.djangoapps.content.course_structures.models import CourseStructure
+from openedx.core.djangoapps.content.course_structures.tasks import update_course_structure
 
 
 TEST_SERVER_HOST = 'http://testserver'

lms/djangoapps/course_structure_api/v0/views.py

@@ -14,7 +14,7 @@ from opaque_keys.edx.keys import CourseKey
 from course_structure_api.v0 import serializers
 from courseware import courses
 from courseware.access import has_access
-from openedx.core.djangoapps.content.course_structures import models
+from openedx.core.djangoapps.content.course_structures import models, tasks
 from openedx.core.lib.api.permissions import IsAuthenticatedOrDebug
 from openedx.core.lib.api.serializers import PaginationSerializer
 from student.roles import CourseInstructorRole, CourseStaffRole
@@ -191,7 +191,7 @@ class CourseStructure(CourseViewMixin, RetrieveAPIView):
             return super(CourseStructure, self).retrieve(request, *args, **kwargs)
         except models.CourseStructure.DoesNotExist:
             # If we don't have data stored, generate it and return a 503.
-            models.update_course_structure.delay(unicode(self.course.id))
+            tasks.update_course_structure.delay(unicode(self.course.id))
             return Response(status=503, headers={'Retry-After': '120'})
 
     def get_object(self, queryset=None):

lms/djangoapps/django_comment_client/base/tests.py

@@ -14,7 +14,7 @@ from lms.lib.comment_client import Thread
 from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE
 from django_comment_client.base import views
 from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, GroupIdAssertionMixin
-from django_comment_client.tests.utils import CohortedContentTestCase
+from django_comment_client.tests.utils import CohortedTestCase
 from django_comment_client.tests.unicode import UnicodeTestMixin
 from django_comment_common.models import Role
 from django_comment_common.utils import seed_permissions_roles
@@ -42,7 +42,7 @@ class MockRequestSetupMixin(object):
 @patch('lms.lib.comment_client.utils.requests.request')
 class CreateThreadGroupIdTestCase(
         MockRequestSetupMixin,
-        CohortedContentTestCase,
+        CohortedTestCase,
         CohortedTopicGroupIdTestMixin,
         NonCohortedTopicGroupIdTestMixin
 ):
@@ -77,7 +77,7 @@ class CreateThreadGroupIdTestCase(
 @patch('lms.lib.comment_client.utils.requests.request')
 class ThreadActionGroupIdTestCase(
         MockRequestSetupMixin,
-        CohortedContentTestCase,
+        CohortedTestCase,
         GroupIdAssertionMixin
 ):
     def call_view(

lms/djangoapps/django_comment_client/base/views.py

@@ -74,7 +74,7 @@ def track_forum_event(request, event_name, course, obj, data, id_map=None):
     user = request.user
     data['id'] = obj.id
     if id_map is None:
-        id_map = get_discussion_id_map(course)
+        id_map = get_discussion_id_map(course, user)
 
     commentable_id = data['commentable_id']
     if commentable_id in id_map:
@@ -173,9 +173,9 @@ def create_thread(request, course_id, commentable_id):
 
     # Calls to id map are expensive, but we need this more than once.
     # Prefetch it.
-    id_map = get_discussion_id_map(course)
+    id_map = get_discussion_id_map(course, request.user)
 
-    add_courseware_context([data], course, id_map=id_map)
+    add_courseware_context([data], course, request.user, id_map=id_map)
 
     track_forum_event(request, 'edx.forum.thread.created',
                       course, thread, event_data, id_map=id_map)
@@ -208,7 +208,7 @@ def update_thread(request, course_id, thread_id):
         thread.thread_type = request.POST["thread_type"]
     if "commentable_id" in request.POST:
         course = get_course_with_access(request.user, 'load', course_key)
-        commentable_ids = get_discussion_categories_ids(course)
+        commentable_ids = get_discussion_categories_ids(course, request.user)
         if request.POST.get("commentable_id") in commentable_ids:
             thread.commentable_id = request.POST["commentable_id"]
         else:

lms/djangoapps/django_comment_client/forum/tests.py

@@ -2,6 +2,7 @@ import json
 import logging
 
 import ddt
+from django.core import cache
 from django.core.urlresolvers import reverse
 from django.http import Http404
 from django.test.client import Client, RequestFactory
@@ -14,7 +15,7 @@ from django_comment_client.tests.group_id import (
     NonCohortedTopicGroupIdTestMixin
 )
 from django_comment_client.tests.unicode import UnicodeTestMixin
-from django_comment_client.tests.utils import CohortedContentTestCase
+from django_comment_client.tests.utils import CohortedTestCase, ContentGroupTestCase
 from django_comment_client.utils import strip_none
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
 from util.testing import UrlResetMixin
@@ -104,13 +105,15 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase):
         self.assertEqual(self.response.status_code, 404)
 
 
-def make_mock_thread_data(text, thread_id, num_children, group_id=None, group_name=None, commentable_id=None):
+def make_mock_thread_data(course, text, thread_id, num_children, group_id=None, group_name=None, commentable_id=None):
     thread_data = {
         "id": thread_id,
         "type": "thread",
         "title": text,
         "body": text,
-        "commentable_id": commentable_id or "dummy_commentable_id",
+        "commentable_id": (
+            commentable_id or course.discussion_topics.get('General', {}).get('id') or "dummy_commentable_id"
+        ),
         "resp_total": 42,
         "resp_skip": 25,
         "resp_limit": 5,
@@ -128,21 +131,33 @@ def make_mock_thread_data(text, thread_id, num_children, group_id=None, group_na
 
 
 def make_mock_request_impl(
-    text,
-    thread_id="dummy_thread_id",
-    group_id=None,
-    commentable_id=None,
-    num_thread_responses=1,
+        course,
+        text,
+        thread_id="dummy_thread_id",
+        group_id=None,
+        commentable_id=None,
+        num_thread_responses=1
 ):
     def mock_request_impl(*args, **kwargs):
         url = args[1]
         data = None
         if url.endswith("threads") or url.endswith("user_profile"):
             data = {
-                "collection": [make_mock_thread_data(text, thread_id, None, group_id=group_id, commentable_id=commentable_id)]
+                "collection": [
+                    make_mock_thread_data(
+                        course=course,
+                        text=text,
+                        thread_id=thread_id,
+                        num_children=None,
+                        group_id=group_id,
+                        commentable_id=commentable_id
+                    )
+                ]
             }
         elif thread_id and url.endswith(thread_id):
-            data = make_mock_thread_data(text, thread_id, num_thread_responses, group_id=group_id)
+            data = make_mock_thread_data(
+                course=course, text=text, thread_id=thread_id, num_children=num_thread_responses, group_id=group_id
+            )
         elif "/users/" in url:
             data = {
                 "default_sort_key": "date",
@@ -186,14 +201,14 @@ class SingleThreadTestCase(ModuleStoreTestCase):
     def setUp(self):
         super(SingleThreadTestCase, self).setUp(create_user=False)
 
-        self.course = CourseFactory.create()
+        self.course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}})
         self.student = UserFactory.create()
         CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id)
 
     def test_ajax(self, mock_request):
         text = "dummy content"
         thread_id = "test_thread_id"
-        mock_request.side_effect = make_mock_request_impl(text, thread_id)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id)
 
         request = RequestFactory().get(
             "dummy_url",
@@ -213,7 +228,7 @@ class SingleThreadTestCase(ModuleStoreTestCase):
         # django view performs prior to writing thread data to the response
         self.assertEquals(
             response_data["content"],
-            strip_none(make_mock_thread_data(text, thread_id, 1))
+            strip_none(make_mock_thread_data(course=self.course, text=text, thread_id=thread_id, num_children=1))
         )
         mock_request.assert_called_with(
             "get",
@@ -229,7 +244,7 @@ class SingleThreadTestCase(ModuleStoreTestCase):
         thread_id = "test_thread_id"
         response_skip = "45"
         response_limit = "15"
-        mock_request.side_effect = make_mock_request_impl(text, thread_id)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id)
 
         request = RequestFactory().get(
             "dummy_url",
@@ -249,7 +264,7 @@ class SingleThreadTestCase(ModuleStoreTestCase):
         # django view performs prior to writing thread data to the response
         self.assertEquals(
             response_data["content"],
-            strip_none(make_mock_thread_data(text, thread_id, 1))
+            strip_none(make_mock_thread_data(course=self.course, text=text, thread_id=thread_id, num_children=1))
         )
         mock_request.assert_called_with(
             "get",
@@ -280,7 +295,7 @@ class SingleThreadTestCase(ModuleStoreTestCase):
         request = RequestFactory().get("dummy_url")
         request.user = self.student
         # Mock request to return 404 for thread request
-        mock_request.side_effect = make_mock_request_impl("dummy", thread_id=None)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy", thread_id=None)
         self.assertRaises(
             Http404,
             views.single_thread,
@@ -301,34 +316,42 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
     MODULESTORE = TEST_DATA_MONGO_MODULESTORE
 
     @ddt.data(
-        # old mongo: number of responses plus 16.  TODO: O(n)!
-        (ModuleStoreEnum.Type.mongo, 1, 17),
-        (ModuleStoreEnum.Type.mongo, 50, 66),
+        # old mongo with cache: number of responses plus 17.  TODO: O(n)!
+        (ModuleStoreEnum.Type.mongo, 1, 23, 18),
+        (ModuleStoreEnum.Type.mongo, 50, 366, 67),
         # split mongo: 3 queries, regardless of thread response size.
-        (ModuleStoreEnum.Type.split, 1, 3),
-        (ModuleStoreEnum.Type.split, 50, 3),
+        (ModuleStoreEnum.Type.split, 1, 3, 3),
+        (ModuleStoreEnum.Type.split, 50, 3, 3),
     )
     @ddt.unpack
-    def test_number_of_mongo_queries(self, default_store, num_thread_responses, num_mongo_calls, mock_request):
-
+    def test_number_of_mongo_queries(
+            self,
+            default_store,
+            num_thread_responses,
+            num_uncached_mongo_calls,
+            num_cached_mongo_calls,
+            mock_request
+    ):
         with modulestore().default_store(default_store):
-            course = CourseFactory.create()
+            course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}})
 
         student = UserFactory.create()
         CourseEnrollmentFactory.create(user=student, course_id=course.id)
 
         test_thread_id = "test_thread_id"
         mock_request.side_effect = make_mock_request_impl(
-            "dummy content",
-            test_thread_id,
-            num_thread_responses=num_thread_responses,
+            course=course, text="dummy content", thread_id=test_thread_id, num_thread_responses=num_thread_responses
         )
         request = RequestFactory().get(
             "dummy_url",
             HTTP_X_REQUESTED_WITH="XMLHttpRequest"
         )
         request.user = student
-        with check_mongo_calls(num_mongo_calls):
+
+        def call_single_thread():
+            """
+            Call single_thread and assert that it returns what we expect.
+            """
             response = views.single_thread(
                 request,
                 course.id.to_deprecated_string(),
@@ -338,15 +361,35 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase):
             self.assertEquals(response.status_code, 200)
             self.assertEquals(len(json.loads(response.content)["content"]["children"]), num_thread_responses)
 
+        # TODO: update this once django cache is disabled in tests
+        # Test with and without cache, clearing before and after use.
+        single_thread_local_cache = cache.get_cache(
+            backend='default',
+            LOCATION='single_thread_local_cache'
+        )
+        single_thread_dummy_cache = cache.get_cache(
+            backend='django.core.cache.backends.dummy.DummyCache',
+            LOCATION='single_thread_local_cache'
+        )
+        cached_calls = {
+            single_thread_dummy_cache: num_uncached_mongo_calls,
+            single_thread_local_cache: num_cached_mongo_calls
+        }
+        for single_thread_cache, expected_calls in cached_calls.items():
+            single_thread_cache.clear()
+            with patch("django_comment_client.permissions.CACHE", single_thread_cache):
+                with check_mongo_calls(expected_calls):
+                    call_single_thread()
+            single_thread_cache.clear()
+
 
 @patch('requests.request')
-class SingleCohortedThreadTestCase(CohortedContentTestCase):
+class SingleCohortedThreadTestCase(CohortedTestCase):
     def _create_mock_cohorted_thread(self, mock_request):
         self.mock_text = "dummy content"
         self.mock_thread_id = "test_thread_id"
         mock_request.side_effect = make_mock_request_impl(
-            self.mock_text, self.mock_thread_id,
-            group_id=self.student_cohort.id
+            course=self.course, text=self.mock_text, thread_id=self.mock_thread_id, group_id=self.student_cohort.id
         )
 
     def test_ajax(self, mock_request):
@@ -360,7 +403,7 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase):
         response = views.single_thread(
             request,
             self.course.id.to_deprecated_string(),
-            "dummy_discussion_id",
+            "cohorted_topic",
             self.mock_thread_id
         )
 
@@ -369,9 +412,12 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase):
         self.assertEquals(
             response_data["content"],
             make_mock_thread_data(
-                self.mock_text, self.mock_thread_id, 1,
+                course=self.course,
+                text=self.mock_text,
+                thread_id=self.mock_thread_id,
+                num_children=1,
                 group_id=self.student_cohort.id,
-                group_name=self.student_cohort.name,
+                group_name=self.student_cohort.name
             )
         )
 
@@ -384,7 +430,7 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase):
         response = views.single_thread(
             request,
             self.course.id.to_deprecated_string(),
-            "dummy_discussion_id",
+            "cohorted_topic",
             self.mock_thread_id
         )
 
@@ -397,10 +443,12 @@ class SingleCohortedThreadTestCase(CohortedContentTestCase):
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
-class SingleThreadAccessTestCase(CohortedContentTestCase):
+class SingleThreadAccessTestCase(CohortedTestCase):
     def call_view(self, mock_request, commentable_id, user, group_id, thread_group_id=None, pass_group_id=True):
         thread_id = "test_thread_id"
-        mock_request.side_effect = make_mock_request_impl("dummy context", thread_id, group_id=thread_group_id)
+        mock_request.side_effect = make_mock_request_impl(
+            course=self.course, text="dummy context", thread_id=thread_id, group_id=thread_group_id
+        )
 
         request_data = {}
         if pass_group_id:
@@ -482,11 +530,13 @@ class SingleThreadAccessTestCase(CohortedContentTestCase):
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
-class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+class SingleThreadGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin):
     cs_endpoint = "/threads"
 
     def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False):
-        mock_request.side_effect = make_mock_request_impl("dummy context", group_id=self.student_cohort.id)
+        mock_request.side_effect = make_mock_request_impl(
+            course=self.course, text="dummy context", group_id=self.student_cohort.id
+        )
 
         request_data = {}
         if pass_group_id:
@@ -504,7 +554,7 @@ class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdT
         return views.single_thread(
             request,
             self.course.id.to_deprecated_string(),
-            "dummy_discussion_id",
+            commentable_id,
             "dummy_thread_id"
         )
 
@@ -531,16 +581,98 @@ class SingleThreadGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdT
         )
 
 
+@patch('requests.request')
+class SingleThreadContentGroupTestCase(ContentGroupTestCase):
+    def assert_can_access(self, user, discussion_id, thread_id, should_have_access):
+        """
+        Verify that a user has access to a thread within a given
+        discussion_id when should_have_access is True, otherwise
+        verify that the user does not have access to that thread.
+        """
+        request = RequestFactory().get("dummy_url")
+        request.user = user
+        mako_middleware_process_request(request)
+
+        def call_single_thread():
+            return views.single_thread(
+                request,
+                unicode(self.course.id),
+                discussion_id,
+                thread_id
+            )
+
+        if should_have_access:
+            self.assertEqual(call_single_thread().status_code, 200)
+        else:
+            with self.assertRaises(Http404):
+                call_single_thread()
+
+    def test_staff_user(self, mock_request):
+        """
+        Verify that the staff user can access threads in the alpha,
+        beta, and global discussion modules.
+        """
+        thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id)
+
+        for discussion_module in [self.alpha_module, self.beta_module, self.global_module]:
+            self.assert_can_access(self.staff_user, discussion_module.discussion_id, thread_id, True)
+
+    def test_alpha_user(self, mock_request):
+        """
+        Verify that the alpha user can access threads in the alpha and
+        global discussion modules.
+        """
+        thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id)
+
+        for discussion_module in [self.alpha_module, self.global_module]:
+            self.assert_can_access(self.alpha_user, discussion_module.discussion_id, thread_id, True)
+
+        self.assert_can_access(self.alpha_user, self.beta_module.discussion_id, thread_id, False)
+
+    def test_beta_user(self, mock_request):
+        """
+        Verify that the beta user can access threads in the beta and
+        global discussion modules.
+        """
+        thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id)
+
+        for discussion_module in [self.beta_module, self.global_module]:
+            self.assert_can_access(self.beta_user, discussion_module.discussion_id, thread_id, True)
+
+        self.assert_can_access(self.beta_user, self.alpha_module.discussion_id, thread_id, False)
+
+    def test_non_cohorted_user(self, mock_request):
+        """
+        Verify that the non-cohorted user can access threads in just the
+        global discussion module.
+        """
+        thread_id = "test_thread_id"
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy content", thread_id=thread_id)
+
+        self.assert_can_access(self.non_cohorted_user, self.global_module.discussion_id, thread_id, True)
+
+        self.assert_can_access(self.non_cohorted_user, self.alpha_module.discussion_id, thread_id, False)
+
+        self.assert_can_access(self.non_cohorted_user, self.beta_module.discussion_id, thread_id, False)
+
+
 @patch('lms.lib.comment_client.utils.requests.request')
 class InlineDiscussionGroupIdTestCase(
-        CohortedContentTestCase,
+        CohortedTestCase,
         CohortedTopicGroupIdTestMixin,
         NonCohortedTopicGroupIdTestMixin
 ):
     cs_endpoint = "/threads"
 
+    def setUp(self):
+        super(InlineDiscussionGroupIdTestCase, self).setUp()
+        self.cohorted_commentable_id = 'cohorted_topic'
+
     def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
-        kwargs = {}
+        kwargs = {'commentable_id': self.cohorted_commentable_id}
         if group_id:
             # avoid causing a server error when the LMS chokes attempting
             # to find a group name for the group_id, when we're testing with
@@ -550,7 +682,7 @@ class InlineDiscussionGroupIdTestCase(
                 kwargs['group_id'] = group_id
             except CourseUserGroup.DoesNotExist:
                 pass
-        mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs)
+        mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs)
 
         request_data = {}
         if pass_group_id:
@@ -569,7 +701,7 @@ class InlineDiscussionGroupIdTestCase(
     def test_group_info_in_ajax_response(self, mock_request):
         response = self.call_view(
             mock_request,
-            "cohorted_topic",
+            self.cohorted_commentable_id,
             self.student,
             self.student_cohort.id
         )
@@ -579,14 +711,14 @@ class InlineDiscussionGroupIdTestCase(
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
-class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+class ForumFormDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin):
     cs_endpoint = "/threads"
 
     def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True, is_ajax=False):
         kwargs = {}
         if group_id:
             kwargs['group_id'] = group_id
-        mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs)
+        mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs)
 
         request_data = {}
         if pass_group_id:
@@ -629,7 +761,7 @@ class ForumFormDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicG
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
-class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+class UserProfileDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin):
     cs_endpoint = "/active_threads"
 
     def call_view_for_profiled_user(
@@ -642,7 +774,7 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi
         kwargs = {}
         if group_id:
             kwargs['group_id'] = group_id
-        mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs)
+        mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs)
 
         request_data = {}
         if pass_group_id:
@@ -795,14 +927,14 @@ class UserProfileDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopi
 
 
 @patch('lms.lib.comment_client.utils.requests.request')
-class FollowedThreadsDiscussionGroupIdTestCase(CohortedContentTestCase, CohortedTopicGroupIdTestMixin):
+class FollowedThreadsDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGroupIdTestMixin):
     cs_endpoint = "/subscribed_threads"
 
     def call_view(self, mock_request, commentable_id, user, group_id, pass_group_id=True):
         kwargs = {}
         if group_id:
             kwargs['group_id'] = group_id
-        mock_request.side_effect = make_mock_request_impl("dummy content", **kwargs)
+        mock_request.side_effect = make_mock_request_impl(self.course, "dummy content", **kwargs)
 
         request_data = {}
         if pass_group_id:
@@ -851,9 +983,13 @@ class InlineDiscussionTestCase(ModuleStoreTestCase):
     def test_courseware_data(self, mock_request):
         request = RequestFactory().get("dummy_url")
         request.user = self.student
-        mock_request.side_effect = make_mock_request_impl("dummy content", commentable_id=self.discussion1.discussion_id)
+        mock_request.side_effect = make_mock_request_impl(
+            course=self.course, text="dummy content", commentable_id=self.discussion1.discussion_id
+        )
 
-        response = views.inline_discussion(request, self.course.id.to_deprecated_string(), "dummy_discussion_id")
+        response = views.inline_discussion(
+            request, self.course.id.to_deprecated_string(), self.discussion1.discussion_id
+        )
         self.assertEqual(response.status_code, 200)
         response_data = json.loads(response.content)
         expected_courseware_url = '/courses/TestX/101/Test_Course/jump_to/i4x://TestX/101/discussion/Discussion1'
@@ -877,7 +1013,9 @@ class UserProfileTestCase(ModuleStoreTestCase):
         CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id)
 
     def get_response(self, mock_request, params, **headers):
-        mock_request.side_effect = make_mock_request_impl(self.TEST_THREAD_TEXT, self.TEST_THREAD_ID)
+        mock_request.side_effect = make_mock_request_impl(
+            course=self.course, text=self.TEST_THREAD_TEXT, thread_id=self.TEST_THREAD_ID
+        )
         request = RequestFactory().get("dummy_url", data=params, **headers)
         request.user = self.student
 
@@ -964,7 +1102,9 @@ class UserProfileTestCase(ModuleStoreTestCase):
             )
 
     def test_post(self, mock_request):
-        mock_request.side_effect = make_mock_request_impl(self.TEST_THREAD_TEXT, self.TEST_THREAD_ID)
+        mock_request.side_effect = make_mock_request_impl(
+            course=self.course, text=self.TEST_THREAD_TEXT, thread_id=self.TEST_THREAD_ID
+        )
         request = RequestFactory().post("dummy_url")
         request.user = self.student
         response = views.user_profile(
@@ -986,7 +1126,7 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase):
 
         # Invoke UrlResetMixin
         super(CommentsServiceRequestHeadersTestCase, self).setUp(create_user=False)
-        self.course = CourseFactory.create()
+        self.course = CourseFactory.create(discussion_topics={'dummy discussion': {'id': 'dummy_discussion_id'}})
         self.student = UserFactory.create(username=username, password=password)
         CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id)
         self.assertTrue(
@@ -1009,14 +1149,14 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase):
         lang = "eo"
         text = "dummy content"
         thread_id = "test_thread_id"
-        mock_request.side_effect = make_mock_request_impl(text, thread_id)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id)
 
         self.client.get(
             reverse(
                 "django_comment_client.forum.views.single_thread",
                 kwargs={
                     "course_id": self.course.id.to_deprecated_string(),
-                    "discussion_id": "dummy",
+                    "discussion_id": "dummy_discussion_id",
                     "thread_id": thread_id,
                 }
             ),
@@ -1026,7 +1166,7 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase):
 
     @override_settings(COMMENTS_SERVICE_KEY="test_api_key")
     def test_api_key(self, mock_request):
-        mock_request.side_effect = make_mock_request_impl("dummy", "dummy")
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text="dummy", thread_id="dummy")
 
         self.client.get(
             reverse(
@@ -1047,11 +1187,13 @@ class InlineDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
-        mock_request.side_effect = make_mock_request_impl(text)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text)
         request = RequestFactory().get("dummy_url")
         request.user = self.student
 
-        response = views.inline_discussion(request, self.course.id.to_deprecated_string(), "dummy_discussion_id")
+        response = views.inline_discussion(
+            request, self.course.id.to_deprecated_string(), self.course.discussion_topics['General']['id']
+        )
         self.assertEqual(response.status_code, 200)
         response_data = json.loads(response.content)
         self.assertEqual(response_data["discussion_data"][0]["title"], text)
@@ -1068,7 +1210,7 @@ class ForumFormDiscussionUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
-        mock_request.side_effect = make_mock_request_impl(text)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text)
         request = RequestFactory().get("dummy_url")
         request.user = self.student
         request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest"  # so request.is_ajax() == True
@@ -1090,7 +1232,7 @@ class ForumDiscussionSearchUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
-        mock_request.side_effect = make_mock_request_impl(text)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text)
         data = {
             "ajax": 1,
             "text": text,
@@ -1110,14 +1252,14 @@ class SingleThreadUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
     def setUp(self):
         super(SingleThreadUnicodeTestCase, self).setUp()
 
-        self.course = CourseFactory.create()
+        self.course = CourseFactory.create(discussion_topics={'dummy_discussion_id': {'id': 'dummy_discussion_id'}})
         self.student = UserFactory.create()
         CourseEnrollmentFactory(user=self.student, course_id=self.course.id)
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
         thread_id = "test_thread_id"
-        mock_request.side_effect = make_mock_request_impl(text, thread_id)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text, thread_id=thread_id)
         request = RequestFactory().get("dummy_url")
         request.user = self.student
         request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest"  # so request.is_ajax() == True
@@ -1139,7 +1281,7 @@ class UserProfileUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
-        mock_request.side_effect = make_mock_request_impl(text)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text)
         request = RequestFactory().get("dummy_url")
         request.user = self.student
         request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest"  # so request.is_ajax() == True
@@ -1161,7 +1303,7 @@ class FollowedThreadsUnicodeTestCase(ModuleStoreTestCase, UnicodeTestMixin):
 
     @patch('lms.lib.comment_client.utils.requests.request')
     def _test_unicode_data(self, text, mock_request):
-        mock_request.side_effect = make_mock_request_impl(text)
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text=text)
         request = RequestFactory().get("dummy_url")
         request.user = self.student
         request.META["HTTP_X_REQUESTED_WITH"] = "XMLHttpRequest"  # so request.is_ajax() == True
@@ -1188,7 +1330,7 @@ class EnrollmentTestCase(ModuleStoreTestCase):
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
     @patch('lms.lib.comment_client.utils.requests.request')
     def test_unenrolled(self, mock_request):
-        mock_request.side_effect = make_mock_request_impl('dummy')
+        mock_request.side_effect = make_mock_request_impl(course=self.course, text='dummy')
         request = RequestFactory().get('dummy_url')
         request.user = self.student
         with self.assertRaises(UserNotEnrolled):

lms/djangoapps/django_comment_client/forum/views.py

@@ -52,7 +52,7 @@ def _attr_safe_json(obj):
 
 
 @newrelic.agent.function_trace()
-def make_course_settings(course):
+def make_course_settings(course, user):
     """
     Generate a JSON-serializable model for course settings, which will be used to initialize a
     DiscussionCourseSettings object on the client.
@@ -63,14 +63,14 @@ def make_course_settings(course):
         'allow_anonymous': course.allow_anonymous,
         'allow_anonymous_to_peers': course.allow_anonymous_to_peers,
         'cohorts': [{"id": str(g.id), "name": g.name} for g in get_course_cohorts(course)],
-        'category_map': utils.get_discussion_category_map(course)
+        'category_map': utils.get_discussion_category_map(course, user)
     }
 
     return obj
 
 
 @newrelic.agent.function_trace()
-def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PAGE):
+def get_threads(request, course, discussion_id=None, per_page=THREADS_PER_PAGE):
     """
     This may raise an appropriate subclass of cc.utils.CommentClientError
     if something goes wrong, or ValueError if the group_id is invalid.
@@ -81,12 +81,16 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA
         'sort_key': 'date',
         'sort_order': 'desc',
         'text': '',
-        'commentable_id': discussion_id,
-        'course_id': course_key.to_deprecated_string(),
+        'course_id': unicode(course.id),
         'user_id': request.user.id,
-        'group_id': get_group_id_for_comments_service(request, course_key, discussion_id),  # may raise ValueError
+        'group_id': get_group_id_for_comments_service(request, course.id, discussion_id),  # may raise ValueError
     }
 
+    # If provided with a discussion id, filter by discussion id in the
+    # comments_service.
+    if discussion_id is not None:
+        default_query_params['commentable_id'] = discussion_id
+
     if not request.GET.get('sort_key'):
         # If the user did not select a sort key, use their last used sort key
         cc_user = cc.User.from_django_user(request.user)
@@ -124,6 +128,15 @@ def get_threads(request, course_key, discussion_id=None, per_page=THREADS_PER_PA
 
     threads, page, num_pages, corrected_text = cc.Thread.search(query_params)
 
+    # If not provided with a discussion id, filter threads by commentable ids
+    # which are accessible to the current user.
+    if discussion_id is None:
+        discussion_category_ids = set(utils.get_discussion_categories_ids(course, request.user))
+        threads = [
+            thread for thread in threads
+            if thread.get('commentable_id') in discussion_category_ids
+        ]
+
     for thread in threads:
         # patch for backward compatibility to comments service
         if 'pinned' not in thread:
@@ -163,7 +176,7 @@ def inline_discussion(request, course_key, discussion_id):
     user_info = cc_user.to_dict()
 
     try:
-        threads, query_params = get_threads(request, course_key, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
+        threads, query_params = get_threads(request, course, discussion_id, per_page=INLINE_THREADS_PER_PAGE)
     except ValueError:
         return HttpResponseBadRequest("Invalid group_id")
 
@@ -172,7 +185,7 @@ def inline_discussion(request, course_key, discussion_id):
     is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
     threads = [utils.prepare_content(thread, course_key, is_staff) for thread in threads]
     with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-        add_courseware_context(threads, course)
+        add_courseware_context(threads, course, request.user)
     return utils.JsonResponse({
         'is_commentable_cohorted': is_commentable_cohorted(course_key, discussion_id),
         'discussion_data': threads,
@@ -181,7 +194,7 @@ def inline_discussion(request, course_key, discussion_id):
         'page': query_params['page'],
         'num_pages': query_params['num_pages'],
         'roles': utils.get_role_ids(course_key),
-        'course_settings': make_course_settings(course)
+        'course_settings': make_course_settings(course, request.user)
     })
 
 
@@ -194,13 +207,13 @@ def forum_form_discussion(request, course_key):
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key, check_if_enrolled=True)
-    course_settings = make_course_settings(course)
+    course_settings = make_course_settings(course, request.user)
 
     user = cc.User.from_django_user(request.user)
     user_info = user.to_dict()
 
     try:
-        unsafethreads, query_params = get_threads(request, course_key)   # This might process a search query
+        unsafethreads, query_params = get_threads(request, course)   # This might process a search query
         is_staff = cached_has_permission(request.user, 'openclose_thread', course.id)
         threads = [utils.prepare_content(thread, course_key, is_staff) for thread in unsafethreads]
     except cc.utils.CommentClientMaintenanceError:
@@ -213,7 +226,7 @@ def forum_form_discussion(request, course_key):
         annotated_content_info = utils.get_metadata_for_threads(course_key, threads, request.user, user_info)
 
     with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-        add_courseware_context(threads, course)
+        add_courseware_context(threads, course, request.user)
 
     if request.is_ajax():
         return utils.JsonResponse({
@@ -258,15 +271,18 @@ def single_thread(request, course_key, discussion_id, thread_id):
     """
     Renders a response to display a single discussion thread.
     """
-
     nr_transaction = newrelic.agent.current_transaction()
 
     course = get_course_with_access(request.user, 'load_forum', course_key)
-    course_settings = make_course_settings(course)
+    course_settings = make_course_settings(course, request.user)
     cc_user = cc.User.from_django_user(request.user)
     user_info = cc_user.to_dict()
     is_moderator = cached_has_permission(request.user, "see_all_cohorts", course_key)
 
+    # Verify that the student has access to this thread if belongs to a discussion module
+    if discussion_id not in utils.get_discussion_categories_ids(course, request.user):
+        raise Http404
+
     # Currently, the front end always loads responses via AJAX, even for this
     # page; it would be a nice optimization to avoid that extra round trip to
     # the comments service.
@@ -294,7 +310,7 @@ def single_thread(request, course_key, discussion_id, thread_id):
             annotated_content_info = utils.get_annotated_content_infos(course_key, thread, request.user, user_info=user_info)
         content = utils.prepare_content(thread.to_dict(), course_key, is_staff)
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-            add_courseware_context([content], course)
+            add_courseware_context([content], course, request.user)
         return utils.JsonResponse({
             'content': content,
             'annotated_content_info': annotated_content_info,
@@ -302,13 +318,13 @@ def single_thread(request, course_key, discussion_id, thread_id):
 
     else:
         try:
-            threads, query_params = get_threads(request, course_key)
+            threads, query_params = get_threads(request, course)
         except ValueError:
             return HttpResponseBadRequest("Invalid group_id")
         threads.append(thread.to_dict())
 
         with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
-            add_courseware_context(threads, course)
+            add_courseware_context(threads, course, request.user)
 
         for thread in threads:
             # patch for backward compatibility with comments service

lms/djangoapps/django_comment_client/tests/test_utils.py

@@ -10,6 +10,7 @@ import mock
 
 from django_comment_client.tests.factories import RoleFactory
 from django_comment_client.tests.unicode import UnicodeTestMixin
+from django_comment_client.tests.utils import ContentGroupTestCase
 import django_comment_client.utils as utils
 from student.tests.factories import UserFactory, CourseEnrollmentFactory
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
@@ -88,7 +89,7 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
     comment client service integration
     """
     def setUp(self):
-        super(CoursewareContextTestCase, self).setUp()
+        super(CoursewareContextTestCase, self).setUp(create_user=True)
 
         self.course = CourseFactory.create(org="TestX", number="101", display_name="Test Course")
         self.discussion1 = ItemFactory.create(
@@ -107,12 +108,12 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
         )
 
     def test_empty(self):
-        utils.add_courseware_context([], self.course)
+        utils.add_courseware_context([], self.course, self.user)
 
     def test_missing_commentable_id(self):
         orig = {"commentable_id": "non-inline"}
         modified = dict(orig)
-        utils.add_courseware_context([modified], self.course)
+        utils.add_courseware_context([modified], self.course, self.user)
         self.assertEqual(modified, orig)
 
     def test_basic(self):
@@ -120,7 +121,7 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
             {"commentable_id": self.discussion1.discussion_id},
             {"commentable_id": self.discussion2.discussion_id}
         ]
-        utils.add_courseware_context(threads, self.course)
+        utils.add_courseware_context(threads, self.course, self.user)
 
         def assertThreadCorrect(thread, discussion, expected_title):  # pylint: disable=invalid-name
             """Asserts that the given thread has the expected set of properties"""
@@ -144,13 +145,29 @@ class CoursewareContextTestCase(ModuleStoreTestCase):
         assertThreadCorrect(threads[1], self.discussion2, "Subsection / Discussion 2")
 
 
-class CategoryMapTestCase(ModuleStoreTestCase):
+class CategoryMapTestMixin(object):
+    """
+    Provides functionality for classes that test
+    `get_discussion_category_map`.
+    """
+    def assert_category_map_equals(self, expected, requesting_user=None):
+        """
+        Call `get_discussion_category_map`, and verify that it returns
+        what is expected.
+        """
+        self.assertEqual(
+            utils.get_discussion_category_map(self.course, requesting_user or self.user),
+            expected
+        )
+
+
+class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase):
     """
     Base testcase class for discussion categories for the
     comment client service integration
     """
     def setUp(self):
-        super(CategoryMapTestCase, self).setUp()
+        super(CategoryMapTestCase, self).setUp(create_user=True)
 
         self.course = CourseFactory.create(
             org="TestX", number="101", display_name="Test Course",
@@ -176,20 +193,8 @@ class CategoryMapTestCase(ModuleStoreTestCase):
             **kwargs
         )
 
-    def assertCategoryMapEquals(self, expected):
-        """
-        Compare a manually-constructed category map to the actual result from `utils.get_discussion_category_map`
-        """
-        self.assertEqual(
-            utils.get_discussion_category_map(self.course),
-            expected
-        )
-
     def test_empty(self):
-        self.assertEqual(
-            utils.get_discussion_category_map(self.course),
-            {"entries": {}, "subcategories": {}, "children": []}
-        )
+        self.assert_category_map_equals({"entries": {}, "subcategories": {}, "children": []})
 
     def test_configured_topics(self):
         self.course.discussion_topics = {
@@ -199,7 +204,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         }
 
         def check_cohorted_topics(expected_ids):  # pylint: disable=missing-docstring
-            self.assertCategoryMapEquals(
+            self.assert_category_map_equals(
                 {
                     "entries": {
                         "Topic A": {"id": "Topic_A", "sort_key": "Topic A", "is_cohorted": "Topic_A" in expected_ids},
@@ -231,7 +236,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
 
     def test_single_inline(self):
         self.create_discussion("Chapter", "Discussion")
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {},
                 "subcategories": {
@@ -261,7 +266,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
 
         def check_cohorted(is_cohorted):
 
-            self.assertCategoryMapEquals(
+            self.assert_category_map_equals(
                 {
                     "entries": {},
                     "subcategories": {
@@ -362,7 +367,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion")
         self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion")  # duplicate
 
-        category_map = utils.get_discussion_category_map(self.course)
+        category_map = utils.get_discussion_category_map(self.course, self.user)
 
         chapter1 = category_map["subcategories"]["Chapter 1"]
         chapter1_discussions = set(["Discussion A", "Discussion B", "Discussion A (1)", "Discussion A (2)"])
@@ -385,7 +390,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion", start=later)
         self.create_discussion("Chapter 3 / Section 1", "Discussion", start=later)
 
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {},
                 "subcategories": {
@@ -423,7 +428,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter", "Discussion 4", sort_key="C")
         self.create_discussion("Chapter", "Discussion 5", sort_key="B")
 
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {},
                 "subcategories": {
@@ -475,7 +480,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
             "Topic B": {"id": "Topic_B", "sort_key": "C"},
             "Topic C": {"id": "Topic_C", "sort_key": "A"}
         }
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {
                     "Topic A": {"id": "Topic_A", "sort_key": "B", "is_cohorted": False},
@@ -496,7 +501,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter", "Discussion C")
         self.create_discussion("Chapter", "Discussion B")
 
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {},
                 "subcategories": {
@@ -549,7 +554,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter B", "Discussion 1")
         self.create_discussion("Chapter A", "Discussion 2")
 
-        self.assertCategoryMapEquals(
+        self.assert_category_map_equals(
             {
                 "entries": {},
                 "subcategories": {
@@ -602,7 +607,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         )
 
     def test_ids_empty(self):
-        self.assertEqual(utils.get_discussion_categories_ids(self.course), [])
+        self.assertEqual(utils.get_discussion_categories_ids(self.course, self.user), [])
 
     def test_ids_configured_topics(self):
         self.course.discussion_topics = {
@@ -611,7 +616,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
             "Topic C": {"id": "Topic_C"}
         }
         self.assertItemsEqual(
-            utils.get_discussion_categories_ids(self.course),
+            utils.get_discussion_categories_ids(self.course, self.user),
             ["Topic_A", "Topic_B", "Topic_C"]
         )
 
@@ -623,7 +628,7 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion")
         self.create_discussion("Chapter 3 / Section 1", "Discussion")
         self.assertItemsEqual(
-            utils.get_discussion_categories_ids(self.course),
+            utils.get_discussion_categories_ids(self.course, self.user),
             ["discussion1", "discussion2", "discussion3", "discussion4", "discussion5", "discussion6"]
         )
 
@@ -637,11 +642,177 @@ class CategoryMapTestCase(ModuleStoreTestCase):
         self.create_discussion("Chapter 2", "Discussion")
         self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion")
         self.assertItemsEqual(
-            utils.get_discussion_categories_ids(self.course),
+            utils.get_discussion_categories_ids(self.course, self.user),
             ["Topic_A", "Topic_B", "Topic_C", "discussion1", "discussion2", "discussion3"]
         )
 
 
+class ContentGroupCategoryMapTestCase(CategoryMapTestMixin, ContentGroupTestCase):
+    """
+    Tests `get_discussion_category_map` on discussion modules which are
+    only visible to some content groups.
+    """
+    def test_staff_user(self):
+        """
+        Verify that the staff user can access the alpha, beta, and
+        global discussion topics.
+        """
+        self.assert_category_map_equals(
+            {
+                'subcategories': {
+                    'Week 1': {
+                        'subcategories': {},
+                        'children': [
+                            'Visible to Alpha',
+                            'Visible to Beta',
+                            'Visible to Everyone'
+                        ],
+                        'entries': {
+                            'Visible to Alpha': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'alpha_group_discussion'
+                            },
+                            'Visible to Beta': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'beta_group_discussion'
+                            },
+                            'Visible to Everyone': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'global_group_discussion'
+                            }
+                        }
+                    }
+                },
+                'children': ['General', 'Week 1'],
+                'entries': {
+                    'General': {
+                        'sort_key': 'General',
+                        'is_cohorted': False,
+                        'id': 'i4x-org-number-course-run'
+                    }
+                }
+            },
+            requesting_user=self.staff_user
+        )
+
+    def test_alpha_user(self):
+        """
+        Verify that the alpha user can access the alpha and global
+        discussion topics.
+        """
+        self.assert_category_map_equals(
+            {
+                'subcategories': {
+                    'Week 1': {
+                        'subcategories': {},
+                        'children': [
+                            'Visible to Alpha',
+                            'Visible to Everyone'
+                        ],
+                        'entries': {
+                            'Visible to Alpha': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'alpha_group_discussion'
+                            },
+                            'Visible to Everyone': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'global_group_discussion'
+                            }
+                        }
+                    }
+                },
+                'children': ['General', 'Week 1'],
+                'entries': {
+                    'General': {
+                        'sort_key': 'General',
+                        'is_cohorted': False,
+                        'id': 'i4x-org-number-course-run'
+                    }
+                }
+            },
+            requesting_user=self.alpha_user
+        )
+
+    def test_beta_user(self):
+        """
+        Verify that the beta user can access the beta and global
+        discussion topics.
+        """
+        self.assert_category_map_equals(
+            {
+                'subcategories': {
+                    'Week 1': {
+                        'subcategories': {},
+                        'children': [
+                            'Visible to Beta',
+                            'Visible to Everyone'
+                        ],
+                        'entries': {
+                            'Visible to Beta': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'beta_group_discussion'
+                            },
+                            'Visible to Everyone': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'global_group_discussion'
+                            }
+                        }
+                    }
+                },
+                'children': ['General', 'Week 1'],
+                'entries': {
+                    'General': {
+                        'sort_key': 'General',
+                        'is_cohorted': False,
+                        'id': 'i4x-org-number-course-run'
+                    }
+                }
+            },
+            requesting_user=self.beta_user
+        )
+
+    def test_non_cohorted_user(self):
+        """
+        Verify that the non-cohorted user can access the global
+        discussion topic.
+        """
+        self.assert_category_map_equals(
+            {
+                'subcategories': {
+                    'Week 1': {
+                        'subcategories': {},
+                        'children': [
+                            'Visible to Everyone'
+                        ],
+                        'entries': {
+                            'Visible to Everyone': {
+                                'sort_key': None,
+                                'is_cohorted': True,
+                                'id': 'global_group_discussion'
+                            }
+                        }
+                    }
+                },
+                'children': ['General', 'Week 1'],
+                'entries': {
+                    'General': {
+                        'sort_key': 'General',
+                        'is_cohorted': False,
+                        'id': 'i4x-org-number-course-run'
+                    }
+                }
+            },
+            requesting_user=self.non_cohorted_user
+        )
+
+
 class JsonResponseTestCase(TestCase, UnicodeTestMixin):
     def _test_unicode_data(self, text):
         response = utils.JsonResponse(text)

lms/djangoapps/django_comment_client/tests/utils.py

+"""
+Utilities for tests within the django_comment_client module.
+"""
+from datetime import datetime
 from mock import patch
+from pytz import UTC
 
-from openedx.core.djangoapps.course_groups.models import CourseUserGroup
+from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup
+from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory
 from django_comment_common.models import Role
 from django_comment_common.utils import seed_permissions_roles
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
-from xmodule.modulestore.tests.factories import CourseFactory
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.partitions.partitions import UserPartition, Group
 
 
-class CohortedContentTestCase(ModuleStoreTestCase):
+class CohortedTestCase(ModuleStoreTestCase):
     """
     Sets up a course with a student, a moderator and their cohorts.
     """
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True})
     def setUp(self):
-        super(CohortedContentTestCase, self).setUp()
+        super(CohortedTestCase, self).setUp()
 
         self.course = CourseFactory.create(
-            discussion_topics={
-                "cohorted topic": {"id": "cohorted_topic"},
-                "non-cohorted topic": {"id": "non_cohorted_topic"},
-            },
             cohort_config={
                 "cohorted": True,
                 "cohorted_discussions": ["cohorted_topic"]
             }
         )
-        self.student_cohort = CourseUserGroup.objects.create(
+        self.student_cohort = CohortFactory.create(
             name="student_cohort",
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
+            course_id=self.course.id
         )
-        self.moderator_cohort = CourseUserGroup.objects.create(
+        self.moderator_cohort = CohortFactory.create(
             name="moderator_cohort",
-            course_id=self.course.id,
-            group_type=CourseUserGroup.COHORT
+            course_id=self.course.id
         )
+        self.course.discussion_topics["cohorted topic"] = {"id": "cohorted_topic"}
+        self.course.discussion_topics["non-cohorted topic"] = {"id": "non_cohorted_topic"}
+        self.store.update_item(self.course, self.user.id)
 
         seed_permissions_roles(self.course.id)
         self.student = UserFactory.create()
@@ -45,3 +49,82 @@ class CohortedContentTestCase(ModuleStoreTestCase):
         self.moderator.roles.add(Role.objects.get(name="Moderator", course_id=self.course.id))
         self.student_cohort.users.add(self.student)
         self.moderator_cohort.users.add(self.moderator)
+
+
+class ContentGroupTestCase(ModuleStoreTestCase):
+    """
+    Sets up discussion modules visible to content groups 'Alpha' and
+    'Beta', as well as a module visible to all students.  Creates a
+    staff user, users with access to Alpha/Beta (by way of cohorts),
+    and a non-cohorted user with no special access.
+    """
+    def setUp(self):
+        super(ContentGroupTestCase, self).setUp()
+
+        self.course = CourseFactory.create(
+            org='org', number='number', run='run',
+            # This test needs to use a course that has already started --
+            # discussion topics only show up if the course has already started,
+            # and the default start date for courses is Jan 1, 2030.
+            start=datetime(2012, 2, 3, tzinfo=UTC),
+            user_partitions=[
+                UserPartition(
+                    0,
+                    'Content Group Configuration',
+                    '',
+                    [Group(1, 'Alpha'), Group(2, 'Beta')],
+                    scheme_id='cohort'
+                )
+            ],
+            cohort_config={'cohorted': True},
+            discussion_topics={}
+        )
+
+        self.staff_user = UserFactory.create(is_staff=True)
+        self.alpha_user = UserFactory.create()
+        self.beta_user = UserFactory.create()
+        self.non_cohorted_user = UserFactory.create()
+        for user in [self.staff_user, self.alpha_user, self.beta_user, self.non_cohorted_user]:
+            CourseEnrollmentFactory.create(user=user, course_id=self.course.id)
+
+        alpha_cohort = CohortFactory(
+            course_id=self.course.id,
+            name='Cohort Alpha',
+            users=[self.alpha_user]
+        )
+        beta_cohort = CohortFactory(
+            course_id=self.course.id,
+            name='Cohort Beta',
+            users=[self.beta_user]
+        )
+        CourseUserGroupPartitionGroup.objects.create(
+            course_user_group=alpha_cohort,
+            partition_id=self.course.user_partitions[0].id,
+            group_id=self.course.user_partitions[0].groups[0].id
+        )
+        CourseUserGroupPartitionGroup.objects.create(
+            course_user_group=beta_cohort,
+            partition_id=self.course.user_partitions[0].id,
+            group_id=self.course.user_partitions[0].groups[1].id
+        )
+        self.alpha_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='alpha_group_discussion',
+            discussion_target='Visible to Alpha',
+            group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[0].id]}
+        )
+        self.beta_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='beta_group_discussion',
+            discussion_target='Visible to Beta',
+            group_access={self.course.user_partitions[0].id: [self.course.user_partitions[0].groups[1].id]}
+        )
+        self.global_module = ItemFactory.create(
+            parent_location=self.course.location,
+            category='discussion',
+            discussion_id='global_group_discussion',
+            discussion_target='Visible to Everyone'
+        )
+        self.course = self.store.get_item(self.course.location)

lms/djangoapps/django_comment_client/utils.py

@@ -18,6 +18,7 @@ from django_comment_common.models import Role, FORUM_ROLE_STUDENT
 from django_comment_client.permissions import check_permissions_by_view, cached_has_permission
 from edxmako import lookup_template
 
+from courseware.access import has_access
 from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, \
     is_course_cohorted
 from openedx.core.djangoapps.course_groups.models import CourseUserGroup
@@ -59,9 +60,11 @@ def has_forum_access(uname, course_id, rolename):
     return role.users.filter(username=uname).exists()
 
 
-def _get_discussion_modules(course):
+# pylint: disable=invalid-name
+def get_accessible_discussion_modules(course, user):
     """
-    Return a list of all valid discussion modules in this course.
+    Return a list of all valid discussion modules in this course that
+    are accessible to the given user.
     """
     all_modules = modulestore().get_items(course.id, qualifiers={'category': 'discussion'})
 
@@ -72,12 +75,16 @@ def _get_discussion_modules(course):
                 return False
         return True
 
-    return filter(has_required_keys, all_modules)
+    return [
+        module for module in all_modules
+        if has_required_keys(module) and has_access(user, 'load', module, course.id)
+    ]
 
 
-def get_discussion_id_map(course):
+def get_discussion_id_map(course, user):
     """
-    Transform the list of this course's discussion modules into a dictionary of metadata keyed by discussion_id.
+    Transform the list of this course's discussion modules (visible to a given user) into a dictionary of metadata keyed
+    by discussion_id.
     """
     def get_entry(module):  # pylint: disable=missing-docstring
         discussion_id = module.discussion_id
@@ -85,7 +92,7 @@ def get_discussion_id_map(course):
         last_category = module.discussion_category.split("/")[-1].strip()
         return (discussion_id, {"location": module.location, "title": last_category + " / " + title})
 
-    return dict(map(get_entry, _get_discussion_modules(course)))
+    return dict(map(get_entry, get_accessible_discussion_modules(course, user)))
 
 
 def _filter_unstarted_categories(category_map):
@@ -138,14 +145,14 @@ def _sort_map_entries(category_map, sort_alpha):
     category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])]
 
 
-def get_discussion_category_map(course):
+def get_discussion_category_map(course, user):
     """
     Transform the list of this course's discussion modules into a recursive dictionary structure.  This is used
-    to render the discussion category map in the discussion tab sidebar.
+    to render the discussion category map in the discussion tab sidebar for a given user.
     """
     unexpanded_category_map = defaultdict(list)
 
-    modules = _get_discussion_modules(course)
+    modules = get_accessible_discussion_modules(course, user)
 
     is_course_cohorted = course.is_cohorted
     cohorted_discussion_ids = course.cohorted_discussions
@@ -218,21 +225,15 @@ def get_discussion_category_map(course):
     return _filter_unstarted_categories(category_map)
 
 
-def get_discussion_categories_ids(course):
+def get_discussion_categories_ids(course, user):
     """
-    Returns a list of available ids of categories for the course.
+    Returns a list of available ids of categories for the course that
+    are accessible to the given user.
     """
-    ids = []
-    queue = [get_discussion_category_map(course)]
-    while queue:
-        category_map = queue.pop()
-        for child in category_map["children"]:
-            if child in category_map["entries"]:
-                ids.append(category_map["entries"][child]["id"])
-            else:
-                queue.append(category_map["subcategories"][child])
-
-    return ids
+    accessible_discussion_ids = [
+        module.discussion_id for module in get_accessible_discussion_modules(course, user)
+    ]
+    return course.top_level_discussion_topic_ids + accessible_discussion_ids
 
 
 class JsonResponse(HttpResponse):
@@ -382,12 +383,12 @@ def extend_content(content):
     return merge_dict(content, content_info)
 
 
-def add_courseware_context(content_list, course, id_map=None):
+def add_courseware_context(content_list, course, user, id_map=None):
     """
     Decorates `content_list` with courseware metadata.
     """
     if id_map is None:
-        id_map = get_discussion_id_map(course)
+        id_map = get_discussion_id_map(course, user)
 
     for content in content_list:
         commentable_id = content['commentable_id']

lms/envs/common.py

@@ -1560,6 +1560,8 @@ INSTALLED_APPS = (
     'provider.oauth2',
     'oauth2_provider',
 
+    'oauth_exchange',
+
     # For the wiki
     'wiki',  # The new django-wiki from benjaoming
     'django_notify',

lms/urls.py

@@ -5,6 +5,7 @@ from django.conf.urls.static import static
 
 import django.contrib.auth.views
 from microsite_configuration import microsite
+import oauth_exchange.views
 
 # Uncomment the next two lines to enable the admin:
 if settings.DEBUG or settings.FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'):
@@ -588,6 +589,11 @@ if settings.FEATURES.get('AUTOMATIC_AUTH_FOR_TESTING'):
 if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH'):
     urlpatterns += (
         url(r'', include('third_party_auth.urls')),
+        url(
+            r'^oauth2/exchange_access_token/(?P<backend>[^/]+)/$',
+            oauth_exchange.views.AccessTokenExchangeView.as_view(),
+            name="exchange_access_token"
+        ),
         url(r'^login_oauth_token/(?P<backend>[^/]+)/$', 'student.views.login_oauth_token'),
     )
 

openedx/core/djangoapps/content/course_structures/management/commands/generate_course_structure.py

@@ -2,14 +2,13 @@ import logging
 from optparse import make_option
 
 from django.core.management.base import BaseCommand
-
 from opaque_keys.edx.keys import CourseKey
 from xmodule.modulestore.django import modulestore
 
-from openedx.core.djangoapps.content.course_structures.models import update_course_structure
+from openedx.core.djangoapps.content.course_structures.tasks import update_course_structure
 
 
-logger = logging.getLogger(__name__)
+log = logging.getLogger(__name__)
 
 
 class Command(BaseCommand):
@@ -31,16 +30,21 @@ class Command(BaseCommand):
             course_keys = [CourseKey.from_string(arg) for arg in args]
 
         if not course_keys:
-            logger.fatal('No courses specified.')
+            log.fatal('No courses specified.')
             return
 
-        logger.info('Generating course structures for %d courses.', len(course_keys))
-        logging.debug('Generating course structure(s) for the following courses: %s', course_keys)
+        log.info('Generating course structures for %d courses.', len(course_keys))
+        log.debug('Generating course structure(s) for the following courses: %s', course_keys)
 
         for course_key in course_keys:
             try:
-                update_course_structure(unicode(course_key))
-            except Exception as e:
-                logger.error('An error occurred while generating course structure for %s: %s', unicode(course_key), e)
-
-        logger.info('Finished generating course structures.')
+                # Run the update task synchronously so that we know when all course structures have been updated.
+                # TODO Future improvement: Use .delay(), add return value to ResultSet, and wait for execution of
+                # all tasks using ResultSet.join(). I (clintonb) am opting not to make this improvement right now
+                # as I do not have time to test it fully.
+                update_course_structure.apply(unicode(course_key))
+            except Exception as ex:
+                log.exception('An error occurred while generating course structure for %s: %s',
+                              unicode(course_key), ex.message)
+
+        log.info('Finished generating course structures.')

openedx/core/djangoapps/content/course_structures/models.py

 import json
 import logging
 
-from celery.task import task
-from django.dispatch import receiver
 from model_utils.models import TimeStampedModel
-from opaque_keys.edx.keys import CourseKey
-from xmodule.modulestore.django import modulestore, SignalHandler
 
 from util.models import CompressedTextField
 from xmodule_django.models import CourseKeyField
@@ -30,66 +26,6 @@ class CourseStructure(TimeStampedModel):
             return json.loads(self.structure_json)
         return None
 
-
-def generate_course_structure(course_key):
-    """
-    Generates a course structure dictionary for the specified course.
-    """
-    course = modulestore().get_course(course_key, depth=None)
-    blocks_stack = [course]
-    blocks_dict = {}
-    while blocks_stack:
-        curr_block = blocks_stack.pop()
-        children = curr_block.get_children() if curr_block.has_children else []
-        blocks_dict[unicode(curr_block.scope_ids.usage_id)] = {
-            "usage_key": unicode(curr_block.scope_ids.usage_id),
-            "block_type": curr_block.category,
-            "display_name": curr_block.display_name,
-            "graded": curr_block.graded,
-            "format": curr_block.format,
-            "children": [unicode(child.scope_ids.usage_id) for child in children]
-        }
-        blocks_stack.extend(children)
-    return {
-        "root": unicode(course.scope_ids.usage_id),
-        "blocks": blocks_dict
-    }
-
-
-@receiver(SignalHandler.course_published)
-def listen_for_course_publish(sender, course_key, **kwargs):
-    # Note: The countdown=0 kwarg is set to to ensure the method below does not attempt to access the course
-    # before the signal emitter has finished all operations. This is also necessary to ensure all tests pass.
-    update_course_structure.delay(unicode(course_key), countdown=0)
-
-
-@task(name=u'openedx.core.djangoapps.content.course_structures.models.update_course_structure')
-def update_course_structure(course_key):
-    """
-    Regenerates and updates the course structure (in the database) for the specified course.
-    """
-    # Ideally we'd like to accept a CourseLocator; however, CourseLocator is not JSON-serializable (by default) so
-    # Celery's delayed tasks fail to start. For this reason, callers should pass the course key as a Unicode string.
-    if not isinstance(course_key, basestring):
-        raise ValueError('course_key must be a string. {} is not acceptable.'.format(type(course_key)))
-
-    course_key = CourseKey.from_string(course_key)
-
-    try:
-        structure = generate_course_structure(course_key)
-    except Exception as e:
-        logger.error('An error occurred while generating course structure: %s', e)
-        raise
-
-    structure_json = json.dumps(structure)
-
-    cs, created = CourseStructure.objects.get_or_create(
-        course_id=course_key,
-        defaults={'structure_json': structure_json}
-    )
-
-    if not created:
-        cs.structure_json = structure_json
-        cs.save()
-
-    return cs
+# Signals must be imported in a file that is automatically loaded at app startup (e.g. models.py). We import them
+# at the end of this file to avoid circular dependencies.
+import signals  # pylint: disable=unused-import

openedx/core/djangoapps/content/course_structures/signals.py

+from django.dispatch.dispatcher import receiver
+
+from xmodule.modulestore.django import SignalHandler
+
+
+@receiver(SignalHandler.course_published)
+def listen_for_course_publish(sender, course_key, **kwargs):  # pylint: disable=unused-argument
+    # Import tasks here to avoid a circular import.
+    from .tasks import update_course_structure
+
+    # Note: The countdown=0 kwarg is set to to ensure the method below does not attempt to access the course
+    # before the signal emitter has finished all operations. This is also necessary to ensure all tests pass.
+    update_course_structure.delay(unicode(course_key), countdown=0)

openedx/core/djangoapps/content/course_structures/tasks.py

+import json
+import logging
+
+from celery.task import task
+from opaque_keys.edx.keys import CourseKey
+from xmodule.modulestore.django import modulestore
+
+
+log = logging.getLogger('edx.celery.task')
+
+
+def _generate_course_structure(course_key):
+    """
+    Generates a course structure dictionary for the specified course.
+    """
+    course = modulestore().get_course(course_key, depth=None)
+    blocks_stack = [course]
+    blocks_dict = {}
+    while blocks_stack:
+        curr_block = blocks_stack.pop()
+        children = curr_block.get_children() if curr_block.has_children else []
+        key = unicode(curr_block.scope_ids.usage_id)
+        block = {
+            "usage_key": key,
+            "block_type": curr_block.category,
+            "display_name": curr_block.display_name,
+            "children": [unicode(child.scope_ids.usage_id) for child in children]
+        }
+
+        # Retrieve these attributes separately so that we can fail gracefully if the block doesn't have the attribute.
+        attrs = (('graded', False), ('format', None))
+        for attr, default in attrs:
+            if hasattr(curr_block, attr):
+                block[attr] = getattr(curr_block, attr, default)
+            else:
+                log.warning('Failed to retrieve %s attribute of block %s. Defaulting to %s.', attr, key, default)
+                block[attr] = default
+
+        blocks_dict[key] = block
+
+        # Add this blocks children to the stack so that we can traverse them as well.
+        blocks_stack.extend(children)
+    return {
+        "root": unicode(course.scope_ids.usage_id),
+        "blocks": blocks_dict
+    }
+
+
+@task(name=u'openedx.core.djangoapps.content.course_structures.tasks.update_course_structure')
+def update_course_structure(course_key):
+    """
+    Regenerates and updates the course structure (in the database) for the specified course.
+    """
+    # Import here to avoid circular import.
+    from .models import CourseStructure
+
+    # Ideally we'd like to accept a CourseLocator; however, CourseLocator is not JSON-serializable (by default) so
+    # Celery's delayed tasks fail to start. For this reason, callers should pass the course key as a Unicode string.
+    if not isinstance(course_key, basestring):
+        raise ValueError('course_key must be a string. {} is not acceptable.'.format(type(course_key)))
+
+    course_key = CourseKey.from_string(course_key)
+
+    try:
+        structure = _generate_course_structure(course_key)
+    except Exception as ex:
+        log.exception('An error occurred while generating course structure: %s', ex.message)
+        raise
+
+    structure_json = json.dumps(structure)
+
+    cs, created = CourseStructure.objects.get_or_create(
+        course_id=course_key,
+        defaults={'structure_json': structure_json}
+    )
+
+    if not created:
+        cs.structure_json = structure_json
+        cs.save()

openedx/core/djangoapps/content/course_structures/tests.py

 import json
+
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 
-from openedx.core.djangoapps.content.course_structures.models import generate_course_structure, CourseStructure
+from openedx.core.djangoapps.content.course_structures.models import CourseStructure
+from openedx.core.djangoapps.content.course_structures.tasks import _generate_course_structure, update_course_structure
 
 
-class CourseStructureTests(ModuleStoreTestCase):
+class CourseStructureTaskTests(ModuleStoreTestCase):
     def setUp(self, **kwargs):
-        super(CourseStructureTests, self).setUp()
+        super(CourseStructureTaskTests, self).setUp()
         self.course = CourseFactory.create()
         self.section = ItemFactory.create(parent=self.course, category='chapter', display_name='Test Section')
         CourseStructure.objects.all().delete()
@@ -38,7 +40,7 @@ class CourseStructureTests(ModuleStoreTestCase):
         }
 
         self.maxDiff = None
-        actual = generate_course_structure(self.course.id)
+        actual = _generate_course_structure(self.course.id)
         self.assertDictEqual(actual, expected)
 
     def test_structure_json(self):
@@ -77,3 +79,41 @@ class CourseStructureTests(ModuleStoreTestCase):
         structure_json = json.dumps(structure)
         cs = CourseStructure.objects.create(course_id=self.course.id, structure_json=structure_json)
         self.assertDictEqual(cs.structure, structure)
+
+    def test_block_with_missing_fields(self):
+        """
+        The generator should continue to operate on blocks/XModule that do not have graded or format fields.
+        """
+        # TODO In the future, test logging using testfixtures.LogCapture
+        # (https://pythonhosted.org/testfixtures/logging.html). Talk to TestEng before adding that library.
+        category = 'peergrading'
+        display_name = 'Testing Module'
+        module = ItemFactory.create(parent=self.section, category=category, display_name=display_name)
+        structure = _generate_course_structure(self.course.id)
+
+        usage_key = unicode(module.location)
+        actual = structure['blocks'][usage_key]
+        expected = {
+            "usage_key": usage_key,
+            "block_type": category,
+            "display_name": display_name,
+            "graded": False,
+            "format": None,
+            "children": []
+        }
+        self.assertEqual(actual, expected)
+
+    def test_update_course_structure(self):
+        """
+        Test the actual task that orchestrates data generation and updating the database.
+        """
+        # Method requires string input
+        course_id = self.course.id
+        self.assertRaises(ValueError, update_course_structure, course_id)
+
+        # Ensure a CourseStructure object is created
+        structure = _generate_course_structure(course_id)
+        update_course_structure(unicode(course_id))
+        cs = CourseStructure.objects.get(course_id=course_id)
+        self.assertEqual(cs.course_id, course_id)
+        self.assertEqual(cs.structure, structure)