From c0fc8669baf70c3d6e2f0e899b4645c7ffac8b84 Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Mon, 27 Jun 2016 18:32:10 +0500 Subject: [PATCH] Only coach can access ccx dashboard --- lms/djangoapps/ccx/plugins.py | 5 +- lms/djangoapps/ccx/tests/test_views.py | 103 +++++++++++++++++++------ lms/djangoapps/ccx/views.py | 33 ++++---- 3 files changed, 99 insertions(+), 42 deletions(-) diff --git a/lms/djangoapps/ccx/plugins.py b/lms/djangoapps/ccx/plugins.py index c081f50c50..44c42fb123 100644 --- a/lms/djangoapps/ccx/plugins.py +++ b/lms/djangoapps/ccx/plugins.py @@ -28,9 +28,12 @@ class CcxCourseTab(CourseTab): if not settings.FEATURES.get('CUSTOM_COURSES_EDX', False) or not course.enable_ccx: # If ccx is not enable do not show ccx coach tab. return False - if has_access(user, 'staff', course) or has_access(user, 'instructor', course): + + is_staff_or_instructor = has_access(user, 'staff', course) or has_access(user, 'instructor', course) + if hasattr(course.id, 'ccx') and is_staff_or_instructor: # if user is staff or instructor then he can always see ccx coach tab. return True + # check if user has coach access. role = CourseCcxCoachRole(course.id) return role.has_user(user) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index e4fce7b45d..92c2634514 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -60,13 +60,15 @@ 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, flatten, ) -from lms.djangoapps.ccx.utils import is_email +from lms.djangoapps.ccx.utils import ( + ccx_course, + is_email +) from lms.djangoapps.ccx.views import get_date from xmodule.modulestore.django import modulestore @@ -137,18 +139,21 @@ class TestAdminAccessCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + def setUp(self): + super(TestAdminAccessCoachDashboard, self).setUp() + self.make_coach() + ccx = self.make_ccx() + ccx_key = CCXLocator.from_course_locator(self.course.id, ccx.id) + self.url = reverse('ccx_coach_dashboard', kwargs={'course_id': ccx_key}) + def test_staff_access_coach_dashboard(self): """ User is staff, should access coach dashboard. """ staff = self.make_staff() self.client.login(username=staff.username, password="test") - self.make_coach() - ccx = self.make_ccx() - url = reverse( - 'ccx_coach_dashboard', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) - response = self.client.get(url) + + response = self.client.get(self.url) self.assertEqual(response.status_code, 200) def test_instructor_access_coach_dashboard(self): @@ -157,12 +162,9 @@ class TestAdminAccessCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): """ instructor = self.make_instructor() self.client.login(username=instructor.username, password="test") - self.make_coach() - ccx = self.make_ccx() - url = reverse( - 'ccx_coach_dashboard', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) - response = self.client.get(url) + + # Now access URL + response = self.client.get(self.url) self.assertEqual(response.status_code, 200) def test_forbidden_user_access_coach_dashboard(self): @@ -171,12 +173,7 @@ class TestAdminAccessCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): """ user = UserFactory.create(password="test") self.client.login(username=user.username, password="test") - self.make_coach() - ccx = self.make_ccx() - url = reverse( - 'ccx_coach_dashboard', - kwargs={'course_id': CCXLocator.from_course_locator(self.course.id, ccx.id)}) - response = self.client.get(url) + response = self.client.get(self.url) self.assertEqual(response.status_code, 403) @@ -1096,7 +1093,7 @@ class TestCCXGrades(FieldOverrideTestMixin, SharedModuleStoreTestCase, LoginEnro @ddt.ddt -class CCXCoachTabTestCase(SharedModuleStoreTestCase): +class CCXCoachTabTestCase(CcxTestCase): """ Test case for CCX coach tab. """ @@ -1114,10 +1111,10 @@ class CCXCoachTabTestCase(SharedModuleStoreTestCase): role = CourseCcxCoachRole(course.id) role.add_users(self.user) - def check_ccx_tab(self, course): + def check_ccx_tab(self, course, user): """Helper function for verifying the ccx tab.""" request = RequestFactory().request() - request.user = self.user + request.user = user all_tabs = get_course_tab_list(request, course) return any(tab.type == 'ccx_coach' for tab in all_tabs) @@ -1137,9 +1134,67 @@ class CCXCoachTabTestCase(SharedModuleStoreTestCase): course = self.ccx_enabled_course if enable_ccx else self.ccx_disabled_course self.assertEquals( expected_result, - self.check_ccx_tab(course) + self.check_ccx_tab(course, self.user) ) + def test_ccx_tab_visibility_for_staff_when_not_coach_master_course(self): + """ + Staff cannot view ccx coach dashboard on master course by default. + """ + staff = self.make_staff() + self.assertFalse(self.check_ccx_tab(self.course, staff)) + + def test_ccx_tab_visibility_for_staff_when_coach_master_course(self): + """ + Staff can view ccx coach dashboard only if he is coach on master course. + """ + staff = self.make_staff() + role = CourseCcxCoachRole(self.course.id) + role.add_users(staff) + self.assertTrue(self.check_ccx_tab(self.course, staff)) + + def test_ccx_tab_visibility_for_staff_ccx_course(self): + """ + Staff can access coach dashboard on ccx course. + """ + staff = self.make_staff() + self.make_coach() + ccx = self.make_ccx() + ccx_key = CCXLocator.from_course_locator(self.course.id, unicode(ccx.id)) + + with ccx_course(ccx_key) as course_ccx: + allow_access(course_ccx, staff, 'staff') + self.assertTrue(self.check_ccx_tab(course_ccx, staff)) + + def test_ccx_tab_visibility_for_instructor_when_not_coach_master_course(self): + """ + Instructor cannot view ccx coach dashboard on master course by default. + """ + instructor = self.make_instructor() + self.assertFalse(self.check_ccx_tab(self.course, instructor)) + + def test_ccx_tab_visibility_for_instructor_when_coach_master_course(self): + """ + Instructor can view ccx coach dashboard only if he is coach on master course. + """ + instructor = self.make_instructor() + role = CourseCcxCoachRole(self.course.id) + role.add_users(instructor) + self.assertTrue(self.check_ccx_tab(self.course, instructor)) + + def test_ccx_tab_visibility_for_instructor_ccx_course(self): + """ + Instructor can access coach dashboard on ccx course. + """ + instructor = self.make_instructor() + self.make_coach() + ccx = self.make_ccx() + ccx_key = CCXLocator.from_course_locator(self.course.id, unicode(ccx.id)) + + with ccx_course(ccx_key) as course_ccx: + allow_access(course_ccx, instructor, 'instructor') + self.assertTrue(self.check_ccx_tab(course_ccx, instructor)) + class TestStudentViewsWithCCX(ModuleStoreTestCase): """ diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index 3cee41dc10..4eec633e9a 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -93,26 +93,27 @@ def coach_dashboard(view): if ccx: course_key = ccx.course_id course = get_course_by_id(course_key, depth=None) - is_staff = has_access(request.user, 'staff', course) - is_instructor = has_access(request.user, 'instructor', course) if not course.enable_ccx: raise Http404 - elif is_staff or is_instructor: - # if user is staff or instructor then he can view ccx coach dashboard. - return view(request, course, ccx) else: - role = CourseCcxCoachRole(course_key) - if not role.has_user(request.user): - return HttpResponseForbidden(_('You must be a CCX Coach to access this view.')) + is_staff = has_access(request.user, 'staff', course) + is_instructor = has_access(request.user, 'instructor', course) - # if there is a ccx, we must validate that it is the ccx for this coach - if ccx is not None: - coach_ccx = get_ccx_by_ccx_id(course, request.user, ccx.id) - if coach_ccx is None: - return HttpResponseForbidden( - _('You must be the coach for this ccx to access this view') - ) + if is_staff or is_instructor: + # if user is staff or instructor then he can view ccx coach dashboard. + return view(request, course, ccx) + else: + # if there is a ccx, we must validate that it is the ccx for this coach + role = CourseCcxCoachRole(course_key) + if not role.has_user(request.user): + return HttpResponseForbidden(_('You must be a CCX Coach to access this view.')) + elif ccx is not None: + coach_ccx = get_ccx_by_ccx_id(course, request.user, ccx.id) + if coach_ccx is None: + return HttpResponseForbidden( + _('You must be the coach for this ccx to access this view') + ) return view(request, course, ccx) return wrapper @@ -144,8 +145,6 @@ def dashboard(request, course, ccx=None): if ccx: ccx_locator = CCXLocator.from_course_locator(course.id, unicode(ccx.id)) - # 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(