Disabled anonymous access for tabs that require enrollment

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)

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.

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):

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)

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(),

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)

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):

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 = ["foo", "boo", "videoannotation"]
-        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 = ["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)

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:

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())}
       </main>
   </section>
 </div>
-% if course.show_calculator or is_edxnotes_enabled(course):
+% if course.show_calculator or is_edxnotes_enabled(course, request.user):
     <nav class="nav-utilities ${"has-utility-calculator" if course.show_calculator else ""}" aria-label="${_('Course Utilities')}">
       ## Utility: Notes
-      % if is_edxnotes_enabled(course):
+      % if is_edxnotes_enabled(course, request.user):
         <%include file="/edxnotes/toggle_notes.html" args="course=course"/>
       % endif
 

lms/templates/courseware/courseware.html

@@ -51,7 +51,7 @@ from openedx.features.course_experience import course_home_page_title, COURSE_OU
 <%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
 
@@ -250,10 +250,10 @@ ${HTML(fragment.foot_html())}
     </div>
   % endif
 </div>
-% if course.show_calculator or is_edxnotes_enabled(course):
+% if course.show_calculator or is_edxnotes_enabled(course, request.user):
     <nav class="nav-utilities ${"has-utility-calculator" if course.show_calculator else ""}" aria-label="${_('Course Utilities')}">
       ## Utility: Notes
-      % if is_edxnotes_enabled(course):
+      % if is_edxnotes_enabled(course, request.user):
         <%include file="/edxnotes/toggle_notes.html" args="course=course"/>
       % endif