From 00a42fe12138e19ef4feca844f87d547b1cb81ba Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Thu, 3 Nov 2016 14:59:39 +0200 Subject: [PATCH 01/32] show button new library in studio depending on flags and user staff status --- cms/djangoapps/contentstore/views/course.py | 24 +++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 70686f0353..1ee0ec20c1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -467,7 +467,7 @@ def course_listing(request): List all courses available to the logged in user """ courses, in_process_course_actions = get_courses_accessible_to_user(request) - libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED else [] + libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and request.user.is_staff else [] programs_config = ProgramsApiConfig.current() raw_programs = get_programs(request.user) if programs_config.is_studio_tab_enabled else [] @@ -518,7 +518,7 @@ def course_listing(request): 'in_process_course_actions': in_process_course_actions, 'libraries_enabled': LIBRARIES_ENABLED, 'libraries': [format_library_for_view(lib) for lib in libraries], - 'show_new_library_button': LIBRARIES_ENABLED and request.user.is_active, + 'show_new_library_button': _get_library_creation_status(request.user), 'user': request.user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user), @@ -1631,6 +1631,7 @@ def _get_course_creator_status(user): If the user passed in has not previously visited the index page, it will be added with status 'unrequested' if the course creator group is in use. """ + if user.is_staff: course_creator_status = 'granted' elif settings.FEATURES.get('DISABLE_COURSE_CREATION', False): @@ -1646,3 +1647,22 @@ def _get_course_creator_status(user): course_creator_status = 'granted' return course_creator_status + + +def _get_library_creation_status(user): + """ + Helper method for returning the library creation status for a particular user, + taking into account the values of DISABLE_LIBRARY_CREATION and LIBRARIES_ENABLED. + + """ + + if LIBRARIES_ENABLED: + if user.is_active and user.is_staff: + if not settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): + return True + else: + return False + else: + return False + else: + return False From 4ac3cae3fe452869dd4c26ec86f330c539741644 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Thu, 3 Nov 2016 15:06:58 +0200 Subject: [PATCH 02/32] add flag DISABLE_LIBRARY_CREATION --- cms/envs/common.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cms/envs/common.py b/cms/envs/common.py index 89d190a0c6..75971f7803 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -165,6 +165,7 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': True, + 'DISABLE_LIBRARY_CREATION': False # Milestones application flag 'MILESTONES_APP': False, From 2de535ba6ccb01b1e6d175c3721b6525e93773ab Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Thu, 3 Nov 2016 15:29:43 +0200 Subject: [PATCH 03/32] add comma --- cms/envs/common.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 75971f7803..36f0fe26a0 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -165,7 +165,7 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': True, - 'DISABLE_LIBRARY_CREATION': False + 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag 'MILESTONES_APP': False, From 633eefb221f5bc5f130eb562447f2fb668f81419 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Tue, 8 Nov 2016 10:14:07 +0200 Subject: [PATCH 04/32] use CourseCreatorRole to determine if user can create a library --- cms/djangoapps/contentstore/views/course.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 1ee0ec20c1..21c47335ff 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -467,7 +467,10 @@ def course_listing(request): List all courses available to the logged in user """ courses, in_process_course_actions = get_courses_accessible_to_user(request) - libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and request.user.is_staff else [] + user = request.user + user_has_permission =\ + user.is_active and (user.is_staff or CourseCreatorRole().has_user(user)) + libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and user_has_permission else [] programs_config = ProgramsApiConfig.current() raw_programs = get_programs(request.user) if programs_config.is_studio_tab_enabled else [] @@ -518,7 +521,7 @@ def course_listing(request): 'in_process_course_actions': in_process_course_actions, 'libraries_enabled': LIBRARIES_ENABLED, 'libraries': [format_library_for_view(lib) for lib in libraries], - 'show_new_library_button': _get_library_creation_status(request.user), + 'show_new_library_button': _get_library_creation_status(user), 'user': request.user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user), @@ -1657,9 +1660,12 @@ def _get_library_creation_status(user): """ if LIBRARIES_ENABLED: - if user.is_active and user.is_staff: - if not settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): - return True + if user.is_active: + if user.is_staff or CourseCreatorRole().has_user(user): + if not settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): + return True + else: + return False else: return False else: From ab49631b255a3e4dab76f1617d5a4763d385446e Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Thu, 3 Nov 2016 14:59:39 +0200 Subject: [PATCH 05/32] add disable library creation feature flag --- cms/djangoapps/contentstore/views/course.py | 35 ++++++++++++++++++--- cms/envs/common.py | 1 + cms/templates/index.html | 6 ++-- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 70686f0353..d49ae2f6c7 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -467,7 +467,10 @@ def course_listing(request): List all courses available to the logged in user """ courses, in_process_course_actions = get_courses_accessible_to_user(request) - libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED else [] + user = request.user + user_has_permission =\ + user.is_active and (user.is_staff or CourseCreatorRole().has_user(user)) + libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and user_has_permission else [] programs_config = ProgramsApiConfig.current() raw_programs = get_programs(request.user) if programs_config.is_studio_tab_enabled else [] @@ -516,18 +519,18 @@ def course_listing(request): return render_to_response('index.html', { 'courses': courses, 'in_process_course_actions': in_process_course_actions, - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [format_library_for_view(lib) for lib in libraries], - 'show_new_library_button': LIBRARIES_ENABLED and request.user.is_active, 'user': request.user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user), 'rerun_creator_status': GlobalStaff().has_user(request.user), 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), + 'libraries_enabled': LIBRARIES_ENABLED, + 'libraries': [format_library_for_view(lib) for lib in libraries], + 'library_creator_status': _get_library_creator_status(user), 'is_programs_enabled': programs_config.is_studio_tab_enabled and request.user.is_staff, 'programs': programs, - 'program_authoring_url': reverse('programs'), + 'program_authoring_url': reverse('programs') }) @@ -1646,3 +1649,25 @@ def _get_course_creator_status(user): course_creator_status = 'granted' return course_creator_status + + +def _get_library_creator_status(user): + """ + Helper method for returning the library creation status for a particular user, + taking into account the values of DISABLE_LIBRARY_CREATION and LIBRARIES_ENABLED. + """ + if user.is_staff: + library_creator_status = 'granted' + elif settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): + library_creator_status = 'disallowed_for_this_site' + elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + library_creator_status = get_course_creator_status(user) + if library_creator_status is None: + # User not grandfathered in as an existing user, has not previously visited the dashboard page. + # Add the user to the course creator admin table with status 'unrequested'. + add_user_with_status_unrequested(user) + library_creator_status = get_course_creator_status(user) + else: + library_creator_status = 'granted' + + return library_creator_status diff --git a/cms/envs/common.py b/cms/envs/common.py index 11c0842b8d..3c94ee9ed1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -168,6 +168,7 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': True, + 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag 'MILESTONES_APP': False, diff --git a/cms/templates/index.html b/cms/templates/index.html index daf518d8a6..ad652dd5e3 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -34,7 +34,7 @@ from openedx.core.djangolib.markup import HTML, Text ${_("Email staff to create course")} % endif - % if show_new_library_button: + % if library_creator_status=='granted': ${_("New Library")} % endif @@ -130,7 +130,7 @@ from openedx.core.djangolib.markup import HTML, Text % endif - %if libraries_enabled and show_new_library_button: + %if libraries_enabled and library_creator_status=='granted':
@@ -497,7 +497,7 @@ from openedx.core.djangolib.markup import HTML, Text
- % if show_new_library_button: + % if library_creator_status=='granted':

${_('Create Your First Library')}

From fe5becb4fe42a0b47ac1a087b7533d6372a67700 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Fri, 18 Nov 2016 13:15:39 +0200 Subject: [PATCH 06/32] ENABLE_CONTENT_LIBRARIES flag --- cms/djangoapps/contentstore/views/course.py | 4 ++-- cms/envs/common.py | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 846b5c024e..21a63c164d 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -524,8 +524,8 @@ def course_listing(request): 'show_new_library_button': _get_library_creation_status(user), 'user': request.user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), - 'course_creator_status': _get_course_creator_status(request.user), - 'rerun_creator_status': GlobalStaff().has_user(request.user), + 'course_creator_status': _get_course_creator_status(user), + 'rerun_creator_status': GlobalStaff().has_user(user), 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), 'is_programs_enabled': programs_config.is_studio_tab_enabled and request.user.is_staff, diff --git a/cms/envs/common.py b/cms/envs/common.py index 3c94ee9ed1..364bc5299d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -167,7 +167,6 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. - 'ENABLE_CONTENT_LIBRARIES': True, 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag From 9cf08261b9d8695af66d1e040a516046efacbff0 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Fri, 18 Nov 2016 14:16:57 +0200 Subject: [PATCH 07/32] check for course creator role for library creation --- cms/djangoapps/contentstore/views/course.py | 39 ++++++++++----------- cms/envs/common.py | 1 + 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 21a63c164d..93b108f4e1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -468,9 +468,9 @@ def course_listing(request): """ courses, in_process_course_actions = get_courses_accessible_to_user(request) user = request.user - user_has_permission =\ + user_can_see_libraries =\ user.is_active and (user.is_staff or CourseCreatorRole().has_user(user)) - libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and user_has_permission else [] + libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED and user_can_see_libraries else [] programs_config = ProgramsApiConfig.current() raw_programs = get_programs(request.user) if programs_config.is_studio_tab_enabled else [] @@ -519,16 +519,16 @@ def course_listing(request): return render_to_response('index.html', { 'courses': courses, 'in_process_course_actions': in_process_course_actions, - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [format_library_for_view(lib) for lib in libraries], - 'show_new_library_button': _get_library_creation_status(user), - 'user': request.user, + 'user': user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(user), 'rerun_creator_status': GlobalStaff().has_user(user), 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), - 'is_programs_enabled': programs_config.is_studio_tab_enabled and request.user.is_staff, + 'libraries_enabled': LIBRARIES_ENABLED, + 'libraries': [format_library_for_view(lib) for lib in libraries], + 'library_creator_status': _get_library_creator_status(user), + 'is_programs_enabled': programs_config.is_studio_tab_enabled and user.is_staff, 'programs': programs, 'program_authoring_url': reverse('programs') }) @@ -1652,23 +1652,22 @@ def _get_course_creator_status(user): return course_creator_status -def _get_library_creation_status(user): +def _get_library_creator_status(user): """ Helper method for returning the library creation status for a particular user, taking into account the values of DISABLE_LIBRARY_CREATION and LIBRARIES_ENABLED. """ - if LIBRARIES_ENABLED: - if user.is_active: - if user.is_staff or CourseCreatorRole().has_user(user): - if not settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): - return True - else: - return False - else: - return False - else: - return False + if not LIBRARIES_ENABLED: + library_creator_status = 'disallowed_for_this_site' + elif user.is_staff: + library_creator_status = 'granted' + elif CourseCreatorRole().has_user(user): + library_creator_status = 'granted' + elif settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): + library_creator_status = 'disallowed_for_this_site' else: - return False + library_creator_status = 'disallowed_for_this_site' + + return library_creator_status diff --git a/cms/envs/common.py b/cms/envs/common.py index 364bc5299d..3c94ee9ed1 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -167,6 +167,7 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. + 'ENABLE_CONTENT_LIBRARIES': True, 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag From fe2c92e6c426445662b1b212fb1ad2fd112880f9 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Wed, 23 Nov 2016 11:13:12 +0200 Subject: [PATCH 08/32] add unit tests --- cms/djangoapps/contentstore/tests/utils.py | 12 +++++++ .../views/tests/test_course_index.py | 34 ++++++++++++++++++- .../xmodule/modulestore/tests/django_utils.py | 16 +++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index af5e7b8e52..55812ab8bb 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -100,6 +100,18 @@ class CourseTestCase(ProceduralCourseTestMixin, ModuleStoreTestCase): nonstaff.is_authenticated = lambda: authenticate return client, nonstaff + def create_staff_authed_user_client(self, authenticate=True): + """ + Create a staff user, log them in (if authenticate=True), and return the client, user to use for testing. + """ + staff, password = self.create_staff_user() + + client = AjaxEnabledTestClient() + if authenticate: + client.login(username=staff.username, password=password) + staff.is_authenticated = lambda: authenticate + return client, staff + def reload_course(self): """ Reloads the course object from the database diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 70b3e6387f..e5d5417825 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -16,7 +16,7 @@ from contentstore.courseware_index import CoursewareSearchIndexer, SearchIndexin from contentstore.tests.utils import CourseTestCase from contentstore.utils import reverse_course_url, reverse_library_url, add_instructor, reverse_usage_url from contentstore.views.course import ( - course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info + course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info, _get_library_creator_status ) from contentstore.views.item import create_xblock_info, VisibilityState from course_action_state.managers import CourseRerunUIStateManager @@ -30,6 +30,8 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory +from student import auth +from student.roles import CourseCreatorRole class TestCourseIndex(CourseTestCase): @@ -48,6 +50,36 @@ class TestCourseIndex(CourseTestCase): display_name='dotted.course.name-2', ) + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", False) + def test_library_creator_status_libraries_not_enabled(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") + + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_is_staff_user(self): + _, staff_user = self.create_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(staff_user), "granted") + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_disable_library_creation(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": True}): + self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_course_creator_role(self): + _, staff_user = self.create_staff_authed_user_client() + _, nostaff_user = self.create_non_staff_authed_user_client() + auth.add_users(staff_user, CourseCreatorRole(), nostaff_user) + self.assertEqual(_get_library_creator_status(nostaff_user), "granted") + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_no_course_creator_role_no_is_staff_no_disable_library_creation(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": False}): + self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") + def check_index_and_outline(self, authed_client): """ Test getting the list of courses and then pulling up their outlines diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index c80317d4d7..4d3a4a02f7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -436,6 +436,22 @@ class ModuleStoreTestCase(ModuleStoreIsolationMixin, TestCase): nonstaff_user.save() return nonstaff_user, password + def create_staff_user(self): + """ + Creates a staff test user. + Returns the staff test user and its password. + """ + uname = 'teststaff' + password = 'bar' + staff_user = User.objects.create_user(uname, 'test+staff@edx.org', password) + + # Note that we do not actually need to do anything + # for registration if we directly mark them active. + staff_user.is_active = True + staff_user.is_staff = True + staff_user.save() + return staff_user, password + def update_course(self, course, user_id): """ Updates the version of course in the modulestore From ce17c1cdcf37f6a69c93fc5aae6756d2ed9b2997 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Mon, 28 Nov 2016 11:19:35 +0200 Subject: [PATCH 09/32] make check of creation of library a true/false for forntend, add security in api call, clean tests --- cms/djangoapps/contentstore/tests/utils.py | 12 ------- cms/djangoapps/contentstore/views/course.py | 24 ++++++-------- cms/djangoapps/contentstore/views/library.py | 9 +++-- .../views/tests/test_course_index.py | 33 +------------------ .../contentstore/views/tests/test_library.py | 32 ++++++++++++++++-- cms/envs/common.py | 1 - cms/templates/index.html | 7 ++-- .../xmodule/modulestore/tests/django_utils.py | 16 --------- 8 files changed, 50 insertions(+), 84 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 55812ab8bb..af5e7b8e52 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -100,18 +100,6 @@ class CourseTestCase(ProceduralCourseTestMixin, ModuleStoreTestCase): nonstaff.is_authenticated = lambda: authenticate return client, nonstaff - def create_staff_authed_user_client(self, authenticate=True): - """ - Create a staff user, log them in (if authenticate=True), and return the client, user to use for testing. - """ - staff, password = self.create_staff_user() - - client = AjaxEnabledTestClient() - if authenticate: - client.login(username=staff.username, password=password) - staff.is_authenticated = lambda: authenticate - return client, staff - def reload_course(self): """ Reloads the course object from the database diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 93b108f4e1..b5895a3bb1 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -519,18 +519,18 @@ def course_listing(request): return render_to_response('index.html', { 'courses': courses, 'in_process_course_actions': in_process_course_actions, + 'libraries_enabled': LIBRARIES_ENABLED, + 'libraries': [format_library_for_view(lib) for lib in libraries], + 'show_new_library_button': _get_library_creator_status(user), 'user': user, 'request_course_creator_url': reverse('contentstore.views.request_course_creator'), 'course_creator_status': _get_course_creator_status(user), 'rerun_creator_status': GlobalStaff().has_user(user), 'allow_unicode_course_id': settings.FEATURES.get('ALLOW_UNICODE_COURSE_ID', False), 'allow_course_reruns': settings.FEATURES.get('ALLOW_COURSE_RERUNS', True), - 'libraries_enabled': LIBRARIES_ENABLED, - 'libraries': [format_library_for_view(lib) for lib in libraries], - 'library_creator_status': _get_library_creator_status(user), 'is_programs_enabled': programs_config.is_studio_tab_enabled and user.is_staff, 'programs': programs, - 'program_authoring_url': reverse('programs') + 'program_authoring_url': reverse('programs'), }) @@ -1655,19 +1655,15 @@ def _get_course_creator_status(user): def _get_library_creator_status(user): """ Helper method for returning the library creation status for a particular user, - taking into account the values of DISABLE_LIBRARY_CREATION and LIBRARIES_ENABLED. + taking into account the value LIBRARIES_ENABLED. """ if not LIBRARIES_ENABLED: - library_creator_status = 'disallowed_for_this_site' + return False elif user.is_staff: - library_creator_status = 'granted' - elif CourseCreatorRole().has_user(user): - library_creator_status = 'granted' - elif settings.FEATURES.get('DISABLE_LIBRARY_CREATION', False): - library_creator_status = 'disallowed_for_this_site' + return True + elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + return CourseCreatorRole().has_user(user) else: - library_creator_status = 'disallowed_for_this_site' - - return library_creator_status + return False diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index cb3cd4e18b..8bae2df764 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -9,7 +9,7 @@ import logging from contentstore.views.item import create_xblock_info from contentstore.utils import reverse_library_url, add_instructor -from django.http import HttpResponseNotAllowed, Http404 +from django.http import HttpResponseNotAllowed, Http404, HttpResponseForbidden from django.contrib.auth.decorators import login_required from django.core.exceptions import PermissionDenied from django.conf import settings @@ -29,7 +29,7 @@ from .component import get_component_templates, CONTAINER_TEMPLATES from student.auth import ( STUDIO_VIEW_USERS, STUDIO_EDIT_ROLES, get_user_permissions, has_studio_read_access, has_studio_write_access ) -from student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole +from student.roles import CourseInstructorRole, CourseStaffRole, LibraryUserRole, CourseCreatorRole from util.json_request import expect_json, JsonResponse, JsonResponseBadRequest __all__ = ['library_handler', 'manage_library_users'] @@ -50,6 +50,11 @@ def library_handler(request, library_key_string=None): log.exception("Attempted to use the content library API when the libraries feature is disabled.") raise Http404 # Should never happen because we test the feature in urls.py also + if not CourseCreatorRole().has_user(request.user): + if not request.user.is_staff: + return HttpResponseForbidden() + + if library_key_string is not None and request.method == 'POST': return HttpResponseNotAllowed(("POST",)) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index e5d5417825..e8756eb896 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -16,7 +16,7 @@ from contentstore.courseware_index import CoursewareSearchIndexer, SearchIndexin from contentstore.tests.utils import CourseTestCase from contentstore.utils import reverse_course_url, reverse_library_url, add_instructor, reverse_usage_url from contentstore.views.course import ( - course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info, _get_library_creator_status + course_outline_initial_state, reindex_course_and_check_access, _deprecated_blocks_info ) from contentstore.views.item import create_xblock_info, VisibilityState from course_action_state.managers import CourseRerunUIStateManager @@ -30,7 +30,6 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory -from student import auth from student.roles import CourseCreatorRole @@ -50,36 +49,6 @@ class TestCourseIndex(CourseTestCase): display_name='dotted.course.name-2', ) - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", False) - def test_library_creator_status_libraries_not_enabled(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_is_staff_user(self): - _, staff_user = self.create_staff_authed_user_client() - self.assertEqual(_get_library_creator_status(staff_user), "granted") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_disable_library_creation(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": True}): - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_course_creator_role(self): - _, staff_user = self.create_staff_authed_user_client() - _, nostaff_user = self.create_non_staff_authed_user_client() - auth.add_users(staff_user, CourseCreatorRole(), nostaff_user) - self.assertEqual(_get_library_creator_status(nostaff_user), "granted") - - @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) - def test_library_creator_status_with_no_course_creator_role_no_is_staff_no_disable_library_creation(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"DISABLE_LIBRARY_CREATION": False}): - self.assertEqual(_get_library_creator_status(nostaff_user), "disallowed_for_this_site") - def check_index_and_outline(self, authed_client): """ Test getting the list of courses and then pulling up their outlines diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 2a704c002d..f8c96df9a9 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -5,13 +5,17 @@ More important high-level tests are in contentstore/tests/test_libraries.py """ from contentstore.tests.utils import AjaxEnabledTestClient, parse_json from contentstore.utils import reverse_course_url, reverse_library_url +from contentstore.tests.utils import CourseTestCase from contentstore.views.component import get_component_templates +from contentstore.views.course import _get_library_creator_status from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import LibraryFactory from mock import patch from opaque_keys.edx.locator import CourseKey, LibraryLocator import ddt -from student.roles import LibraryUserRole +import mock +from student.auth import add_users +from student.roles import CourseCreatorRole, LibraryUserRole LIBRARY_REST_URL = '/library/' # URL for GET/POST requests involving libraries @@ -24,7 +28,7 @@ def make_url_for_lib(key): @ddt.ddt -class UnitTestLibraries(ModuleStoreTestCase): +class UnitTestLibraries(CourseTestCase): """ Unit tests for library views """ @@ -38,6 +42,28 @@ class UnitTestLibraries(ModuleStoreTestCase): ###################################################### # Tests for /library/ - list and create libraries: + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", False) + def test_library_creator_status_libraries_not_enabled(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), False) + + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_is_staff_user(self): + self.assertEqual(_get_library_creator_status(self.user), True) + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_course_creator_role(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + add_users(self.user, CourseCreatorRole(), nostaff_user) + self.assertEqual(_get_library_creator_status(nostaff_user), True) + + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) + def test_library_creator_status_with_no_course_creator_role(self): + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), False) + @patch("contentstore.views.library.LIBRARIES_ENABLED", False) def test_with_libraries_disabled(self): """ @@ -93,7 +119,7 @@ class UnitTestLibraries(ModuleStoreTestCase): self.client.logout() ns_user, password = self.create_non_staff_user() self.client.login(username=ns_user.username, password=password) - + add_users(self.user, CourseCreatorRole(), ns_user) response = self.client.ajax_post(LIBRARY_REST_URL, { 'org': 'org', 'library': 'lib', 'display_name': "New Library", }) diff --git a/cms/envs/common.py b/cms/envs/common.py index 3c94ee9ed1..11c0842b8d 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -168,7 +168,6 @@ FEATURES = { # Enable support for content libraries. Note that content libraries are # only supported in courses using split mongo. 'ENABLE_CONTENT_LIBRARIES': True, - 'DISABLE_LIBRARY_CREATION': False, # Milestones application flag 'MILESTONES_APP': False, diff --git a/cms/templates/index.html b/cms/templates/index.html index ad652dd5e3..8760d5951f 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -33,8 +33,7 @@ from openedx.core.djangolib.markup import HTML, Text % elif course_creator_status=='disallowed_for_this_site' and settings.FEATURES.get('STUDIO_REQUEST_EMAIL',''): ${_("Email staff to create course")} % endif - - % if library_creator_status=='granted': + % if show_new_library_button: ${_("New Library")} % endif @@ -130,7 +129,7 @@ from openedx.core.djangolib.markup import HTML, Text % endif - %if libraries_enabled and library_creator_status=='granted': + %if libraries_enabled and show_new_library_button:
@@ -497,7 +496,7 @@ from openedx.core.djangolib.markup import HTML, Text
- % if library_creator_status=='granted': + % if show_new_library_button:

${_('Create Your First Library')}

diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 4d3a4a02f7..c80317d4d7 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -436,22 +436,6 @@ class ModuleStoreTestCase(ModuleStoreIsolationMixin, TestCase): nonstaff_user.save() return nonstaff_user, password - def create_staff_user(self): - """ - Creates a staff test user. - Returns the staff test user and its password. - """ - uname = 'teststaff' - password = 'bar' - staff_user = User.objects.create_user(uname, 'test+staff@edx.org', password) - - # Note that we do not actually need to do anything - # for registration if we directly mark them active. - staff_user.is_active = True - staff_user.is_staff = True - staff_user.save() - return staff_user, password - def update_course(self, course, user_id): """ Updates the version of course in the modulestore From 4f50874fd070ea4adb36a22e16e01272ad37c873 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Wed, 14 Dec 2016 12:24:10 +0200 Subject: [PATCH 10/32] update tests --- cms/djangoapps/contentstore/views/course.py | 21 ++--------- cms/djangoapps/contentstore/views/library.py | 17 ++++++++- .../contentstore/views/tests/test_library.py | 35 ++++++++++++++++--- 3 files changed, 48 insertions(+), 25 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index b5895a3bb1..d67e2f9703 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -26,7 +26,7 @@ from .component import ( ADVANCED_COMPONENT_TYPES, ) from .item import create_xblock_info -from .library import LIBRARIES_ENABLED +from .library import LIBRARIES_ENABLED, _get_library_creator_status from ccx_keys.locator import CCXLocator from contentstore.course_group_config import ( COHORT_SCHEME, @@ -1634,7 +1634,7 @@ def _get_course_creator_status(user): If the user passed in has not previously visited the index page, it will be added with status 'unrequested' if the course creator group is in use. """ - + if user.is_staff: course_creator_status = 'granted' elif settings.FEATURES.get('DISABLE_COURSE_CREATION', False): @@ -1650,20 +1650,3 @@ def _get_course_creator_status(user): course_creator_status = 'granted' return course_creator_status - - -def _get_library_creator_status(user): - """ - Helper method for returning the library creation status for a particular user, - taking into account the value LIBRARIES_ENABLED. - - """ - - if not LIBRARIES_ENABLED: - return False - elif user.is_staff: - return True - elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): - return CourseCreatorRole().has_user(user) - else: - return False diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 8bae2df764..62f2bff17a 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -38,6 +38,21 @@ log = logging.getLogger(__name__) LIBRARIES_ENABLED = settings.FEATURES.get('ENABLE_CONTENT_LIBRARIES', False) +def _get_library_creator_status(user): + """ + Helper method for returning the library creation status for a particular user, + taking into account the value LIBRARIES_ENABLED. + + """ + + if not LIBRARIES_ENABLED: + return False + elif user.is_staff: + return True + elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + return CourseCreatorRole().has_user(user) + else: + return True @login_required @ensure_csrf_cookie @@ -50,7 +65,7 @@ def library_handler(request, library_key_string=None): log.exception("Attempted to use the content library API when the libraries feature is disabled.") raise Http404 # Should never happen because we test the feature in urls.py also - if not CourseCreatorRole().has_user(request.user): + if not _get_library_creator_status(request.user): if not request.user.is_staff: return HttpResponseForbidden() diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index f8c96df9a9..785ad3ffb7 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -46,7 +46,7 @@ class UnitTestLibraries(CourseTestCase): def test_library_creator_status_libraries_not_enabled(self): _, nostaff_user = self.create_non_staff_authed_user_client() self.assertEqual(_get_library_creator_status(nostaff_user), False) - + @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_is_staff_user(self): @@ -61,8 +61,8 @@ class UnitTestLibraries(CourseTestCase): @mock.patch("contentstore.views.library.LIBRARIES_ENABLED", True) def test_library_creator_status_with_no_course_creator_role(self): - _, nostaff_user = self.create_non_staff_authed_user_client() - self.assertEqual(_get_library_creator_status(nostaff_user), False) + _, nostaff_user = self.create_non_staff_authed_user_client() + self.assertEqual(_get_library_creator_status(nostaff_user), True) @patch("contentstore.views.library.LIBRARIES_ENABLED", False) def test_with_libraries_disabled(self): @@ -113,8 +113,7 @@ class UnitTestLibraries(CourseTestCase): @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) def test_lib_create_permission(self): """ - Users who are not given course creator roles should still be able to - create libraries. + Users who are given course creator roles should be able to create libraries. """ self.client.logout() ns_user, password = self.create_non_staff_user() @@ -125,6 +124,32 @@ class UnitTestLibraries(CourseTestCase): }) self.assertEqual(response.status_code, 200) + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': False}) + def test_lib_create_permission_no_course_creator_role_and_no_course_creator_group(self): + """ + Users who are not given course creator roles should still be able to create libraries if COURSE_CREATOR_GROUP is not enabled + """ + self.client.logout() + ns_user, password = self.create_non_staff_user() + self.client.login(username=ns_user.username, password=password) + response = self.client.ajax_post(LIBRARY_REST_URL, { + 'org': 'org', 'library': 'lib', 'display_name': "New Library", + }) + self.assertEqual(response.status_code, 200) + + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) + def test_lib_create_permission_no_course_creator_role_and_course_creator_group(self): + """ + Users who are not given course creator roles should not be able to create libraries if COURSE_CREATOR_GROUP is enabled. + """ + self.client.logout() + ns_user, password = self.create_non_staff_user() + self.client.login(username=ns_user.username, password=password) + response = self.client.ajax_post(LIBRARY_REST_URL, { + 'org': 'org', 'library': 'lib', 'display_name': "New Library", + }) + self.assertEqual(response.status_code, 403) + @ddt.data( {}, {'org': 'org'}, From 0682618a57e78ffaf531b9d7e856cb0927ec2851 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Fri, 16 Dec 2016 08:26:16 +0200 Subject: [PATCH 11/32] fix docstring of tests --- cms/djangoapps/contentstore/views/tests/test_library.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 785ad3ffb7..68b90d720b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -127,7 +127,7 @@ class UnitTestLibraries(CourseTestCase): @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': False}) def test_lib_create_permission_no_course_creator_role_and_no_course_creator_group(self): """ - Users who are not given course creator roles should still be able to create libraries if COURSE_CREATOR_GROUP is not enabled + Users who are not given course creator roles should still be able to create libraries if ENABLE_CREATOR_GROUP is not enabled. """ self.client.logout() ns_user, password = self.create_non_staff_user() @@ -140,7 +140,7 @@ class UnitTestLibraries(CourseTestCase): @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) def test_lib_create_permission_no_course_creator_role_and_course_creator_group(self): """ - Users who are not given course creator roles should not be able to create libraries if COURSE_CREATOR_GROUP is enabled. + Users who are not given course creator roles should not be able to create libraries if ENABLE_CREATOR_GROUP is enabled. """ self.client.logout() ns_user, password = self.create_non_staff_user() From 7494e5e6c8a02c1354d0a3924da08eb1126919bb Mon Sep 17 00:00:00 2001 From: noraiz-anwar Date: Thu, 5 Jan 2017 23:07:56 +0500 Subject: [PATCH 12/32] Revert "Error while exporting course with too long filename" --- .../tests/test_transcripts_utils.py | 12 ------- .../js/spec/video/transcripts/utils_spec.js | 36 ------------------- .../js/views/video/transcripts/utils.js | 8 ++--- .../xmodule/video_module/transcripts_utils.py | 6 ++-- 4 files changed, 4 insertions(+), 58 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py b/cms/djangoapps/contentstore/tests/test_transcripts_utils.py index 1f2cdef395..3cdf01e543 100644 --- a/cms/djangoapps/contentstore/tests/test_transcripts_utils.py +++ b/cms/djangoapps/contentstore/tests/test_transcripts_utils.py @@ -9,7 +9,6 @@ from mock import patch, Mock from django.test.utils import override_settings from django.conf import settings from django.utils import translation -from django.utils.crypto import get_random_string from nose.plugins.skip import SkipTest @@ -231,17 +230,6 @@ class TestDownloadYoutubeSubs(SharedModuleStoreTestCase): self.assertEqual(html5_ids[2], 'baz.1.4') self.assertEqual(html5_ids[3], 'foo') - def test_html5_id_length(self): - """ - Test that html5_id is parsed with length less than 255, as html5 ids are - used as name for transcript objects and ultimately as filename while creating - file for transcript at the time of exporting a course. - Filename can't be longer than 255 characters. - 150 chars is agreed length. - """ - html5_ids = transcripts_utils.get_html5_ids([get_random_string(255)]) - self.assertEqual(len(html5_ids[0]), 150) - @patch('xmodule.video_module.transcripts_utils.requests.get') def test_fail_downloading_subs(self, mock_get): diff --git a/cms/static/js/spec/video/transcripts/utils_spec.js b/cms/static/js/spec/video/transcripts/utils_spec.js index b5f448989d..f9ca4e6ae9 100644 --- a/cms/static/js/spec/video/transcripts/utils_spec.js +++ b/cms/static/js/spec/video/transcripts/utils_spec.js @@ -215,42 +215,6 @@ function($, _, Utils, _str) { }); }); }); - - describe('Too long arguments ', function() { - var longFileName = (function() { - var text = ''; - var possibleChars = 'abcdefghijklmnopqrstuvwxyz'; - /* eslint vars-on-top: 0 */ - for (var i = 0; i < 255; i++) { - text += possibleChars.charAt(Math.floor(Math.random() * possibleChars.length)); - } - return text; - }()), - html5LongUrls = (function(videoName) { - var links = [ - 'http://somelink.com/%s?param=1¶m=2#hash', - 'http://somelink.com/%s#hash', - 'http://somelink.com/%s?param=1¶m=2', - 'http://somelink.com/%s', - 'ftp://somelink.com/%s', - 'https://somelink.com/%s', - 'https://somelink.com/sub/sub/%s', - 'http://cdn.somecdn.net/v/%s', - 'somelink.com/%s', - '%s' - ]; - return $.map(links, function(link) { - return _str.sprintf(link, videoName); - }); - }(longFileName)); - - $.each(html5LongUrls, function(index, link) { - it(link, function() { - var result = Utils.parseHTML5Link(link); - expect(result.video.length).toBe(150); - }); - }); - }); }); it('Method: getYoutubeLink', function() { diff --git a/cms/static/js/views/video/transcripts/utils.js b/cms/static/js/views/video/transcripts/utils.js index 4c2d74fc83..4945d87801 100644 --- a/cms/static/js/views/video/transcripts/utils.js +++ b/cms/static/js/views/video/transcripts/utils.js @@ -110,7 +110,6 @@ define(['jquery', 'underscore', 'jquery.ajaxQueue'], function($) { */ var _videoLinkParser = (function() { var cache = {}; - var maxVideoNameLength = 150; return function(url) { if (typeof url !== 'string') { @@ -130,10 +129,7 @@ define(['jquery', 'underscore', 'jquery.ajaxQueue'], function($) { match = link.pathname.match(/\/{1}([^\/]+)\.([^\/]+)$/); if (match) { cache[url] = { - /* avoid too long video name, as it will be used as filename for video's transcript - and a filename can not be more that 255 chars, limiting here to 150. - */ - video: match[1].slice(0, maxVideoNameLength), + video: match[1], type: match[2] }; } else { @@ -143,7 +139,7 @@ define(['jquery', 'underscore', 'jquery.ajaxQueue'], function($) { match = link.pathname.match(/\/{1}([^\/\.]+)$/); if (match) { cache[url] = { - video: match[1].slice(0, maxVideoNameLength), + video: match[1], type: 'other' }; } diff --git a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py index 22d6786e3b..f464a9d1db 100644 --- a/common/lib/xmodule/xmodule/video_module/transcripts_utils.py +++ b/common/lib/xmodule/xmodule/video_module/transcripts_utils.py @@ -296,11 +296,9 @@ def copy_or_rename_transcript(new_name, old_name, item, delete_old=False, user=N def get_html5_ids(html5_sources): """ Helper method to parse out an HTML5 source into the ideas - NOTE: This assumes that '/' are not in the filename. - Slices each id by 150, restricting too long strings as video names. + NOTE: This assumes that '/' are not in the filename """ - html5_ids = [x.split('/')[-1].rsplit('.', 1)[0][:150] for x in html5_sources] - + html5_ids = [x.split('/')[-1].rsplit('.', 1)[0] for x in html5_sources] return html5_ids From 69ae314dd8a3af7f30cbcb9df8d527ded5096ed2 Mon Sep 17 00:00:00 2001 From: Anthony Mangano Date: Thu, 5 Jan 2017 09:48:13 -0500 Subject: [PATCH 13/32] enable users to select course when making support requests ECOM-5936 --- .../util/tests/test_submit_feedback.py | 173 ++++++++++++++++-- common/djangoapps/util/views.py | 52 +++++- lms/envs/aws.py | 2 + lms/envs/common.py | 1 + lms/static/sass/_build-lms-v1.scss | 1 + lms/static/sass/shared-v2/_help-tab.scss | 6 + lms/static/sass/shared/_help-tab.scss | 5 + lms/templates/help_modal.html | 121 ++++++++++-- 8 files changed, 325 insertions(+), 36 deletions(-) create mode 100644 lms/static/sass/shared/_help-tab.scss diff --git a/common/djangoapps/util/tests/test_submit_feedback.py b/common/djangoapps/util/tests/test_submit_feedback.py index 077765e1d7..f3969a7b55 100644 --- a/common/djangoapps/util/tests/test_submit_feedback.py +++ b/common/djangoapps/util/tests/test_submit_feedback.py @@ -11,8 +11,10 @@ from util import views from zendesk import ZendeskError import json import mock +from ddt import ddt, data, unpack from student.tests.test_configuration_overrides import fake_get_value +from student.tests.factories import CourseEnrollmentFactory def fake_support_backend_values(name, default=None): # pylint: disable=unused-argument @@ -26,8 +28,9 @@ def fake_support_backend_values(name, default=None): # pylint: disable=unused-a return config_dict[name] +@ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": True}) -@override_settings(ZENDESK_URL="dummy", ZENDESK_USER="dummy", ZENDESK_API_KEY="dummy") +@override_settings(ZENDESK_URL="dummy", ZENDESK_USER="dummy", ZENDESK_API_KEY="dummy", ZENDESK_CUSTOM_FIELDS={}) @mock.patch("util.views.dog_stats_api") @mock.patch("util.views._ZendeskApi", autospec=True) class SubmitFeedbackTest(TestCase): @@ -44,14 +47,12 @@ class SubmitFeedbackTest(TestCase): username="test", profile__name="Test User" ) - # This contains issue_type and course_id to ensure that tags are submitted correctly self._anon_fields = { "email": "test@edx.org", "name": "Test User", "subject": "a subject", "details": "some details", - "issue_type": "test_issue", - "course_id": "test_course" + "issue_type": "test_issue" } # This does not contain issue_type nor course_id to ensure that they are optional self._auth_fields = {"subject": "a subject", "details": "some details"} @@ -130,13 +131,9 @@ class SubmitFeedbackTest(TestCase): resp = self._build_and_run_request(user, fields) self.assertEqual(resp.status_code, 200) - def _assert_datadog_called(self, datadog_mock, with_tags): - expected_datadog_calls = [ - mock.call.increment( - views.DATADOG_FEEDBACK_METRIC, - tags=(["course_id:test_course", "issue_type:test_issue"] if with_tags else []) - ) - ] + def _assert_datadog_called(self, datadog_mock, tags): + """Assert that datadog was called with the correct tags.""" + expected_datadog_calls = [mock.call.increment(views.DATADOG_FEEDBACK_METRIC, tags=tags)] self.assertEqual(datadog_mock.mock_calls, expected_datadog_calls) def test_bad_request_anon_user_no_name(self, zendesk_mock_class, datadog_mock): @@ -184,7 +181,7 @@ class SubmitFeedbackTest(TestCase): "requester": {"name": "Test User", "email": "test@edx.org"}, "subject": "a subject", "comment": {"body": "some details"}, - "tags": ["test_course", "test_issue", "LMS"] + "tags": ["test_issue", "LMS"] } } ), @@ -206,7 +203,7 @@ class SubmitFeedbackTest(TestCase): ) ] self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:test_issue"]) @mock.patch("openedx.core.djangoapps.site_configuration.helpers.get_value", fake_get_value) def test_valid_request_anon_user_configuration_override(self, zendesk_mock_class, datadog_mock): @@ -228,7 +225,7 @@ class SubmitFeedbackTest(TestCase): "requester": {"name": "Test User", "email": "test@edx.org"}, "subject": "a subject", "comment": {"body": "some details"}, - "tags": ["test_course", "test_issue", "LMS", "whitelabel_fakeorg"] + "tags": ["test_issue", "LMS", "whitelabel_fakeorg"] } } ), @@ -250,7 +247,69 @@ class SubmitFeedbackTest(TestCase): ) ] self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:test_issue"]) + + @data("course-v1:testOrg+testCourseNumber+testCourseRun", "", None) + @override_settings(ZENDESK_CUSTOM_FIELDS={"course_id": 1234, "enrollment_mode": 5678}) + def test_valid_request_anon_user_with_custom_fields(self, course_id, zendesk_mock, datadog_mock): + """ + Test a valid request from an anonymous user when configured to use Zendesk Custom Fields. + + The response should have a 200 (success) status code, and a ticket with + the given information should have been submitted via the Zendesk API. When course_id is + present, it should be sent to Zendesk via a custom field. When course_id is blank or missing, + the request should still be processed successfully. + """ + zendesk_mock_instance = zendesk_mock.return_value + zendesk_mock_instance.create_ticket.return_value = 42 + + fields = self._anon_fields.copy() + + expected_zendesk_tags = None + expected_datadog_tags = None + if course_id is not None: + fields["course_id"] = course_id + expected_zendesk_tags = [course_id, "test_issue", "LMS"] + expected_datadog_tags = ["course_id:{}".format(course_id), "issue_type:test_issue"] + else: + expected_zendesk_tags = ["test_issue", "LMS"] + expected_datadog_tags = ["issue_type:test_issue"] + + expected_create_ticket_request = { + "ticket": { + "recipient": "registration@example.com", + "requester": {"name": "Test User", "email": "test@edx.org"}, + "subject": "a subject", + "comment": {"body": "some details"}, + "tags": expected_zendesk_tags + } + } + + expected_update_ticket_request = { + "ticket": { + "comment": { + "public": False, + "body": + "Additional information:\n\n" + "Client IP: 1.2.3.4\n" + "Host: test_server\n" + "Page: test_referer\n" + "Browser: test_user_agent" + } + } + } + + if fields.get("course_id"): + expected_custom_fields = [{"id": 1234, "value": fields["course_id"]}] + expected_create_ticket_request["ticket"]["custom_fields"] = expected_custom_fields + + self._test_success(self._anon_user, fields) + expected_zendesk_calls = [ + mock.call.create_ticket(expected_create_ticket_request), + mock.call.update_ticket(42, expected_update_ticket_request) + ] + self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) + self._assert_datadog_called(datadog_mock, expected_datadog_tags) def test_bad_request_auth_user_no_subject(self, zendesk_mock_class, datadog_mock): """Test a request from an authenticated user not specifying `subject`.""" @@ -303,7 +362,85 @@ class SubmitFeedbackTest(TestCase): ) ] self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) - self._assert_datadog_called(datadog_mock, with_tags=False) + self._assert_datadog_called(datadog_mock, []) + + @data( + ("course-v1:testOrg+testCourseNumber+testCourseRun", True), + ("course-v1:testOrg+testCourseNumber+testCourseRun", False), + ("", None), + (None, None) + ) + @unpack + @override_settings(ZENDESK_CUSTOM_FIELDS={"course_id": 1234, "enrollment_mode": 5678}) + def test_valid_request_auth_user_with_custom_fields(self, course_id, enrollment_state, zendesk_mock, datadog_mock): + """ + Test a valid request from an authenticated user when configured to use Zendesk Custom Fields. + + The response should have a 200 (success) status code, and a ticket with + the given information should have been submitted via the Zendesk API. When course_id is + present, it should be sent to Zendesk via a custom field, along with the enrollment mode + if the user has an active enrollment for that course. When course_id is blank or missing, + the request should still be processed successfully. + """ + zendesk_mock_instance = zendesk_mock.return_value + zendesk_mock_instance.create_ticket.return_value = 42 + + fields = self._auth_fields.copy() + + expected_zendesk_tags = None + expected_datadog_tags = None + if course_id is not None: + fields["course_id"] = course_id + expected_zendesk_tags = [course_id, "LMS"] + expected_datadog_tags = ["course_id:{}".format(course_id)] + else: + expected_zendesk_tags = ["LMS"] + expected_datadog_tags = [] + + expected_create_ticket_request = { + "ticket": { + "recipient": "registration@example.com", + "requester": {"name": "Test User", "email": "test@edx.org"}, + "subject": "a subject", + "comment": {"body": "some details"}, + "tags": expected_zendesk_tags + } + } + + expected_update_ticket_request = { + "ticket": { + "comment": { + "public": False, + "body": + "Additional information:\n\n" + "username: test\n" + "Client IP: 1.2.3.4\n" + "Host: test_server\n" + "Page: test_referer\n" + "Browser: test_user_agent", + } + } + } + + if fields.get("course_id"): + expected_custom_fields = [{"id": 1234, "value": fields["course_id"]}] + if enrollment_state is not None: + enrollment = CourseEnrollmentFactory.create( + user=self._auth_user, + course_id=course_id, + is_active=enrollment_state + ) + if enrollment.is_active: + expected_custom_fields.append({"id": 5678, "value": enrollment.mode}) + expected_create_ticket_request["ticket"]["custom_fields"] = expected_custom_fields + + self._test_success(self._auth_user, fields) + expected_zendesk_calls = [ + mock.call.create_ticket(expected_create_ticket_request), + mock.call.update_ticket(42, expected_update_ticket_request) + ] + self.assertEqual(zendesk_mock_instance.mock_calls, expected_zendesk_calls) + self._assert_datadog_called(datadog_mock, expected_datadog_tags) def test_get_request(self, zendesk_mock_class, datadog_mock): """Test that a GET results in a 405 even with all required fields""" @@ -329,7 +466,7 @@ class SubmitFeedbackTest(TestCase): resp = self._build_and_run_request(self._anon_user, self._anon_fields) self.assertEqual(resp.status_code, 500) self.assertFalse(resp.content) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:test_issue"]) def test_zendesk_error_on_update(self, zendesk_mock_class, datadog_mock): """ @@ -344,7 +481,7 @@ class SubmitFeedbackTest(TestCase): zendesk_mock_instance.update_ticket.side_effect = err resp = self._build_and_run_request(self._anon_user, self._anon_fields) self.assertEqual(resp.status_code, 200) - self._assert_datadog_called(datadog_mock, with_tags=True) + self._assert_datadog_called(datadog_mock, ["issue_type:test_issue"]) @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_FEEDBACK_SUBMISSION": False}) def test_not_enabled(self, zendesk_mock_class, datadog_mock): diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index ea886e7777..c5560fedea 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -25,6 +25,7 @@ from edxmako.shortcuts import render_to_response, render_to_string from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers import track.views from student.roles import GlobalStaff +from student.models import CourseEnrollment log = logging.getLogger(__name__) @@ -222,6 +223,40 @@ class _ZendeskApi(object): return None +def _get_zendesk_custom_field_context(request): + """ + Construct a dictionary of data that can be stored in Zendesk custom fields. + """ + context = {} + + course_id = request.POST.get("course_id") + if not course_id: + return context + + context["course_id"] = course_id + if not request.user.is_authenticated(): + return context + + enrollment = CourseEnrollment.get_enrollment(request.user, CourseKey.from_string(course_id)) + if enrollment and enrollment.is_active: + context["enrollment_mode"] = enrollment.mode + + return context + + +def _format_zendesk_custom_fields(context): + """ + Format the data in `context` for compatibility with the Zendesk API. + Ignore any keys that have not been configured in `ZENDESK_CUSTOM_FIELDS`. + """ + custom_fields = [] + for key, val, in settings.ZENDESK_CUSTOM_FIELDS.items(): + if key in context: + custom_fields.append({"id": val, "value": context[key]}) + + return custom_fields + + def _record_feedback_in_zendesk( realname, email, @@ -231,7 +266,8 @@ def _record_feedback_in_zendesk( additional_info, group_name=None, require_update=False, - support_email=None + support_email=None, + custom_fields=None ): """ Create a new user-requested Zendesk ticket. @@ -246,6 +282,8 @@ def _record_feedback_in_zendesk( If `require_update` is provided, returns False when the update does not succeed. This allows using the private comment to add necessary information which the user will not see in followup emails from support. + + If `custom_fields` is provided, submits data to those fields in Zendesk. """ zendesk_api = _ZendeskApi() @@ -271,6 +309,10 @@ def _record_feedback_in_zendesk( "tags": zendesk_tags } } + + if custom_fields: + new_ticket["ticket"]["custom_fields"] = custom_fields + group = None if group_name is not None: group = zendesk_api.get_group(group_name) @@ -412,6 +454,11 @@ def submit_feedback(request): if not settings.ZENDESK_URL or not settings.ZENDESK_USER or not settings.ZENDESK_API_KEY: raise Exception("Zendesk enabled but not configured") + custom_fields = None + if settings.ZENDESK_CUSTOM_FIELDS: + custom_field_context = _get_zendesk_custom_field_context(request) + custom_fields = _format_zendesk_custom_fields(custom_field_context) + success = _record_feedback_in_zendesk( context["realname"], context["email"], @@ -419,7 +466,8 @@ def submit_feedback(request): context["details"], context["tags"], context["additional_info"], - support_email=context["support_email"] + support_email=context["support_email"], + custom_fields=custom_fields ) _record_feedback_in_datadog(context["tags"]) diff --git a/lms/envs/aws.py b/lms/envs/aws.py index 340c2079fa..3e85617b0f 100644 --- a/lms/envs/aws.py +++ b/lms/envs/aws.py @@ -333,6 +333,8 @@ COMMENTS_SERVICE_URL = ENV_TOKENS.get("COMMENTS_SERVICE_URL", '') COMMENTS_SERVICE_KEY = ENV_TOKENS.get("COMMENTS_SERVICE_KEY", '') CERT_QUEUE = ENV_TOKENS.get("CERT_QUEUE", 'test-pull') ZENDESK_URL = ENV_TOKENS.get('ZENDESK_URL', ZENDESK_URL) +ZENDESK_CUSTOM_FIELDS = ENV_TOKENS.get('ZENDESK_CUSTOM_FIELDS', ZENDESK_CUSTOM_FIELDS) + FEEDBACK_SUBMISSION_EMAIL = ENV_TOKENS.get("FEEDBACK_SUBMISSION_EMAIL") MKTG_URLS = ENV_TOKENS.get('MKTG_URLS', MKTG_URLS) diff --git a/lms/envs/common.py b/lms/envs/common.py index a3ef8fca31..e71ccdc03b 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1003,6 +1003,7 @@ FEEDBACK_SUBMISSION_EMAIL = None ZENDESK_URL = None ZENDESK_USER = None ZENDESK_API_KEY = None +ZENDESK_CUSTOM_FIELDS = {} ##### EMBARGO ##### EMBARGO_SITE_REDIRECT_URL = None diff --git a/lms/static/sass/_build-lms-v1.scss b/lms/static/sass/_build-lms-v1.scss index bb376fec9f..460e2dbc2d 100644 --- a/lms/static/sass/_build-lms-v1.scss +++ b/lms/static/sass/_build-lms-v1.scss @@ -35,6 +35,7 @@ @import 'shared/modal'; @import 'shared/activation_messages'; @import 'shared/unsubscribe'; +@import 'shared/help-tab'; // shared - platform @import 'multicourse/home'; diff --git a/lms/static/sass/shared-v2/_help-tab.scss b/lms/static/sass/shared-v2/_help-tab.scss index 080c5f4e6c..99cf9b3836 100644 --- a/lms/static/sass/shared-v2/_help-tab.scss +++ b/lms/static/sass/shared-v2/_help-tab.scss @@ -78,3 +78,9 @@ padding: 0 $baseline $baseline; } } + +.feedback-form-select { + background: $white; + margin-bottom: $baseline; + width: 100%; +} diff --git a/lms/static/sass/shared/_help-tab.scss b/lms/static/sass/shared/_help-tab.scss new file mode 100644 index 0000000000..7af1407ac0 --- /dev/null +++ b/lms/static/sass/shared/_help-tab.scss @@ -0,0 +1,5 @@ +.feedback-form-select { + background: $white; + margin-bottom: $baseline; + width: 100%; +} diff --git a/lms/templates/help_modal.html b/lms/templates/help_modal.html index abe1779d99..5198e893c1 100644 --- a/lms/templates/help_modal.html +++ b/lms/templates/help_modal.html @@ -99,15 +99,21 @@ from xmodule.tabs import CourseTabList % endif + +
+ % if course: + + % endif +
+ + ${_('Describe what you were doing when you encountered the issue. Include any details that will help us to troubleshoot, including error messages that you saw.')} + - % if course: - - % endif
@@ -138,7 +144,12 @@ from xmodule.tabs import CourseTabList