Merge pull request #11787 from…

Merge pull request #11787 from mitocw/enhancement/aq/add_master_course_staff_in_ccx_fix_migration_issue

Adds staff and instructor users of the master course to CCX

cms/djangoapps/contentstore/tests/test_course_listing.py

@@ -9,6 +9,7 @@ from mock import patch, Mock
 import ddt
 
 from django.conf import settings
+from ccx_keys.locator import CCXLocator
 from django.test import RequestFactory
 from django.test.client import Client
 
@@ -26,6 +27,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls
 from xmodule.modulestore import ModuleStoreEnum
 from opaque_keys.edx.locations import CourseLocator
+from opaque_keys.edx.keys import CourseKey
 from xmodule.error_module import ErrorDescriptor
 from course_action_state.models import CourseRerunState
 
@@ -131,6 +133,39 @@ class TestCourseListing(ModuleStoreTestCase, XssTestMixin):
         # check both course lists have same courses
         self.assertEqual(courses_list, courses_list_by_groups)
 
+    def test_get_course_list_when_ccx(self):
+        """
+        Assert that courses with CCXLocator are filter in course listing.
+        """
+        course_location = self.store.make_course_key('Org1', 'Course1', 'Run1')
+        self._create_course_with_access_groups(course_location, self.user)
+
+        # get courses through iterating all courses
+        courses_list, __ = _accessible_courses_list(self.request)
+        self.assertEqual(len(courses_list), 1)
+
+        # get courses by reversing group name formats
+        courses_list_by_groups, __ = _accessible_courses_list_from_groups(self.request)
+        self.assertEqual(len(courses_list_by_groups), 1)
+
+        # assert no course in listing with ccx id
+        ccx_course = Mock()
+        course_key = CourseKey.from_string('course-v1:FakeOrg+CN1+CR-FALLNEVER1')
+        ccx_course.id = CCXLocator.from_course_locator(course_key, u"1")
+
+        with patch(
+            'xmodule.modulestore.mixed.MixedModuleStore.get_course',
+            return_value=ccx_course
+        ), patch(
+            'xmodule.modulestore.mixed.MixedModuleStore.get_courses',
+            Mock(return_value=[ccx_course])
+        ):
+            courses_list, __ = _accessible_courses_list_from_groups(self.request)
+            self.assertEqual(len(courses_list), 0)
+
+            courses_list, __ = _accessible_courses_list(self.request)
+            self.assertEqual(len(courses_list), 0)
+
     @ddt.data(
         (ModuleStoreEnum.Type.split, 'xmodule.modulestore.split_mongo.split_mongo_kvs.SplitMongoKVS'),
         (ModuleStoreEnum.Type.mongo, 'xmodule.modulestore.mongo.base.MongoKeyValueStore')

cms/djangoapps/contentstore/views/course.py

@@ -27,6 +27,7 @@ from .component import (
 )
 from .item import create_xblock_info
 from .library import LIBRARIES_ENABLED
+from ccx_keys.locator import CCXLocator
 from contentstore import utils
 from contentstore.course_group_config import (
     COHORT_SCHEME,
@@ -390,6 +391,11 @@ def _accessible_courses_list(request):
         if isinstance(course, ErrorDescriptor):
             return False
 
+        # Custom Courses for edX (CCX) is an edX feature for re-using course content.
+        # CCXs cannot be edited in Studio (aka cms) and should not be shown in this dashboard.
+        if isinstance(course.id, CCXLocator):
+            return False
+
         # pylint: disable=fixme
         # TODO remove this condition when templates purged from db
         if course.location.course == 'templates':
@@ -434,8 +440,11 @@ def _accessible_courses_list_from_groups(request):
             except ItemNotFoundError:
                 # If a user has access to a course that doesn't exist, don't do anything with that course
                 pass
-            if course is not None and not isinstance(course, ErrorDescriptor):
-                # ignore deleted or errored courses
+
+            # Custom Courses for edX (CCX) is an edX feature for re-using course content.
+            # CCXs cannot be edited in Studio (aka cms) and should not be shown in this dashboard.
+            if course is not None and not isinstance(course, ErrorDescriptor) and not isinstance(course.id, CCXLocator):
+                # ignore deleted, errored or ccx courses
                 courses_list[course_key] = course
 
     return courses_list.values(), in_process_course_actions

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

@@ -8,6 +8,7 @@ import pytz
 import string
 import urllib
 import urlparse
+from itertools import izip
 
 import ddt
 import mock
@@ -30,6 +31,8 @@ from rest_framework.test import APITestCase
 from courseware import courses
 from ccx_keys.locator import CCXLocator
 from student.models import CourseEnrollment
+from student.tests.factories import UserFactory
+from instructor.access import allow_access, list_with_level
 from instructor.enrollment import (
     enroll_email,
     get_email_params,
@@ -39,6 +42,7 @@ from lms.djangoapps.ccx.models import CcxFieldOverride, CustomCourseForEdX
 from lms.djangoapps.ccx.overrides import override_field_for_ccx
 from lms.djangoapps.ccx.tests.utils import CcxTestCase
 from lms.djangoapps.ccx.utils import get_course_chapters
+from lms.djangoapps.ccx.utils import ccx_course as ccx_course_cm
 from opaque_keys.edx.keys import CourseKey
 from student.roles import (
     CourseInstructorRole,
@@ -66,9 +70,14 @@ class CcxRestApiTest(CcxTestCase, APITestCase):
         self.master_course_key_str = unicode(self.master_course_key)
         # OAUTH2 setup
         # create a specific user for the application
-        app_user = User.objects.create_user('test_app_user', 'test_app_user@openedx.org', 'test')
+        app_user = UserFactory(username='test_app_user', email='test_app_user@openedx.org', password='test')
         # add staff role to the app user
         CourseStaffRole(self.master_course_key).add_users(app_user)
+
+        # adding instructor to master course.
+        instructor = UserFactory()
+        allow_access(self.course, instructor, 'instructor')
+
         # create an oauth client app entry
         self.app_client = Client.objects.create(
             user=app_user,
@@ -172,7 +181,7 @@ class CcxListTest(CcxRestApiTest):
         Check authorization for staff users logged in without oauth
         """
         # create a staff user
-        staff_user = User.objects.create_user('test_staff_user', 'test_staff_user@openedx.org', 'test')
+        staff_user = UserFactory(username='test_staff_user', email='test_staff_user@openedx.org', password='test')
         # add staff role to the staff user
         CourseStaffRole(self.master_course_key).add_users(staff_user)
 
@@ -194,7 +203,9 @@ class CcxListTest(CcxRestApiTest):
         Check authorization for instructor users logged in without oauth
         """
         # create an instructor user
-        instructor_user = User.objects.create_user('test_instructor_user', 'test_instructor_user@openedx.org', 'test')
+        instructor_user = UserFactory(
+            username='test_instructor_user', email='test_instructor_user@openedx.org', password='test'
+        )
         # add instructor role to the instructor user
         CourseInstructorRole(self.master_course_key).add_users(instructor_user)
 
@@ -217,7 +228,9 @@ class CcxListTest(CcxRestApiTest):
         Check authorization for coach users logged in without oauth
         """
         # create an coach user
-        coach_user = User.objects.create_user('test_coach_user', 'test_coach_user@openedx.org', 'test')
+        coach_user = UserFactory(
+            username='test_coach_user', email='test_coach_user@openedx.org', password='test'
+        )
         # add coach role to the coach user
         CourseCcxCoachRole(self.master_course_key).add_users(coach_user)
 
@@ -607,6 +620,38 @@ class CcxListTest(CcxRestApiTest):
         self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
         self.assertEqual(resp.data.get('course_modules'), chapters)  # pylint: disable=no-member
 
+    def test_post_list_staff_master_course_in_ccx(self):
+        """
+        Specific test to check that the staff and instructor of the master
+        course are assigned to the CCX.
+        """
+        outbox = self.get_outbox()
+        data = {
+            'master_course_id': self.master_course_key_str,
+            'max_students_allowed': 111,
+            'display_name': 'CCX Test Title',
+            'coach_email': self.coach.email
+        }
+        resp = self.client.post(self.list_url, data, format='json', HTTP_AUTHORIZATION=self.auth)
+        self.assertEqual(resp.status_code, status.HTTP_201_CREATED)
+        # check that only one email has been sent and it is to to the coach
+        self.assertEqual(len(outbox), 1)
+        self.assertIn(self.coach.email, outbox[0].recipients())  # pylint: disable=no-member
+
+        list_staff_master_course = list_with_level(self.course, 'staff')
+        list_instructor_master_course = list_with_level(self.course, 'instructor')
+        course_key = CourseKey.from_string(resp.data.get('ccx_course_id'))  # pylint: disable=no-member
+        with ccx_course_cm(course_key) as course_ccx:
+            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)
+        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)
+
 
 @attr('shard_1')
 @ddt.ddt

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

@@ -34,6 +34,7 @@ from lms.djangoapps.ccx.overrides import (
     override_field_for_ccx,
 )
 from lms.djangoapps.ccx.utils import (
+    add_master_course_staff_to_ccx,
     assign_coach_role_to_ccx,
     is_email,
     get_course_chapters,
@@ -506,6 +507,13 @@ class CCXListView(GenericAPIView):
             )
             # 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 all the staff and instructor of the master course to the newly created ccx
+            add_master_course_staff_to_ccx(
+                master_course_object,
+                ccx_course_key,
+                ccx_course_object.display_name,
+                send_email=False
+            )
 
         serializer = self.get_serializer(ccx_course_object)
         return Response(

lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from ccx_keys.locator import CCXLocator
+from courseware.courses import get_course_by_id
+from django.db import migrations
+
+from lms.djangoapps.ccx.utils import (
+    add_master_course_staff_to_ccx,
+    remove_master_course_staff_from_ccx,
+)
+
+
+def add_master_course_staff_to_ccx_for_existing_ccx(apps, schema_editor):
+    """
+    Add all staff and admin of master course to respective CCX(s).
+
+    Arguments:
+        apps (Applications): Apps in edX platform.
+        schema_editor (SchemaEditor): For editing database schema i.e create, delete field (column)
+
+    """
+    CustomCourseForEdX = apps.get_model("ccx", "CustomCourseForEdX")
+    list_ccx = CustomCourseForEdX.objects.all()
+    for ccx in list_ccx:
+        if ccx.course_id.deprecated:
+            # prevent migration for deprecated course ids.
+            continue
+        ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
+        add_master_course_staff_to_ccx(
+            get_course_by_id(ccx.course_id),
+            ccx_locator,
+            ccx.display_name,
+            send_email=False
+        )
+
+
+def remove_master_course_staff_from_ccx_for_existing_ccx(apps, schema_editor):
+    """
+    Remove all staff and instructors of master course from respective CCX(s).
+
+    Arguments:
+        apps (Applications): Apps in edX platform.
+        schema_editor (SchemaEditor): For editing database schema i.e create, delete field (column)
+
+    """
+    CustomCourseForEdX = apps.get_model("ccx", "CustomCourseForEdX")
+    list_ccx = CustomCourseForEdX.objects.all()
+    for ccx in list_ccx:
+        if ccx.course_id.deprecated:
+            # prevent migration for deprecated course ids.
+            continue
+        ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id))
+        remove_master_course_staff_from_ccx(
+            get_course_by_id(ccx.course_id),
+            ccx_locator,
+            ccx.display_name,
+            send_email=False
+        )
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('ccx', '0001_initial'),
+        ('ccx', '0002_customcourseforedx_structure_json'),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            code=add_master_course_staff_to_ccx_for_existing_ccx,
+            reverse_code=remove_master_course_staff_from_ccx_for_existing_ccx
+        )
+    ]

lms/djangoapps/ccx/tests/test_views.py

@@ -15,6 +15,11 @@ from courseware.courses import get_course_by_id
 from courseware.tests.factories import StudentModuleFactory
 from courseware.tests.helpers import LoginEnrollmentTestCase
 from courseware.tabs import get_course_tab_list
+from instructor.access import (
+    allow_access,
+    list_with_level,
+)
+
 from django.conf import settings
 from django.core.urlresolvers import reverse, resolve
 from django.utils.translation import ugettext as _
@@ -27,7 +32,7 @@ from opaque_keys.edx.keys import CourseKey
 from student.roles import (
     CourseCcxCoachRole,
     CourseInstructorRole,
-    CourseStaffRole
+    CourseStaffRole,
 )
 from student.models import (
     CourseEnrollment,
@@ -54,6 +59,7 @@ from ccx_keys.locator import CCXLocator
 
 from lms.djangoapps.ccx.models import CustomCourseForEdX
 from lms.djangoapps.ccx.overrides import get_override_for_ccx, override_field_for_ccx
+from lms.djangoapps.ccx.views import ccx_course
 from lms.djangoapps.ccx.tests.factories import CcxFactory
 from lms.djangoapps.ccx.tests.utils import (
     CcxTestCase,
@@ -210,6 +216,16 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase):
         # Login with the instructor account
         self.client.login(username=self.coach.username, password="test")
 
+        # adding staff to master course.
+        staff = UserFactory()
+        allow_access(self.course, staff, 'staff')
+        self.assertTrue(CourseStaffRole(self.course.id).has_user(staff))
+
+        # adding instructor to master course.
+        instructor = UserFactory()
+        allow_access(self.course, instructor, 'instructor')
+        self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor))
+
     def assert_elements_in_schedule(self, url, n_chapters=2, n_sequentials=4, n_verticals=8):
         """
         Helper function to count visible elements in the schedule
@@ -324,6 +340,19 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase):
         role = CourseCcxCoachRole(course_key)
         self.assertTrue(role.has_user(self.coach))
 
+        # assert that staff and instructors of master course has staff and instructor roles on ccx
+        list_staff_master_course = list_with_level(self.course, 'staff')
+        list_instructor_master_course = list_with_level(self.course, 'instructor')
+
+        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)
+
+            list_instructor_ccx_course = list_with_level(course_ccx, 'instructor')
+            self.assertEqual(len(list_instructor_ccx_course), len(list_instructor_master_course))
+            self.assertEqual(list_instructor_ccx_course[0].email, list_instructor_master_course[0].email)
+
     @ddt.data("CCX demo 1", "CCX demo 2", "CCX demo 3")
     def test_create_multiple_ccx(self, ccx_name):
         self.test_create_ccx(ccx_name)

lms/djangoapps/ccx/tests/utils.py

@@ -8,9 +8,13 @@ from django.conf import settings
 
 from lms.djangoapps.ccx.overrides import override_field_for_ccx
 from lms.djangoapps.ccx.tests.factories import CcxFactory
-from student.roles import CourseCcxCoachRole
+from student.roles import (
+    CourseCcxCoachRole,
+    CourseInstructorRole,
+    CourseStaffRole
+)
 from student.tests.factories import (
-    AdminFactory,
+    UserFactory,
 )
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.django_utils import (
@@ -76,10 +80,30 @@ class CcxTestCase(SharedModuleStoreTestCase):
         super(CcxTestCase, self).setUp()
 
         # Create instructor account
-        self.coach = AdminFactory.create()
+        self.coach = UserFactory.create()
         # create an instance of modulestore
         self.mstore = modulestore()
 
+    def make_staff(self):
+        """
+        create staff user.
+        """
+        staff = UserFactory.create(password="test")
+        role = CourseStaffRole(self.course.id)
+        role.add_users(staff)
+
+        return staff
+
+    def make_instructor(self):
+        """
+        create instructor user.
+        """
+        instructor = UserFactory.create(password="test")
+        role = CourseInstructorRole(self.course.id)
+        role.add_users(instructor)
+
+        return instructor
+
     def make_coach(self):
         """
         create coach user

lms/djangoapps/ccx/utils.py

@@ -13,24 +13,30 @@ from django.core.exceptions import ValidationError
 from django.utils.translation import ugettext as _
 from django.core.validators import validate_email
 from django.core.urlresolvers import reverse
+from smtplib import SMTPException
 
 from courseware.courses import get_course_by_id
 from courseware.model_data import FieldDataCache
 from courseware.module_render import get_module_for_descriptor
 from instructor.enrollment import (
     enroll_email,
+    get_email_params,
     unenroll_email,
 )
-from instructor.access import allow_access
+from instructor.access import (
+    allow_access,
+    list_with_level,
+    revoke_access,
+)
 from instructor.views.tools import get_student_from_identifier
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 from openedx.core.djangoapps.content.course_structures.models import CourseStructure
-from student.models import CourseEnrollment
+from student.models import CourseEnrollment, CourseEnrollmentException
 from student.roles import CourseCcxCoachRole
 
-from lms.djangoapps.ccx.models import CustomCourseForEdX
 from lms.djangoapps.ccx.overrides import get_override_for_ccx
 from lms.djangoapps.ccx.custom_exception import CCXUserValidationException
+from lms.djangoapps.ccx.models import CustomCourseForEdX
 
 log = logging.getLogger("edx.ccx")
 
@@ -334,3 +340,117 @@ def get_course_chapters(course_key):
         return course_struct['blocks'][course_struct['root']].get('children', [])
     except KeyError:
         return []
+
+
+def add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_email=True):
+    """
+    Add staff and instructor roles on ccx to all the staff and instructors members of master course.
+
+    Arguments:
+        master_course (CourseDescriptorWithMixins): Master course instance.
+        ccx_key (CCXLocator): CCX course key.
+        display_name (str): ccx display name for email.
+        send_email (bool): flag to switch on or off email to the users on access grant.
+
+    """
+    list_staff = list_with_level(master_course, 'staff')
+    list_instructor = list_with_level(master_course, 'instructor')
+
+    with ccx_course(ccx_key) as course_ccx:
+        email_params = get_email_params(course_ccx, auto_enroll=True, course_key=ccx_key, display_name=display_name)
+        list_staff_ccx = list_with_level(course_ccx, 'staff')
+        list_instructor_ccx = list_with_level(course_ccx, 'instructor')
+        for staff in list_staff:
+            # this call should be idempotent
+            if staff not in list_staff_ccx:
+                try:
+                    # Enroll the staff in the ccx
+                    enroll_email(
+                        course_id=ccx_key,
+                        student_email=staff.email,
+                        auto_enroll=True,
+                        email_students=send_email,
+                        email_params=email_params,
+                    )
+
+                    # allow 'staff' access on ccx to staff of master course
+                    allow_access(course_ccx, staff, 'staff')
+                except CourseEnrollmentException:
+                    log.warning(
+                        "Unable to enroll staff %s to course with id %s",
+                        staff.email,
+                        ccx_key
+                    )
+                    continue
+                except SMTPException:
+                    continue
+
+        for instructor in list_instructor:
+            # this call should be idempotent
+            if instructor not in list_instructor_ccx:
+                try:
+                    # Enroll the instructor in the ccx
+                    enroll_email(
+                        course_id=ccx_key,
+                        student_email=instructor.email,
+                        auto_enroll=True,
+                        email_students=send_email,
+                        email_params=email_params,
+                    )
+
+                    # allow 'instructor' access on ccx to instructor of master course
+                    allow_access(course_ccx, instructor, 'instructor')
+                except CourseEnrollmentException:
+                    log.warning(
+                        "Unable to enroll instructor %s to course with id %s",
+                        instructor.email,
+                        ccx_key
+                    )
+                    continue
+                except SMTPException:
+                    continue
+
+
+def remove_master_course_staff_from_ccx(master_course, ccx_key, display_name, send_email=True):
+    """
+    Remove staff and instructor roles on ccx to all the staff and instructors members of master course.
+
+    Arguments:
+        master_course (CourseDescriptorWithMixins): Master course instance.
+        ccx_key (CCXLocator): CCX course key.
+        display_name (str): ccx display name for email.
+        send_email (bool): flag to switch on or off email to the users on revoke access.
+
+    """
+    list_staff = list_with_level(master_course, 'staff')
+    list_instructor = list_with_level(master_course, 'instructor')
+
+    with ccx_course(ccx_key) as course_ccx:
+        list_staff_ccx = list_with_level(course_ccx, 'staff')
+        list_instructor_ccx = list_with_level(course_ccx, 'instructor')
+        email_params = get_email_params(course_ccx, auto_enroll=True, course_key=ccx_key, display_name=display_name)
+        for staff in list_staff:
+            if staff in list_staff_ccx:
+                # revoke 'staff' access on ccx.
+                revoke_access(course_ccx, staff, 'staff')
+
+                # Unenroll the staff on ccx.
+                unenroll_email(
+                    course_id=ccx_key,
+                    student_email=staff.email,
+                    email_students=send_email,
+                    email_params=email_params,
+                )
+
+        for instructor in list_instructor:
+            if instructor in list_instructor_ccx:
+                # revoke 'instructor' access on ccx.
+                revoke_access(course_ccx, instructor, 'instructor')
+
+                # Unenroll the instructor on ccx.
+                unenroll_email(
+                    course_id=ccx_key,
+                    student_email=instructor.email,
+                    email_students=send_email,
+                    email_params=email_params,
+                )

lms/djangoapps/ccx/views.py

@@ -53,6 +53,7 @@ from lms.djangoapps.ccx.overrides import (
     bulk_delete_ccx_override_fields,
 )
 from lms.djangoapps.ccx.utils import (
+    add_master_course_staff_to_ccx,
     assign_coach_role_to_ccx,
     ccx_course,
     ccx_students_enrolling_center,
@@ -155,6 +156,9 @@ def dashboard(request, course, ccx=None):
         context['grading_policy_url'] = reverse(
             'ccx_set_grading_policy', kwargs={'course_id': ccx_locator})
 
+        with ccx_course(ccx_locator) as course:
+            context['course'] = course
+
     else:
         context['create_ccx_url'] = reverse(
             'create_ccx', kwargs={'course_id': course.id})
@@ -224,7 +228,7 @@ def create_ccx(request, course, ccx=None):
     )
 
     assign_coach_role_to_ccx(ccx_id, request.user, course.id)
-
+    add_master_course_staff_to_ccx(course, ccx_id, ccx.display_name)
     return redirect(url)
 
 

lms/djangoapps/courseware/access.py

@@ -135,9 +135,6 @@ def has_access(user, action, obj, course_key=None):
     if not user:
         user = AnonymousUser()
 
-    if isinstance(course_key, CCXLocator):
-        course_key = course_key.to_course_locator()
-
     if in_preview_mode():
         if not bool(has_staff_access_to_preview_mode(user=user, obj=obj, course_key=course_key)):
             return ACCESS_DENIED