Skip to content

E
edx-platform
Commit d2e0f27a by Greg Price
Options

Merge pull request #1510 from edx/gprice/lms-forum-perf 

Improve forums performance
parents c4434cb1 e6ecb6ec
Showing with 59 additions and 207 deletions
CHANGELOG.rst
...@@ -9,6 +9,9 @@ LMS: Improve forum error handling so that errors in the logs are ...@@ -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 clearer and HTTP status codes from the comments service indicating
client error are correctly passed through to the client. 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 LMS: The wiki markup cheatsheet dialog is now accessible to people with
disabilites. (LMS-1303) disabilites. (LMS-1303)
......
lms/djangoapps/courseware/views.py
...@@ -27,8 +27,6 @@ from .module_render import toc_for_course, get_module_for_descriptor, get_module ...@@ -27,8 +27,6 @@ from .module_render import toc_for_course, get_module_for_descriptor, get_module
from courseware.models import StudentModule, StudentModuleHistory from courseware.models import StudentModule, StudentModuleHistory
from course_modes.models import CourseMode from course_modes.models import CourseMode
from django_comment_client.utils import get_discussion_title
from student.models import UserTestGroup, CourseEnrollment from student.models import UserTestGroup, CourseEnrollment
from util.cache import cache, cache_if_anonymous from util.cache import cache, cache_if_anonymous
from xblock.fragment import Fragment from xblock.fragment import Fragment
...@@ -39,8 +37,6 @@ from xmodule.modulestore.search import path_to_location ...@@ -39,8 +37,6 @@ from xmodule.modulestore.search import path_to_location
from xmodule.course_module import CourseDescriptor from xmodule.course_module import CourseDescriptor
import shoppingcart import shoppingcart
import comment_client
log = logging.getLogger("mitx.courseware") log = logging.getLogger("mitx.courseware")
template_imports = {'urllib': urllib} template_imports = {'urllib': urllib}
...@@ -674,29 +670,6 @@ def mktg_course_about(request, course_id): ...@@ -674,29 +670,6 @@ def mktg_course_about(request, course_id):
}) })
def render_notifications(request, course, notifications):
context = {
'notifications': notifications,
'get_discussion_title': partial(get_discussion_title, request=request, course=course),
'course': course,
}
return render_to_string('courseware/notifications.html', context)
@login_required
def news(request, course_id):
course = get_course_with_access(request.user, course_id, 'load')
notifications = comment_client.get_notifications(request.user.id)
context = {
'course': course,
'content': render_notifications(request, course, notifications),
}
return render_to_response('courseware/news.html', context)
@login_required @login_required
@cache_control(no_cache=True, no_store=True, must_revalidate=True) @cache_control(no_cache=True, no_store=True, must_revalidate=True)
def progress(request, course_id, student_id=None): def progress(request, course_id, student_id=None):
......
lms/djangoapps/django_comment_client/base/views.py
...@@ -23,7 +23,7 @@ from mitxmako.shortcuts import render_to_string ...@@ -23,7 +23,7 @@ from mitxmako.shortcuts import render_to_string
from courseware.courses import get_course_with_access, get_course_by_id from courseware.courses import get_course_with_access, get_course_by_id
from course_groups.cohorts import get_cohort_id, is_commentable_cohorted 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_client.permissions import check_permissions_by_view, cached_has_permission
from django_comment_common.models import Role from django_comment_common.models import Role
...@@ -128,10 +128,8 @@ def create_thread(request, course_id, commentable_id): ...@@ -128,10 +128,8 @@ def create_thread(request, course_id, commentable_id):
if post.get('auto_subscribe', 'false').lower() == 'true': if post.get('auto_subscribe', 'false').lower() == 'true':
user = cc.User.from_django_user(request.user) user = cc.User.from_django_user(request.user)
user.follow(thread) user.follow(thread)
courseware_context = get_courseware_context(thread, course)
data = thread.to_dict() data = thread.to_dict()
if courseware_context: add_courseware_context([data], course)
data.update(courseware_context)
if request.is_ajax(): if request.is_ajax():
return ajax_content_response(request, course_id, data, 'discussion/ajax_create_thread.html') return ajax_content_response(request, course_id, data, 'discussion/ajax_create_thread.html')
else: else:
......
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 ...@@ -15,7 +15,7 @@ from course_groups.cohorts import (is_course_cohorted, get_cohort_id, is_comment
from courseware.access import has_access from courseware.access import has_access
from django_comment_client.permissions import cached_has_permission 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 django_comment_client.utils as utils
import comment_client as cc import comment_client as cc
...@@ -184,11 +184,8 @@ def forum_form_discussion(request, course_id): ...@@ -184,11 +184,8 @@ def forum_form_discussion(request, course_id):
with newrelic.agent.FunctionTrace(nr_transaction, "get_metadata_for_threads"): 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) 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"): with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
for thread in threads: add_courseware_context(threads, course)
courseware_context = get_courseware_context(thread, course)
if courseware_context:
thread.update(courseware_context)
if request.is_ajax(): if request.is_ajax():
return utils.JsonResponse({ return utils.JsonResponse({
...@@ -248,16 +245,14 @@ def single_thread(request, course_id, discussion_id, thread_id): ...@@ -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) thread = cc.Thread.find(thread_id).retrieve(recursive=True, user_id=request.user.id)
if request.is_ajax(): 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"): 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) 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} context = {'thread': thread.to_dict(), 'course_id': course_id}
# TODO: Remove completely or switch back to server side rendering # TODO: Remove completely or switch back to server side rendering
# html = render_to_string('discussion/_ajax_single_thread.html', context) # html = render_to_string('discussion/_ajax_single_thread.html', context)
content = utils.safe_content(thread.to_dict()) content = utils.safe_content(thread.to_dict())
if courseware_context: with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
content.update(courseware_context) add_courseware_context([content], course)
return utils.JsonResponse({ return utils.JsonResponse({
#'html': html, #'html': html,
'content': content, 'content': content,
...@@ -273,17 +268,16 @@ def single_thread(request, course_id, discussion_id, thread_id): ...@@ -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') course = get_course_with_access(request.user, course_id, 'load_forum')
with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context_to_threads"): with newrelic.agent.FunctionTrace(nr_transaction, "add_courseware_context"):
for thread in threads: add_courseware_context(threads, course)
courseware_context = get_courseware_context(thread, course)
if courseware_context: for thread in threads:
thread.update(courseware_context) if thread.get('group_id') and not thread.get('group_name'):
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
thread['group_name'] = get_cohort_by_id(course_id, thread.get('group_id')).name
#patch for backward compatibility with comments service
#patch for backward compatibility with comments service if not "pinned" in thread:
if not "pinned" in thread: thread["pinned"] = False
thread["pinned"] = False
threads = [utils.safe_content(thread) for thread in threads] threads = [utils.safe_content(thread) for thread in threads]
......
lms/djangoapps/django_comment_client/utils.py
...@@ -20,10 +20,6 @@ from django.utils.timezone import UTC ...@@ -20,10 +20,6 @@ from django.utils.timezone import UTC
log = logging.getLogger(__name__) log = logging.getLogger(__name__)
# TODO these should be cached via django's caching rather than in-memory globals
_FULLMODULES = None
_DISCUSSIONINFO = defaultdict(dict)
def extract(dic, keys): def extract(dic, keys):
return {k: dic.get(k) for k in keys} return {k: dic.get(k) for k in keys}
...@@ -62,33 +58,33 @@ def has_forum_access(uname, course_id, rolename): ...@@ -62,33 +58,33 @@ def has_forum_access(uname, course_id, rolename):
return role.users.filter(username=uname).exists() return role.users.filter(username=uname).exists()
def get_full_modules(): def _get_discussion_modules(course):
global _FULLMODULES all_modules = modulestore().get_items(
if not _FULLMODULES: ['i4x', course.location.org, course.location.course, 'discussion', None],
_FULLMODULES = modulestore().modules course_id=course.id
return _FULLMODULES )
def get_discussion_id_map(course): def has_required_keys(module):
""" for key in ('discussion_id', 'discussion_category', 'discussion_target'):
return a dict of the form {category: modules} if getattr(module, key) is None:
""" log.warning("Required key '%s' not in discussion %s, leaving out of category map" % (key, module.location))
initialize_discussion_info(course) return False
return _DISCUSSIONINFO[course.id]['id_map'] return True
return filter(has_required_keys, all_modules)
def get_discussion_title(course, discussion_id):
initialize_discussion_info(course)
title = _DISCUSSIONINFO[course.id]['id_map'].get(discussion_id, {}).get('title', '(no title)')
return title
def _get_discussion_id_map(course):
def get_entry(module):
discussion_id = module.discussion_id
title = module.discussion_target
last_category = module.discussion_category.split("/")[-1].strip()
return (discussion_id, {"location": module.location, "title": last_category + " / " + title})
def get_discussion_category_map(course): return dict(map(get_entry, _get_discussion_modules(course)))
initialize_discussion_info(course)
return filter_unstarted_categories(_DISCUSSIONINFO[course.id]['category_map'])
def filter_unstarted_categories(category_map): def _filter_unstarted_categories(category_map):
now = datetime.now(UTC()) now = datetime.now(UTC())
...@@ -126,7 +122,7 @@ def filter_unstarted_categories(category_map): ...@@ -126,7 +122,7 @@ def filter_unstarted_categories(category_map):
return result_map return result_map
def sort_map_entries(category_map, sort_alpha): def _sort_map_entries(category_map, sort_alpha):
things = [] things = []
for title, entry in category_map["entries"].items(): for title, entry in category_map["entries"].items():
if entry["sort_key"] == None and sort_alpha: if entry["sort_key"] == None and sort_alpha:
...@@ -134,37 +130,22 @@ def sort_map_entries(category_map, sort_alpha): ...@@ -134,37 +130,22 @@ def sort_map_entries(category_map, sort_alpha):
things.append((title, entry)) things.append((title, entry))
for title, category in category_map["subcategories"].items(): for title, category in category_map["subcategories"].items():
things.append((title, category)) things.append((title, category))
sort_map_entries(category_map["subcategories"][title], sort_alpha) _sort_map_entries(category_map["subcategories"][title], sort_alpha)
category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])] category_map["children"] = [x[0] for x in sorted(things, key=lambda x: x[1]["sort_key"])]
def initialize_discussion_info(course): def get_discussion_category_map(course):
course_id = course.id course_id = course.id
discussion_id_map = {}
unexpanded_category_map = defaultdict(list) unexpanded_category_map = defaultdict(list)
# get all discussion models within this course_id modules = _get_discussion_modules(course)
all_modules = modulestore().get_items(['i4x', course.location.org, course.location.course,
'discussion', None], course_id=course_id)
for module in all_modules:
skip_module = False
for key in ('discussion_id', 'discussion_category', 'discussion_target'):
if getattr(module, key) is None:
log.warning("Required key '%s' not in discussion %s, leaving out of category map" % (key, module.location))
skip_module = True
if skip_module:
continue
for module in modules:
id = module.discussion_id id = module.discussion_id
category = module.discussion_category
title = module.discussion_target title = module.discussion_target
sort_key = module.sort_key sort_key = module.sort_key
category = " / ".join([x.strip() for x in category.split("/")]) category = " / ".join([x.strip() for x in module.discussion_category.split("/")])
last_category = category.split("/")[-1]
discussion_id_map[id] = {"location": module.location, "title": last_category + " / " + title}
#Handle case where module.start is None #Handle case where module.start is None
entry_start_date = module.start if module.start else datetime.max.replace(tzinfo=pytz.UTC) entry_start_date = module.start if module.start else datetime.max.replace(tzinfo=pytz.UTC)
unexpanded_category_map[category].append({"title": title, "id": id, "sort_key": sort_key, "start_date": entry_start_date}) unexpanded_category_map[category].append({"title": title, "id": id, "sort_key": sort_key, "start_date": entry_start_date})
...@@ -214,11 +195,9 @@ def initialize_discussion_info(course): ...@@ -214,11 +195,9 @@ def initialize_discussion_info(course):
"sort_key": entry.get("sort_key", topic), "sort_key": entry.get("sort_key", topic),
"start_date": datetime.now(UTC())} "start_date": datetime.now(UTC())}
sort_map_entries(category_map, course.discussion_sort_alpha) _sort_map_entries(category_map, course.discussion_sort_alpha)
_DISCUSSIONINFO[course.id]['id_map'] = discussion_id_map return _filter_unstarted_categories(category_map)
_DISCUSSIONINFO[course.id]['category_map'] = category_map
_DISCUSSIONINFO[course.id]['timestamp'] = datetime.now(UTC())
class JsonResponse(HttpResponse): class JsonResponse(HttpResponse):
...@@ -368,19 +347,19 @@ def extend_content(content): ...@@ -368,19 +347,19 @@ def extend_content(content):
return merge_dict(content, content_info) 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_map = _get_discussion_id_map(course)
id = content['commentable_id']
content_info = None for content in content_list:
if id in id_map: commentable_id = content['commentable_id']
location = id_map[id]["location"].url() if commentable_id in id_map:
title = id_map[id]["title"] location = id_map[commentable_id]["location"].url()
title = id_map[commentable_id]["title"]
url = reverse('jump_to', kwargs={"course_id": course.location.course_id, url = reverse('jump_to', kwargs={"course_id": course.location.course_id,
"location": location}) "location": location})
content_info = {"courseware_url": url, "courseware_title": title} content.update({"courseware_url": url, "courseware_title": title})
return content_info
def safe_content(content): def safe_content(content):
......
lms/templates/courseware/notifications.html deleted 100644 → 0
<%! from django.utils.translation import ugettext as _ %>
<%! from django.core.urlresolvers import reverse %>
<%
def url_for_thread(discussion_id, thread_id):
return reverse('django_comment_client.forum.views.single_thread', args=[course.id, discussion_id, thread_id])
%>
<%
def url_for_comment(discussion_id, thread_id, comment_id):
return url_for_thread(discussion_id, thread_id) + "#" + comment_id
%>
<%
def url_for_discussion(discussion_id):
return reverse('django_comment_client.forum.views.forum_form_discussion', args=[course.id, discussion_id])
%>
<%
def discussion_title(discussion_id):
return get_discussion_title(discussion_id=discussion_id)
%>
<%
def url_for_user(user_id): #TODO
return "javascript:void(0)"
%>
<div class="notifications">
% for notification in notifications:
${render_notification(notification)}
% endfor
</div>
<%def name="render_user_link(notification)">
<% info = notification['info'] %>
% if notification.get('actor_id', None):
<a href="${url_for_user(notification['actor_id'])}">${info['actor_username']}</a>
% else:
${_("Anonymous")}
% endif
</%def>
<%def name="render_thread_link(notification)">
<% info = notification['info'] %>
<a href="${url_for_thread(info['commentable_id'], info['thread_id'])}">${info['thread_title']}</a>
</%def>
<%def name="render_comment_link(notification)">
<% info = notification['info'] %>
<a href="${url_for_comment(info['commentable_id'], info['thread_id'], info['comment_id'])}">${_("comment")}</a>
</%def>
<%def name="render_discussion_link(notification)">
<% info = notification['info'] %>
<a href="${url_for_discussion(info['commentable_id'])}">${discussion_title(info['commentable_id'])}</a>
</%def>
<%def name="render_notification(notification)">
<div class="notification">
% if notification['notification_type'] == 'post_reply':
${_("{user} posted a {comment} to the thread {thread} in discussion {discussion}").format(
user=render_user_link(notification),
comment=render_comment_link(notification),
thread=render_thread_link(notification),
discussion=render_discussion_link(notification),
)}
% elif notification['notification_type'] == 'post_topic':
${_("{user} posted a new thread {thread} in discussion {discussion}").format(
user=render_user_link(notification),
thread=render_thread_link(notification),
discussion=render_discussion_link(notification),
)}
% elif notification['notification_type'] == 'at_user':
% if notification['info']['content_type'] == 'thread':
${_("{user} mentioned you in the thread {thread} in disucssion {discussion}").format(
user=render_user(info),
thread=render_thread_link(notification),
discussion=render_discussion_link(notification),
)}
% else:
${_("{user} mentioned you in {comment} to the thread {thread} in discussion {discussion}").format(
user=render_user(info),
comment=render_comment_link(notification),
thread=render_thread_link(notification),
discussion=render_discussion_link(notification),
)}
% endif
% endif
</div>
</%def>
lms/urls.py
...@@ -329,8 +329,6 @@ if settings.COURSEWARE_ENABLED: ...@@ -329,8 +329,6 @@ if settings.COURSEWARE_ENABLED:
# discussion forums live within courseware, so courseware must be enabled first # discussion forums live within courseware, so courseware must be enabled first
if settings.MITX_FEATURES.get('ENABLE_DISCUSSION_SERVICE'): if settings.MITX_FEATURES.get('ENABLE_DISCUSSION_SERVICE'):
urlpatterns += ( urlpatterns += (
url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/news$',
'courseware.views.news', name="news"),
url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/discussion/', url(r'^courses/(?P<course_id>[^/]+/[^/]+/[^/]+)/discussion/',
include('django_comment_client.urls')), include('django_comment_client.urls')),
url(r'^notification_prefs/enable/', 'notification_prefs.views.ajax_enable'), url(r'^notification_prefs/enable/', 'notification_prefs.views.ajax_enable'),
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment