Merge pull request #13196 from open-craft/haikuginger/change-coaches-to-staff

Change CCX coaches to have staff role on CCX courses.

common/djangoapps/student/roles.py

@@ -153,7 +153,7 @@ class RoleBase(AccessRole):
         # legit get updated.
         from student.models import CourseAccessRole
         for user in users:
-            if user.is_authenticated and user.is_active and not self.has_user(user):
+            if user.is_authenticated() and user.is_active and not self.has_user(user):
                 entry = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org)
                 entry.save()
                 if hasattr(user, '_roles'):
@@ -349,7 +349,7 @@ class UserBasedRole(object):
         """
         Grant this object's user the object's role for the supplied courses
         """
-        if self.user.is_authenticated and self.user.is_active:
+        if self.user.is_authenticated() and self.user.is_active:
             for course_key in course_keys:
                 entry = CourseAccessRole(user=self.user, role=self.role, course_id=course_key, org=course_key.org)
                 entry.save()

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

@@ -37,7 +37,7 @@ from lms.djangoapps.ccx.overrides import (
 )
 from lms.djangoapps.ccx.utils import (
     add_master_course_staff_to_ccx,
-    assign_coach_role_to_ccx,
+    assign_staff_role_to_ccx,
     is_email,
     get_course_chapters,
 )
@@ -507,8 +507,8 @@ class CCXListView(GenericAPIView):
                 email_students=True,
                 email_params=email_params,
             )
-            # assign coach role for the coach to the newly created ccx
-            assign_coach_role_to_ccx(ccx_course_key, coach, master_course_object.id)
+            # assign staff role for the coach to the newly created ccx
+            assign_staff_role_to_ccx(ccx_course_key, coach, master_course_object.id)
             # assign staff role for all the staff and instructor of the master course to the newly created ccx
             add_master_course_staff_to_ccx(
                 master_course_object,
@@ -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/0005_change_ccx_coach_to_staff.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+import logging
+
+from django.contrib.auth.models import User
+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.
+
+    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
+    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.
+
+    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
+    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'),
+        ('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
+        )
+    ]

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 openedx.core.djangoapps.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/ccx/utils.py

@@ -292,9 +292,9 @@ def ccx_course(ccx_locator):
     yield course
 
 
-def assign_coach_role_to_ccx(ccx_locator, user, master_course_id):
+def assign_staff_role_to_ccx(ccx_locator, user, master_course_id):
     """
-    Check if user has ccx_coach role on master course then assign him coach role on ccx only
+    Check if user has ccx_coach role on master course then assign him staff role on ccx only
     if role is not already assigned. Because of this coach can open dashboard from master course
     as well as ccx.
     :param ccx_locator: CCX key
@@ -304,12 +304,12 @@ def assign_coach_role_to_ccx(ccx_locator, user, master_course_id):
     coach_role_on_master_course = CourseCcxCoachRole(master_course_id)
     # check if user has coach role on master course
     if coach_role_on_master_course.has_user(user):
-        # Check if user has coach role on ccx.
-        role = CourseCcxCoachRole(ccx_locator)
+        # Check if user has staff role on ccx.
+        role = CourseStaffRole(ccx_locator)
         if not role.has_user(user):
-            # assign user role coach on ccx
+            # assign user the staff role on ccx
             with ccx_course(ccx_locator) as course:
-                allow_access(course, user, "ccx_coach", send_email=False)
+                allow_access(course, user, "staff", send_email=False)
 
 
 def is_email(identifier):

lms/djangoapps/ccx/views.py

@@ -56,7 +56,7 @@ from lms.djangoapps.ccx.overrides import (
 )
 from lms.djangoapps.ccx.utils import (
     add_master_course_staff_to_ccx,
-    assign_coach_role_to_ccx,
+    assign_staff_role_to_ccx,
     ccx_course,
     ccx_students_enrolling_center,
     get_ccx_for_coach,
@@ -147,7 +147,8 @@ def dashboard(request, course, ccx=None):
 
     if ccx:
         ccx_locator = CCXLocator.from_course_locator(course.id, unicode(ccx.id))
-
+        # At this point we are done with verification that current user is ccx coach.
+        assign_staff_role_to_ccx(ccx_locator, request.user, course.id)
         schedule = get_ccx_schedule(course, ccx)
         grading_policy = get_override_for_ccx(
             ccx, course, 'grading_policy', course.grading_policy)
@@ -239,7 +240,7 @@ def create_ccx(request, course, ccx=None):
         email_params=email_params,
     )
 
-    assign_coach_role_to_ccx(ccx_id, request.user, course.id)
+    assign_staff_role_to_ccx(ccx_id, request.user, course.id)
     add_master_course_staff_to_ccx(course, ccx_id, ccx.display_name)
 
     # using CCX object as sender here.

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

@@ -78,6 +78,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):
@@ -115,7 +129,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)
@@ -172,7 +186,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))
 
@@ -424,7 +439,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
@@ -474,10 +489,12 @@ def _section_membership(course, access, is_white_label):
 def _section_cohort_management(course, access):
     """ Provide data for the corresponding cohort management section """
     course_key = course.id
+    ccx_enabled = hasattr(course_key, 'ccx')
     section_data = {
         'section_key': 'cohort_management',
         'section_display_name': _('Cohorts'),
         'access': access,
+        'ccx_is_enabled': ccx_enabled,
         'course_cohort_settings_url': reverse(
             'course_cohort_settings',
             kwargs={'course_key_string': unicode(course_key)}

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

lms/static/js/groups/views/cohort_form.js

@@ -35,7 +35,8 @@
                         cohort: this.model,
                         isDefaultCohort: this.isDefault(this.model.get('name')),
                         contentGroups: this.contentGroups,
-                        studioGroupConfigurationsUrl: this.context.studioGroupConfigurationsUrl
+                        studioGroupConfigurationsUrl: this.context.studioGroupConfigurationsUrl,
+                        isCcxEnabled: this.context.isCcxEnabled
                     }));
                     return this;
                 },

lms/static/js/groups/views/cohorts_dashboard_factory.js

@@ -31,7 +31,8 @@
                         discussionTopicsSettingsModel: discussionTopicsSettings,
                         uploadCohortsCsvUrl: cohortManagementElement.data('upload_cohorts_csv_url'),
                         verifiedTrackCohortingUrl: cohortManagementElement.data('verified_track_cohorting_url'),
-                        studioGroupConfigurationsUrl: studioGroupConfigurationsUrl
+                        studioGroupConfigurationsUrl: studioGroupConfigurationsUrl,
+                        isCcxEnabled: cohortManagementElement.data('is_ccx_enabled')
                     }
                 });
 

lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore

@@ -17,7 +17,6 @@
             var cohort_name = cohort.get('name');
             var cohort_name_value = isNewCohort ? '' : cohort_name;
             var placeholder_value = isNewCohort ? gettext('Enter the name of the cohort') : '';
-
             %>
             <div class="form-field">
                 <div class="cohort-management-settings-form-name field field-text">
@@ -126,7 +125,11 @@
                                             }
                                         )
                                         %>
-                                        <a class="link-to-group-settings" href="<%- studioGroupConfigurationsUrl %>"><%- gettext("Create a content group") %></a>
+                                        <% if (isCcxEnabled) { %>
+                                            <%- gettext("Only the parent course staff of a CCX can create content groups.") %>
+                                        <% } else { %>
+                                            <a class="link-to-group-settings" href="<%- studioGroupConfigurationsUrl %>"><%- gettext("Create a content group") %></a>
+                                        <% } %>
                                     </p>
                                 </div>
                             </div>

lms/templates/instructor/instructor_dashboard_2/cohort_management.html

@@ -15,6 +15,7 @@ from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_
      data-course_cohort_settings_url="${section_data['course_cohort_settings_url']}"
      data-discussion-topics-url="${section_data['discussion_topics_url']}"
      data-verified_track_cohorting_url="${section_data['verified_track_cohorting_url']}"
+     data-is_ccx_enabled="${'true' if section_data['ccx_is_enabled'] else 'false'}"
 >
 </div>