From 795ead82b5877e78512970f88d9b7fb6f02a06e3 Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Tue, 23 Feb 2016 15:31:28 +0500 Subject: [PATCH] Added master course staff and admins to ccx and fixed same issues related to ccx --- .../contentstore/tests/test_course_listing.py | 35 ++ cms/djangoapps/contentstore/views/course.py | 13 +- lms/djangoapps/ccx/api/v0/tests/test_views.py | 53 ++- lms/djangoapps/ccx/api/v0/views.py | 8 + .../0003_add_master_course_staff_in_ccx.py | 74 ++++ lms/djangoapps/ccx/tests/test_utils.py | 332 +++++++++++++++++- lms/djangoapps/ccx/tests/test_views.py | 31 +- lms/djangoapps/ccx/tests/utils.py | 30 +- lms/djangoapps/ccx/utils.py | 126 ++++++- lms/djangoapps/ccx/views.py | 6 +- lms/djangoapps/courseware/access.py | 3 - 11 files changed, 691 insertions(+), 20 deletions(-) create mode 100644 lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py diff --git a/cms/djangoapps/contentstore/tests/test_course_listing.py b/cms/djangoapps/contentstore/tests/test_course_listing.py index 5800be50b1..e2da5f83e3 100644 --- a/cms/djangoapps/contentstore/tests/test_course_listing.py +++ b/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') diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 00cf07215f..6ed88035c1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/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 diff --git a/lms/djangoapps/ccx/api/v0/tests/test_views.py b/lms/djangoapps/ccx/api/v0/tests/test_views.py index d308de985d..43d7e9b03a 100644 --- a/lms/djangoapps/ccx/api/v0/tests/test_views.py +++ b/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 diff --git a/lms/djangoapps/ccx/api/v0/views.py b/lms/djangoapps/ccx/api/v0/views.py index 32eebc51fd..1e3d14de47 100644 --- a/lms/djangoapps/ccx/api/v0/views.py +++ b/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( 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 0000000000..a9deb47387 --- /dev/null +++ b/lms/djangoapps/ccx/migrations/0003_add_master_course_staff_in_ccx.py @@ -0,0 +1,74 @@ +# -*- 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 + ) + ] diff --git a/lms/djangoapps/ccx/tests/test_utils.py b/lms/djangoapps/ccx/tests/test_utils.py index 503e39739d..edefacec79 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -2,22 +2,44 @@ test utils """ import mock +import uuid from nose.plugins.attrib import attr +from smtplib import SMTPException -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 student.models import CourseEnrollment, CourseEnrollmentException + 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 ( + list_with_level, +) +from lms.djangoapps.ccx.utils import ( + add_master_course_staff_to_ccx, + ccx_course, + remove_master_course_staff_from_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 +115,307 @@ class TestGetCourseChapters(CcxTestCase): 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() + + 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 + """ + # adding staff to master course. + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + + 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_add_master_course_staff_to_ccx_with_exception(self): + """ + When exception raise from ``enroll_email`` assert that enrollment skipped for that staff or + instructor. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + + with mock.patch.object(CourseEnrollment, 'enroll_by_email', side_effect=CourseEnrollmentException()): + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + + self.assertFalse( + CourseEnrollment.objects.filter(course_id=self.ccx_locator, user=staff).exists() + ) + self.assertFalse( + CourseEnrollment.objects.filter(course_id=self.ccx_locator, user=instructor).exists() + ) + + with mock.patch.object(CourseEnrollment, 'enroll_by_email', side_effect=SMTPException()): + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name) + + self.assertFalse( + CourseEnrollment.objects.filter(course_id=self.ccx_locator, user=staff).exists() + ) + self.assertFalse( + CourseEnrollment.objects.filter(course_id=self.ccx_locator, user=instructor).exists() + ) + + def test_remove_master_course_staff_from_ccx(self): + """ + Test remove staff of master course to ccx course + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) + + 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) + + # assert that role of staff and instructors of master course removed from ccx. + remove_master_course_staff_from_ccx( + self.course, self.ccx_locator, self.ccx.display_name, send_email=False + ) + 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)) + + for user in list_staff_master_course: + self.assertNotIn(user, list_staff_ccx_course) + for user in list_instructor_master_course: + self.assertNotIn(user, list_instructor_ccx_course) + + def test_remove_master_course_staff_from_ccx_idempotent(self): + """ + Test remove staff of master course from ccx course + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + + 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) + + 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) + + # assert that role of staff and instructors of master course removed from ccx. + remove_master_course_staff_from_ccx( + self.course, self.ccx_locator, self.ccx.display_name, send_email=True + ) + self.assertEqual(len(outbox), len(list_staff_master_course) + len(list_instructor_master_course)) + + 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)) + + for user in list_staff_master_course: + self.assertNotIn(user, list_staff_ccx_course) + for user in list_instructor_master_course: + self.assertNotIn(user, list_instructor_ccx_course) + + # Run again + remove_master_course_staff_from_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') + 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)) + + for user in list_staff_master_course: + self.assertNotIn(user, list_staff_ccx_course) + for user in list_instructor_master_course: + self.assertNotIn(user, list_instructor_ccx_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. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + 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_remove_master_course_staff_from_ccx_display_name(self): + """ + Test remove role of staff of master course on ccx course. + Specific test to check that a passed display name is in the + subject of the email sent to the unenrolled users. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + outbox = self.get_outbox() + add_master_course_staff_to_ccx(self.course, self.ccx_locator, self.ccx.display_name, send_email=False) + # 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 + remove_master_course_staff_from_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. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + 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. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + 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) + + def test_remove_master_course_staff_from_ccx_no_email(self): + """ + Test remove role of staff of master course on ccx course without + sending enrollment email. + """ + staff = self.make_staff() + self.assertTrue(CourseStaffRole(self.course.id).has_user(staff)) + + # adding instructor to master course. + instructor = self.make_instructor() + self.assertTrue(CourseInstructorRole(self.course.id).has_user(instructor)) + outbox = self.get_outbox() + self.assertEqual(len(outbox), 0) + remove_master_course_staff_from_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 44b9bc64e2..637e1b4695 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/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) diff --git a/lms/djangoapps/ccx/tests/utils.py b/lms/djangoapps/ccx/tests/utils.py index 2cfd20d1d3..9962bddba6 100644 --- a/lms/djangoapps/ccx/tests/utils.py +++ b/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 diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index a3eb2e4c85..8f79b99fef 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/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, + ) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index bc24349a64..44ad0ed05b 100644 --- a/lms/djangoapps/ccx/views.py +++ b/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) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 98306e7a60..178a6fd21b 100644 --- a/lms/djangoapps/courseware/access.py +++ b/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