From 94e1c42314d081f12e6bf046ae077d25c12a2683 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Tue, 5 May 2015 11:14:05 -0400 Subject: [PATCH 1/2] Add extensible course view types for edX platform --- .../tests/test_course_settings.py | 40 +-- .../contentstore/tests/test_utils.py | 55 ---- cms/djangoapps/contentstore/tests/utils.py | 2 +- cms/djangoapps/contentstore/utils.py | 44 +-- cms/djangoapps/contentstore/views/course.py | 109 +++---- cms/djangoapps/contentstore/views/tabs.py | 5 +- .../views/tests/test_import_export.py | 2 +- .../models/settings/course_metadata.py | 12 +- common/djangoapps/student/models.py | 2 + common/lib/xmodule/xmodule/tabs.py | 286 ++++++++---------- common/lib/xmodule/xmodule/tests/test_tabs.py | 154 ++++------ lms/djangoapps/ccx/overrides.py | 9 - lms/djangoapps/ccx/plugins.py | 31 ++ lms/djangoapps/courseware/tabs.py | 49 +-- lms/djangoapps/courseware/tests/test_tabs.py | 20 +- lms/djangoapps/edxnotes/plugins.py | 32 ++ lms/djangoapps/edxnotes/tests.py | 22 +- .../tests/views/test_instructor_dashboard.py | 19 ++ .../instructor/views/instructor_dashboard.py | 20 +- .../courseware/course_navigation.html | 2 +- lms/templates/help_modal.html | 2 +- openedx/core/lib/plugins/__init__.py | 0 openedx/core/lib/plugins/api.py | 89 ++++++ openedx/core/lib/plugins/tests/__init__.py | 0 openedx/core/lib/plugins/tests/test_api.py | 23 ++ setup.py | 17 +- 26 files changed, 536 insertions(+), 510 deletions(-) create mode 100644 lms/djangoapps/ccx/plugins.py create mode 100644 lms/djangoapps/edxnotes/plugins.py create mode 100644 openedx/core/lib/plugins/__init__.py create mode 100644 openedx/core/lib/plugins/api.py create mode 100644 openedx/core/lib/plugins/tests/__init__.py create mode 100644 openedx/core/lib/plugins/tests/test_api.py diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index a060f21eac..a68cdd593e 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -662,7 +662,7 @@ class CourseMetadataEditingTest(CourseTestCase): If feature flag is off, then giturl must be filtered. """ # pylint: disable=unused-variable - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "giturl": {"value": "http://example.com"}, @@ -677,7 +677,7 @@ class CourseMetadataEditingTest(CourseTestCase): If feature flag is on, then giturl must not be filtered. """ # pylint: disable=unused-variable - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "giturl": {"value": "http://example.com"}, @@ -736,7 +736,7 @@ class CourseMetadataEditingTest(CourseTestCase): If feature flag is off, then edxnotes must be filtered. """ # pylint: disable=unused-variable - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "edxnotes": {"value": "true"}, @@ -751,7 +751,7 @@ class CourseMetadataEditingTest(CourseTestCase): If feature flag is on, then edxnotes must not be filtered. """ # pylint: disable=unused-variable - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "edxnotes": {"value": "true"}, @@ -788,8 +788,8 @@ class CourseMetadataEditingTest(CourseTestCase): ) self.assertNotIn('edxnotes', test_model) - def test_validate_and_update_from_json_correct_inputs(self): - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + def test_validate_from_json_correct_inputs(self): + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "advertised_start": {"value": "start A"}, @@ -802,18 +802,13 @@ class CourseMetadataEditingTest(CourseTestCase): self.assertTrue(len(errors) == 0) self.update_check(test_model) - # fresh fetch to ensure persistence - fresh = modulestore().get_course(self.course.id) - test_model = CourseMetadata.fetch(fresh) - self.update_check(test_model) - # Tab gets tested in test_advanced_settings_munge_tabs self.assertIn('advanced_modules', test_model, 'Missing advanced_modules') self.assertEqual(test_model['advanced_modules']['value'], ['combinedopenended'], 'advanced_module is not updated') - def test_validate_and_update_from_json_wrong_inputs(self): + def test_validate_from_json_wrong_inputs(self): # input incorrectly formatted data - is_valid, errors, test_model = CourseMetadata.validate_and_update_from_json( + is_valid, errors, test_model = CourseMetadata.validate_from_json( self.course, { "advertised_start": {"value": 1, "display_name": "Course Advertised Start Date", }, @@ -824,7 +819,7 @@ class CourseMetadataEditingTest(CourseTestCase): user=self.user ) - # Check valid results from validate_and_update_from_json + # Check valid results from validate_from_json self.assertFalse(is_valid) self.assertEqual(len(errors), 3) self.assertFalse(test_model) @@ -947,23 +942,6 @@ class CourseMetadataEditingTest(CourseTestCase): course = modulestore().get_course(self.course.id) self.assertNotIn(EXTRA_TAB_PANELS.get("open_ended"), course.tabs) - @patch.dict(settings.FEATURES, {'ENABLE_EDXNOTES': True}) - def test_course_settings_munge_tabs(self): - """ - Test that adding and removing specific course settings adds and removes tabs. - """ - self.assertNotIn(EXTRA_TAB_PANELS.get("edxnotes"), self.course.tabs) - self.client.ajax_post(self.course_setting_url, { - "edxnotes": {"value": True} - }) - course = modulestore().get_course(self.course.id) - self.assertIn(EXTRA_TAB_PANELS.get("edxnotes"), course.tabs) - self.client.ajax_post(self.course_setting_url, { - "edxnotes": {"value": False} - }) - course = modulestore().get_course(self.course.id) - self.assertNotIn(EXTRA_TAB_PANELS.get("edxnotes"), course.tabs) - class CourseGraderUpdatesTest(CourseTestCase): """ diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index a9a0c42474..c29231b2ea 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -108,61 +108,6 @@ class ExtraPanelTabTestCase(TestCase): course.tabs = tabs return course - def test_add_extra_panel_tab(self): - """ Tests if a tab can be added to a course tab list. """ - for tab_type in utils.EXTRA_TAB_PANELS.keys(): - tab = utils.EXTRA_TAB_PANELS.get(tab_type) - - # test adding with changed = True - for tab_setup in ['', 'x', 'x,y,z']: - course = self.get_course_with_tabs(tab_setup) - expected_tabs = copy.copy(course.tabs) - expected_tabs.append(tab) - changed, actual_tabs = utils.add_extra_panel_tab(tab_type, course) - self.assertTrue(changed) - self.assertEqual(actual_tabs, expected_tabs) - - # test adding with changed = False - tab_test_setup = [ - [tab], - [tab, self.get_tab_type_dicts('x,y,z')], - [self.get_tab_type_dicts('x,y'), tab, self.get_tab_type_dicts('z')], - [self.get_tab_type_dicts('x,y,z'), tab]] - - for tab_setup in tab_test_setup: - course = self.get_course_with_tabs(tab_setup) - expected_tabs = copy.copy(course.tabs) - changed, actual_tabs = utils.add_extra_panel_tab(tab_type, course) - self.assertFalse(changed) - self.assertEqual(actual_tabs, expected_tabs) - - def test_remove_extra_panel_tab(self): - """ Tests if a tab can be removed from a course tab list. """ - for tab_type in utils.EXTRA_TAB_PANELS.keys(): - tab = utils.EXTRA_TAB_PANELS.get(tab_type) - - # test removing with changed = True - tab_test_setup = [ - [tab], - [tab, self.get_tab_type_dicts('x,y,z')], - [self.get_tab_type_dicts('x,y'), tab, self.get_tab_type_dicts('z')], - [self.get_tab_type_dicts('x,y,z'), tab]] - - for tab_setup in tab_test_setup: - course = self.get_course_with_tabs(tab_setup) - expected_tabs = [t for t in course.tabs if t != utils.EXTRA_TAB_PANELS.get(tab_type)] - changed, actual_tabs = utils.remove_extra_panel_tab(tab_type, course) - self.assertTrue(changed) - self.assertEqual(actual_tabs, expected_tabs) - - # test removing with changed = False - for tab_setup in ['', 'x', 'x,y,z']: - course = self.get_course_with_tabs(tab_setup) - expected_tabs = copy.copy(course.tabs) - changed, actual_tabs = utils.remove_extra_panel_tab(tab_type, course) - self.assertFalse(changed) - self.assertEqual(actual_tabs, expected_tabs) - class CourseImageTestCase(ModuleStoreTestCase): """Tests for course image URLs.""" diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 3eaf65b592..551f5ef17a 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -95,7 +95,7 @@ class CourseTestCase(ModuleStoreTestCase): client = AjaxEnabledTestClient() if authenticate: client.login(username=nonstaff.username, password=password) - nonstaff.is_authenticated = True + nonstaff.is_authenticated = lambda: authenticate return client, nonstaff def populate_course(self, branching=2): diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index c8fe7ce15d..5e3812542e 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -3,7 +3,6 @@ Common utility functions useful throughout the contentstore """ # pylint: disable=no-member -import copy import logging import re from datetime import datetime @@ -30,8 +29,7 @@ log = logging.getLogger(__name__) # In order to instantiate an open ended tab automatically, need to have this data OPEN_ENDED_PANEL = {"name": _("Open Ended Panel"), "type": "open_ended"} NOTES_PANEL = {"name": _("My Notes"), "type": "notes"} -EDXNOTES_PANEL = {"name": _("Notes"), "type": "edxnotes"} -EXTRA_TAB_PANELS = dict([(p['type'], p) for p in [OPEN_ENDED_PANEL, NOTES_PANEL, EDXNOTES_PANEL]]) +EXTRA_TAB_PANELS = {p['type']: p for p in [OPEN_ENDED_PANEL, NOTES_PANEL]} def add_instructor(course_key, requesting_user, new_instructor): @@ -287,46 +285,6 @@ def ancestor_has_staff_lock(xblock, parent_xblock=None): return parent_xblock.visible_to_staff_only -def add_extra_panel_tab(tab_type, course): - """ - Used to add the panel tab to a course if it does not exist. - @param tab_type: A string representing the tab type. - @param course: A course object from the modulestore. - @return: Boolean indicating whether or not a tab was added and a list of tabs for the course. - """ - # Copy course tabs - course_tabs = copy.copy(course.tabs) - changed = False - # Check to see if open ended panel is defined in the course - - tab_panel = EXTRA_TAB_PANELS.get(tab_type) - if tab_panel not in course_tabs: - # Add panel to the tabs if it is not defined - course_tabs.append(tab_panel) - changed = True - return changed, course_tabs - - -def remove_extra_panel_tab(tab_type, course): - """ - Used to remove the panel tab from a course if it exists. - @param tab_type: A string representing the tab type. - @param course: A course object from the modulestore. - @return: Boolean indicating whether or not a tab was added and a list of tabs for the course. - """ - # Copy course tabs - course_tabs = copy.copy(course.tabs) - changed = False - # Check to see if open ended panel is defined in the course - - tab_panel = EXTRA_TAB_PANELS.get(tab_type) - if tab_panel in course_tabs: - # Add panel to the tabs if it is not defined - course_tabs = [ct for ct in course_tabs if ct != tab_panel] - changed = True - return changed, course_tabs - - def reverse_url(handler_name, key_name=None, key_value=None, kwargs=None): """ Creates the URL for the given handler. diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4fe48d432c..e708818a6e 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -1,6 +1,7 @@ """ Views related to operations on course objects """ +import copy from django.shortcuts import redirect import json import random @@ -22,12 +23,13 @@ from xmodule.course_module import DEFAULT_START_DATE from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule.contentstore.content import StaticContent -from xmodule.tabs import PDFTextbookTabs +from xmodule.tabs import PDFTextbookTabs, CourseTab, CourseTabManager from xmodule.modulestore import EdxJSONEncoder from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError from opaque_keys import InvalidKeyError from opaque_keys.edx.locations import Location from opaque_keys.edx.keys import CourseKey +from openedx.core.lib.plugins.api import CourseViewType from django_future.csrf import ensure_csrf_cookie from contentstore.course_info_model import get_course_updates, update_course_updates, delete_course_update @@ -42,13 +44,12 @@ from contentstore.utils import ( add_instructor, initialize_permissions, get_lms_link_for_item, - add_extra_panel_tab, - remove_extra_panel_tab, reverse_course_url, reverse_library_url, reverse_usage_url, reverse_url, remove_all_instructors, + EXTRA_TAB_PANELS, ) from models.settings.course_details import CourseDetails, CourseSettingsEncoder from models.settings.course_grading import CourseGradingModel @@ -993,37 +994,6 @@ def grading_handler(request, course_key_string, grader_index=None): return JsonResponse() -# pylint: disable=invalid-name -def _add_tab(request, tab_type, course_module): - """ - Adds tab to the course. - """ - # Add tab to the course if needed - changed, new_tabs = add_extra_panel_tab(tab_type, course_module) - # If a tab has been added to the course, then send the - # metadata along to CourseMetadata.update_from_json - if changed: - course_module.tabs = new_tabs - request.json.update({'tabs': {'value': new_tabs}}) - # Indicate that tabs should not be filtered out of - # the metadata - return True - return False - - -# pylint: disable=invalid-name -def _remove_tab(request, tab_type, course_module): - """ - Removes the tab from the course. - """ - changed, new_tabs = remove_extra_panel_tab(tab_type, course_module) - if changed: - course_module.tabs = new_tabs - request.json.update({'tabs': {'value': new_tabs}}) - return True - return False - - def is_advanced_component_present(request, advanced_components): """ Return True when one of `advanced_components` is present in the request. @@ -1047,13 +1017,9 @@ def is_field_value_true(request, field_list): return any([request.json.get(field, {}).get('value') for field in field_list]) -# pylint: disable=invalid-name -def _modify_tabs_to_components(request, course_module): +def _refresh_course_tabs(request, course_module): """ - Automatically adds/removes tabs if user indicated that they want - respective modules enabled in the course - - Return True when tab configuration has been modified. + Automatically adds/removes tabs if changes to the course require them. """ tab_component_map = { # 'tab_type': (check_function, list_of_checked_components_or_values), @@ -1062,11 +1028,20 @@ def _modify_tabs_to_components(request, course_module): 'open_ended': (is_advanced_component_present, OPEN_ENDED_COMPONENT_TYPES), # notes tab 'notes': (is_advanced_component_present, NOTE_COMPONENT_TYPES), - # student notes tab - 'edxnotes': (is_field_value_true, ['edxnotes']) } - tabs_changed = False + def update_tab(tabs, tab_type, tab_enabled): + """ + Adds or removes a course tab based upon whether it is enabled. + """ + tab_panel = _get_tab_panel_for_type(tab_type) + if tab_enabled: + tabs.append(CourseTab.from_json(tab_panel)) + elif tab_panel in tabs: + tabs.remove(tab_panel) + + course_tabs = copy.copy(course_module.tabs) + for tab_type in tab_component_map.keys(): check, component_types = tab_component_map[tab_type] try: @@ -1075,19 +1050,30 @@ def _modify_tabs_to_components(request, course_module): # user has failed to put iterable value into advanced component list. # return immediately and let validation handle. return + update_tab(course_tabs, tab_type, tab_enabled) - if tab_enabled: - # check passed, some of this component_types are present, adding tab - if _add_tab(request, tab_type, course_module): - # tab indeed was added, the change needs to propagate - tabs_changed = True - else: - # the tab should not be present (anymore) - if _remove_tab(request, tab_type, course_module): - # tab indeed was removed, the change needs to propagate - tabs_changed = True + # Additionally update any persistent tabs provided by course views + for tab_type in CourseTabManager.get_tab_types().values(): + if issubclass(tab_type, CourseViewType) and tab_type.is_persistent: + tab_enabled = tab_type.is_enabled(course_module, settings, user=request.user) + update_tab(course_tabs, tab_type, tab_enabled) - return tabs_changed + # Save the tabs into the course if they have been changed + if not course_tabs == course_module.tabs: + course_module.tabs = course_tabs + + +def _get_tab_panel_for_type(tab_type): + """ + Returns a tab panel representation for the specified tab type. + """ + tab_panel = EXTRA_TAB_PANELS.get(tab_type) + if tab_panel: + return tab_panel + return { + "name": tab_type.title, + "type": tab_type.name + } @login_required @@ -1119,18 +1105,21 @@ def advanced_settings_handler(request, course_key_string): return JsonResponse(CourseMetadata.fetch(course_module)) else: try: - # do not process tabs unless they were modified according to course metadata - filter_tabs = not _modify_tabs_to_components(request, course_module) - - # validate data formats and update - is_valid, errors, updated_data = CourseMetadata.validate_and_update_from_json( + # validate data formats and update the course module. + # Note: don't update mongo yet, but wait until after any tabs are changed + is_valid, errors, updated_data = CourseMetadata.validate_from_json( course_module, request.json, - filter_tabs=filter_tabs, user=request.user, ) if is_valid: + # update the course tabs if required by any setting changes + _refresh_course_tabs(request, course_module) + + # now update mongo + modulestore().update_item(course_module, request.user.id) + return JsonResponse(updated_data) else: return JsonResponseBadRequest(errors) diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index 131be1d946..4764363a7f 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -61,10 +61,7 @@ def tabs_handler(request, course_key_string): # present in the same order they are displayed in LMS tabs_to_render = [] - for tab in CourseTabList.iterate_displayable_cms( - course_item, - settings, - ): + for tab in CourseTabList.iterate_displayable(course_item, settings, inline_collections=False): if isinstance(tab, StaticTab): # static tab needs its locator information to render itself as an xmodule static_tab_loc = course_key.make_usage_key('static_tab', tab.url_slug) diff --git a/cms/djangoapps/contentstore/views/tests/test_import_export.py b/cms/djangoapps/contentstore/views/tests/test_import_export.py index 17c30f51fa..3375a30d09 100644 --- a/cms/djangoapps/contentstore/views/tests/test_import_export.py +++ b/cms/djangoapps/contentstore/views/tests/test_import_export.py @@ -116,7 +116,7 @@ class ImportTestCase(CourseTestCase): Check that course is imported successfully in existing course and users have their access roles """ # Create a non_staff user and add it to course staff only - __, nonstaff_user = self.create_non_staff_authed_user_client(authenticate=False) + __, nonstaff_user = self.create_non_staff_authed_user_client() auth.add_users(self.user, CourseStaffRole(self.course.id), nonstaff_user) course = self.store.get_course(self.course.id) diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 0eb74623da..a62902e0e9 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -150,11 +150,11 @@ class CourseMetadata(object): return cls.update_from_dict(key_values, descriptor, user) @classmethod - def validate_and_update_from_json(cls, descriptor, jsondict, user, filter_tabs=True): + def validate_from_json(cls, descriptor, jsondict, user, filter_tabs=True): """ Validate the values in the json dict (validated by xblock fields from_json method) - If all fields validate, go ahead and update those values in the database. + If all fields validate, go ahead and update those values on the object and return it. If not, return the error objects list. Returns: @@ -183,19 +183,19 @@ class CourseMetadata(object): # If did validate, go ahead and update the metadata if did_validate: - updated_data = cls.update_from_dict(key_values, descriptor, user) + updated_data = cls.update_from_dict(key_values, descriptor, user, save=False) return did_validate, errors, updated_data @classmethod - def update_from_dict(cls, key_values, descriptor, user): + def update_from_dict(cls, key_values, descriptor, user, save=True): """ - Update metadata descriptor in modulestore from key_values. + Update metadata descriptor from key_values. Saves to modulestore if save is true. """ for key, value in key_values.iteritems(): setattr(descriptor, key, value) - if len(key_values): + if save and len(key_values): modulestore().update_item(descriptor, user.id) return cls.fetch(descriptor) diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 647a636d5a..5a4b880572 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -1171,6 +1171,8 @@ class CourseEnrollment(models.Model): `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) """ + if not user.is_authenticated(): + return False try: record = CourseEnrollment.objects.get(user=user, course_id=course_key) return record.is_active diff --git a/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index 1dd6b72eba..424d789ce0 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -2,6 +2,7 @@ Implement CourseTab """ from abc import ABCMeta, abstractmethod + from xblock.fields import List # We should only scrape strings for i18n in this file, since the target language is known only when @@ -51,11 +52,11 @@ class CourseTab(object): self.link_func = link_func - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): # pylint: disable=unused-argument + def is_enabled(self, course, settings, user=None): # pylint: disable=unused-argument """ - Determines whether the tab should be displayed in the UI for the given course and a particular user. - This method is to be overridden by subclasses when applicable. The base class implementation - always returns True. + Determines whether the tab is enabled for the given course and a particular user. + This method is to be overridden by subclasses when applicable. The base class + implementation always returns True. Args: course: An xModule CourseDescriptor @@ -67,17 +68,11 @@ class CourseTab(object): FEATURES['ENABLE_STUDENT_NOTES'] FEATURES['ENABLE_TEXTBOOK'] - is_user_authenticated: Indicates whether the user is authenticated. If the tab is of - type AuthenticatedCourseTab and this value is False, then can_display will return False. - - is_user_staff: Indicates whether the user has staff access to the course. If the tab is of - type StaffTab and this value is False, then can_display will return False. - - is_user_enrolled: Indicates whether the user is enrolled in the course + user: An optional user for whom the tab will be displayed. If none, + then the code should assume a staff user or an author. Returns: - A boolean value to indicate whether this instance of the tab should be displayed to a - given user for the given course. + A boolean value to indicate whether this instance of the tab is enabled. """ return True @@ -174,62 +169,104 @@ class CourseTab(object): Raises: InvalidTabsException if the given tab doesn't have the right keys. """ - sub_class_types = { - 'courseware': CoursewareTab, - 'course_info': CourseInfoTab, - 'wiki': WikiTab, - 'discussion': DiscussionTab, - 'external_discussion': ExternalDiscussionTab, - 'external_link': ExternalLinkTab, - 'textbooks': TextbookTabs, - 'pdf_textbooks': PDFTextbookTabs, - 'html_textbooks': HtmlTextbookTabs, - 'progress': ProgressTab, - 'static_tab': StaticTab, - 'peer_grading': PeerGradingTab, - 'staff_grading': StaffGradingTab, - 'open_ended': OpenEndedGradingTab, - 'notes': NotesTab, - 'edxnotes': EdxNotesTab, - 'syllabus': SyllabusTab, - 'instructor': InstructorTab, # not persisted - 'ccx_coach': CcxCoachTab, # not persisted - } - - tab_type = tab_dict.get('type') - if tab_type not in sub_class_types: + available_tab_types = CourseTabManager.get_tab_types() + tab_type_name = tab_dict.get('type') + if tab_type_name not in available_tab_types: raise InvalidTabsException( - 'Unknown tab type {0}. Known types: {1}'.format(tab_type, sub_class_types) + 'Unknown tab type {0}. Known types: {1}'.format(tab_type_name, available_tab_types) ) + tab_type = available_tab_types[tab_dict['type']] + tab_type.validate(tab_dict) + # TODO: don't import openedx capabilities from common + from openedx.core.lib.plugins.api import CourseViewType + if issubclass(tab_type, CourseViewType): + return CourseViewTab(tab_type, tab_dict=tab_dict) + else: + return tab_type(tab_dict=tab_dict) - tab_class = sub_class_types[tab_dict['type']] - tab_class.validate(tab_dict) - return tab_class(tab_dict=tab_dict) + +class CourseTabManager(object): + """ + A manager that handles the set of available course tabs. + """ + + @classmethod + def get_tab_types(cls): + """ + Returns the list of available tab types. + """ + if not hasattr(cls, "_tab_types"): + tab_types = { + 'courseware': CoursewareTab, + 'course_info': CourseInfoTab, + 'wiki': WikiTab, + 'discussion': DiscussionTab, + 'external_discussion': ExternalDiscussionTab, + 'external_link': ExternalLinkTab, + 'textbooks': TextbookTabs, + 'pdf_textbooks': PDFTextbookTabs, + 'html_textbooks': HtmlTextbookTabs, + 'progress': ProgressTab, + 'static_tab': StaticTab, + 'peer_grading': PeerGradingTab, + 'staff_grading': StaffGradingTab, + 'open_ended': OpenEndedGradingTab, + 'notes': NotesTab, + 'syllabus': SyllabusTab, + } + + # Add any registered course views + # TODO: don't import openedx capabilities from common + from openedx.core.lib.plugins.api import CourseViewTypeManager + for course_view_type in CourseViewTypeManager.get_available_plugins().values(): + tab_types[course_view_type.name] = course_view_type + + cls._tab_types = tab_types + return cls._tab_types class AuthenticatedCourseTab(CourseTab): """ Abstract class for tabs that can be accessed by only authenticated users. """ - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - return is_user_authenticated + def is_enabled(self, course, settings, user=None): + return not user or user.is_authenticated() class StaffTab(AuthenticatedCourseTab): """ Abstract class for tabs that can be accessed by only users with staff access. """ - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): # pylint: disable=unused-argument - return is_user_staff + def is_enabled(self, course, settings, user=None): # pylint: disable=unused-argument + return not user or is_user_staff(course, user) -class EnrolledOrStaffTab(CourseTab): +def is_user_staff(course, user): + """ + Returns true if the user is staff in the specified course, or globally. + """ + from courseware.access import has_access # pylint: disable=import-error + return has_access(user, 'staff', course, course.id) + + +def is_user_enrolled_or_staff(course, user): + """ + Returns true if the user is enrolled in the specified course, + or if the user is staff. + """ + from student.models import CourseEnrollment # pylint: disable=import-error + return is_user_staff(course, user) or CourseEnrollment.is_enrolled(user, course.id) + + +class EnrolledOrStaffTab(AuthenticatedCourseTab): """ Abstract class for tabs that can be accessed by only users with staff access or users enrolled in the course. """ - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): # pylint: disable=unused-argument - return is_user_authenticated and (is_user_staff or is_user_enrolled) + def is_enabled(self, course, settings, user=None): # pylint: disable=unused-argument + if not user: + return True + return is_user_enrolled_or_staff(course, user) class HideableTab(CourseTab): @@ -323,10 +360,8 @@ class ProgressTab(EnrolledOrStaffTab): link_func=link_reverse_func(self.type), ) - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - super_can_display = super(ProgressTab, self).can_display( - course, settings, is_user_authenticated, is_user_staff, is_user_enrolled - ) + def is_enabled(self, course, settings, user=None): + super_can_display = super(ProgressTab, self).is_enabled(course, settings, user=user) return super_can_display and not course.hide_progress_tab @classmethod @@ -350,10 +385,12 @@ class WikiTab(HideableTab): tab_dict=tab_dict, ) - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - return settings.WIKI_ENABLED and ( - course.allow_public_wiki_access or is_user_enrolled or is_user_staff - ) + def is_enabled(self, course, settings, user=None): + if not settings.WIKI_ENABLED: + return False + if not user or course.allow_public_wiki_access: + return True + return is_user_enrolled_or_staff(course, user) @classmethod def validate(cls, tab_dict, raise_error=True): @@ -375,14 +412,12 @@ class DiscussionTab(EnrolledOrStaffTab): link_func=link_reverse_func('django_comment_client.forum.views.forum_form_discussion'), ) - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): + def is_enabled(self, course, settings, user=None): if settings.FEATURES.get('CUSTOM_COURSES_EDX', False): from ccx.overrides import get_current_ccx # pylint: disable=import-error if get_current_ccx(): return False - super_can_display = super(DiscussionTab, self).can_display( - course, settings, is_user_authenticated, is_user_staff, is_user_enrolled - ) + super_can_display = super(DiscussionTab, self).is_enabled(course, settings, user=user) return settings.FEATURES.get('ENABLE_DISCUSSION_SERVICE') and super_can_display @classmethod @@ -549,7 +584,7 @@ class TextbookTabs(TextbookTabsBase): tab_id=self.type, ) - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): + def is_enabled(self, course, settings, user=None): return settings.FEATURES.get('ENABLE_TEXTBOOK') def items(self, course): @@ -668,7 +703,7 @@ class SyllabusTab(CourseTab): """ type = 'syllabus' - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): + def is_enabled(self, course, settings, user=None): return hasattr(course, 'syllabus_present') and course.syllabus_present def __init__(self, tab_dict=None): # pylint: disable=unused-argument @@ -686,7 +721,7 @@ class NotesTab(AuthenticatedCourseTab): """ type = 'notes' - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): + def is_enabled(self, course, settings, user=None): return settings.FEATURES.get('ENABLE_STUDENT_NOTES') def __init__(self, tab_dict=None): @@ -701,77 +736,24 @@ class NotesTab(AuthenticatedCourseTab): return super(NotesTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) -class EdxNotesTab(AuthenticatedCourseTab): +class CourseViewTab(AuthenticatedCourseTab): """ - A tab for the course student notes. + A tab that renders a course view. """ - type = 'edxnotes' - def can_display(self, course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - return settings.FEATURES.get('ENABLE_EDXNOTES') - - def __init__(self, tab_dict=None): - super(EdxNotesTab, self).__init__( - name=tab_dict['name'] if tab_dict else _('Notes'), - tab_id=self.type, - link_func=link_reverse_func(self.type), + def __init__(self, course_view_type, tab_dict=None): + super(CourseViewTab, self).__init__( + name=tab_dict['name'] if tab_dict else course_view_type.title, + tab_id=course_view_type.name, + link_func=link_reverse_func(course_view_type.view_name), ) + self.type = course_view_type.name + self.course_view_type = course_view_type - @classmethod - def validate(cls, tab_dict, raise_error=True): - return super(EdxNotesTab, cls).validate(tab_dict, raise_error) and need_name(tab_dict, raise_error) - - -class InstructorTab(StaffTab): - """ - A tab for the course instructors. - """ - type = 'instructor' - - def __init__(self, tab_dict=None): # pylint: disable=unused-argument - super(InstructorTab, self).__init__( - # Translators: 'Instructor' appears on the tab that leads to the instructor dashboard, which is - # a portal where an instructor can get data and perform various actions on their course - name=_('Instructor'), - tab_id=self.type, - link_func=link_reverse_func('instructor_dashboard'), - ) - - -class CcxCoachTab(CourseTab): - """ - A tab for the custom course coaches. - """ - type = 'ccx_coach' - - def __init__(self, tab_dict=None): # pylint: disable=unused-argument - super(CcxCoachTab, self).__init__( - name=_('CCX Coach'), - tab_id=self.type, - link_func=link_reverse_func('ccx_coach_dashboard'), - ) - - def can_display(self, course, settings, *args, **kw): - """ - Since we don't get the user here, we use a thread local defined in the ccx - overrides to get it, then use the course to get the coach role and find out if - the user is one. - """ - user_is_coach = False - if settings.FEATURES.get('CUSTOM_COURSES_EDX', False) and course.enable_ccx: - from opaque_keys.edx.locations import SlashSeparatedCourseKey - from student.roles import CourseCcxCoachRole # pylint: disable=import-error - from ccx.overrides import get_current_request # pylint: disable=import-error - course_id = course.id.to_deprecated_string() - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - role = CourseCcxCoachRole(course_key) - request = get_current_request() - if request is not None: - user_is_coach = role.has_user(request.user) - super_can_display = super(CcxCoachTab, self).can_display( - course, settings, *args, **kw - ) - return user_is_coach and super_can_display + def is_enabled(self, course, settings, user=None): + if not super(CourseViewTab, self).is_enabled(course, settings, user=user): + return False + return self.course_view_type.is_enabled(course, settings, user=user) class CourseTabList(List): @@ -851,48 +833,23 @@ class CourseTabList(List): return next((tab for tab in tab_list if tab.tab_id == tab_id), None) @staticmethod - def iterate_displayable( - course, - settings, - is_user_authenticated=True, - is_user_staff=True, - is_user_enrolled=False - ): + def iterate_displayable(course, settings, user=None, inline_collections=True): """ Generator method for iterating through all tabs that can be displayed for the given course and the given user with the provided access settings. """ for tab in course.tabs: - if tab.can_display( - course, settings, is_user_authenticated, is_user_staff, is_user_enrolled - ) and (not tab.is_hideable or not tab.is_hidden): + if tab.is_enabled(course, settings, user=user) and (not user or not tab.is_hideable or not tab.is_hidden): if tab.is_collection: - for item in tab.items(course): - yield item + # If rendering inline that add each item in the collection, + # else just show the tab itself as long as it is not empty. + if inline_collections: + for item in tab.items(course): + yield item + elif len(list(tab.items(course))) > 0: + yield tab else: yield tab - instructor_tab = InstructorTab() - if instructor_tab.can_display(course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - yield instructor_tab - ccx_coach_tab = CcxCoachTab() - if ccx_coach_tab.can_display(course, settings, is_user_authenticated, is_user_staff, is_user_enrolled): - yield ccx_coach_tab - - @staticmethod - def iterate_displayable_cms( - course, - settings - ): - """ - Generator method for iterating through all tabs that can be displayed for the given course - with the provided settings. - """ - for tab in course.tabs: - if tab.can_display(course, settings, is_user_authenticated=True, is_user_staff=True, is_user_enrolled=True): - if tab.is_collection and not len(list(tab.items(course))): - # do not yield collections that have no items - continue - yield tab @classmethod def validate_tabs(cls, tabs): @@ -927,7 +884,8 @@ class CourseTabList(List): TextbookTabs.type, PDFTextbookTabs.type, HtmlTextbookTabs.type, - EdxNotesTab.type]: + CourseViewTab.type + ]: cls._validate_num_tabs_of_type(tabs, tab_type, 1) @staticmethod diff --git a/common/lib/xmodule/xmodule/tests/test_tabs.py b/common/lib/xmodule/xmodule/tests/test_tabs.py index 3c9b54bb43..f15fbe2a5f 100644 --- a/common/lib/xmodule/xmodule/tests/test_tabs.py +++ b/common/lib/xmodule/xmodule/tests/test_tabs.py @@ -1,5 +1,5 @@ """Tests for Tab classes""" -from mock import MagicMock +from mock import MagicMock, patch import xmodule.tabs as tabs import unittest from opaque_keys.edx.locations import SlashSeparatedCourseKey @@ -18,6 +18,28 @@ class TabTestCase(unittest.TestCase): self.reverse = lambda name, args: "name/{0}/args/{1}".format(name, ",".join(str(a) for a in args)) self.books = None + def create_mock_user(self, is_authenticated=True, is_staff=True, is_enrolled=True): + """ + Creates a mock user with the specified properties. + """ + user = MagicMock() + user.name = 'mock_user' + user.is_staff = is_staff + user.is_enrolled = is_enrolled + user.is_authenticated = lambda: is_authenticated + return user + + @patch('xmodule.tabs.is_user_enrolled_or_staff') + @patch('xmodule.tabs.is_user_staff') + def is_tab_enabled(self, tab, course, settings, user, is_staff_mock=None, is_enrolled_or_staff_mock=None): + """ + Returns true if the specified tab is enabled. + """ + is_staff_mock.return_value = user.is_staff + is_enrolled_or_staff_mock.return_value = user.is_enrolled or user.is_staff + + return tab.is_enabled(course, settings, user=user) + def set_up_books(self, num_books): """Initializes the textbooks in the course and adds the given number of books to each textbook""" self.books = [MagicMock() for _ in range(num_books)] @@ -101,33 +123,17 @@ class TabTestCase(unittest.TestCase): ): """Checks can display results for various users""" if for_staff_only: - self.assertEquals( - expected_value, - tab.can_display( - self.course, self.settings, is_user_authenticated=True, is_user_staff=True, is_user_enrolled=True - ) - ) + user = self.create_mock_user(is_authenticated=True, is_staff=True, is_enrolled=True) + self.assertEquals(expected_value, self.is_tab_enabled(tab, self.course, self.settings, user)) if for_authenticated_users_only: - self.assertEquals( - expected_value, - tab.can_display( - self.course, self.settings, is_user_authenticated=True, is_user_staff=False, is_user_enrolled=False - ) - ) + user = self.create_mock_user(is_authenticated=True, is_staff=False, is_enrolled=False) + self.assertEquals(expected_value, self.is_tab_enabled(tab, self.course, self.settings, user)) if not for_staff_only and not for_authenticated_users_only and not for_enrolled_users_only: - self.assertEquals( - expected_value, - tab.can_display( - self.course, self.settings, is_user_authenticated=False, is_user_staff=False, is_user_enrolled=False - ) - ) + user = self.create_mock_user(is_authenticated=False, is_staff=False, is_enrolled=False) + self.assertEquals(expected_value, self.is_tab_enabled(tab, self.course, self.settings, user)) if for_enrolled_users_only: - self.assertEquals( - expected_value, - tab.can_display( - self.course, self.settings, is_user_authenticated=True, is_user_staff=False, is_user_enrolled=True - ) - ) + user = self.create_mock_user(is_authenticated=True, is_staff=False, is_enrolled=True) + self.assertEquals(expected_value, self.is_tab_enabled(tab, self.course, self.settings, user)) def check_get_and_set_methods(self, tab): """Test __getitem__ and __setitem__ calls""" @@ -285,13 +291,16 @@ class TextbooksTestCase(TabTestCase): self.num_textbook_tabs = sum(1 for tab in self.course.tabs if isinstance(tab, tabs.TextbookTabsBase)) self.num_textbooks = self.num_textbook_tabs * len(self.books) - def test_textbooks_enabled(self): + @patch('xmodule.tabs.is_user_enrolled_or_staff') + def test_textbooks_enabled(self, is_enrolled_or_staff_mock): + is_enrolled_or_staff_mock.return_value = True type_to_reverse_name = {'textbook': 'book', 'pdftextbook': 'pdf_book', 'htmltextbook': 'html_book'} self.settings.FEATURES['ENABLE_TEXTBOOK'] = True num_textbooks_found = 0 - for tab in tabs.CourseTabList.iterate_displayable(self.course, self.settings): + user = self.create_mock_user(is_authenticated=True, is_staff=False, is_enrolled=True) + for tab in tabs.CourseTabList.iterate_displayable(self.course, self.settings, user=user): # verify all textbook type tabs if isinstance(tab, tabs.SingleTextbookTab): book_type, book_index = tab.tab_id.split("/", 1) @@ -397,56 +406,6 @@ class SyllabusTestCase(TabTestCase): self.check_syllabus_tab(False) -class InstructorTestCase(TabTestCase): - """Test cases for Instructor Tab.""" - - def test_instructor_tab(self): - name = 'Instructor' - tab = self.check_tab( - tab_class=tabs.InstructorTab, - dict_tab={'type': tabs.InstructorTab.type, 'name': name}, - expected_name=name, - expected_link=self.reverse('instructor_dashboard', args=[self.course.id.to_deprecated_string()]), - expected_tab_id=tabs.InstructorTab.type, - invalid_dict_tab=None, - ) - self.check_can_display_results(tab, for_staff_only=True) - - -class EdxNotesTestCase(TabTestCase): - """ - Test cases for Notes Tab. - """ - - def check_edxnotes_tab(self): - """ - Helper function for verifying the edxnotes tab. - """ - return self.check_tab( - tab_class=tabs.EdxNotesTab, - dict_tab={'type': tabs.EdxNotesTab.type, 'name': 'same'}, - expected_link=self.reverse('edxnotes', args=[self.course.id.to_deprecated_string()]), - expected_tab_id=tabs.EdxNotesTab.type, - invalid_dict_tab=self.fake_dict_tab, - ) - - def test_edxnotes_tabs_enabled(self): - """ - Tests that edxnotes tab is shown when feature is enabled. - """ - self.settings.FEATURES['ENABLE_EDXNOTES'] = True - tab = self.check_edxnotes_tab() - self.check_can_display_results(tab, for_authenticated_users_only=True) - - def test_edxnotes_tabs_disabled(self): - """ - Tests that edxnotes tab is not shown when feature is disabled. - """ - self.settings.FEATURES['ENABLE_EDXNOTES'] = False - tab = self.check_edxnotes_tab() - self.check_can_display_results(tab, expected_value=False) - - class KeyCheckerTestCase(unittest.TestCase): """Test cases for KeyChecker class""" @@ -510,7 +469,6 @@ class TabListTestCase(TabTestCase): tabs.TextbookTabs.type, tabs.PDFTextbookTabs.type, tabs.HtmlTextbookTabs.type, - tabs.EdxNotesTab.type, ] for unique_tab_type in unique_tab_types: @@ -543,7 +501,6 @@ class TabListTestCase(TabTestCase): {'type': tabs.OpenEndedGradingTab.type}, {'type': tabs.NotesTab.type, 'name': 'fake_name'}, {'type': tabs.SyllabusTab.type}, - {'type': tabs.EdxNotesTab.type, 'name': 'fake_name'}, ], # with external discussion [ @@ -599,7 +556,11 @@ class CourseTabListTestCase(TabListTestCase): self.assertTrue(tabs.ExternalDiscussionTab() not in self.course.tabs) self.assertTrue(tabs.DiscussionTab() in self.course.tabs) - def test_iterate_displayable(self): + @patch('xmodule.tabs.is_user_enrolled_or_staff') + @patch('xmodule.tabs.is_user_staff') + def test_iterate_displayable(self, is_staff_mock, is_enrolled_or_staff_mock): + is_staff_mock.return_value = True + is_enrolled_or_staff_mock.return_value = True # enable all tab types self.settings.FEATURES['ENABLE_TEXTBOOK'] = True self.settings.FEATURES['ENABLE_DISCUSSION_SERVICE'] = True @@ -613,24 +574,24 @@ class CourseTabListTestCase(TabListTestCase): # initialize the course tabs to a list of all valid tabs self.course.tabs = self.all_valid_tab_list - # enumerate the tabs using the CMS call - for i, tab in enumerate(tabs.CourseTabList.iterate_displayable_cms( - self.course, - self.settings, + # enumerate the tabs with no user + for i, tab in enumerate(tabs.CourseTabList.iterate_displayable( + self.course, + self.settings, + inline_collections=False )): self.assertEquals(tab.type, self.course.tabs[i].type) - # enumerate the tabs and verify textbooks and the instructor tab + # enumerate the tabs with a staff user + user = self.create_mock_user(is_authenticated=True, is_staff=True, is_enrolled=True) for i, tab in enumerate(tabs.CourseTabList.iterate_displayable( - self.course, - self.settings, + self.course, + self.settings, + user=user )): if getattr(tab, 'is_collection_item', False): # a collection item was found as a result of a collection tab self.assertTrue(getattr(self.course.tabs[i], 'is_collection', False)) - elif i == len(self.course.tabs): - # the last tab must be the Instructor tab - self.assertEquals(tab.type, tabs.InstructorTab.type) else: # all other tabs must match the expected type self.assertEquals(tab.type, self.course.tabs[i].type) @@ -638,14 +599,14 @@ class CourseTabListTestCase(TabListTestCase): # test including non-empty collections self.assertIn( tabs.HtmlTextbookTabs(), - list(tabs.CourseTabList.iterate_displayable_cms(self.course, self.settings)), + list(tabs.CourseTabList.iterate_displayable(self.course, self.settings, inline_collections=False)), ) # test not including empty collections self.course.html_textbooks = [] self.assertNotIn( tabs.HtmlTextbookTabs(), - list(tabs.CourseTabList.iterate_displayable_cms(self.course, self.settings)), + list(tabs.CourseTabList.iterate_displayable(self.course, self.settings, inline_collections=False)), ) def test_get_tab_by_methods(self): @@ -698,12 +659,13 @@ class DiscussionLinkTestCase(TabTestCase): """Helper function to verify whether the discussion tab exists and can be displayed""" self.course.tabs = tab_list self.course.discussion_link = discussion_link_in_course - discussion = tabs.CourseTabList.get_discussion(self.course) + discussion_tab = tabs.CourseTabList.get_discussion(self.course) + user = self.create_mock_user(is_authenticated=True, is_staff=is_staff, is_enrolled=is_enrolled) self.assertEquals( ( - discussion is not None and - discussion.can_display(self.course, self.settings, True, is_staff, is_enrolled) and - (discussion.link_func(self.course, self._reverse(self.course)) == expected_discussion_link) + discussion_tab is not None and + self.is_tab_enabled(discussion_tab, self.course, self.settings, user) and + (discussion_tab.link_func(self.course, self._reverse(self.course)) == expected_discussion_link) ), expected_can_display_value ) diff --git a/lms/djangoapps/ccx/overrides.py b/lms/djangoapps/ccx/overrides.py index 752de4bca6..b9573a027f 100644 --- a/lms/djangoapps/ccx/overrides.py +++ b/lms/djangoapps/ccx/overrides.py @@ -64,15 +64,6 @@ def get_current_ccx(): return _CCX_CONTEXT.ccx -def get_current_request(): - """ - Return the active request, so that we can get context information in places - where it is limited, like in the tabs. - """ - request = _CCX_CONTEXT.request - return request - - def get_override_for_ccx(ccx, block, name, default=None): """ Gets the value of the overridden field for the `ccx`. `block` and `name` diff --git a/lms/djangoapps/ccx/plugins.py b/lms/djangoapps/ccx/plugins.py new file mode 100644 index 0000000000..b1611d93c3 --- /dev/null +++ b/lms/djangoapps/ccx/plugins.py @@ -0,0 +1,31 @@ +""" +Registers the CCX feature for the edX platform. +""" + +from django.utils.translation import ugettext as _ + +from openedx.core.lib.plugins.api import CourseViewType +from student.roles import CourseCcxCoachRole + + +class CcxCourseViewType(CourseViewType): + """ + The representation of the CCX course view type. + """ + + name = "ccx_coach" + title = _("CCX Coach") + view_name = "ccx_coach_dashboard" + is_persistent = False + + @classmethod + def is_enabled(cls, course, settings, user=None): + """ + Returns true if CCX has been enabled and the specified user is a coach + """ + if not user: + return True + if not settings.FEATURES.get('CUSTOM_COURSES_EDX', False) or not course.enable_ccx: + return False + role = CourseCcxCoachRole(course.id) + return role.has_user(user) diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index cca02a73bb..ddcdbebcc6 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -3,44 +3,51 @@ This module is essentially a broker to xmodule/tabs.py -- it was originally intr perform some LMS-specific tab display gymnastics for the Entrance Exams feature """ from django.conf import settings -from django.test.client import RequestFactory from django.utils.translation import ugettext as _ -from courseware.access import has_access from courseware.entrance_exams import user_must_complete_entrance_exam -from student.models import CourseEnrollment, EntranceExamConfiguration -from xmodule.tabs import CourseTabList - -from util import milestones_helpers +from xmodule.tabs import CourseTabList, CourseViewTab, CourseTabManager -def get_course_tab_list(course, user): +def get_course_tab_list(request, course): """ Retrieves the course tab list from xmodule.tabs and manipulates the set as necessary """ - user_is_enrolled = user.is_authenticated() and CourseEnrollment.is_enrolled(user, course.id) - xmodule_tab_list = CourseTabList.iterate_displayable( - course, - settings, - user.is_authenticated(), - has_access(user, 'staff', course, course.id), - user_is_enrolled - ) + user = request.user + xmodule_tab_list = CourseTabList.iterate_displayable(course, settings, user=user) # Now that we've loaded the tabs for this course, perform the Entrance Exam work # If the user has to take an entrance exam, we'll need to hide away all of the tabs # except for the Courseware and Instructor tabs (latter is only viewed if applicable) # We don't have access to the true request object in this context, but we can use a mock - request = RequestFactory().request() - request.user = user course_tab_list = [] for tab in xmodule_tab_list: if user_must_complete_entrance_exam(request, user, course): - # Hide all of the tabs except for 'Courseware' and 'Instructor' + # Hide all of the tabs except for 'Courseware' # Rename 'Courseware' tab to 'Entrance Exam' - if tab.type not in ['courseware', 'instructor']: + if tab.type is not 'courseware': continue - if tab.type == 'courseware': - tab.name = _("Entrance Exam") + tab.name = _("Entrance Exam") course_tab_list.append(tab) + + # Add in any dynamic tabs, i.e. those that are not persisted + course_tab_list += _get_dynamic_tabs(course, user) + return course_tab_list + + +def _get_dynamic_tabs(course, user): + """ + Returns the dynamic tab types for the current user. + + Note: dynamic tabs are those that are not persisted in the course, but are + instead added dynamically based upon the user's role. + """ + dynamic_tabs = list() + for tab_type in CourseTabManager.get_tab_types().values(): + if not getattr(tab_type, "is_persistent", True): + tab = CourseViewTab(tab_type) + if tab.is_enabled(course, settings, user=user): + dynamic_tabs.append(tab) + dynamic_tabs.sort(key=lambda dynamic_tab: dynamic_tab.name) + return dynamic_tabs diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 704977f934..b7b40cab31 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -54,13 +54,15 @@ class StaticTabDateTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): self.assertIn("OOGIE BLOOGIE", resp.content) def test_invalid_course_key(self): - request = get_request_for_user(UserFactory.create()) + self.setup_user() + request = get_request_for_user(self.user) with self.assertRaises(Http404): static_tab(request, course_id='edX/toy', tab_slug='new_tab') def test_get_static_tab_contents(self): + self.setup_user() course = get_course_by_id(self.toy_course_key) - request = get_request_for_user(UserFactory.create()) + request = get_request_for_user(self.user) tab = tabs.CourseTabList.get_tab_by_slug(course.tabs, 'resources') # Test render works okay @@ -162,6 +164,7 @@ class EntranceExamsTabsTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): 'description': 'Testing Courseware Tabs' } self.user.is_staff = False + request = get_request_for_user(self.user) self.course.entrance_exam_enabled = True self.course.entrance_exam_id = unicode(entrance_exam.location) milestone = milestones_helpers.add_milestone(milestone) @@ -176,7 +179,7 @@ class EntranceExamsTabsTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): self.relationship_types['FULFILLS'], milestone ) - course_tab_list = get_course_tab_list(self.course, self.user) + course_tab_list = get_course_tab_list(request, self.course) self.assertEqual(len(course_tab_list), 1) self.assertEqual(course_tab_list[0]['tab_id'], 'courseware') self.assertEqual(course_tab_list[0]['name'], 'Entrance Exam') @@ -201,7 +204,8 @@ class EntranceExamsTabsTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): # log in again as student self.client.logout() self.login(self.email, self.password) - course_tab_list = get_course_tab_list(self.course, self.user) + request = get_request_for_user(self.user) + course_tab_list = get_course_tab_list(request, self.course) self.assertEqual(len(course_tab_list), 5) def test_course_tabs_list_for_staff_members(self): @@ -213,8 +217,8 @@ class EntranceExamsTabsTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): self.client.logout() staff_user = StaffFactory(course_key=self.course.id) self.client.login(username=staff_user.username, password='test') - - course_tab_list = get_course_tab_list(self.course, staff_user) + request = get_request_for_user(staff_user) + course_tab_list = get_course_tab_list(request, self.course) self.assertEqual(len(course_tab_list), 5) @@ -256,8 +260,8 @@ class TextBookTabsTestCase(LoginEnrollmentTestCase, ModuleStoreTestCase): Test that all textbooks tab links generating correctly. """ type_to_reverse_name = {'textbook': 'book', 'pdftextbook': 'pdf_book', 'htmltextbook': 'html_book'} - - course_tab_list = get_course_tab_list(self.course, self.user) + request = get_request_for_user(self.user) + course_tab_list = get_course_tab_list(request, self.course) num_of_textbooks_found = 0 for tab in course_tab_list: # Verify links of all textbook type tabs. diff --git a/lms/djangoapps/edxnotes/plugins.py b/lms/djangoapps/edxnotes/plugins.py new file mode 100644 index 0000000000..62b1e10e9b --- /dev/null +++ b/lms/djangoapps/edxnotes/plugins.py @@ -0,0 +1,32 @@ +""" +Registers the "edX Notes" feature for the edX platform. +""" + +from django.utils.translation import ugettext as _ + +from openedx.core.lib.plugins.api import CourseViewType + + +class EdxNotesCourseViewType(CourseViewType): + """ + The representation of the edX Notes course view type. + """ + + name = "edxnotes" + title = _("Notes") + view_name = "edxnotes" + is_persistent = True + + # The course field that indicates that this feature is enabled + feature_flag_field_name = "edxnotes" + + @classmethod + def is_enabled(cls, course, settings, user=None): # pylint: disable=unused-argument + """Returns true if the edX Notes feature is enabled in the course. + + Args: + course (CourseDescriptor): the course using the feature + settings (dict): a dict of configuration settings + user (User): the user interacting with the course + """ + return course.edxnotes diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index cce3aeb9f7..398a64fda7 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -6,6 +6,7 @@ import jwt from mock import patch, MagicMock from unittest import skipUnless from datetime import datetime + from edxmako.shortcuts import render_to_string from edxnotes import helpers from edxnotes.decorators import edxnotes @@ -13,15 +14,17 @@ from edxnotes.exceptions import EdxNotesParseError, EdxNotesServiceUnavailable from django.conf import settings from django.core.urlresolvers import reverse from django.core.exceptions import ImproperlyConfigured +from django.test.client import RequestFactory from oauth2_provider.tests.factories import ClientFactory from provider.oauth2.models import Client -from xmodule.tabs import EdxNotesTab from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore +from xmodule.tabs import CourseTab from courseware.model_data import FieldDataCache from courseware.module_render import get_module_for_descriptor +from courseware.tabs import get_course_tab_list from student.tests.factories import UserFactory @@ -29,7 +32,7 @@ def enable_edxnotes_for_the_course(course, user_id): """ Enable EdxNotes for the course. """ - course.tabs.append(EdxNotesTab()) + course.tabs.append(CourseTab.from_json({"type": "edxnotes", "name": "Notes"})) modulestore().update_item(course, user_id) @@ -808,6 +811,21 @@ class EdxNotesViewsTest(ModuleStoreTestCase): field_data_cache = FieldDataCache([self.course], self.course.id, self.user) return get_module_for_descriptor(self.user, MagicMock(), self.course, field_data_cache, self.course.id) + def test_edxnotes_tab(self): + """ + Tests that edxnotes tab is shown only when the feature is enabled. + """ + def has_notes_tab(user, course): + """Returns true if the "Notes" tab is shown.""" + request = RequestFactory().request() + request.user = user + tabs = get_course_tab_list(request, course) + return len([tab for tab in tabs if tab.name == 'Notes']) == 1 + + self.assertFalse(has_notes_tab(self.user, self.course)) + enable_edxnotes_for_the_course(self.course, self.user.id) + self.assertTrue(has_notes_tab(self.user, self.course)) + # pylint: disable=unused-argument @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) @patch("edxnotes.views.get_notes", return_value=[]) diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index e53e41535b..0439c25e13 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -6,7 +6,11 @@ from mock import patch from django.conf import settings from django.core.urlresolvers import reverse +from django.test.client import RequestFactory from django.test.utils import override_settings + +from courseware.tabs import get_course_tab_list +from courseware.tests.factories import UserFactory from courseware.tests.helpers import LoginEnrollmentTestCase from student.tests.factories import AdminFactory, UserFactory @@ -56,6 +60,21 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase): return 'Demographic data is now available in Example.'.format(unicode(self.course.id)) + def test_instructor_tab(self): + """ + Verify that the instructor tab appears for staff only. + """ + def has_instructor_tab(user, course): + """Returns true if the "Instructor" tab is shown.""" + request = RequestFactory().request() + request.user = user + tabs = get_course_tab_list(request, course) + return len([tab for tab in tabs if tab.name == 'Instructor']) == 1 + + self.assertTrue(has_instructor_tab(self.instructor, self.course)) + student = UserFactory.create() + self.assertFalse(has_instructor_tab(student, self.course)) + def test_default_currency_in_the_html_response(self): """ Test that checks the default currency_symbol ($) in the response diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index ab93882d3e..869ce50534 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -38,15 +38,33 @@ from course_modes.models import CourseMode, CourseModesArchive from student.roles import CourseFinanceAdminRole, CourseSalesAdminRole from certificates.models import CertificateGenerationConfiguration from certificates import api as certs_api +from openedx.core.lib.plugins.api import CourseViewType from class_dashboard.dashboard_data import get_section_display_name, get_array_section_has_problem from .tools import get_units_with_due_date, title_or_url, bulk_email_is_enabled_for_course from opaque_keys.edx.locations import SlashSeparatedCourseKey - log = logging.getLogger(__name__) +class InstructorDashboardViewType(CourseViewType): + """ + Defines the Instructor Dashboard view type that is shown as a course tab. + """ + + name = "instructor" + title = _('Instructor') + view_name = "instructor_dashboard" + is_persistent = False + + @classmethod + def is_enabled(cls, course, settings, user=None): # pylint: disable=unused-argument,redefined-outer-name + """ + Returns true if the specified user has staff access. + """ + return user and has_access(user, 'staff', course, course.id) + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) def instructor_dashboard_2(request, course_id): diff --git a/lms/templates/courseware/course_navigation.html b/lms/templates/courseware/course_navigation.html index 801279c7d6..6c78313fa3 100644 --- a/lms/templates/courseware/course_navigation.html +++ b/lms/templates/courseware/course_navigation.html @@ -54,7 +54,7 @@ def url_class(is_active):