Finish implementing CCX coach as staff

lms/djangoapps/ccx/api/v0/tests/test_views.py

@@ -647,9 +647,14 @@ class CcxListTest(CcxRestApiTest):
             list_staff_ccx_course = list_with_level(course_ccx, 'staff')
             list_instructor_ccx_course = list_with_level(course_ccx, 'instructor')
 
-        self.assertEqual(len(list_staff_master_course), len(list_staff_ccx_course))
-        for course_user, ccx_user in izip(sorted(list_staff_master_course), sorted(list_staff_ccx_course)):
-            self.assertEqual(course_user, ccx_user)
+        # The "Coach" in the parent course becomes "Staff" on the CCX, so the CCX should have 1 "Staff"
+        # user more than the parent course
+        self.assertEqual(len(list_staff_master_course) + 1, len(list_staff_ccx_course))
+        # Make sure all of the existing course staff are passed to the CCX
+        for course_user in list_staff_master_course:
+            self.assertIn(course_user, list_staff_ccx_course)
+        # Make sure the "Coach" on the parent course is "Staff" on the CCX
+        self.assertIn(self.coach, list_staff_ccx_course)
         self.assertEqual(len(list_instructor_master_course), len(list_instructor_ccx_course))
         for course_user, ccx_user in izip(sorted(list_instructor_master_course), sorted(list_instructor_ccx_course)):
             self.assertEqual(course_user, ccx_user)

lms/djangoapps/ccx/api/v0/views.py

@@ -766,8 +766,8 @@ class CCXDetailView(GenericAPIView):
                     email_students=True,
                     email_params=email_params,
                 )
-                # enroll the coach to the newly created ccx
-                assign_coach_role_to_ccx(ccx_course_key, coach, master_course_object.id)
+                # make the new coach staff on the CCX
+                assign_staff_role_to_ccx(ccx_course_key, coach, master_course_object.id)
 
         # using CCX object as sender here.
         responses = SignalHandler.course_published.send(

lms/djangoapps/ccx/migrations/0004_change_ccx_coach_to_staff.py → lms/djangoapps/ccx/migrations/0005_change_ccx_coach_to_staff.py

 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
+import logging
 
-from django.db import migrations, models
-
-# We're doing something awful here, but it's necessary for the greater good:
 from django.contrib.auth.models import User
-
-from instructor.access import allow_access, revoke_access
+from django.db import migrations
 
 from ccx_keys.locator import CCXLocator
+from instructor.access import allow_access, revoke_access
 from lms.djangoapps.ccx.utils import ccx_course
 
+
+log = logging.getLogger("edx.ccx")
+
 def change_existing_ccx_coaches_to_staff(apps, schema_editor):
-	"""
-	Modify all coaches of CCX courses so that they have the staff role on the
-	CCX course they coach, but retain the CCX Coach role on the parent course.
+    """
+    Modify all coaches of CCX courses so that they have the staff role on the
+    CCX course they coach, but retain the CCX Coach role on the parent course.
 
-	Arguments:
-		apps (Applications): Apps in edX platform.
-		schema_editor (SchemaEditor): For editing database schema (unused)
+    Arguments:
+        apps (Applications): Apps in edX platform.
+        schema_editor (SchemaEditor): For editing database schema (unused)
 
-	"""
-	CustomCourseForEdX = apps.get_model('ccx', 'CustomCourseForEdX')
-	db_alias = schema_editor.connection.alias
-	list_ccx = CustomCourseForEdX.objects.using(db_alias).all()
-	for ccx in list_ccx:
-		ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
-		with ccx_course(ccx_locator) as course:
-			coach = User.objects.get(id=ccx.coach.id)
-			allow_access(course, coach, 'staff', send_email=False)
-			revoke_access(course, coach, 'ccx_coach', send_email=False)
+    """
+    CustomCourseForEdX = apps.get_model('ccx', 'CustomCourseForEdX')
+    db_alias = schema_editor.connection.alias
+    if not db_alias == 'default':
+        # This migration is not intended to run against the student_module_history database and
+        # will fail if it does. Ensure that it'll only run against the default database.
+        return
+    list_ccx = CustomCourseForEdX.objects.using(db_alias).all()
+    for ccx in list_ccx:
+        ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
+        with ccx_course(ccx_locator) as course:
+            coach = User.objects.get(id=ccx.coach.id)
+            allow_access(course, coach, 'staff', send_email=False)
+            revoke_access(course, coach, 'ccx_coach', send_email=False)
+            log.info(
+                'The CCX coach of CCX %s has been switched from "CCX Coach" to "Staff".',
+                unicode(ccx_locator)
+            )
 
 def revert_ccx_staff_to_coaches(apps, schema_editor):
-	"""
-	Modify all staff on CCX courses so that they no longer have the staff role
-	on the course that they coach.
+    """
+    Modify all staff on CCX courses so that they no longer have the staff role
+    on the course that they coach.
 
-	Arguments:
-		apps (Applications): Apps in edX platform.
-		schema_editor (SchemaEditor): For editing database schema (unused)
+    Arguments:
+        apps (Applications): Apps in edX platform.
+        schema_editor (SchemaEditor): For editing database schema (unused)
 
-	"""
-	CustomCourseForEdX = apps.get_model('ccx', 'CustomCourseForEdX')
-	db_alias = schema_editor.connection.alias
-	list_ccx = CustomCourseForEdX.objects.using(db_alias).all()
-	for ccx in list_ccx:
-		ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
-		with ccx_course(ccx_locator) as course:
-			coach = User.objects.get(id=ccx.coach.id)
-			allow_access(course, coach, 'ccx_coach', send_email=False)
-			revoke_access(course, coach, 'staff', send_email=False)
+    """
+    CustomCourseForEdX = apps.get_model('ccx', 'CustomCourseForEdX')
+    db_alias = schema_editor.connection.alias
+    if not db_alias == 'default':
+        return
+    list_ccx = CustomCourseForEdX.objects.using(db_alias).all()
+    for ccx in list_ccx:
+        ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
+        with ccx_course(ccx_locator) as course:
+            coach = User.objects.get(id=ccx.coach.id)
+            allow_access(course, coach, 'ccx_coach', send_email=False)
+            revoke_access(course, coach, 'staff', send_email=False)
+            log.info(
+                'The CCX coach of CCX %s has been switched from "Staff" to "CCX Coach".',
+                unicode(ccx_locator)
+            )
 
 class Migration(migrations.Migration):
 
-	dependencies = [
-		('ccx', '0001_initial'),
-		('ccx', '0002_customcourseforedx_structure_json'),
-		('ccx', '0003_add_master_course_staff_in_ccx'),
-	]
+    dependencies = [
+        ('ccx', '0001_initial'),
+        ('ccx', '0002_customcourseforedx_structure_json'),
+        ('ccx', '0003_add_master_course_staff_in_ccx'),
+        ('ccx', '0004_seed_forum_roles_in_ccx_courses'),
+    ]
 
-	operations = [
-		migrations.RunPython(
-			code=change_existing_ccx_coaches_to_staff,
-			reverse_code=revert_ccx_staff_to_coaches
-		)
-	]
+    operations = [
+        migrations.RunPython(
+            code=change_existing_ccx_coaches_to_staff,
+            reverse_code=revert_ccx_staff_to_coaches
+        )
+    ]

lms/djangoapps/ccx/models.py

 """
 Models for the custom course feature
 """
+from __future__ import unicode_literals
 import json
 import logging
 from datetime import datetime
@@ -8,8 +9,9 @@ from datetime import datetime
 from django.contrib.auth.models import User
 from django.db import models
 from pytz import utc
-
 from lazy import lazy
+
+from ccx_keys.locator import CCXLocator
 from openedx.core.lib.time_zone_utils import get_time_zone_abbr
 from xmodule_django.models import CourseKeyField, LocationKeyField
 from xmodule.error_module import ErrorDescriptor
@@ -121,6 +123,16 @@ class CustomCourseForEdX(models.Model):
             return json.loads(self.structure_json)
         return None
 
+    @property
+    def locator(self):
+        """
+        Helper property that gets a corresponding CCXLocator for this CCX.
+
+        Returns:
+            The CCXLocator corresponding to this CCX.
+        """
+        return CCXLocator.from_course_locator(self.course_id, unicode(self.id))
+
 
 class CcxFieldOverride(models.Model):
     """

lms/djangoapps/ccx/tests/test_models.py

@@ -12,7 +12,10 @@ from student.tests.factories import (
     AdminFactory,
 )
 from util.tests.test_date_utils import fake_ugettext
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.django_utils import (
+    ModuleStoreTestCase,
+    TEST_DATA_SPLIT_MODULESTORE
+)
 from xmodule.modulestore.tests.factories import (
     CourseFactory,
     check_mongo_calls
@@ -30,6 +33,8 @@ class TestCCX(ModuleStoreTestCase):
     """Unit tests for the CustomCourseForEdX model
     """
 
+    MODULESTORE = TEST_DATA_SPLIT_MODULESTORE
+
     def setUp(self):
         """common setup for all tests"""
         super(TestCCX, self).setUp()
@@ -51,7 +56,7 @@ class TestCCX(ModuleStoreTestCase):
 
     def test_ccx_course_caching(self):
         """verify that caching the propery works to limit queries"""
-        with check_mongo_calls(1):
+        with check_mongo_calls(3):
             # these statements are used entirely to demonstrate the
             # instance-level caching of these values on CCX objects. The
             # check_mongo_calls context is the point here.
@@ -77,7 +82,7 @@ class TestCCX(ModuleStoreTestCase):
         """verify that caching the start property works to limit queries"""
         now = datetime.now(utc)
         self.set_ccx_override('start', now)
-        with check_mongo_calls(1):
+        with check_mongo_calls(3):
             # these statements are used entirely to demonstrate the
             # instance-level caching of these values on CCX objects. The
             # check_mongo_calls context is the point here.
@@ -102,7 +107,7 @@ class TestCCX(ModuleStoreTestCase):
         """verify that caching the due property works to limit queries"""
         expected = datetime.now(utc)
         self.set_ccx_override('due', expected)
-        with check_mongo_calls(1):
+        with check_mongo_calls(3):
             # these statements are used entirely to demonstrate the
             # instance-level caching of these values on CCX objects. The
             # check_mongo_calls context is the point here.
@@ -269,3 +274,10 @@ class TestCCX(ModuleStoreTestCase):
         )
         self.assertEqual(ccx.structure_json, json_struct)  # pylint: disable=no-member
         self.assertEqual(ccx.structure, dummy_struct)  # pylint: disable=no-member
+
+    def test_locator_property(self):
+        """
+        Verify that the locator helper property returns a correct CCXLocator
+        """
+        locator = self.ccx.locator  # pylint: disable=no-member
+        self.assertEqual(self.ccx.id, long(locator.ccx))

lms/djangoapps/ccx/tests/test_views.py

@@ -418,8 +418,8 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase):
         course_enrollments = get_override_for_ccx(ccx, self.course, 'max_student_enrollments_allowed')
         self.assertEqual(course_enrollments, settings.CCX_MAX_STUDENTS_ALLOWED)
 
-        # assert ccx creator has role=ccx_coach
-        role = CourseCcxCoachRole(course_key)
+        # assert ccx creator has role=staff
+        role = CourseStaffRole(course_key)
         self.assertTrue(role.has_user(self.coach))
 
         # assert that staff and instructors of master course has staff and instructor roles on ccx
@@ -432,8 +432,12 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase):
 
         with ccx_course(course_key) as course_ccx:
             list_staff_ccx_course = list_with_level(course_ccx, 'staff')
-            self.assertEqual(len(list_staff_master_course), len(list_staff_ccx_course))
-            self.assertEqual(list_staff_master_course[0].email, list_staff_ccx_course[0].email)
+            # The "Coach" in the parent course becomes "Staff" on the CCX, so the CCX should have 1 "Staff"
+            # user more than the parent course
+            self.assertEqual(len(list_staff_master_course) + 1, len(list_staff_ccx_course))
+            self.assertIn(list_staff_master_course[0].email, [ccx_staff.email for ccx_staff in list_staff_ccx_course])
+            # Make sure the "Coach" on the parent course is "Staff" on the CCX
+            self.assertIn(self.coach, list_staff_ccx_course)
 
             list_instructor_ccx_course = list_with_level(course_ccx, 'instructor')
             self.assertEqual(len(list_instructor_ccx_course), len(list_instructor_master_course))

lms/djangoapps/certificates/queue.py

@@ -199,6 +199,8 @@ class XQueueCertInterface(object):
         Will change the certificate status to 'generating' or
         `downloadable` in case of web view certificates.
 
+        The course must not be a CCX.
+
         Certificate must be in the 'unavailable', 'error',
         'deleted' or 'generating' state.
 
@@ -214,6 +216,18 @@ class XQueueCertInterface(object):
         Returns the newly created certificate instance
         """
 
+        if hasattr(course_id, 'ccx'):
+            LOGGER.warning(
+                (
+                    u"Cannot create certificate generation task for user %s "
+                    u"in the course '%s'; "
+                    u"certificates are not allowed for CCX courses."
+                ),
+                student.id,
+                unicode(course_id)
+            )
+            return None
+
         valid_statuses = [
             status.generating,
             status.unavailable,

lms/djangoapps/instructor/views/instructor_dashboard.py

@@ -77,6 +77,20 @@ class InstructorDashboardTab(CourseTab):
         return bool(user and has_access(user, 'staff', course, course.id))
 
 
+def show_analytics_dashboard_message(course_key):
+    """
+    Defines whether or not the analytics dashboard URL should be displayed.
+
+    Arguments:
+        course_key (CourseLocator): The course locator to display the analytics dashboard message on.
+    """
+    if hasattr(course_key, 'ccx'):
+        ccx_analytics_enabled = settings.FEATURES.get('ENABLE_CCX_ANALYTICS_DASHBOARD_URL', False)
+        return settings.ANALYTICS_DASHBOARD_URL and ccx_analytics_enabled
+
+    return settings.ANALYTICS_DASHBOARD_URL
+
+
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 def instructor_dashboard_2(request, course_id):
@@ -112,7 +126,7 @@ def instructor_dashboard_2(request, course_id):
     ]
 
     analytics_dashboard_message = None
-    if settings.ANALYTICS_DASHBOARD_URL:
+    if show_analytics_dashboard_message(course_key):
         # Construct a URL to the external analytics dashboard
         analytics_dashboard_url = '{0}/courses/{1}'.format(settings.ANALYTICS_DASHBOARD_URL, unicode(course_key))
         link_start = HTML("<a href=\"{}\" target=\"_blank\">").format(analytics_dashboard_url)
@@ -169,7 +183,8 @@ def instructor_dashboard_2(request, course_id):
     # Certificates panel
     # This is used to generate example certificates
     # and enable self-generated certificates for a course.
-    certs_enabled = CertificateGenerationConfiguration.current().enabled
+    # Note: This is hidden for all CCXs
+    certs_enabled = CertificateGenerationConfiguration.current().enabled and not hasattr(course_key, 'ccx')
     if certs_enabled and access['admin']:
         sections.append(_section_certificates(course))
 
@@ -421,7 +436,7 @@ def _section_course_info(course, access):
     if settings.FEATURES.get('DISPLAY_ANALYTICS_ENROLLMENTS'):
         section_data['enrollment_count'] = CourseEnrollment.objects.enrollment_counts(course_key)
 
-    if settings.ANALYTICS_DASHBOARD_URL:
+    if show_analytics_dashboard_message(course_key):
         #  dashboard_link is already made safe in _get_dashboard_link
         dashboard_link = _get_dashboard_link(course_key)
         #  so we can use Text() here so it's not double-escaped and rendering HTML on the front-end

lms/envs/common.py

@@ -356,6 +356,11 @@ FEATURES = {
     # lives in the Extended table, saving the frontend from
     # making multiple queries.
     'ENABLE_READING_FROM_MULTIPLE_HISTORY_TABLES': True,
+
+    # Display the 'Analytics' tab in the instructor dashboard for CCX courses.
+    # Note: This has no effect unless ANALYTICS_DASHBOARD_URL is already set,
+    #       because without that setting, the tab does not show up for any courses.
+    'ENABLE_CCX_ANALYTICS_DASHBOARD_URL': False,
 }
 
 # Ignore static asset files on import which match this pattern