Merge remote-tracking branch 'origin/release'

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
 

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

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(

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

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)

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,

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,
+        })