From bf5d2034a161e141e3be48d36a81dd55c4599b66 Mon Sep 17 00:00:00 2001 From: Michael Katz Date: Mon, 22 Feb 2016 15:33:01 -0500 Subject: [PATCH] Revert "Added staff role on ccx to staff of master course and added a data migration for existing ccx courses." This reverts commit 91bf48fc8caa34848d3b4ea96df13c5352a552af. --- cms/djangoapps/contentstore/views/course.py | 13 +-- .../0002_add_master_course_staff_in_ccx.py | 45 ---------- lms/djangoapps/ccx/tests/test_utils.py | 67 +-------------- lms/djangoapps/ccx/tests/test_views.py | 32 +------ lms/djangoapps/ccx/utils.py | 85 +------------------ lms/djangoapps/ccx/views.py | 6 +- lms/djangoapps/courseware/access.py | 3 + 7 files changed, 12 insertions(+), 239 deletions(-) delete mode 100644 lms/djangoapps/ccx/migrations/0002_add_master_course_staff_in_ccx.py diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 6c31418c52..ed2d9e3670 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -27,7 +27,6 @@ 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,11 +389,6 @@ 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, CCXLocator): - return False - # pylint: disable=fixme # TODO remove this condition when templates purged from db if course.location.course == 'templates': @@ -439,11 +433,8 @@ 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 - - # 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 + if course is not None and not isinstance(course, ErrorDescriptor): + # ignore deleted or errored courses courses_list[course_key] = course return courses_list.values(), in_process_course_actions diff --git a/lms/djangoapps/ccx/migrations/0002_add_master_course_staff_in_ccx.py b/lms/djangoapps/ccx/migrations/0002_add_master_course_staff_in_ccx.py deleted file mode 100644 index c55b9c7f31..0000000000 --- a/lms/djangoapps/ccx/migrations/0002_add_master_course_staff_in_ccx.py +++ /dev/null @@ -1,45 +0,0 @@ -# -*- coding: utf-8 -*- -from __future__ import unicode_literals - -from ccx_keys.locator import CCXLocator -from django.db import migrations - -from lms.djangoapps.ccx.models import CustomCourseForEdX -from lms.djangoapps.ccx.utils import ( - add_master_course_staff_to_ccx, - reverse_add_master_course_staff_to_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). - """ - list_ccx = CustomCourseForEdX.objects.all() - for ccx in list_ccx: - ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id)) - add_master_course_staff_to_ccx(ccx.course, 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). - """ - list_ccx = CustomCourseForEdX.objects.all() - for ccx in list_ccx: - ccx_locator = CCXLocator.from_course_locator(ccx.course_id, unicode(ccx.id)) - reverse_add_master_course_staff_to_ccx(ccx.course, ccx_locator, ccx.display_name) - - -class Migration(migrations.Migration): - - dependencies = [ - ('ccx', '0001_initial'), - ] - - operations = [ - migrations.RunPython( - 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 0601a92e0f..837913ab6e 100644 --- a/lms/djangoapps/ccx/tests/test_utils.py +++ b/lms/djangoapps/ccx/tests/test_utils.py @@ -3,30 +3,17 @@ test utils """ from nose.plugins.attrib import attr -from ccx_keys.locator import CCXLocator -from student.roles import ( - CourseCcxCoachRole, - CourseInstructorRole, - CourseStaffRole, -) +from lms.djangoapps.ccx.tests.factories import CcxFactory +from student.roles import CourseCcxCoachRole from student.tests.factories import ( AdminFactory, - CourseEnrollmentFactory, - UserFactory ) from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, - SharedModuleStoreTestCase, TEST_DATA_SPLIT_MODULESTORE) from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.modulestore.django import modulestore -from lms.djangoapps.instructor.access import list_with_level, allow_access - -from lms.djangoapps.ccx.utils import add_master_course_staff_to_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 +from ccx_keys.locator import CCXLocator @attr('shard_1') @@ -60,51 +47,3 @@ class TestGetCCXFromCCXLocator(ModuleStoreTestCase): course_key = CCXLocator.from_course_locator(self.course.id, ccx.id) result = self.call_fut(course_key) self.assertEqual(result, ccx) - - -class TestStaffOnCCX(CcxTestCase, SharedModuleStoreTestCase): - """ - 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 = 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 test_add_master_course_staff_to_ccx(self): - """ - Test add staff of master course to ccx course - """ - self.make_coach() - ccx = self.make_ccx() - ccx_locator = CCXLocator.from_course_locator(self.course.id, ccx.id) - add_master_course_staff_to_ccx(self.course, ccx_locator, 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(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) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index ebbab7540c..921ce16851 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -15,8 +15,6 @@ 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 list_with_level, allow_access - from django.conf import settings from django.core.urlresolvers import reverse, resolve from django.utils.timezone import UTC @@ -25,11 +23,7 @@ from django.test import RequestFactory 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, - CourseInstructorRole, - CourseStaffRole, -) +from student.roles import CourseCcxCoachRole from student.models import ( CourseEnrollment, CourseEnrollmentAllowed, @@ -55,7 +49,6 @@ 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, @@ -143,16 +136,6 @@ 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 @@ -239,19 +222,6 @@ 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) - def test_get_date(self): """ Assert that get_date returns valid date. diff --git a/lms/djangoapps/ccx/utils.py b/lms/djangoapps/ccx/utils.py index 765476c6d8..5a83fd6505 100644 --- a/lms/djangoapps/ccx/utils.py +++ b/lms/djangoapps/ccx/utils.py @@ -18,18 +18,17 @@ 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, list_with_level, revoke_access +from instructor.access import allow_access from instructor.views.tools import get_student_from_identifier from openedx.core.djangoapps.content.course_overviews.models import CourseOverview 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") @@ -261,83 +260,3 @@ def is_email(identifier): except ValidationError: return False return True - - -def add_master_course_staff_to_ccx(master_course, ccx_key, display_name): - """ - 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) - for staff in list_staff: - # 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=True, - email_params=email_params, - ) - - for instructor in list_instructor: - # 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=True, - email_params=email_params, - ) - - -def reverse_add_master_course_staff_to_ccx(master_course, ccx_key, display_name): - """ - 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=True, - 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=True, - email_params=email_params, - ) diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 2a825698b1..9f9b449d6e 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -52,7 +52,6 @@ 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, @@ -147,9 +146,6 @@ 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}) @@ -212,7 +208,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 0a2f1fa646..178568dd8e 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -132,6 +132,9 @@ 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):