diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 2d9a66be1e..b188036492 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -16,7 +16,7 @@ from xmodule.modulestore.django import modulestore from xmodule.x_module import ModuleSystem from xblock.runtime import DbModel -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData from util.sandboxing import can_execute_unsafe_code @@ -149,7 +149,7 @@ def load_preview_module(request, preview_id, descriptor): student_data = DbModel(SessionKeyValueStore(request)) descriptor.bind_for_student( preview_module_system(request, preview_id, descriptor), - lms_field_data(descriptor._field_data, student_data), # pylint: disable=protected-access + LmsFieldData(descriptor._field_data, student_data), # pylint: disable=protected-access ) return descriptor diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index c9f8ca4eab..5a739e5fa4 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -751,6 +751,7 @@ class XModuleDescriptor(XModuleMixin, HTMLSnippet, ResourceTemplates, XBlock): handle_ajax = module_attr('handle_ajax') max_score = module_attr('max_score') student_view = module_attr('student_view') + get_child_descriptors = module_attr('get_child_descriptors') # ~~~~~~~~~~~~~~~ XBlock API Wrappers ~~~~~~~~~~~~~~~~ def studio_view(self, _context): diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index f4b1c64a9f..65c72cdbe1 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -38,7 +38,7 @@ from xblock.runtime import KeyValueStore from xblock.fields import Scope from util.sandboxing import can_execute_unsafe_code from util.json_request import JsonResponse -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData log = logging.getLogger(__name__) @@ -220,7 +220,7 @@ def get_module_for_descriptor_internal(user, descriptor, field_data_cache, cours return None student_data = DbModel(DjangoKeyValueStore(field_data_cache)) - descriptor._field_data = lms_field_data(descriptor._field_data, student_data) + descriptor._field_data = LmsFieldData(descriptor._field_data, student_data) # Setup system context for module instance ajax_url = reverse( diff --git a/lms/djangoapps/courseware/tests/__init__.py b/lms/djangoapps/courseware/tests/__init__.py index 91be45516d..3fcfd42324 100644 --- a/lms/djangoapps/courseware/tests/__init__.py +++ b/lms/djangoapps/courseware/tests/__init__.py @@ -21,7 +21,7 @@ from xmodule.modulestore import Location from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from lms.xblock.field_data import lms_field_data +from lms.xblock.field_data import LmsFieldData @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -107,7 +107,7 @@ class BaseTestXmodule(ModuleStoreTestCase): field_data = {} field_data.update(self.MODEL_DATA) student_data = DictFieldData(field_data) - self.item_descriptor._field_data = lms_field_data(self.item_descriptor._field_data, student_data) + self.item_descriptor._field_data = LmsFieldData(self.item_descriptor._field_data, student_data) self.item_descriptor.xmodule_runtime = self.new_module_runtime() self.item_module = self.item_descriptor diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 2cb80b6eda..1ab22060ce 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -2,6 +2,8 @@ Test for LMS courseware app. """ import mock +from mock import Mock +from unittest import TestCase from django.core.urlresolvers import reverse from django.test.utils import override_settings @@ -18,6 +20,7 @@ from courseware.tests.modulestore_config import TEST_DATA_DIR, \ TEST_DATA_MONGO_MODULESTORE, \ TEST_DATA_DRAFT_MONGO_MODULESTORE, \ TEST_DATA_MIXED_MODULESTORE +from lms.xblock.field_data import LmsFieldData class ActivateLoginTest(LoginEnrollmentTestCase): @@ -183,3 +186,24 @@ class TestDraftModuleStore(ModuleStoreTestCase): # test success is just getting through the above statement. # The bug was that 'course_id' argument was # not allowed to be passed in (i.e. was throwing exception) + + +class TestLmsFieldData(TestCase): + """ + Tests of the LmsFieldData class + """ + def test_lms_field_data_wont_nest(self): + # Verify that if an LmsFieldData is passed into LmsFieldData as the + # authored_data, that it doesn't produced a nested field data. + # + # This fixes a bug where re-use of the same descriptor for many modules + # would cause more and more nesting, until the recursion depth would be + # reached on any attribute access + + # pylint: disable=protected-access + base_authored = Mock() + base_student = Mock() + first_level = LmsFieldData(base_authored, base_student) + second_level = LmsFieldData(first_level, base_student) + self.assertEquals(second_level._authored_data, first_level._authored_data) + self.assertNotIsInstance(second_level._authored_data, LmsFieldData) diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index 148dbcdae2..7281e779d5 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -6,6 +6,7 @@ from django.contrib.auth.decorators import login_required from django.http import Http404 from django.core.context_processors import csrf from django.contrib.auth.models import User +import newrelic.agent from mitxmako.shortcuts import render_to_response from courseware.courses import get_course_with_access @@ -25,7 +26,7 @@ escapedict = {'"': '"'} log = logging.getLogger("edx.discussions") -@login_required +@newrelic.agent.function_trace() def get_threads(request, course_id, discussion_id=None, per_page=THREADS_PER_PAGE): """ This may raise cc.utils.CommentClientError or @@ -108,6 +109,8 @@ def inline_discussion(request, course_id, discussion_id): """ Renders JSON for DiscussionModules """ + nr_transaction = newrelic.agent.current_transaction() + course = get_course_with_access(request.user, course_id, 'load_forum') try: @@ -118,7 +121,8 @@ def inline_discussion(request, course_id, discussion_id): log.error("Error loading inline discussion threads.") raise - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): + annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) allow_anonymous = course.allow_anonymous allow_anonymous_to_peers = course.allow_anonymous_to_peers @@ -165,9 +169,11 @@ def forum_form_discussion(request, course_id): """ Renders the main Discussion page, potentially filtered by a search query """ + nr_transaction = newrelic.agent.current_transaction() course = get_course_with_access(request.user, course_id, 'load_forum') - category_map = utils.get_discussion_category_map(course) + with newrelic.agent.FunctionTrace(nr_transaction, "get_discussion_category_map"): + category_map = utils.get_discussion_category_map(course) try: unsafethreads, query_params = get_threads(request, course_id) # This might process a search query @@ -182,12 +188,15 @@ def forum_form_discussion(request, course_id): user = cc.User.from_django_user(request.user) user_info = user.to_dict() - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): + annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + + with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context_to_threads"): + for thread in threads: + courseware_context = get_courseware_context(thread, course) + if courseware_context: + thread.update(courseware_context) - for thread in threads: - courseware_context = get_courseware_context(thread, course) - if courseware_context: - thread.update(courseware_context) if request.is_ajax(): return utils.JsonResponse({ 'discussion_data': threads, # TODO: Standardize on 'discussion_data' vs 'threads' @@ -205,10 +214,11 @@ def forum_form_discussion(request, course_id): #trending_tags = cc.search_trending_tags( # course_id, #) - cohorts = get_course_cohorts(course_id) - cohorted_commentables = get_cohorted_commentables(course_id) + with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"): + cohorts = get_course_cohorts(course_id) + cohorted_commentables = get_cohorted_commentables(course_id) - user_cohort_id = get_cohort_id(request.user, course_id) + user_cohort_id = get_cohort_id(request.user, course_id) context = { 'csrf': csrf(request)['csrf_token'], @@ -236,6 +246,8 @@ def forum_form_discussion(request, course_id): @login_required def single_thread(request, course_id, discussion_id, thread_id): + nr_transaction = newrelic.agent.current_transaction() + course = get_course_with_access(request.user, course_id, 'load_forum') cc_user = cc.User.from_django_user(request.user) user_info = cc_user.to_dict() @@ -247,8 +259,10 @@ def single_thread(request, course_id, discussion_id, thread_id): raise if request.is_ajax(): - courseware_context = get_courseware_context(thread, course) - annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_courseware_context"): + courseware_context = get_courseware_context(thread, course) + with newrelic.agent.FunctionTrace(nr_transaction, "get_annotated_content_infos"): + annotated_content_info = utils.get_annotated_content_infos(course_id, thread, request.user, user_info=user_info) context = {'thread': thread.to_dict(), 'course_id': course_id} # TODO: Remove completely or switch back to server side rendering # html = render_to_string('discussion/_ajax_single_thread.html', context) @@ -262,7 +276,8 @@ def single_thread(request, course_id, discussion_id, thread_id): }) else: - category_map = utils.get_discussion_category_map(course) + with newrelic.agent.FunctionTrace(nr_transaction, "get_discussion_category_map"): + category_map = utils.get_discussion_category_map(course) try: threads, query_params = get_threads(request, course_id) @@ -273,16 +288,17 @@ def single_thread(request, course_id, discussion_id, thread_id): course = get_course_with_access(request.user, course_id, 'load_forum') - for thread in threads: - courseware_context = get_courseware_context(thread, course) - if courseware_context: - thread.update(courseware_context) - if thread.get('group_id') and not thread.get('group_name'): - thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name + with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context_to_threads"): + for thread in threads: + courseware_context = get_courseware_context(thread, course) + if courseware_context: + thread.update(courseware_context) + if thread.get('group_id') and not thread.get('group_name'): + thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name - #patch for backward compatibility with comments service - if not "pinned" in thread: - thread["pinned"] = False + #patch for backward compatibility with comments service + if not "pinned" in thread: + thread["pinned"] = False threads = [utils.safe_content(thread) for thread in threads] @@ -296,11 +312,13 @@ def single_thread(request, course_id, discussion_id, thread_id): # course_id, #) - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): + annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) - cohorts = get_course_cohorts(course_id) - cohorted_commentables = get_cohorted_commentables(course_id) - user_cohort = get_cohort_id(request.user, course_id) + with newrelic.agent.FunctionTrace(nr_transaction, "get_cohort_info"): + cohorts = get_course_cohorts(course_id) + cohorted_commentables = get_cohorted_commentables(course_id) + user_cohort = get_cohort_id(request.user, course_id) context = { 'discussion_id': discussion_id, @@ -330,6 +348,8 @@ def single_thread(request, course_id, discussion_id, thread_id): @login_required def user_profile(request, course_id, user_id): + nr_transaction = newrelic.agent.current_transaction() + #TODO: Allow sorting? course = get_course_with_access(request.user, course_id, 'load_forum') try: @@ -345,7 +365,8 @@ def user_profile(request, course_id, user_id): query_params['num_pages'] = num_pages user_info = cc.User.from_django_user(request.user).to_dict() - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): + annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) if request.is_ajax(): return utils.JsonResponse({ @@ -373,6 +394,8 @@ def user_profile(request, course_id, user_id): @login_required def followed_threads(request, course_id, user_id): + nr_transaction = newrelic.agent.current_transaction() + course = get_course_with_access(request.user, course_id, 'load_forum') try: profiled_user = cc.User(id=user_id, course_id=course_id) @@ -389,7 +412,8 @@ def followed_threads(request, course_id, user_id): query_params['num_pages'] = num_pages user_info = cc.User.from_django_user(request.user).to_dict() - annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) + with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): + annotated_content_info = utils.get_metadata_for_threads(course_id, threads, request.user, user_info) if request.is_ajax(): return utils.JsonResponse({ 'annotated_content_info': annotated_content_info, diff --git a/lms/xblock/field_data.py b/lms/xblock/field_data.py index 6073a86863..03b3deafac 100644 --- a/lms/xblock/field_data.py +++ b/lms/xblock/field_data.py @@ -6,21 +6,30 @@ from xblock.field_data import ReadOnlyFieldData, SplitFieldData from xblock.fields import Scope -def lms_field_data(authored_data, student_data): +class LmsFieldData(SplitFieldData): """ - Returns a new :class:`~xblock.field_data.FieldData` that + A :class:`~xblock.field_data.FieldData` that reads all UserScope.ONE and UserScope.ALL fields from `student_data` and all UserScope.NONE fields from `authored_data`. It also prevents writing to `authored_data`. """ - authored_data = ReadOnlyFieldData(authored_data) - return SplitFieldData({ - Scope.content: authored_data, - Scope.settings: authored_data, - Scope.parent: authored_data, - Scope.children: authored_data, - Scope.user_state_summary: student_data, - Scope.user_state: student_data, - Scope.user_info: student_data, - Scope.preferences: student_data, - }) + def __init__(self, authored_data, student_data): + # Make sure that we don't repeatedly nest LmsFieldData instances + if isinstance(authored_data, LmsFieldData): + authored_data = authored_data._authored_data + else: + authored_data = ReadOnlyFieldData(authored_data) + + self._authored_data = authored_data + self._student_data = student_data + + super(LmsFieldData, self).__init__({ + Scope.content: authored_data, + Scope.settings: authored_data, + Scope.parent: authored_data, + Scope.children: authored_data, + Scope.user_state_summary: student_data, + Scope.user_state: student_data, + Scope.user_info: student_data, + Scope.preferences: student_data, + })