From 695b036282346ba0ad21ed8f5c42d1e4e40b3f37 Mon Sep 17 00:00:00 2001 From: Douglas Hall Date: Tue, 24 Apr 2018 19:42:21 -0400 Subject: [PATCH] Store Discussions ID Map on CourseDiscussionSettings model when course publish signal is fired. --- ...sediscussionsettings_discussions_id_map.py | 21 ++++++++++ .../django_comment_common/models.py | 6 +++ .../djangoapps/django_comment_common/utils.py | 9 +++- lms/djangoapps/discussion/settings/common.py | 4 ++ lms/djangoapps/discussion/signals/handlers.py | 23 +++++++++- lms/djangoapps/discussion/tasks.py | 21 +++++++++- .../discussion/tests/test_signals.py | 42 ++++++++++++++++++- lms/djangoapps/discussion/tests/test_tasks.py | 3 -- .../django_comment_client/base/tests.py | 8 ++-- lms/djangoapps/django_comment_client/utils.py | 6 +-- lms/envs/common.py | 4 -- 11 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 common/djangoapps/django_comment_common/migrations/0006_coursediscussionsettings_discussions_id_map.py diff --git a/common/djangoapps/django_comment_common/migrations/0006_coursediscussionsettings_discussions_id_map.py b/common/djangoapps/django_comment_common/migrations/0006_coursediscussionsettings_discussions_id_map.py new file mode 100644 index 0000000000..6891133e65 --- /dev/null +++ b/common/djangoapps/django_comment_common/migrations/0006_coursediscussionsettings_discussions_id_map.py @@ -0,0 +1,21 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.12 on 2018-04-23 21:09 +from __future__ import unicode_literals + +from django.db import migrations +import jsonfield.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('django_comment_common', '0005_coursediscussionsettings'), + ] + + operations = [ + migrations.AddField( + model_name='coursediscussionsettings', + name='discussions_id_map', + field=jsonfield.fields.JSONField(blank=True, help_text=b'Key/value store mapping discussion IDs to discussion XBlock usage keys.', null=True), + ), + ] diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index 48f3040044..1f7b16a6ec 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -8,6 +8,7 @@ from django.db import models from django.db.models.signals import post_save from django.dispatch import receiver from django.utils.translation import ugettext_noop +from jsonfield.fields import JSONField from opaque_keys.edx.django.models import CourseKeyField from six import text_type @@ -180,6 +181,11 @@ class CourseDiscussionSettings(models.Model): db_index=True, help_text="Which course are these settings associated with?", ) + discussions_id_map = JSONField( + null=True, + blank=True, + help_text="Key/value store mapping discussion IDs to discussion XBlock usage keys.", + ) always_divide_inline_discussions = models.BooleanField(default=False) _divided_discussions = models.TextField(db_column='divided_discussions', null=True, blank=True) # JSON list diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index dcdd986a7b..dc810c33bb 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -139,11 +139,18 @@ def set_course_discussion_settings(course_key, **kwargs): divided_discussions (list): List of discussion ids. division_scheme (str): `CourseDiscussionSettings.NONE`, `CourseDiscussionSettings.COHORT`, or `CourseDiscussionSettings.ENROLLMENT_TRACK` + discussions_id_map (dict): Dict containing discussion IDs as keys and the associated discussion + XBlock usage keys as values. Returns: A CourseDiscussionSettings object. """ - fields = {'division_scheme': basestring, 'always_divide_inline_discussions': bool, 'divided_discussions': list} + fields = { + 'division_scheme': basestring, + 'always_divide_inline_discussions': bool, + 'divided_discussions': list, + 'discussions_id_map': dict, + } course_discussion_settings = get_course_discussion_settings(course_key) for field, field_type in fields.items(): if field in kwargs: diff --git a/lms/djangoapps/discussion/settings/common.py b/lms/djangoapps/discussion/settings/common.py index 313c569b2d..bbc524982c 100644 --- a/lms/djangoapps/discussion/settings/common.py +++ b/lms/djangoapps/discussion/settings/common.py @@ -4,3 +4,7 @@ def plugin_settings(settings): """Settings for the discussions plugin. """ settings.FEATURES['ALLOW_HIDING_DISCUSSION_TAB'] = False + settings.DISCUSSION_SETTINGS = { + 'MAX_COMMENT_DEPTH': 2, + 'COURSE_PUBLISH_TASK_DELAY': 30, + } diff --git a/lms/djangoapps/discussion/signals/handlers.py b/lms/djangoapps/discussion/signals/handlers.py index 44941e91de..97bf2cb349 100644 --- a/lms/djangoapps/discussion/signals/handlers.py +++ b/lms/djangoapps/discussion/signals/handlers.py @@ -3,13 +3,15 @@ Signal handlers related to discussions. """ import logging +from django.conf import settings from django.dispatch import receiver -from opaque_keys.edx.keys import CourseKey from django_comment_common import signals from lms.djangoapps.discussion import tasks +from opaque_keys.edx.locator import LibraryLocator from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from openedx.core.djangoapps.theming.helpers import get_current_site +from xmodule.modulestore.django import SignalHandler log = logging.getLogger(__name__) @@ -18,6 +20,25 @@ log = logging.getLogger(__name__) ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY = 'enable_forum_notifications' +@receiver(SignalHandler.course_published) +def update_discussions_on_course_publish(sender, course_key, **kwargs): # pylint: disable=unused-argument + """ + Catches the signal that a course has been published in the module + store and creates/updates the corresponding cache entry. + Ignores publish signals from content libraries. + """ + if isinstance(course_key, LibraryLocator): + return + + context = { + 'course_id': unicode(course_key), + } + tasks.update_discussions_map.apply_async( + args=[context], + countdown=settings.DISCUSSION_SETTINGS['COURSE_PUBLISH_TASK_DELAY'], + ) + + @receiver(signals.comment_created) def send_discussion_email_notification(sender, user, post, **kwargs): current_site = get_current_site() diff --git a/lms/djangoapps/discussion/tasks.py b/lms/djangoapps/discussion/tasks.py index 764bbc2a43..1c46efc66c 100644 --- a/lms/djangoapps/discussion/tasks.py +++ b/lms/djangoapps/discussion/tasks.py @@ -12,12 +12,13 @@ from django.contrib.auth.models import User from django.contrib.sites.models import Site from celery_utils.logged_task import LoggedTask +from django_comment_common.utils import set_course_discussion_settings from edx_ace import ace from edx_ace.utils import date from edx_ace.message import MessageType from edx_ace.recipient import Recipient from opaque_keys.edx.keys import CourseKey -from lms.djangoapps.django_comment_client.utils import permalink +from lms.djangoapps.django_comment_client.utils import permalink, get_accessible_discussion_xblocks_by_course_id import lms.lib.comment_client as cc from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -32,6 +33,24 @@ DEFAULT_LANGUAGE = 'en' ROUTING_KEY = getattr(settings, 'ACE_ROUTING_KEY', None) +@task(base=LoggedTask) +def update_discussions_map(context): + """ + Updates the mapping between discussion_id to discussion block usage key + for all discussion blocks in the given course. + + context is a dict that contains: + course_id (string): identifier of the course + """ + course_key = CourseKey.from_string(context['course_id']) + discussion_blocks = get_accessible_discussion_xblocks_by_course_id(course_key, include_all=True) + discussions_id_map = { + discussion_block.discussion_id: unicode(discussion_block.location) + for discussion_block in discussion_blocks + } + set_course_discussion_settings(course_key, discussions_id_map=discussions_id_map) + + class ResponseNotification(MessageType): pass diff --git a/lms/djangoapps/discussion/tests/test_signals.py b/lms/djangoapps/discussion/tests/test_signals.py index 8cc3eec34f..d15dbdd04c 100644 --- a/lms/djangoapps/discussion/tests/test_signals.py +++ b/lms/djangoapps/discussion/tests/test_signals.py @@ -1,10 +1,12 @@ from django.test import TestCase import mock -from django_comment_common import signals +from django_comment_common import signals, models from lms.djangoapps.discussion.signals.handlers import ENABLE_FORUM_NOTIFICATIONS_FOR_SITE_KEY +import openedx.core.djangoapps.request_cache as request_cache from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory, SiteConfigurationFactory -from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory class SendMessageHandlerTestCase(TestCase): @@ -56,3 +58,39 @@ class SendMessageHandlerTestCase(TestCase): signals.comment_created.send(sender=self.sender, user=self.user, post=self.post) self.assertFalse(mock_send_message.called) + + +class CoursePublishHandlerTestCase(ModuleStoreTestCase): + """ + Tests for discussion updates on course publish. + """ + ENABLED_SIGNALS = ['course_published'] + + def test_discussion_id_map_updates_on_publish(self): + course_key_args = dict(org='org', course='number', run='run') + course_key = self.store.make_course_key(**course_key_args) + + with self.assertRaises(models.CourseDiscussionSettings.DoesNotExist): + models.CourseDiscussionSettings.objects.get(course_id=course_key) + + # create course + course = CourseFactory(emit_signals=True, **course_key_args) + self.assertEqual(course.id, course_key) + self._assert_discussion_id_map(course_key, {}) + + # create discussion block + request_cache.clear_cache(name=None) + discussion_id = 'discussion1' + discussion_block = ItemFactory.create( + parent_location=course.location, + category="discussion", + discussion_id=discussion_id, + ) + self._assert_discussion_id_map(course_key, {discussion_id: str(discussion_block.location)}) + + def _assert_discussion_id_map(self, course_key, expected_map): + """ + Verifies the discussion ID map for the given course matches the expected value. + """ + discussion_settings = models.CourseDiscussionSettings.objects.get(course_id=course_key) + self.assertDictEqual(discussion_settings.discussions_id_map, expected_map) diff --git a/lms/djangoapps/discussion/tests/test_tasks.py b/lms/djangoapps/discussion/tests/test_tasks.py index 7c59c23d24..ff4191c33b 100644 --- a/lms/djangoapps/discussion/tests/test_tasks.py +++ b/lms/djangoapps/discussion/tests/test_tasks.py @@ -5,7 +5,6 @@ from datetime import datetime, timedelta import json import math -from crum import CurrentRequestUserMiddleware import ddt from django.contrib.sites.models import Site import mock @@ -22,8 +21,6 @@ from lms.djangoapps.discussion.tasks import _should_send_message, _track_notific from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.site_configuration.tests.factories import SiteConfigurationFactory -from openedx.core.djangoapps.theming.middleware import CurrentSiteThemeMiddleware -from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from openedx.core.lib.celery.task_utils import emulate_http_request from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index a2946010f2..603f0c37eb 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -406,8 +406,8 @@ class ViewsQueryCountTestCase( return inner @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 4, 35), - (ModuleStoreEnum.Type.split, 3, 13, 35), + (ModuleStoreEnum.Type.mongo, 3, 3, 30), + (ModuleStoreEnum.Type.split, 3, 11, 30), ) @ddt.unpack @count_queries @@ -415,8 +415,8 @@ class ViewsQueryCountTestCase( self.create_thread_helper(mock_request) @ddt.data( - (ModuleStoreEnum.Type.mongo, 3, 3, 31), - (ModuleStoreEnum.Type.split, 3, 10, 31), + (ModuleStoreEnum.Type.mongo, 3, 2, 26), + (ModuleStoreEnum.Type.split, 3, 8, 26), ) @ddt.unpack @count_queries diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 64bab192b4..24c928c349 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -125,10 +125,10 @@ def get_accessible_discussion_xblocks(course, user, include_all=False): # pylin @request_cached -def get_accessible_discussion_xblocks_by_course_id(course_id, user, include_all=False): # pylint: disable=invalid-name +def get_accessible_discussion_xblocks_by_course_id(course_id, user=None, include_all=False): # pylint: disable=invalid-name """ - Return a list of all valid discussion xblocks in this course that - are accessible to the given user. + Return a list of all valid discussion xblocks in this course. + Checks for the given user's access if include_all is False. """ all_xblocks = modulestore().get_items(course_id, qualifiers={'category': 'discussion'}, include_orphans=False) diff --git a/lms/envs/common.py b/lms/envs/common.py index 846a71a88d..b0fc9f3bb5 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -59,10 +59,6 @@ PLATFORM_TWITTER_ACCOUNT = "@YourPlatformTwitterAccount" ENABLE_JASMINE = False -DISCUSSION_SETTINGS = { - 'MAX_COMMENT_DEPTH': 2, -} - LMS_ROOT_URL = "http://localhost:8000" LMS_INTERNAL_ROOT_URL = LMS_ROOT_URL LMS_ENROLLMENT_API_PATH = "/api/enrollment/v1/"