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
4 changes: 4 additions & 0 deletions lms/djangoapps/ccx/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,10 @@ def test_create_ccx(self):
course_enrollments = get_override_for_ccx(ccx, self.course, 'max_student_enrollments_allowed')
self.assertEqual(course_enrollments, settings.CCX_MAX_STUDENTS_ALLOWED)

# assert ccx creator has role=ccx_coach
role = CourseCcxCoachRole(course_key)
self.assertTrue(role.has_user(self.coach))

@SharedModuleStoreTestCase.modifies_courseware
@patch('ccx.views.render_to_response', intercept_renderer)
@patch('ccx.views.TODAY')
Expand Down
29 changes: 28 additions & 1 deletion lms/djangoapps/ccx/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
from student.roles import CourseCcxCoachRole
from student.models import CourseEnrollment

from instructor.access import allow_access
from instructor.views.api import _split_input_list
from instructor.views.gradebook_api import get_grade_book_page
from instructor.views.tools import get_student_from_identifier
Expand Down Expand Up @@ -132,7 +133,10 @@ def dashboard(request, course, ccx=None):
}

if ccx:
ccx_locator = CCXLocator.from_course_locator(course.id, ccx.id)
ccx_locator = CCXLocator.from_course_locator(course.id, unicode(ccx.id))

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 we address this upstream in https://github.com/edx/ccx-keys ?

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.

@pdpinch
yeah we can but then we need to hold this PR. it depends on this fix

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.

Ok. Leave it as is, but let's open a PR on ccx-keys too.

# At this point we are done with verification that current user is ccx coach.
assign_coach_role_to_ccx(ccx_locator, request.user, course.id)

schedule = get_ccx_schedule(course, ccx)
grading_policy = get_override_for_ccx(
ccx, course, 'grading_policy', course.grading_policy)
Expand All @@ -147,6 +151,7 @@ def dashboard(request, course, ccx=None):
context['grading_policy'] = json.dumps(grading_policy, indent=4)
context['grading_policy_url'] = reverse(
'ccx_set_grading_policy', kwargs={'course_id': ccx_locator})

else:
context['create_ccx_url'] = reverse(
'create_ccx', kwargs={'course_id': course.id})
Expand Down Expand Up @@ -208,9 +213,31 @@ def create_ccx(request, course, ccx=None):
email_params=email_params,
)

assign_coach_role_to_ccx(ccx_id, request.user, course.id)

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.

Why do we need this in addition to L138?

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.

@pdpinch It is for the user who already have created ccx.
if user dont have coach right on his ccx but he has coach rights on master course then when he opens his dashboard from master course there (L138) I assign him coach role on ccx if the role dont exist.

In sum this is for existing ccx course, to show coach dashboard tab


return redirect(url)


def assign_coach_role_to_ccx(ccx_locator, user, master_course_id):
"""
Check if user has ccx_coach role on master course then assign him coach role on ccx only
if role is not already assigned. Because of this coach can open dashboard from master course
as well as ccx.
:param ccx_locator: CCX key
:param user: User to whom we want to assign role.
:param master_course_id: Master course key
"""
coach_role_on_master_course = CourseCcxCoachRole(master_course_id)
# check if user has coach role on master course
if coach_role_on_master_course.has_user(user):
# Check if user has coach role on ccx.
role = CourseCcxCoachRole(ccx_locator)
if not role.has_user(user):
# assign user role coach on ccx
with ccx_course(ccx_locator) as course:
allow_access(course, user, "ccx_coach", send_email=False)


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.

extra line

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.

this is ok @pdpinch because this is a global method. And we put two spaces before and after a global method or vaiable

@ensure_csrf_cookie
@cache_control(no_cache=True, no_store=True, must_revalidate=True)
@coach_dashboard
Expand Down
12 changes: 6 additions & 6 deletions lms/djangoapps/instructor/access.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,25 @@ def list_with_level(course, level):
return ROLES[level](course.id).users_with_role()


def allow_access(course, user, level):
def allow_access(course, user, level, send_email=True):
"""
Allow user access to course modification.

`level` is one of ['instructor', 'staff', 'beta']
"""
_change_access(course, user, level, 'allow')
_change_access(course, user, level, 'allow', send_email)


def revoke_access(course, user, level):
def revoke_access(course, user, level, send_email=True):
"""
Revoke access from user to course modification.

`level` is one of ['instructor', 'staff', 'beta']
"""
_change_access(course, user, level, 'revoke')
_change_access(course, user, level, 'revoke', send_email)


def _change_access(course, user, level, action):
def _change_access(course, user, level, action, send_email=True):
"""
Change access of user.

Expand All @@ -85,7 +85,7 @@ def _change_access(course, user, level, action):
course_id=course.id,
student_email=user.email,
auto_enroll=True,
email_students=True,
email_students=send_email,
email_params=email_params,
)
role.add_users(user)
Expand Down