Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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':
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# -*- 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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: did you test that using ccx.course actually works?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes @giocalitri , I tested it by re-runing migration.

- python manage.py lms migrate ccx 0001  --fake --settings=devstack 
- python manage.py lms migrate ccx 0002 --settings=devstack 

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the migration be reversed?

I'm just remembering an unfortunate PR that we had to revert and it was further complicated because we couldn't revert the migrations.

@bdero may have some advice.

  • Peter



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
)
]
67 changes: 64 additions & 3 deletions lms/djangoapps/ccx/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,30 @@
"""
from nose.plugins.attrib import attr

from lms.djangoapps.ccx.tests.factories import CcxFactory
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,
SharedModuleStoreTestCase,
TEST_DATA_SPLIT_MODULESTORE)
from xmodule.modulestore.tests.factories import CourseFactory
from xmodule.modulestore.django import modulestore

from ccx_keys.locator import CCXLocator
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


@attr('shard_1')
Expand Down Expand Up @@ -47,3 +60,51 @@ def test_ccx_locator(self):
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)
32 changes: 31 additions & 1 deletion lms/djangoapps/ccx/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand All @@ -49,6 +55,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,
Expand Down Expand Up @@ -136,6 +143,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
Expand Down Expand Up @@ -222,6 +239,19 @@ def test_create_ccx(self):
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.
Expand Down
85 changes: 83 additions & 2 deletions lms/djangoapps/ccx/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,18 @@
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 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")

Expand Down Expand Up @@ -260,3 +261,83 @@ 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,
)
6 changes: 5 additions & 1 deletion lms/djangoapps/ccx/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -146,6 +147,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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code adds 'course' to the context - but I don't see any accompanying changes to the template, nor any reference in the existing template? Why was this attribute added to the context?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I see it now.

This view uses the passed-in course to find the CCX - and then uses the found CCX's locator to retrieve the course again. Why not just use the passed-in course and skip the ccx_course() call?


else:
context['create_ccx_url'] = reverse(
'create_ccx', kwargs={'course_id': course.id})
Expand Down Expand Up @@ -208,7 +212,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)


Expand Down
3 changes: 0 additions & 3 deletions lms/djangoapps/courseware/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the net effect of not converting CCXLocators to CourseLocators here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@doctoryes in courseware app the masquerading code which we use for viewing courseware content as staff, student , specific student, it use same cache which it stores against course_key/ccx key. So when we use this line course_key = course_key.to_course_locator() it returns master course key instead of ccx key but masquerade code needs ccx (for viewing ccx courseware as staff, student or specific student)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense - so this change is a bug fix.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,

Expand Down