diff --git a/cms/djangoapps/contentstore/views/tabs.py b/cms/djangoapps/contentstore/views/tabs.py index a05f4f79d1..bfa08e4661 100644 --- a/cms/djangoapps/contentstore/views/tabs.py +++ b/cms/djangoapps/contentstore/views/tabs.py @@ -60,7 +60,8 @@ 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(course_item, inline_collections=False): + for tab in CourseTabList.iterate_displayable(course_item, user=request.user, inline_collections=False, + include_hidden=True): 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/common/lib/xmodule/xmodule/tabs.py b/common/lib/xmodule/xmodule/tabs.py index fbea4aa858..339a39011f 100644 --- a/common/lib/xmodule/xmodule/tabs.py +++ b/common/lib/xmodule/xmodule/tabs.py @@ -442,13 +442,13 @@ class CourseTabList(List): return next((tab for tab in tab_list if tab.tab_id == tab_id), None) @staticmethod - def iterate_displayable(course, user=None, inline_collections=True): + def iterate_displayable(course, user=None, inline_collections=True, include_hidden=False): """ 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.is_enabled(course, user=user) and not (user and tab.is_hidden): + if tab.is_enabled(course, user=user) and (include_hidden or not (user and tab.is_hidden)): if tab.is_collection: # If rendering inline that add each item in the collection, # else just show the tab itself as long as it is not empty. diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index bcfd6ca6eb..2111a1ae46 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -19,9 +19,8 @@ class EnrolledTab(CourseTab): """ @classmethod def is_enabled(cls, course, user=None): - if user is None: - return True - return bool(CourseEnrollment.is_enrolled(user, course.id) or has_access(user, 'staff', course, course.id)) + return user and user.is_authenticated() and \ + bool(CourseEnrollment.is_enrolled(user, course.id) or has_access(user, 'staff', course, course.id)) class CoursewareTab(EnrolledTab): diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index db49724d16..88c5502770 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -7,7 +7,6 @@ from django.http import Http404 from milestones.tests.utils import MilestonesTestCaseMixin from mock import MagicMock, Mock, patch from nose.plugins.attrib import attr -from waffle.testutils import override_flag from courseware.courses import get_course_by_id from courseware.tabs import ( @@ -651,11 +650,10 @@ class CourseTabListTestCase(TabListTestCase): self.course.tabs = self.all_valid_tab_list # enumerate the tabs with no user - for i, tab in enumerate(xmodule_tabs.CourseTabList.iterate_displayable( - self.course, - inline_collections=False - )): - self.assertEquals(tab.type, self.course.tabs[i].type) + expected = [tab.type for tab in + xmodule_tabs.CourseTabList.iterate_displayable(self.course, inline_collections=False)] + actual = [tab.type for tab in self.course.tabs if tab.is_enabled(self.course, user=None)] + assert actual == expected # enumerate the tabs with a staff user user = UserFactory(is_staff=True) diff --git a/lms/djangoapps/edxnotes/decorators.py b/lms/djangoapps/edxnotes/decorators.py index 31b85db80f..3e8b3ed327 100644 --- a/lms/djangoapps/edxnotes/decorators.py +++ b/lms/djangoapps/edxnotes/decorators.py @@ -30,10 +30,10 @@ def edxnotes(cls): # - the feature flag or `edxnotes` setting of the course is set to False # - the user is not authenticated user = self.runtime.get_real_user(self.runtime.anonymous_student_id) - if is_studio or not is_feature_enabled(course) or not user.is_authenticated(): + + if is_studio or not is_feature_enabled(course, user): return original_get_html(self, *args, **kwargs) else: - return render_to_string("edxnotes_wrapper.html", { "content": original_get_html(self, *args, **kwargs), "uid": generate_uid(), diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index 04dbb1ffbb..de6050339a 100644 --- a/lms/djangoapps/edxnotes/helpers.py +++ b/lms/djangoapps/edxnotes/helpers.py @@ -423,8 +423,8 @@ def generate_uid(): return uuid4().int # pylint: disable=no-member -def is_feature_enabled(course): +def is_feature_enabled(course, user): """ Returns True if Student Notes feature is enabled for the course, False otherwise. """ - return EdxNotesTab.is_enabled(course) + return EdxNotesTab.is_enabled(course, user) diff --git a/lms/djangoapps/edxnotes/plugins.py b/lms/djangoapps/edxnotes/plugins.py index 81243e13d9..74ddad5b12 100644 --- a/lms/djangoapps/edxnotes/plugins.py +++ b/lms/djangoapps/edxnotes/plugins.py @@ -22,7 +22,6 @@ class EdxNotesTab(EnrolledTab): Args: course (CourseDescriptor): the course using the feature - settings (dict): a dict of configuration settings user (User): the user interacting with the course """ if not super(EdxNotesTab, cls).is_enabled(course, user=user): diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index 8ee63cf361..7115a68995 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -9,6 +9,7 @@ from unittest import skipUnless import ddt import jwt +import pytest from django.conf import settings from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ImproperlyConfigured @@ -76,8 +77,8 @@ class TestProblem(object): def __init__(self, course, user=None): self.system = MagicMock(is_author_mode=False) self.scope_ids = MagicMock(usage_id="test_usage_id") - self.user = user or UserFactory() - self.runtime = MagicMock(course_id=course.id, get_real_user=lambda anon_id: self.user) + user = user or UserFactory() + self.runtime = MagicMock(course_id=course.id, get_real_user=lambda __: user) self.descriptor = MagicMock() self.descriptor.runtime.modulestore.get_course.return_value = course @@ -104,7 +105,7 @@ class EdxNotesDecoratorTest(ModuleStoreTestCase): self.course = CourseFactory(edxnotes=True, default_store=ModuleStoreEnum.Type.mongo) self.user = UserFactory() self.client.login(username=self.user.username, password=UserFactory._DEFAULT_PASSWORD) - self.problem = TestProblem(self.course) + self.problem = TestProblem(self.course, self.user) @patch.dict("django.conf.settings.FEATURES", {'ENABLE_EDXNOTES': True}) @patch("edxnotes.helpers.get_public_endpoint", autospec=True) @@ -116,18 +117,23 @@ class EdxNotesDecoratorTest(ModuleStoreTestCase): Tests if get_html is wrapped when feature flag is on and edxnotes are enabled for the course. """ + course = CourseFactory(edxnotes=True) + enrollment = CourseEnrollmentFactory(course_id=course.id) + user = enrollment.user + problem = TestProblem(course, user) + mock_generate_uid.return_value = "uid" mock_get_id_token.return_value = "token" mock_get_token_url.return_value = "/tokenUrl" mock_get_endpoint.return_value = "/endpoint" - enable_edxnotes_for_the_course(self.course, self.user.id) + enable_edxnotes_for_the_course(course, user.id) expected_context = { "content": "original_get_html", "uid": "uid", "edxnotes_visibility": "true", "params": { - "usageId": u"test_usage_id", - "courseId": unicode(self.course.id).encode("utf-8"), + "usageId": "test_usage_id", + "courseId": course.id, "token": "token", "tokenUrl": "/tokenUrl", "endpoint": "/endpoint", @@ -136,7 +142,7 @@ class EdxNotesDecoratorTest(ModuleStoreTestCase): }, } self.assertEqual( - self.problem.get_html(), + problem.get_html(), render_to_string("edxnotes_wrapper.html", expected_context), ) @@ -225,8 +231,8 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): self.child_vertical = self.store.get_item(self.child_vertical.location) self.child_html_module = self.store.get_item(self.child_html_module.location) - self.user = UserFactory.create(username="Joe", email="joe@example.com", password="edx") - self.client.login(username=self.user.username, password="edx") + self.user = UserFactory() + self.client.login(username=self.user.username, password=UserFactory._DEFAULT_PASSWORD) self.request = RequestFactory().request() self.request.user = self.user @@ -246,29 +252,17 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): """ Tests that edxnotes are disabled when Harvard Annotation Tool is enabled. """ - self.course.advanced_modules = ["foo", "imageannotation", "boo"] - self.assertFalse(helpers.is_feature_enabled(self.course)) + self.course.advanced_modules = ['imageannotation', 'textannotation', 'videoannotation'] + assert not helpers.is_feature_enabled(self.course, self.user) - self.course.advanced_modules = ["foo", "boo", "videoannotation"] - self.assertFalse(helpers.is_feature_enabled(self.course)) - - self.course.advanced_modules = ["textannotation", "foo", "boo"] - self.assertFalse(helpers.is_feature_enabled(self.course)) - - self.course.advanced_modules = ["textannotation", "videoannotation", "imageannotation"] - self.assertFalse(helpers.is_feature_enabled(self.course)) - - @ddt.unpack - @ddt.data( - {'_edxnotes': True}, - {'_edxnotes': False} - ) - def test_is_feature_enabled(self, _edxnotes): + @ddt.data(True, False) + def test_is_feature_enabled(self, enabled): """ Tests that is_feature_enabled shows correct behavior. """ - self.course.edxnotes = _edxnotes - self.assertEqual(helpers.is_feature_enabled(self.course), _edxnotes) + course = CourseFactory(edxnotes=enabled) + enrollment = CourseEnrollmentFactory(course_id=course.id) + assert helpers.is_feature_enabled(course, enrollment.user) == enabled @ddt.data( helpers.get_public_endpoint, @@ -947,10 +941,10 @@ class EdxNotesViewsTest(ModuleStoreTestCase): def setUp(self): ClientFactory(name="edx-notes") super(EdxNotesViewsTest, self).setUp() - self.course = CourseFactory.create(edxnotes=True) - self.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx") - CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) - self.client.login(username=self.user.username, password="edx") + self.course = CourseFactory(edxnotes=True) + self.user = UserFactory() + CourseEnrollmentFactory(user=self.user, course_id=self.course.id) + self.client.login(username=self.user.username, password=UserFactory._DEFAULT_PASSWORD) self.notes_page_url = reverse("edxnotes", args=[unicode(self.course.id)]) self.notes_url = reverse("notes", args=[unicode(self.course.id)]) self.get_token_url = reverse("get_token", args=[unicode(self.course.id)]) @@ -1144,38 +1138,27 @@ class EdxNotesPluginTest(ModuleStoreTestCase): def setUp(self): super(EdxNotesPluginTest, self).setUp() self.course = CourseFactory.create(edxnotes=True) - self.user = UserFactory.create(username="ma", email="ma@ma.info", password="edx") + self.user = UserFactory() CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) - def test_edxnotes_tab_with_unauthorized_user(self): - """ - Verify EdxNotesTab visibility when user is unauthroized. - """ - user = UserFactory.create(username="ma1", email="ma1@ma1.info", password="edx") - self.assertFalse(EdxNotesTab.is_enabled(self.course, user=user)) + def test_edxnotes_tab_with_unenrolled_user(self): + user = UserFactory() + assert not EdxNotesTab.is_enabled(self.course, user=user) - @ddt.unpack - @ddt.data( - {'enable_edxnotes': False}, - {'enable_edxnotes': True} - ) - def test_edxnotes_tab_with_feature_flag(self, enable_edxnotes): + @ddt.data(True, False) + def test_edxnotes_tab_with_feature_flag(self, enabled): """ Verify EdxNotesTab visibility when ENABLE_EDXNOTES feature flag is enabled/disabled. """ - FEATURES['ENABLE_EDXNOTES'] = enable_edxnotes + FEATURES['ENABLE_EDXNOTES'] = enabled with override_settings(FEATURES=FEATURES): - self.assertEqual(EdxNotesTab.is_enabled(self.course), enable_edxnotes) + assert EdxNotesTab.is_enabled(self.course, self.user) == enabled - @ddt.unpack - @ddt.data( - {'harvard_notes_enabled': False}, - {'harvard_notes_enabled': True} - ) + @ddt.data(True, False) def test_edxnotes_tab_with_harvard_notes(self, harvard_notes_enabled): """ Verify EdxNotesTab visibility when harvard notes feature is enabled/disabled. """ with patch("edxnotes.plugins.is_harvard_notes_enabled") as mock_harvard_notes_enabled: mock_harvard_notes_enabled.return_value = harvard_notes_enabled - self.assertEqual(EdxNotesTab.is_enabled(self.course), not harvard_notes_enabled) + assert EdxNotesTab.is_enabled(self.course, self.user) == (not harvard_notes_enabled) diff --git a/lms/djangoapps/edxnotes/views.py b/lms/djangoapps/edxnotes/views.py index 96a582976b..1c0a889ae2 100644 --- a/lms/djangoapps/edxnotes/views.py +++ b/lms/djangoapps/edxnotes/views.py @@ -45,7 +45,7 @@ def edxnotes(request, course_id): course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, "load", course_key) - if not is_feature_enabled(course): + if not is_feature_enabled(course, request.user): raise Http404 notes_info = get_notes(request, course) @@ -149,7 +149,7 @@ def notes(request, course_id): course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, 'load', course_key) - if not is_feature_enabled(course): + if not is_feature_enabled(course, request.user): raise Http404 page = request.GET.get('page') or DEFAULT_PAGE @@ -191,7 +191,7 @@ def edxnotes_visibility(request, course_id): request.user, request, course, field_data_cache, course_key, course=course ) - if not is_feature_enabled(course): + if not is_feature_enabled(course, request.user): raise Http404 try: diff --git a/lms/templates/courseware/courseware-chromeless.html b/lms/templates/courseware/courseware-chromeless.html index 9ab5124a3f..845af68d8e 100644 --- a/lms/templates/courseware/courseware-chromeless.html +++ b/lms/templates/courseware/courseware-chromeless.html @@ -36,7 +36,7 @@ ${static.get_page_title_breadcrumbs(course_name())} <%static:css group='style-course-vendor'/> <%static:css group='style-course'/> ## Utility: Notes -% if is_edxnotes_enabled(course): +% if is_edxnotes_enabled(course, request.user): <%static:css group='style-student-notes'/> % endif @@ -76,10 +76,10 @@ ${HTML(fragment.foot_html())} -% if course.show_calculator or is_edxnotes_enabled(course): +% if course.show_calculator or is_edxnotes_enabled(course, request.user):