diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a4b5d31fea..2e480ba3bf 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -9,6 +9,9 @@ LMS: Improve forum error handling so that errors in the logs are clearer and HTTP status codes from the comments service indicating client error are correctly passed through to the client. +LMS: Improve performance of page load and thread list load for +discussion tab + LMS: The wiki markup cheatsheet dialog is now accessible to people with disabilites. (LMS-1303) diff --git a/lms/djangoapps/django_comment_client/base/views.py b/lms/djangoapps/django_comment_client/base/views.py index b2e3ea3e08..c6ea414f82 100644 --- a/lms/djangoapps/django_comment_client/base/views.py +++ b/lms/djangoapps/django_comment_client/base/views.py @@ -23,7 +23,7 @@ from mitxmako.shortcuts import render_to_string from courseware.courses import get_course_with_access, get_course_by_id from course_groups.cohorts import get_cohort_id, is_commentable_cohorted -from django_comment_client.utils import JsonResponse, JsonError, extract, get_courseware_context +from django_comment_client.utils import JsonResponse, JsonError, extract, add_courseware_context from django_comment_client.permissions import check_permissions_by_view, cached_has_permission from django_comment_common.models import Role @@ -128,10 +128,8 @@ def create_thread(request, course_id, commentable_id): if post.get('auto_subscribe', 'false').lower() == 'true': user = cc.User.from_django_user(request.user) user.follow(thread) - courseware_context = get_courseware_context(thread, course) data = thread.to_dict() - if courseware_context: - data.update(courseware_context) + add_courseware_context([data], course) if request.is_ajax(): return ajax_content_response(request, course_id, data, 'discussion/ajax_create_thread.html') else: diff --git a/lms/djangoapps/django_comment_client/forum/views.py b/lms/djangoapps/django_comment_client/forum/views.py index f9cb2b2713..fb53f3c0b7 100644 --- a/lms/djangoapps/django_comment_client/forum/views.py +++ b/lms/djangoapps/django_comment_client/forum/views.py @@ -15,7 +15,7 @@ from course_groups.cohorts import (is_course_cohorted, get_cohort_id, is_comment from courseware.access import has_access from django_comment_client.permissions import cached_has_permission -from django_comment_client.utils import (merge_dict, extract, strip_none, get_courseware_context) +from django_comment_client.utils import (merge_dict, extract, strip_none, add_courseware_context) import django_comment_client.utils as utils import comment_client as cc @@ -184,11 +184,8 @@ def forum_form_discussion(request, course_id): 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) + with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): + add_courseware_context(threads, course) if request.is_ajax(): return utils.JsonResponse({ @@ -248,16 +245,14 @@ def single_thread(request, course_id, discussion_id, thread_id): thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id) if request.is_ajax(): - 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) content = utils.safe_content(thread.to_dict()) - if courseware_context: - content.update(courseware_context) + with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): + add_courseware_context([content], course) return utils.JsonResponse({ #'html': html, 'content': content, @@ -273,17 +268,16 @@ def single_thread(request, course_id, discussion_id, thread_id): course = get_course_with_access(request.user, course_id, 'load_forum') - 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 + with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"): + add_courseware_context(threads, course) - #patch for backward compatibility with comments service - if not "pinned" in thread: - thread["pinned"] = False + for thread in threads: + 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 threads = [utils.safe_content(thread) for thread in threads] diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 9189fe8625..c275036319 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -83,17 +83,29 @@ class CoursewareContextTestCase(ModuleStoreTestCase): discussion_target="Discussion 2" ) + def test_empty(self): + utils.add_courseware_context([], self.course) + def test_missing_commentable_id(self): - thread = {"commentable_id": "non-inline"} - self.assertEqual(utils.get_courseware_context(thread, self.course), None) + orig = {"commentable_id": "non-inline"} + modified = dict(orig) + utils.add_courseware_context([modified], self.course) + self.assertEqual(modified, orig) def test_basic(self): - def test_discussion(discussion, expected_title): - thread = {"commentable_id": discussion.discussion_id} - courseware_context = utils.get_courseware_context(thread, self.course) - self.assertEqual(set(courseware_context.keys()), set(["courseware_url", "courseware_title"])) + threads = [ + {"commentable_id": self.discussion1.discussion_id}, + {"commentable_id": self.discussion2.discussion_id} + ] + utils.add_courseware_context(threads, self.course) + + def assertThreadCorrect(thread, discussion, expected_title): #pylint: disable=C0103 self.assertEqual( - courseware_context["courseware_url"], + set(thread.keys()), + set(["commentable_id", "courseware_url", "courseware_title"]) + ) + self.assertEqual( + thread.get("courseware_url"), reverse( "jump_to", kwargs={ @@ -102,13 +114,10 @@ class CoursewareContextTestCase(ModuleStoreTestCase): } ) ) - self.assertEqual( - courseware_context["courseware_title"], - expected_title - ) + self.assertEqual(thread.get("courseware_title"), expected_title) - test_discussion(self.discussion1, "Chapter / Discussion 1") - test_discussion(self.discussion2, "Subsection / Discussion 2") + assertThreadCorrect(threads[0], self.discussion1, "Chapter / Discussion 1") + assertThreadCorrect(threads[1], self.discussion2, "Subsection / Discussion 2") @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 258672fe4e..6550c4d1e5 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -347,19 +347,19 @@ def extend_content(content): return merge_dict(content, content_info) -def get_courseware_context(content, course): +def add_courseware_context(content_list, course): id_map = _get_discussion_id_map(course) - id = content['commentable_id'] - content_info = None - if id in id_map: - location = id_map[id]["location"].url() - title = id_map[id]["title"] - url = reverse('jump_to', kwargs={"course_id": course.location.course_id, - "location": location}) + for content in content_list: + commentable_id = content['commentable_id'] + if commentable_id in id_map: + location = id_map[commentable_id]["location"].url() + title = id_map[commentable_id]["title"] - content_info = {"courseware_url": url, "courseware_title": title} - return content_info + url = reverse('jump_to', kwargs={"course_id": course.location.course_id, + "location": location}) + + content.update({"courseware_url": url, "courseware_title": title}) def safe_content(content):