From 4d75c18098b39d9dd271e5894e99cb95a1591e3d Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Mon, 8 Dec 2014 18:30:44 -0500 Subject: [PATCH] Fix Mako templates to use latest request context Fixed the edX middleware to lazily create request context for Mako templates, so that it is based on any changes made to the request object. Verified that with this fix the "Instructor" tab is correctly hidden when a staff member is viewing the course as a student. --- common/djangoapps/edxmako/middleware.py | 26 +++++++++++++---- common/djangoapps/edxmako/shortcuts.py | 13 +++++---- common/djangoapps/edxmako/template.py | 21 +++++++------- common/djangoapps/edxmako/tests.py | 5 ++-- .../test/acceptance/pages/lms/course_page.py | 9 ++++++ .../test/acceptance/pages/lms/staff_view.py | 5 +++- common/test/acceptance/pages/lms/tab_nav.py | 4 +-- .../tests/lms/test_lms_staff_view.py | 29 ++++++++++++++++--- .../tests/studio/test_studio_container.py | 2 +- .../tests/studio/test_studio_outline.py | 4 +-- lms/djangoapps/courseware/module_render.py | 4 +-- .../courseware/tests/test_module_render.py | 6 ++-- lms/djangoapps/courseware/tests/test_views.py | 3 +- lms/djangoapps/courseware/views.py | 13 +++++---- 14 files changed, 100 insertions(+), 44 deletions(-) diff --git a/common/djangoapps/edxmako/middleware.py b/common/djangoapps/edxmako/middleware.py index 17f164272a..5d9f8cbdfc 100644 --- a/common/djangoapps/edxmako/middleware.py +++ b/common/djangoapps/edxmako/middleware.py @@ -7,7 +7,7 @@ # http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software -# distribuetd under the License is distributed on an "AS IS" BASIS, +# distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. @@ -22,10 +22,24 @@ REQUEST_CONTEXT = threading.local() class MakoMiddleware(object): def process_request(self, request): - REQUEST_CONTEXT.context = RequestContext(request) - REQUEST_CONTEXT.context['is_secure'] = request.is_secure() - REQUEST_CONTEXT.context['site'] = safe_get_host(request) + """ Process the middleware request. """ + REQUEST_CONTEXT.request = request - def process_response(self, request, response): - REQUEST_CONTEXT.context = None + def process_response(self, __, response): + """ Process the middleware response. """ + REQUEST_CONTEXT.request = None return response + + +def get_template_request_context(): + """ + Returns the template processing context to use for the current request, + or returns None if there is not a current request. + """ + request = getattr(REQUEST_CONTEXT, "request", None) + if not request: + return None + context = RequestContext(request) + context['is_secure'] = request.is_secure() + context['site'] = safe_get_host(request) + return context diff --git a/common/djangoapps/edxmako/shortcuts.py b/common/djangoapps/edxmako/shortcuts.py index 1e2566e184..f7e90823d8 100644 --- a/common/djangoapps/edxmako/shortcuts.py +++ b/common/djangoapps/edxmako/shortcuts.py @@ -19,7 +19,7 @@ import logging from microsite_configuration import microsite from edxmako import lookup_template -import edxmako.middleware +from edxmako.middleware import get_template_request_context from django.conf import settings from django.core.urlresolvers import reverse log = logging.getLogger(__name__) @@ -114,11 +114,12 @@ def render_to_string(template_name, dictionary, context=None, namespace='main'): context_instance['marketing_link'] = marketing_link # In various testing contexts, there might not be a current request context. - if getattr(edxmako.middleware.REQUEST_CONTEXT, "context", None): - for d in edxmako.middleware.REQUEST_CONTEXT.context: - context_dictionary.update(d) - for d in context_instance: - context_dictionary.update(d) + request_context = get_template_request_context() + if request_context: + for item in request_context: + context_dictionary.update(item) + for item in context_instance: + context_dictionary.update(item) if context: context_dictionary.update(context) # fetch and render template diff --git a/common/djangoapps/edxmako/template.py b/common/djangoapps/edxmako/template.py index 677e88bcbe..3d25f64d4e 100644 --- a/common/djangoapps/edxmako/template.py +++ b/common/djangoapps/edxmako/template.py @@ -12,12 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. -from django.conf import settings -from mako.template import Template as MakoTemplate -from edxmako.shortcuts import marketing_link - import edxmako -import edxmako.middleware + +from django.conf import settings +from edxmako.middleware import get_template_request_context +from edxmako.shortcuts import marketing_link +from mako.template import Template as MakoTemplate DJANGO_VARIABLES = ['output_encoding', 'encoding_errors'] @@ -48,11 +48,12 @@ class Template(MakoTemplate): context_dictionary = {} # In various testing contexts, there might not be a current request context. - if getattr(edxmako.middleware.REQUEST_CONTEXT, "context", None): - for d in edxmako.middleware.REQUEST_CONTEXT.context: - context_dictionary.update(d) - for d in context_instance: - context_dictionary.update(d) + request_context = get_template_request_context() + if request_context: + for item in request_context: + context_dictionary.update(item) + for item in context_instance: + context_dictionary.update(item) context_dictionary['settings'] = settings context_dictionary['EDX_ROOT_URL'] = settings.EDX_ROOT_URL context_dictionary['django_context'] = context_instance diff --git a/common/djangoapps/edxmako/tests.py b/common/djangoapps/edxmako/tests.py index 318af76cb8..16b7ced407 100644 --- a/common/djangoapps/edxmako/tests.py +++ b/common/djangoapps/edxmako/tests.py @@ -10,6 +10,7 @@ from django.test.utils import override_settings from django.test.client import RequestFactory from django.core.urlresolvers import reverse import edxmako.middleware +from edxmako.middleware import get_template_request_context from edxmako import add_lookup, LOOKUP from edxmako.shortcuts import ( marketing_link, @@ -83,11 +84,11 @@ class MakoMiddlewareTest(TestCase): self.middleware.process_request(self.request) # requestcontext should not be None. - self.assertIsNotNone(edxmako.middleware.REQUEST_CONTEXT.context) + self.assertIsNotNone(get_template_request_context()) self.middleware.process_response(self.request, self.response) # requestcontext should be None. - self.assertIsNone(edxmako.middleware.REQUEST_CONTEXT.context) + self.assertIsNone(get_template_request_context()) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') @patch("edxmako.middleware.REQUEST_CONTEXT") diff --git a/common/test/acceptance/pages/lms/course_page.py b/common/test/acceptance/pages/lms/course_page.py index 1453f5cd3c..94943a9424 100644 --- a/common/test/acceptance/pages/lms/course_page.py +++ b/common/test/acceptance/pages/lms/course_page.py @@ -4,6 +4,7 @@ Base class for pages in courseware. from bok_choy.page_object import PageObject from . import BASE_URL +from .tab_nav import TabNavPage class CoursePage(PageObject): @@ -29,3 +30,11 @@ class CoursePage(PageObject): Construct a URL to the page within the course. """ return BASE_URL + "/courses/" + self.course_id + "/" + self.url_path + + def has_tab(self, tab_name): + """ + Returns true if the current page is showing a tab with the given name. + :return: + """ + tab_nav = TabNavPage(self.browser) + return tab_name in tab_nav.tab_names diff --git a/common/test/acceptance/pages/lms/staff_view.py b/common/test/acceptance/pages/lms/staff_view.py index 48dfe7a3d6..a434b7cc69 100644 --- a/common/test/acceptance/pages/lms/staff_view.py +++ b/common/test/acceptance/pages/lms/staff_view.py @@ -2,9 +2,10 @@ Staff view of courseware """ from bok_choy.page_object import PageObject +from .courseware import CoursewarePage -class StaffPage(PageObject): +class StaffPage(CoursewarePage): """ View of courseware pages while logged in as course staff """ @@ -13,6 +14,8 @@ class StaffPage(PageObject): STAFF_STATUS_CSS = '#staffstatus' def is_browser_on_page(self): + if not super(StaffPage, self).is_browser_on_page(): + return False return self.q(css=self.STAFF_STATUS_CSS).present @property diff --git a/common/test/acceptance/pages/lms/tab_nav.py b/common/test/acceptance/pages/lms/tab_nav.py index 96f36bfd88..861ee5dd8b 100644 --- a/common/test/acceptance/pages/lms/tab_nav.py +++ b/common/test/acceptance/pages/lms/tab_nav.py @@ -48,7 +48,7 @@ class TabNavPage(PageObject): Return the CSS to click for `tab_name`. If no tabs exist for that name, return `None`. """ - all_tabs = self._tab_names + all_tabs = self.tab_names try: tab_index = all_tabs.index(tab_name) @@ -58,7 +58,7 @@ class TabNavPage(PageObject): return 'ol.course-tabs li:nth-of-type({0}) a'.format(tab_index + 1) @property - def _tab_names(self): + def tab_names(self): """ Return the list of available tab names. If no tab names are available, wait for them to load. Raises a `BrokenPromiseError` diff --git a/common/test/acceptance/tests/lms/test_lms_staff_view.py b/common/test/acceptance/tests/lms/test_lms_staff_view.py index 3d6bf77c09..c148c43fb1 100644 --- a/common/test/acceptance/tests/lms/test_lms_staff_view.py +++ b/common/test/acceptance/tests/lms/test_lms_staff_view.py @@ -11,15 +11,15 @@ from ...fixtures.course import CourseFixture, XBlockFixtureDesc from textwrap import dedent -class StaffDebugTest(UniqueCourseTest): +class StaffViewTest(UniqueCourseTest): """ - Tests that verify the staff debug info. + Tests that verify the staff view. """ USERNAME = "STAFF_TESTER" EMAIL = "johndoe@example.com" def setUp(self): - super(StaffDebugTest, self).setUp() + super(StaffViewTest, self).setUp() self.courseware_page = CoursewarePage(self.browser, self.course_id) @@ -59,10 +59,31 @@ class StaffDebugTest(UniqueCourseTest): Open staff page with assertion """ self.courseware_page.visit() - staff_page = StaffPage(self.browser) + staff_page = StaffPage(self.browser, self.course_id) self.assertEqual(staff_page.staff_status, 'Staff view') return staff_page + +class StaffViewToggleTest(StaffViewTest): + """ + Tests for the staff view toggle button. + """ + def test_instructor_tab_visibility(self): + """ + Test that the instructor tab is hidden when viewing as a student. + """ + + course_page = self._goto_staff_page() + self.assertTrue(course_page.has_tab('Instructor')) + course_page.toggle_staff_view() + self.assertEqual(course_page.staff_status, 'Student view') + self.assertFalse(course_page.has_tab('Instructor')) + + +class StaffDebugTest(StaffViewTest): + """ + Tests that verify the staff debug info. + """ def test_reset_attempts_empty(self): """ Test that we reset even when there is no student state diff --git a/common/test/acceptance/tests/studio/test_studio_container.py b/common/test/acceptance/tests/studio/test_studio_container.py index 39c4f5432c..bf9ce68c58 100644 --- a/common/test/acceptance/tests/studio/test_studio_container.py +++ b/common/test/acceptance/tests/studio/test_studio_container.py @@ -678,7 +678,7 @@ class UnitPublishingTest(ContainerBase): """ Verifies that the browser is on the staff page and returns a StaffPage. """ - page = StaffPage(self.browser) + page = StaffPage(self.browser, self.course_id) EmptyPromise(page.is_browser_on_page, 'Browser is on staff page in LMS').fulfill() return page diff --git a/common/test/acceptance/tests/studio/test_studio_outline.py b/common/test/acceptance/tests/studio/test_studio_outline.py index 1879580b21..fc787d7ed7 100644 --- a/common/test/acceptance/tests/studio/test_studio_outline.py +++ b/common/test/acceptance/tests/studio/test_studio_outline.py @@ -718,7 +718,7 @@ class StaffLockTest(CourseOutlineTest): courseware = CoursewarePage(self.browser, self.course_id) courseware.wait_for_page() self.assertEqual(courseware.num_sections, 2) - StaffPage(self.browser).toggle_staff_view() + StaffPage(self.browser, self.course_id).toggle_staff_view() self.assertEqual(courseware.num_sections, 1) def test_locked_subsections_do_not_appear_in_lms(self): @@ -737,7 +737,7 @@ class StaffLockTest(CourseOutlineTest): courseware = CoursewarePage(self.browser, self.course_id) courseware.wait_for_page() self.assertEqual(courseware.num_subsections, 2) - StaffPage(self.browser).toggle_staff_view() + StaffPage(self.browser, self.course_id).toggle_staff_view() self.assertEqual(courseware.num_subsections, 1) def test_toggling_staff_lock_on_section_does_not_publish_draft_units(self): diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 98d44fe9df..5b79b9dd78 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -92,7 +92,7 @@ def make_track_function(request): return function -def toc_for_course(user, request, course, active_chapter, active_section, field_data_cache): +def toc_for_course(request, course, active_chapter, active_section, field_data_cache): ''' Create a table of contents from the module store @@ -117,7 +117,7 @@ def toc_for_course(user, request, course, active_chapter, active_section, field_ ''' with modulestore().bulk_operations(course.id): - course_module = get_module_for_descriptor(user, request, course, field_data_cache, course.id) + course_module = get_module_for_descriptor(request.user, request, course, field_data_cache, course.id) if course_module is None: return None diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index f3fee3113d..28c595fe8a 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -394,7 +394,7 @@ class TestTOC(ModuleStoreTestCase): with check_mongo_calls(toc_finds): actual = render.toc_for_course( - self.request.user, self.request, self.toy_course, self.chapter, None, self.field_data_cache + self.request, self.toy_course, self.chapter, None, self.field_data_cache ) for toc_section in expected: self.assertIn(toc_section, actual) @@ -432,7 +432,9 @@ class TestTOC(ModuleStoreTestCase): 'url_name': 'secret:magic', 'display_name': 'secret:magic'}]) with check_mongo_calls(toc_finds): - actual = render.toc_for_course(self.request.user, self.request, self.toy_course, self.chapter, section, self.field_data_cache) + actual = render.toc_for_course( + self.request, self.toy_course, self.chapter, section, self.field_data_cache + ) for toc_section in expected: self.assertIn(toc_section, actual) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index fd3d33d94f..0a56131d8e 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -555,7 +555,8 @@ class TestAccordionDueDate(BaseDueDateTests): def get_text(self, course): """ Returns the HTML for the accordion """ return views.render_accordion( - self.request, course, course.get_children()[0].scope_ids.usage_id.to_deprecated_string(), None, None + self.request, course, course.get_children()[0].scope_ids.usage_id.to_deprecated_string(), + None, None ) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index f8393c43f1..d452d7a3a7 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -118,9 +118,7 @@ def render_accordion(request, course, chapter, section, field_data_cache): Returns the html string """ # grab the table of contents - user = User.objects.prefetch_related("groups").get(id=request.user.id) - request.user = user # keep just one instance of User - toc = toc_for_course(user, request, course, chapter, section, field_data_cache) + toc = toc_for_course(request, course, chapter, section, field_data_cache) context = dict([ ('toc', toc), @@ -325,10 +323,15 @@ def index(request, course_id, chapter=None, section=None, request.user = user # keep just one instance of User with modulestore().bulk_operations(course_key): - return _index_bulk_op(request, user, course_key, chapter, section, position) + return _index_bulk_op(request, course_key, chapter, section, position) -def _index_bulk_op(request, user, course_key, chapter, section, position): +# pylint: disable=too-many-statements +def _index_bulk_op(request, course_key, chapter, section, position): + """ + Render the index page for the specified course. + """ + user = request.user course = get_course_with_access(user, 'load', course_key, depth=2) staff_access = has_access(user, 'staff', course)