diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 9670ee3cf559..43bc4ce7d5f4 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -27,6 +27,7 @@ ) 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, @@ -389,6 +390,11 @@ def course_filter(course): 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, CCXLocator): + return False + # pylint: disable=fixme # TODO remove this condition when templates purged from db if course.location.course == 'templates': @@ -433,8 +439,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 diff --git a/lms/djangoapps/ccx/api/v0/views.py b/lms/djangoapps/ccx/api/v0/views.py index 32eebc51fdaa..1e3d14de4792 100644 --- a/lms/djangoapps/ccx/api/v0/views.py +++ b/lms/djangoapps/ccx/api/v0/views.py @@ -34,6 +34,7 @@ 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 @@ def post(self, request): ) # 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( diff --git a/lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py b/lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py new file mode 100644 index 000000000000..75cc349769da --- /dev/null +++ b/lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py @@ -0,0 +1,161 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from ccx_keys.locator import CCXLocator +from courseware.courses import get_course_by_id +from contextlib import contextmanager +from django.db import migrations + +from lms.djangoapps.instructor.enrollment import ( + enroll_email, + get_email_params, + unenroll_email, +) +from lms.djangoapps.instructor.access import ( + allow_access, + list_with_level, + revoke_access, +) + + +@contextmanager +def ccx_course(ccx_locator): + """Create a context in which the course identified by course_locator exists + """ + course = get_course_by_id(ccx_locator) + yield course + + +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). + """ + 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 + ) + + +def reverse_add_master_course_staff_to_ccx_for_existing_ccx(apps, schema_editor): + """ + Add all staff and admin of master course to respective CCX(s). + """ + 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)) + reverse_add_master_course_staff_to_ccx( + get_course_by_id(ccx.course_id), + ccx_locator + ) + + +def add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_email=False): + """ + Added staff role on ccx to all the staff 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 + """ + 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: + # allow 'staff' access on ccx to staff of master course + allow_access(course_ccx, staff, 'staff') + + # 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, + ) + + for instructor in list_instructor: + # this call should be idempotent + if instructor not in list_instructor_ccx: + # allow 'instructor' access on ccx to instructor of master course + allow_access(course_ccx, instructor, 'instructor') + + # 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, + ) + + +def reverse_add_master_course_staff_to_ccx(master_course, ccx_key, send_email=False): + """ + Remove staff of ccx. + + Arguments: + master_course (CourseDescriptorWithMixins): Master course instance + ccx_key (CCXLocator): CCX course key + display_name (str): ccx display name for email + """ + 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) + for staff in list_staff: + # allow 'staff' access on ccx to staff of master course + revoke_access(course_ccx, staff, 'staff') + + # Enroll the staff in the ccx + unenroll_email( + course_id=ccx_key, + student_email=staff.email, + email_students=send_email, + email_params=email_params, + ) + + for instructor in list_instructor: + # allow 'instructor' access on ccx to instructor of master course + revoke_access(course_ccx, instructor, 'instructor') + + # Enroll the instructor in the ccx + unenroll_email( + course_id=ccx_key, + student_email=instructor.email, + email_students=send_email, + email_params=email_params, + ) + + +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=reverse_add_master_course_staff_to_ccx_for_existing_ccx + ) + ] diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 503e39739d9d..8983eee63c16 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -2,22 +2,41 @@ test utils """ import mock +import uuid from nose.plugins.attrib import attr -from student.roles import CourseCcxCoachRole +from ccx_keys.locator import CCXLocator +from student.roles import ( + CourseCcxCoachRole, + CourseInstructorRole, + CourseStaffRole, +) from student.tests.factories import ( AdminFactory, + CourseEnrollmentFactory, + UserFactory ) from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, - TEST_DATA_SPLIT_MODULESTORE) + SharedModuleStoreTestCase, + TEST_DATA_SPLIT_MODULESTORE +) from xmodule.modulestore.tests.factories import CourseFactory from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import modulestore from lms.djangoapps.ccx import utils +from lms.djangoapps.instructor.access import ( + allow_access, + list_with_level, +) +from lms.djangoapps.ccx.utils import ( + add_master_course_staff_to_ccx, + ccx_course, + reverse_add_master_course_staff_to_ccx +) from lms.djangoapps.ccx.tests.factories import CcxFactory from lms.djangoapps.ccx.tests.utils import CcxTestCase -from ccx_keys.locator import CCXLocator @attr('shard_1') @@ -93,3 +112,135 @@ def test_get_chapters(self): sorted(course_chapters), sorted([unicode(child) for child in self.course.children]) ) + + +class TestStaffOnCCX(CcxTestCase): + """ + Tests for staff on ccx courses. + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + super(TestStaffOnCCX, self).setUp() + + # Create instructor account + self.client.login(username=self.coach.username, password="test") + + # create an instance of modulestore + self.mstore = modulestore() + + # adding staff to master course. + staff = AdminFactory() + 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)) + + self.make_coach() + self.ccx = self.make_ccx() + self.ccx_locator = CCXLocator.from_course_locator(self.course.id, self.ccx.id) + + def test_add_master_course_staff_to_ccx(self): + """ + Test add staff of master course to ccx course + """ + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + + # 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(self.ccx_locator) 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) + + def test_reverse_add_master_course_staff_to_ccx(self): + """ + Test add staff of master course to ccx course + """ + reverse_add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + + # 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(self.ccx_locator) as course_ccx: + list_staff_ccx_course = list_with_level(course_ccx, 'staff') + self.assertNotEqual(len(list_staff_master_course), len(list_staff_ccx_course)) + + list_instructor_ccx_course = list_with_level(course_ccx, 'instructor') + self.assertNotEqual(len(list_instructor_ccx_course), len(list_instructor_master_course)) + + def test_add_master_course_staff_to_ccx_display_name(self): + """ + Test add staff of master course to ccx course. + Specific test to check that a passed display name is in the + subject of the email sent to the enrolled users. + """ + outbox = self.get_outbox() + # create a unique display name + display_name = 'custom_display_{}'.format(uuid.uuid4()) + list_staff_master_course = list_with_level(self.course, 'staff') + list_instructor_master_course = list_with_level(self.course, 'instructor') + self.assertEqual(len(outbox), 0) + # give access to the course staff/instructor + add_master_course_staff_to_ccx(self.course, self.ccx_locator, display_name) + self.assertEqual(len(outbox), len(list_staff_master_course) + len(list_instructor_master_course)) + for email in outbox: + self.assertIn(display_name, email.subject) + + def test_add_master_course_staff_to_ccx_idempotent(self): + """ + Test add staff of master course to ccx course multiple time will + not result in multiple enrollments. + """ + outbox = self.get_outbox() + list_staff_master_course = list_with_level(self.course, 'staff') + list_instructor_master_course = list_with_level(self.course, 'instructor') + self.assertEqual(len(outbox), 0) + + # run the assignment the first time + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + self.assertEqual(len(outbox), len(list_staff_master_course) + len(list_instructor_master_course)) + with ccx_course(self.ccx_locator) 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 user in list_staff_master_course: + self.assertIn(user, list_staff_ccx_course) + self.assertEqual(len(list_instructor_master_course), len(list_instructor_ccx_course)) + for user in list_instructor_master_course: + self.assertIn(user, list_instructor_ccx_course) + + # run the assignment again + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + # there are no new duplicated email + self.assertEqual(len(outbox), len(list_staff_master_course) + len(list_instructor_master_course)) + # there are no duplicated staffs + with ccx_course(self.ccx_locator) 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 user in list_staff_master_course: + self.assertIn(user, list_staff_ccx_course) + self.assertEqual(len(list_instructor_master_course), len(list_instructor_ccx_course)) + for user in list_instructor_master_course: + self.assertIn(user, list_instructor_ccx_course) + + def test_add_master_course_staff_to_ccx_no_email(self): + """ + Test add staff of master course to ccx course without + sending enrollment email. + """ + outbox = self.get_outbox() + self.assertEqual(len(outbox), 0) + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) + self.assertEqual(len(outbox), 0) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index e40e9df734c7..57d56af57593 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -15,6 +15,8 @@ from courseware.tests.factories import StudentModuleFactory from courseware.tests.helpers import LoginEnrollmentTestCase from courseware.tabs import get_course_tab_list +from instructor.access import list_with_level, allow_access + from django.conf import settings from django.core.urlresolvers import reverse, resolve from django.utils.timezone import UTC @@ -23,7 +25,11 @@ from edxmako.shortcuts import render_to_response from request_cache.middleware import RequestCache from opaque_keys.edx.keys import CourseKey -from student.roles import CourseCcxCoachRole +from student.roles import ( + CourseCcxCoachRole, + CourseInstructorRole, + CourseStaffRole, +) from student.models import ( CourseEnrollment, CourseEnrollmentAllowed, @@ -48,6 +54,7 @@ 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, @@ -135,6 +142,16 @@ def setUp(self): # 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 @@ -221,6 +238,19 @@ def test_create_ccx(self, ccx_name='New CCX'): role = CourseCcxCoachRole(course_key) self.assertTrue(role.has_user(self.coach, refresh=True)) + # 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) diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 93e394674d0c..713b545bc562 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -18,18 +18,23 @@ 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.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") @@ -311,3 +316,88 @@ 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): + """ + Added staff role on ccx to all the staff 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 + """ + 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: + # allow 'staff' access on ccx to staff of master course + allow_access(course_ccx, staff, 'staff') + + # 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, + ) + + for instructor in list_instructor: + # this call should be idempotent + if instructor not in list_instructor_ccx: + # allow 'instructor' access on ccx to instructor of master course + allow_access(course_ccx, instructor, 'instructor') + + # 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, + ) + + +def reverse_add_master_course_staff_to_ccx(master_course, ccx_key, display_name, send_email=True): + """ + Remove staff of ccx. + + Arguments: + master_course (CourseDescriptorWithMixins): Master course instance + ccx_key (CCXLocator): CCX course key + display_name (str): ccx display name for email + """ + 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) + for staff in list_staff: + # allow 'staff' access on ccx to staff of master course + revoke_access(course_ccx, staff, 'staff') + + # Enroll the staff in the ccx + unenroll_email( + course_id=ccx_key, + student_email=staff.email, + email_students=send_email, + email_params=email_params, + ) + + for instructor in list_instructor: + # allow 'instructor' access on ccx to instructor of master course + revoke_access(course_ccx, instructor, 'instructor') + + # Enroll the instructor in the ccx + unenroll_email( + course_id=ccx_key, + student_email=instructor.email, + email_students=send_email, + email_params=email_params, + ) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 5b9aae0b6f3d..97ec3934cb36 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -52,6 +52,7 @@ 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, @@ -147,6 +148,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}) @@ -209,7 +213,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) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 178568dd8eda..0a2f1fa6468e 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -132,9 +132,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() - # delegate the work to type-specific functions. # (start with more specific types, then get more general) if isinstance(obj, CourseDescriptor):