From 54ad4110785bd25eee110666eb5397645663e9d9 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Thu, 24 Oct 2013 13:10:20 -0400 Subject: [PATCH 1/4] Remove unused code --- lms/djangoapps/courseware/views.py | 27 ------ lms/djangoapps/django_comment_client/utils.py | 15 --- lms/templates/courseware/notifications.html | 93 ------------------- lms/urls.py | 2 - 4 files changed, 137 deletions(-) delete mode 100644 lms/templates/courseware/notifications.html diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 75a8d8db1c..819f65fca6 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -27,8 +27,6 @@ from .module_render import toc_for_course, get_module_for_descriptor, get_module from courseware.models import StudentModule, StudentModuleHistory from course_modes.models import CourseMode -from django_comment_client.utils import get_discussion_title - from student.models import UserTestGroup, CourseEnrollment from util.cache import cache, cache_if_anonymous from xblock.fragment import Fragment @@ -39,8 +37,6 @@ from xmodule.modulestore.search import path_to_location from xmodule.course_module import CourseDescriptor import shoppingcart -import comment_client - log = logging.getLogger("mitx.courseware") template_imports = {'urllib': urllib} @@ -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 @cache_control(no_cache=True, no_store=True, must_revalidate=True) def progress(request, course_id, student_id=None): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index d449f288a4..3e0699f2ef 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -20,8 +20,6 @@ from django.utils.timezone import UTC log = logging.getLogger(__name__) -# TODO these should be cached via django's caching rather than in-memory globals -_FULLMODULES = None _DISCUSSIONINFO = defaultdict(dict) @@ -62,13 +60,6 @@ def has_forum_access(uname, course_id, rolename): return role.users.filter(username=uname).exists() -def get_full_modules(): - global _FULLMODULES - if not _FULLMODULES: - _FULLMODULES = modulestore().modules - return _FULLMODULES - - def get_discussion_id_map(course): """ return a dict of the form {category: modules} @@ -77,12 +68,6 @@ def get_discussion_id_map(course): return _DISCUSSIONINFO[course.id]['id_map'] -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_category_map(course): initialize_discussion_info(course) return filter_unstarted_categories(_DISCUSSIONINFO[course.id]['category_map']) diff --git a/lms/templates/courseware/notifications.html b/lms/templates/courseware/notifications.html deleted file mode 100644 index 39eaaa14b9..0000000000 --- a/lms/templates/courseware/notifications.html +++ /dev/null @@ -1,93 +0,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)" -%> - - -
- % for notification in notifications: - ${render_notification(notification)} - % endfor -
- -<%def name="render_user_link(notification)"> - <% info = notification['info'] %> - % if notification.get('actor_id', None): - ${info['actor_username']} - % else: - ${_("Anonymous")} - % endif - - -<%def name="render_thread_link(notification)"> - <% info = notification['info'] %> - ${info['thread_title']} - - -<%def name="render_comment_link(notification)"> - <% info = notification['info'] %> - ${_("comment")} - - -<%def name="render_discussion_link(notification)"> - <% info = notification['info'] %> - ${discussion_title(info['commentable_id'])} - - -<%def name="render_notification(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 -
- - diff --git a/lms/urls.py b/lms/urls.py index e187cf3ff6..54223cb806 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -329,8 +329,6 @@ if settings.COURSEWARE_ENABLED: # discussion forums live within courseware, so courseware must be enabled first if settings.MITX_FEATURES.get('ENABLE_DISCUSSION_SERVICE'): urlpatterns += ( - url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/news$', - 'courseware.views.news', name="news"), url(r'^courses/(?P[^/]+/[^/]+/[^/]+)/discussion/', include('django_comment_client.urls')), url(r'^notification_prefs/enable/', 'notification_prefs.views.ajax_enable'), From 545701d520a8f4b87a414a2cb5b60d8515fefa99 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 25 Oct 2013 17:16:37 -0400 Subject: [PATCH 2/4] Add forum utility code unit test coverage This is in preparation for significant refactoring of the code in question. --- .../django_comment_client/tests/test_utils.py | 542 ++++++++++++++---- lms/djangoapps/django_comment_client/utils.py | 10 +- 2 files changed, 424 insertions(+), 128 deletions(-) diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index efc6e6c7a3..5132ccc7c5 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -1,9 +1,14 @@ +from datetime import datetime +from django.core.urlresolvers import reverse from django.test import TestCase +from django.test.utils import override_settings from student.tests.factories import UserFactory, CourseEnrollmentFactory from django_comment_common.models import Role, Permission from factories import RoleFactory import django_comment_client.utils as utils - +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE class DictionaryTestCase(TestCase): def test_extract(self): @@ -29,128 +34,6 @@ class DictionaryTestCase(TestCase): self.assertEqual(utils.merge_dict(d1, d2), expected) -class CategorySortTestCase(TestCase): - def setUp(self): - self.category_map = { - 'entries': { - u'General': { - 'sort_key': u'General' - } - }, - 'subcategories': { - u'Tests': { - 'sort_key': u'Tests', - 'subcategories': {}, - 'entries': { - u'Quizzes': { - 'sort_key': None - }, u'All': { - 'sort_key': None - }, u'Final Exam': { - 'sort_key': None - }, - } - }, - u'Assignments': { - 'sort_key': u'Assignments', - 'subcategories': {}, - 'entries': { - u'Homework': { - 'sort_key': None - }, - u'All': { - 'sort_key': None - }, - } - } - } - } - - def test_alpha_sort_true(self): - expected_true = { - 'entries': { - u'General': { - 'sort_key': u'General' - } - }, - 'children': [u'Assignments', u'General', u'Tests'], - 'subcategories': { - u'Tests': { - 'sort_key': u'Tests', - 'subcategories': {}, - 'children': [u'All', u'Final Exam', u'Quizzes'], - 'entries': { - u'All': { - 'sort_key': 'All' - }, u'Final Exam': { - 'sort_key': 'Final Exam' - }, u'Quizzes': { - 'sort_key': 'Quizzes' - } - } - }, - u'Assignments': { - 'sort_key': u'Assignments', - 'subcategories': {}, - 'children': [u'All', u'Homework'], - 'entries': { - u'Homework': { - 'sort_key': 'Homework' - }, - u'All': { - 'sort_key': 'All' - }, - } - } - } - } - - utils.sort_map_entries(self.category_map, True) - self.assertEqual(self.category_map, expected_true) - - def test_alpha_sort_false(self): - expected_false = { - 'entries': { - u'General': { - 'sort_key': u'General' - } - }, - 'children': [u'Assignments', u'General', u'Tests'], - 'subcategories': { - u'Tests': { - 'sort_key': u'Tests', - 'subcategories': {}, - 'children': [u'Quizzes', u'All', u'Final Exam'], - 'entries': { - u'Quizzes': { - 'sort_key': None - }, u'All': { - 'sort_key': None - }, u'Final Exam': { - 'sort_key': None - }, - } - }, - u'Assignments': { - 'sort_key': u'Assignments', - 'subcategories': {}, - 'children': [u'All', u'Homework'], - 'entries': { - u'Homework': { - 'sort_key': None - }, - u'All': { - 'sort_key': None - }, - } - } - } - } - - utils.sort_map_entries(self.category_map, False) - self.assertEqual(self.category_map, expected_false) - - class AccessUtilsTestCase(TestCase): def setUp(self): self.course_id = 'edX/toy/2012_Fall' @@ -179,3 +62,416 @@ class AccessUtilsTestCase(TestCase): ret = utils.has_forum_access('student', self.course_id, 'NotARole') self.assertFalse(ret) + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class CoursewareContextTestCase(ModuleStoreTestCase): + def setUp(self): + self.course = CourseFactory.create(org="TestX", number="101", display_name="Test Course") + self.discussion1 = ItemFactory.create( + parent_location=self.course.location, + category="discussion", + discussion_id="discussion1", + discussion_category="Chapter", + discussion_target="Discussion 1" + ) + self.discussion2 = ItemFactory.create( + parent_location=self.course.location, + category="discussion", + discussion_id="discussion2", + discussion_category="Chapter / Section / Subsection", + discussion_target="Discussion 2" + ) + + def test_missing_commentable_id(self): + thread = {"commentable_id": "non-inline"} + self.assertEqual(utils.get_courseware_context(thread, self.course), None) + + 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"])) + self.assertEqual( + courseware_context["courseware_url"], + reverse( + "jump_to", + kwargs={ + "course_id": self.course.location.course_id, + "location": discussion.location + } + ) + ) + self.assertEqual( + courseware_context["courseware_title"], + expected_title + ) + + test_discussion(self.discussion1, "Chapter / Discussion 1") + test_discussion(self.discussion2, " Subsection / Discussion 2") + + +@override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE) +class CategoryMapTestCase(ModuleStoreTestCase): + def setUp(self): + self.course = CourseFactory.create(org="TestX", number="101", display_name="Test Course") + # Courses get a default discussion topic on creation, so remove it + self.course.discussion_topics = {} + self.course.save() + self.discussion_num = 0 + self.maxDiff = None #pylint: disable=C0103 + + def create_discussion(self, discussion_category, discussion_target, **kwargs): + self.discussion_num += 1 + ItemFactory.create( + parent_location=self.course.location, + category="discussion", + discussion_id="discussion{}".format(self.discussion_num), + discussion_category=discussion_category, + discussion_target=discussion_target, + **kwargs + ) + + def assertCategoryMapEquals(self, expected): + self.assertEqual( + utils.get_discussion_category_map(self.course), + expected + ) + + def test_empty(self): + self.assertEqual( + utils.get_discussion_category_map(self.course), + {"entries": {}, "subcategories": {}, "children": []} + ) + + def test_configured_topics(self): + self.course.discussion_topics = { + "Topic A": {"id": "Topic_A"}, + "Topic B": {"id": "Topic_B"}, + "Topic C": {"id": "Topic_C"} + } + self.assertCategoryMapEquals( + { + "entries": { + "Topic A": {"id": "Topic_A", "sort_key": "Topic A"}, + "Topic B": {"id": "Topic_B", "sort_key": "Topic B"}, + "Topic C": {"id": "Topic_C", "sort_key": "Topic C"}, + }, + "subcategories": {}, + "children": ["Topic A", "Topic B", "Topic C"] + } + ) + + def test_single_inline(self): + self.create_discussion("Chapter", "Discussion") + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter": { + "entries": { + "Discussion": { + "id": "discussion1", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + } + }, + "children": ["Chapter"] + } + ) + + def test_tree(self): + self.create_discussion("Chapter 1", "Discussion 1") + self.create_discussion("Chapter 1", "Discussion 2") + self.create_discussion("Chapter 2", "Discussion") + self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion") + self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion") + self.create_discussion("Chapter 3 / Section 1", "Discussion") + + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter 1": { + "entries": { + "Discussion 1": { + "id": "discussion1", + "sort_key": None + }, + "Discussion 2": { + "id": "discussion2", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion 1", "Discussion 2"] + }, + "Chapter 2": { + "entries": { + "Discussion": { + "id": "discussion3", + "sort_key": None + } + }, + "subcategories": { + "Section 1": { + "entries": {}, + "subcategories": { + "Subsection 1": { + "entries": { + "Discussion": { + "id": "discussion4", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + }, + "Subsection 2": { + "entries": { + "Discussion": { + "id": "discussion5", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + } + }, + "children": ["Subsection 1", "Subsection 2"] + } + }, + "children": ["Discussion", "Section 1"] + }, + "Chapter 3": { + "entries": {}, + "subcategories": { + "Section 1": { + "entries": { + "Discussion": { + "id": "discussion6", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + } + }, + "children": ["Section 1"] + } + }, + "children": ["Chapter 1", "Chapter 2", "Chapter 3"] + } + ) + + def test_start_date_filter(self): + now = datetime.now() + later = datetime.max + self.create_discussion("Chapter 1", "Discussion 1", start=now) + self.create_discussion("Chapter 1", "Discussion 2", start=later) + self.create_discussion("Chapter 2", "Discussion", start=now) + self.create_discussion("Chapter 2 / Section 1 / Subsection 1", "Discussion", start=later) + self.create_discussion("Chapter 2 / Section 1 / Subsection 2", "Discussion", start=later) + self.create_discussion("Chapter 3 / Section 1", "Discussion", start=later) + + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter 1": { + "entries": { + "Discussion 1": { + "id": "discussion1", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion 1"] + }, + "Chapter 2": { + "entries": { + "Discussion": { + "id": "discussion3", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + } + }, + "children": ["Chapter 1", "Chapter 2"] + } + ) + + def test_sort_inline_explicit(self): + self.create_discussion("Chapter", "Discussion 1", sort_key="D") + self.create_discussion("Chapter", "Discussion 2", sort_key="A") + self.create_discussion("Chapter", "Discussion 3", sort_key="E") + self.create_discussion("Chapter", "Discussion 4", sort_key="C") + self.create_discussion("Chapter", "Discussion 5", sort_key="B") + + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter": { + "entries": { + "Discussion 1": { + "id": "discussion1", + "sort_key": "D" + }, + "Discussion 2": { + "id": "discussion2", + "sort_key": "A" + }, + "Discussion 3": { + "id": "discussion3", + "sort_key": "E" + }, + "Discussion 4": { + "id": "discussion4", + "sort_key": "C" + }, + "Discussion 5": { + "id": "discussion5", + "sort_key": "B" + } + }, + "subcategories": {}, + "children": [ + "Discussion 2", + "Discussion 5", + "Discussion 4", + "Discussion 1", + "Discussion 3" + ] + } + }, + "children": ["Chapter"] + } + ) + + def test_sort_configured_topics_explicit(self): + self.course.discussion_topics = { + "Topic A": {"id": "Topic_A", "sort_key": "B"}, + "Topic B": {"id": "Topic_B", "sort_key": "C"}, + "Topic C": {"id": "Topic_C", "sort_key": "A"} + } + self.assertCategoryMapEquals( + { + "entries": { + "Topic A": {"id": "Topic_A", "sort_key": "B"}, + "Topic B": {"id": "Topic_B", "sort_key": "C"}, + "Topic C": {"id": "Topic_C", "sort_key": "A"}, + }, + "subcategories": {}, + "children": ["Topic C", "Topic A", "Topic B"] + } + ) + + def test_sort_alpha(self): + self.course.discussion_sort_alpha = True + self.course.save() + self.create_discussion("Chapter", "Discussion D") + self.create_discussion("Chapter", "Discussion A") + self.create_discussion("Chapter", "Discussion E") + self.create_discussion("Chapter", "Discussion C") + self.create_discussion("Chapter", "Discussion B") + + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter": { + "entries": { + "Discussion D": { + "id": "discussion1", + "sort_key": "Discussion D" + }, + "Discussion A": { + "id": "discussion2", + "sort_key": "Discussion A" + }, + "Discussion E": { + "id": "discussion3", + "sort_key": "Discussion E" + }, + "Discussion C": { + "id": "discussion4", + "sort_key": "Discussion C" + }, + "Discussion B": { + "id": "discussion5", + "sort_key": "Discussion B" + } + }, + "subcategories": {}, + "children": [ + "Discussion A", + "Discussion B", + "Discussion C", + "Discussion D", + "Discussion E" + ] + } + }, + "children": ["Chapter"] + } + ) + + def test_sort_intermediates(self): + self.create_discussion("Chapter B", "Discussion 2") + self.create_discussion("Chapter C", "Discussion") + self.create_discussion("Chapter A", "Discussion 1") + self.create_discussion("Chapter B", "Discussion 1") + self.create_discussion("Chapter A", "Discussion 2") + + self.assertCategoryMapEquals( + { + "entries": {}, + "subcategories": { + "Chapter A": { + "entries": { + "Discussion 1": { + "id": "discussion3", + "sort_key": None + }, + "Discussion 2": { + "id": "discussion5", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion 1", "Discussion 2"] + }, + "Chapter B": { + "entries": { + "Discussion 1": { + "id": "discussion4", + "sort_key": None + }, + "Discussion 2": { + "id": "discussion1", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion 1", "Discussion 2"] + }, + "Chapter C": { + "entries": { + "Discussion": { + "id": "discussion2", + "sort_key": None + } + }, + "subcategories": {}, + "children": ["Discussion"] + } + }, + "children": ["Chapter A", "Chapter B", "Chapter C"] + } + ) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 3e0699f2ef..691334f0d7 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -70,10 +70,10 @@ def get_discussion_id_map(course): def get_discussion_category_map(course): initialize_discussion_info(course) - return filter_unstarted_categories(_DISCUSSIONINFO[course.id]['category_map']) + 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()) @@ -111,7 +111,7 @@ def filter_unstarted_categories(category_map): return result_map -def sort_map_entries(category_map, sort_alpha): +def _sort_map_entries(category_map, sort_alpha): things = [] for title, entry in category_map["entries"].items(): if entry["sort_key"] == None and sort_alpha: @@ -119,7 +119,7 @@ def sort_map_entries(category_map, sort_alpha): things.append((title, entry)) for title, category in category_map["subcategories"].items(): 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"])] @@ -199,7 +199,7 @@ def initialize_discussion_info(course): "sort_key": entry.get("sort_key", topic), "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 _DISCUSSIONINFO[course.id]['category_map'] = category_map From a7d769dfea0d2cfab23ec803b97ce003aa414541 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Fri, 25 Oct 2013 17:17:24 -0400 Subject: [PATCH 3/4] Remove an unnecessary global The _DISCUSSIONINFO global was originally used as a cache, but has since lost that capability and is therefore just harmful. This is a precursor to more refactoring that will improve the performance of the forums and may itself provide some performance improvement because it separates the computation done by two functions that each previously computed the entirety of _DISCUSSIONINFO. --- .../django_comment_client/tests/test_utils.py | 2 +- lms/djangoapps/django_comment_client/utils.py | 62 +++++++++---------- 2 files changed, 29 insertions(+), 35 deletions(-) diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 5132ccc7c5..9189fe8625 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -108,7 +108,7 @@ class CoursewareContextTestCase(ModuleStoreTestCase): ) test_discussion(self.discussion1, "Chapter / Discussion 1") - test_discussion(self.discussion2, " Subsection / Discussion 2") + test_discussion(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 691334f0d7..258672fe4e 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -20,8 +20,6 @@ from django.utils.timezone import UTC log = logging.getLogger(__name__) -_DISCUSSIONINFO = defaultdict(dict) - def extract(dic, keys): return {k: dic.get(k) for k in keys} @@ -60,17 +58,30 @@ def has_forum_access(uname, course_id, rolename): return role.users.filter(username=uname).exists() -def get_discussion_id_map(course): - """ - return a dict of the form {category: modules} - """ - initialize_discussion_info(course) - return _DISCUSSIONINFO[course.id]['id_map'] +def _get_discussion_modules(course): + all_modules = modulestore().get_items( + ['i4x', course.location.org, course.location.course, 'discussion', None], + course_id=course.id + ) + + def has_required_keys(module): + 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)) + return False + return True + + return filter(has_required_keys, all_modules) -def get_discussion_category_map(course): - initialize_discussion_info(course) - return _filter_unstarted_categories(_DISCUSSIONINFO[course.id]['category_map']) +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}) + + return dict(map(get_entry, _get_discussion_modules(course))) def _filter_unstarted_categories(category_map): @@ -123,33 +134,18 @@ def _sort_map_entries(category_map, sort_alpha): 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 - discussion_id_map = {} unexpanded_category_map = defaultdict(list) - # get all discussion models within this course_id - 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 + modules = _get_discussion_modules(course) + for module in modules: id = module.discussion_id - category = module.discussion_category title = module.discussion_target sort_key = module.sort_key - category = " / ".join([x.strip() for x in category.split("/")]) - last_category = category.split("/")[-1] - discussion_id_map[id] = {"location": module.location, "title": last_category + " / " + title} + category = " / ".join([x.strip() for x in module.discussion_category.split("/")]) #Handle case where module.start is None 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}) @@ -201,9 +197,7 @@ def initialize_discussion_info(course): _sort_map_entries(category_map, course.discussion_sort_alpha) - _DISCUSSIONINFO[course.id]['id_map'] = discussion_id_map - _DISCUSSIONINFO[course.id]['category_map'] = category_map - _DISCUSSIONINFO[course.id]['timestamp'] = datetime.now(UTC()) + return _filter_unstarted_categories(category_map) class JsonResponse(HttpResponse): @@ -354,7 +348,7 @@ def extend_content(content): def get_courseware_context(content, course): - id_map = get_discussion_id_map(course) + id_map = _get_discussion_id_map(course) id = content['commentable_id'] content_info = None if id in id_map: From e6ecb6ecfeb0bd87935eb1acd4e24c779534daf1 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Wed, 23 Oct 2013 18:19:33 -0400 Subject: [PATCH 4/4] Improve performance of forum views Avoid recomputing course module information for every thread, which should dramatically improve the performance of high-percentile latency queries. JIRA: FOR-250 --- CHANGELOG.rst | 3 ++ .../django_comment_client/base/views.py | 6 ++-- .../django_comment_client/forum/views.py | 34 ++++++++---------- .../django_comment_client/tests/test_utils.py | 35 ++++++++++++------- lms/djangoapps/django_comment_client/utils.py | 20 +++++------ 5 files changed, 51 insertions(+), 47 deletions(-) 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):