From d112c0b81b06b2ed45c17c91eff1889f84bf59a5 Mon Sep 17 00:00:00 2001 From: jagonzalr Date: Thu, 3 Nov 2016 14:59:39 +0200 Subject: [PATCH] show button new library in studio depending on flags and user staff status add flag DISABLE_LIBRARY_CREATION add comma use CourseCreatorRole to determine if user can create a library add disable library creation feature flag Conflicts: cms/djangoapps/contentstore/views/course.py ENABLE_CONTENT_LIBRARIES flag check for course creator role for library creation Conflicts: cms/djangoapps/contentstore/views/course.py add unit tests make check of creation of library a true/false for forntend, add security in api call, clean tests update tests fix docstring of tests fixed quality violation fixed broken unit test and quality violations Feedback changes and unit test to assert libraries are visible to non staff users too fixed quality violation and feedback changes --- .../contentstore/tests/test_libraries.py | 11 +++- cms/djangoapps/contentstore/views/course.py | 12 ++-- cms/djangoapps/contentstore/views/library.py | 23 ++++++- .../views/tests/test_course_index.py | 38 ++++++++---- .../contentstore/views/tests/test_library.py | 61 +++++++++++++++++-- cms/templates/index.html | 1 - 6 files changed, 120 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 6f95cd26da..cf8f113a0d 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -11,7 +11,7 @@ import ddt from mock import patch from student.auth import has_studio_read_access, has_studio_write_access from student.roles import ( - CourseInstructorRole, CourseStaffRole, CourseCreatorRole, LibraryUserRole, + CourseInstructorRole, CourseStaffRole, LibraryUserRole, OrgStaffRole, OrgInstructorRole, OrgLibraryUserRole, ) from xmodule.modulestore import ModuleStoreEnum @@ -25,6 +25,7 @@ from xblock_django.user_service import DjangoXBlockUserService from xmodule.x_module import STUDIO_VIEW from student import auth from student.tests.factories import UserFactory +from course_creators.views import add_user_with_status_granted class LibraryTestCase(ModuleStoreTestCase): @@ -533,9 +534,13 @@ class TestLibraryAccess(SignalDisconnectTestMixin, LibraryTestCase): self.client.logout() self._assert_cannot_create_library(expected_code=302) # 302 redirect to login expected - # Now check that logged-in users without CourseCreator role can still create libraries + # Now check that logged-in users without CourseCreator role cannot create libraries self._login_as_non_staff_user(logout_first=False) - self.assertFalse(CourseCreatorRole().has_user(self.non_staff_user)) + with patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): + self._assert_cannot_create_library(expected_code=403) # 403 user is not CourseCreator + + # Now check that logged-in users with CourseCreator role can create libraries + add_user_with_status_granted(self.user, self.non_staff_user) with patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): lib_key2 = self._create_library(library="lib2", display_name="Test Library 2") library2 = modulestore().get_library(lib_key2) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 08666660f3..df5ba575ae 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, @@ -465,6 +465,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) + user = request.user libraries = _accessible_libraries_list(request.user) if LIBRARIES_ENABLED else [] def format_in_process_course_view(uca): @@ -509,11 +510,11 @@ 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, - 'user': request.user, + '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(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), }) @@ -1619,6 +1620,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): diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index cb3cd4e18b..7a642d474a 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 @@ -25,6 +25,7 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from .user import user_with_role +from course_creators.views import get_course_creator_status 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 @@ -39,6 +40,22 @@ 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 get_course_creator_status(user) == 'granted' + else: + return True + + @login_required @ensure_csrf_cookie @require_http_methods(('GET', 'POST')) @@ -50,6 +67,10 @@ 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 get_library_creator_status(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 70b3e6387f..337335654a 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -24,6 +24,7 @@ from course_action_state.models import CourseRerunState from opaque_keys.edx.locator import CourseLocator from search.api import perform_search from student.auth import has_course_author_access +from student.roles import LibraryUserRole from student.tests.factories import UserFactory from util.date_utils import get_default_time_display from xmodule.modulestore import ModuleStoreEnum @@ -75,22 +76,37 @@ class TestCourseIndex(CourseTestCase): """ Test getting the list of libraries from the course listing page """ + def _assert_library_link_present(response, library): + """ + Asserts there's a valid library link on libraries tab. + """ + parsed_html = lxml.html.fromstring(response.content) + library_link_elements = parsed_html.find_class('library-link') + self.assertEqual(len(library_link_elements), 1) + link = library_link_elements[0] + self.assertEqual( + link.get("href"), + reverse_library_url('library_handler', library.location.library_key), + ) + # now test that url + outline_response = self.client.get(link.get("href"), {}, HTTP_ACCEPT='text/html') + self.assertEqual(outline_response.status_code, 200) + # Add a library: lib1 = LibraryFactory.create() index_url = '/home/' index_response = self.client.get(index_url, {}, HTTP_ACCEPT='text/html') - parsed_html = lxml.html.fromstring(index_response.content) - library_link_elements = parsed_html.find_class('library-link') - self.assertEqual(len(library_link_elements), 1) - link = library_link_elements[0] - self.assertEqual( - link.get("href"), - reverse_library_url('library_handler', lib1.location.library_key), - ) - # now test that url - outline_response = self.client.get(link.get("href"), {}, HTTP_ACCEPT='text/html') - self.assertEqual(outline_response.status_code, 200) + _assert_library_link_present(index_response, lib1) + + # Make sure libraries are visible to non-staff users too + self.client.logout() + non_staff_user, non_staff_userpassword = self.create_non_staff_user() + lib2 = LibraryFactory.create(user_id=non_staff_user.id) + LibraryUserRole(lib2.location.library_key).add_users(non_staff_user) + self.client.login(username=non_staff_user.username, password=non_staff_userpassword) + index_response = self.client.get(index_url, {}, HTTP_ACCEPT='text/html') + _assert_library_link_present(index_response, lib2) def test_is_staff_access(self): """ diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 2a704c002d..0e4c7b5521 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -5,12 +5,15 @@ 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 xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from contentstore.views.library import get_library_creator_status +from course_creators.views import add_user_with_status_granted as grant_course_creator_status from xmodule.modulestore.tests.factories import LibraryFactory from mock import patch from opaque_keys.edx.locator import CourseKey, LibraryLocator import ddt +import mock from student.roles import LibraryUserRole LIBRARY_REST_URL = '/library/' # URL for GET/POST requests involving libraries @@ -24,7 +27,7 @@ def make_url_for_lib(key): @ddt.ddt -class UnitTestLibraries(ModuleStoreTestCase): +class UnitTestLibraries(CourseTestCase): """ Unit tests for library views """ @@ -38,6 +41,27 @@ 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}): + grant_course_creator_status(self.user, 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), True) + @patch("contentstore.views.library.LIBRARIES_ENABLED", False) def test_with_libraries_disabled(self): """ @@ -87,18 +111,45 @@ class UnitTestLibraries(ModuleStoreTestCase): @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() self.client.login(username=ns_user.username, password=password) - + grant_course_creator_status(self.user, ns_user) 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': 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 ENABLE_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 ENABLE_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'}, diff --git a/cms/templates/index.html b/cms/templates/index.html index ccf85c37f5..977443d545 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -33,7 +33,6 @@ 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 show_new_library_button: ${_("New Library")}