From 2acb4a6cad26ce6ca253a028424acf62e18c78bb Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Tue, 25 Oct 2016 13:57:13 -0400 Subject: [PATCH] [PERF-386] Utilize ForumsConfig to enable and disable forums. This specifically enables/disables the underlying comment service client used to make calls to the service. When disabled, this client will now throw an exception which can be propagated upwards so that callers can make the right decision about how to notify users of the error, or handle retry, etc etc. --- .../django_comment_common/models.py | 12 ++- .../student/tests/test_create_account.py | 5 ++ common/test/acceptance/fixtures/discussion.py | 10 +++ .../acceptance/tests/discussion/helpers.py | 6 +- .../tests/discussion/test_discussion.py | 4 +- .../test/acceptance/tests/lms/test_teams.py | 7 +- lms/djangoapps/discussion/tests/test_views.py | 58 +++++++++++---- .../discussion_api/tests/test_api.py | 18 +++-- .../discussion_api/tests/test_serializers.py | 12 ++- .../discussion_api/tests/test_views.py | 3 +- .../django_comment_client/base/tests.py | 74 ++++++++++++++++--- .../django_comment_client/tests/test_utils.py | 32 ++++++++ .../django_comment_client/tests/utils.py | 16 +++- lms/lib/comment_client/utils.py | 9 ++- lms/urls.py | 2 + 15 files changed, 220 insertions(+), 48 deletions(-) diff --git a/common/djangoapps/django_comment_common/models.py b/common/djangoapps/django_comment_common/models.py index b3c046f920..333df46451 100644 --- a/common/djangoapps/django_comment_common/models.py +++ b/common/djangoapps/django_comment_common/models.py @@ -1,5 +1,6 @@ import logging +from django.conf import settings from django.db import models from django.contrib.auth.models import User @@ -150,8 +151,15 @@ def all_permissions_for_user_in_course(user, course_id): # pylint: disable=inva class ForumsConfig(ConfigurationModel): """Config for the connection to the cs_comments_service forums backend.""" - # For now, just tweak the connection timeout settings. We can add more later. - connection_timeout = models.FloatField(default=5.0) + connection_timeout = models.FloatField( + default=5.0, + help_text="Seconds to wait when trying to connect to the comment service.", + ) + + @property + def api_key(self): + """The API key used to authenticate to the comments service.""" + return getattr(settings, "COMMENTS_SERVICE_KEY", None) def __unicode__(self): """Simple representation so the admin screen looks less ugly.""" diff --git a/common/djangoapps/student/tests/test_create_account.py b/common/djangoapps/student/tests/test_create_account.py index a802827899..1aa79fed35 100644 --- a/common/djangoapps/student/tests/test_create_account.py +++ b/common/djangoapps/student/tests/test_create_account.py @@ -21,6 +21,7 @@ from openedx.core.djangoapps.external_auth.models import ExternalAuthMap import student from student.models import UserAttribute from student.views import REGISTRATION_AFFILIATE_ID, REGISTRATION_UTM_PARAMETERS, REGISTRATION_UTM_CREATED_AT +from django_comment_common.models import ForumsConfig TEST_CS_URL = 'https://comments.service.test:123/' @@ -694,6 +695,10 @@ class TestCreateCommentsServiceUser(TransactionTestCase): "terms_of_service": "true", } + config = ForumsConfig.current() + config.enabled = True + config.save() + def test_cs_user_created(self, request): "If user account creation succeeds, we should create a comments service user" response = self.client.post(self.url, self.params) diff --git a/common/test/acceptance/fixtures/discussion.py b/common/test/acceptance/fixtures/discussion.py index 3c91e2fce1..0607625fb0 100644 --- a/common/test/acceptance/fixtures/discussion.py +++ b/common/test/acceptance/fixtures/discussion.py @@ -9,6 +9,7 @@ import factory import requests from common.test.acceptance.fixtures import COMMENTS_STUB_URL +from common.test.acceptance.fixtures.config import ConfigModelFixture class ContentFactory(factory.Factory): @@ -170,3 +171,12 @@ class SearchResultFixture(DiscussionContentFixture): def get_config_data(self): return {"search_result": json.dumps(self.result)} + + +class ForumsConfigMixin(object): + """Mixin providing a method used to configure the forums integration.""" + def enable_forums(self, is_enabled=True): + """Configures whether or not forums are enabled.""" + ConfigModelFixture('/config/forums', { + 'enabled': is_enabled, + }).install() diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index c29d1a032c..0da35d8b21 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/common/test/acceptance/tests/discussion/helpers.py @@ -11,6 +11,7 @@ from common.test.acceptance.fixtures.discussion import ( SingleThreadViewFixture, Thread, Response, + ForumsConfigMixin, ) from common.test.acceptance.pages.lms.discussion import DiscussionTabSingleThreadPage from common.test.acceptance.tests.helpers import UniqueCourseTest @@ -86,7 +87,8 @@ class CohortTestMixin(object): self.assertTrue(response.ok, "Failed to add user to cohort") -class BaseDiscussionTestCase(UniqueCourseTest): +class BaseDiscussionTestCase(UniqueCourseTest, ForumsConfigMixin): + """Base test case class for all discussions-related tests.""" def setUp(self): super(BaseDiscussionTestCase, self).setUp() @@ -97,6 +99,8 @@ class BaseDiscussionTestCase(UniqueCourseTest): ) self.course_fixture.install() + self.enable_forums() + def create_single_thread_page(self, thread_id): """ Sets up a `DiscussionTabSingleThreadPage` for a given diff --git a/common/test/acceptance/tests/discussion/test_discussion.py b/common/test/acceptance/tests/discussion/test_discussion.py index ea57057cb4..897628eacf 100644 --- a/common/test/acceptance/tests/discussion/test_discussion.py +++ b/common/test/acceptance/tests/discussion/test_discussion.py @@ -180,7 +180,8 @@ class DiscussionResponsePaginationTestMixin(BaseDiscussionMixin): self.assertFalse(self.thread_page.has_add_response_button()) -class DiscussionHomePageTest(UniqueCourseTest): +@attr(shard=2) +class DiscussionHomePageTest(BaseDiscussionTestCase): """ Tests for the discussion home page. """ @@ -189,7 +190,6 @@ class DiscussionHomePageTest(UniqueCourseTest): def setUp(self): super(DiscussionHomePageTest, self).setUp() - CourseFixture(**self.course_info).install() AutoAuthPage(self.browser, course_id=self.course_id).visit() self.page = DiscussionTabHomePage(self.browser, self.course_id) self.page.visit() diff --git a/common/test/acceptance/tests/lms/test_teams.py b/common/test/acceptance/tests/lms/test_teams.py index 181621b5dc..089da322c7 100644 --- a/common/test/acceptance/tests/lms/test_teams.py +++ b/common/test/acceptance/tests/lms/test_teams.py @@ -16,7 +16,8 @@ from common.test.acceptance.fixtures import LMS_BASE_URL from common.test.acceptance.fixtures.course import CourseFixture from common.test.acceptance.fixtures.discussion import ( Thread, - MultipleThreadFixture + MultipleThreadFixture, + ForumsConfigMixin, ) from common.test.acceptance.pages.lms.auto_auth import AutoAuthPage from common.test.acceptance.pages.lms.course_info import CourseInfoPage @@ -37,7 +38,7 @@ from common.test.acceptance.pages.common.utils import confirm_prompt TOPICS_PER_PAGE = 12 -class TeamsTabBase(EventsTestMixin, UniqueCourseTest): +class TeamsTabBase(EventsTestMixin, ForumsConfigMixin, UniqueCourseTest): """Base class for Teams Tab tests""" def setUp(self): super(TeamsTabBase, self).setUp() @@ -47,6 +48,8 @@ class TeamsTabBase(EventsTestMixin, UniqueCourseTest): # TODO: Refactor so resetting events database is not necessary self.reset_event_tracking() + self.enable_forums() + def create_topics(self, num_topics): """Create `num_topics` test topics.""" return [{u"description": i, u"name": i, u"id": i} for i in map(str, xrange(num_topics))] diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 6f8dfb7f4d..00dafece4a 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -10,6 +10,7 @@ from django.utils import translation from lms.lib.comment_client.utils import CommentClientPaginatedResult from django_comment_common.utils import ThreadContext +from django_comment_common.models import ForumsConfig from django_comment_client.permissions import get_team from django_comment_client.tests.group_id import ( GroupIdAssertionMixin, @@ -17,7 +18,7 @@ from django_comment_client.tests.group_id import ( NonCohortedTopicGroupIdTestMixin, ) from django_comment_client.tests.unicode import UnicodeTestMixin -from django_comment_client.tests.utils import CohortedTestCase +from django_comment_client.tests.utils import CohortedTestCase, ForumsEnableMixin from django_comment_client.utils import strip_none from lms.djangoapps.discussion import views from student.tests.factories import UserFactory, CourseEnrollmentFactory @@ -76,6 +77,10 @@ class ViewsExceptionTestCase(UrlResetMixin, ModuleStoreTestCase): self.client = Client() assert_true(self.client.login(username=uname, password=password)) + config = ForumsConfig.current() + config.enabled = True + config.save() + @patch('student.models.cc.User.from_django_user') @patch('student.models.cc.User.active_threads') def test_user_profile_exception(self, mock_threads, mock_from_django_user): @@ -220,7 +225,7 @@ class PartialDictMatcher(object): @patch('requests.request', autospec=True) -class SingleThreadTestCase(ModuleStoreTestCase): +class SingleThreadTestCase(ForumsEnableMixin, ModuleStoreTestCase): CREATE_USER = False @@ -334,13 +339,16 @@ class SingleThreadTestCase(ModuleStoreTestCase): @ddt.ddt @patch('requests.request', autospec=True) -class SingleThreadQueryCountTestCase(ModuleStoreTestCase): +class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): """ Ensures the number of modulestore queries and number of sql queries are independent of the number of responses retrieved for a given discussion thread. """ MODULESTORE = TEST_DATA_MONGO_MODULESTORE + def setUp(self): + super(SingleThreadQueryCountTestCase, self).setUp() + @ddt.data( # Old mongo with cache. There is an additional SQL query for old mongo # because the first time that disabled_xblocks is queried is in call_single_thread, @@ -598,7 +606,7 @@ class SingleThreadGroupIdTestCase(CohortedTestCase, GroupIdAssertionMixin): @patch('requests.request', autospec=True) -class SingleThreadContentGroupTestCase(UrlResetMixin, ContentGroupTestCase): +class SingleThreadContentGroupTestCase(ForumsEnableMixin, UrlResetMixin, ContentGroupTestCase): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): @@ -708,7 +716,7 @@ class SingleThreadContentGroupTestCase(UrlResetMixin, ContentGroupTestCase): @patch('lms.lib.comment_client.utils.requests.request', autospec=True) -class InlineDiscussionContextTestCase(ModuleStoreTestCase): +class InlineDiscussionContextTestCase(ForumsEnableMixin, ModuleStoreTestCase): def setUp(self): super(InlineDiscussionContextTestCase, self).setUp() self.course = CourseFactory.create() @@ -1039,7 +1047,7 @@ class FollowedThreadsDiscussionGroupIdTestCase(CohortedTestCase, CohortedTopicGr @patch('lms.lib.comment_client.utils.requests.request', autospec=True) -class InlineDiscussionTestCase(ModuleStoreTestCase): +class InlineDiscussionTestCase(ForumsEnableMixin, ModuleStoreTestCase): def setUp(self): super(InlineDiscussionTestCase, self).setUp() @@ -1097,7 +1105,7 @@ class InlineDiscussionTestCase(ModuleStoreTestCase): @patch('requests.request', autospec=True) -class UserProfileTestCase(UrlResetMixin, ModuleStoreTestCase): +class UserProfileTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): TEST_THREAD_TEXT = 'userprofile-test-text' TEST_THREAD_ID = 'userprofile-test-thread-id' @@ -1216,7 +1224,7 @@ class UserProfileTestCase(UrlResetMixin, ModuleStoreTestCase): @patch('requests.request', autospec=True) -class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase): +class CommentsServiceRequestHeadersTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): CREATE_USER = False @@ -1282,7 +1290,7 @@ class CommentsServiceRequestHeadersTestCase(UrlResetMixin, ModuleStoreTestCase): self.assert_all_calls_have_header(mock_request, "X-Edx-Api-Key", "test_api_key") -class InlineDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class InlineDiscussionUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): @@ -1297,6 +1305,9 @@ class InlineDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixi cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(InlineDiscussionUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) @@ -1312,7 +1323,7 @@ class InlineDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixi self.assertEqual(response_data["discussion_data"][0]["body"], text) -class ForumFormDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class ForumFormDiscussionUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): # pylint: disable=super-method-not-called @@ -1326,6 +1337,9 @@ class ForumFormDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestM cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(ForumFormDiscussionUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) @@ -1342,7 +1356,7 @@ class ForumFormDiscussionUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestM @ddt.ddt @patch('lms.lib.comment_client.utils.requests.request', autospec=True) -class ForumDiscussionXSSTestCase(UrlResetMixin, ModuleStoreTestCase): +class ForumDiscussionXSSTestCase(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): @patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): super(ForumDiscussionXSSTestCase, self).setUp() @@ -1391,7 +1405,7 @@ class ForumDiscussionXSSTestCase(UrlResetMixin, ModuleStoreTestCase): self.assertNotIn(malicious_code, resp.content) -class ForumDiscussionSearchUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class ForumDiscussionSearchUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): @@ -1406,6 +1420,9 @@ class ForumDiscussionSearchUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTes cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(ForumDiscussionSearchUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) @@ -1424,7 +1441,7 @@ class ForumDiscussionSearchUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTes self.assertEqual(response_data["discussion_data"][0]["body"], text) -class SingleThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class SingleThreadUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): @@ -1439,6 +1456,9 @@ class SingleThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(SingleThreadUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): thread_id = "test_thread_id" @@ -1454,7 +1474,7 @@ class SingleThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): self.assertEqual(response_data["content"]["body"], text) -class UserProfileUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class UserProfileUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): @@ -1469,6 +1489,9 @@ class UserProfileUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(UserProfileUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) @@ -1483,7 +1506,7 @@ class UserProfileUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): self.assertEqual(response_data["discussion_data"][0]["body"], text) -class FollowedThreadsUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin): +class FollowedThreadsUnicodeTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, UnicodeTestMixin): @classmethod def setUpClass(cls): @@ -1498,6 +1521,9 @@ class FollowedThreadsUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(FollowedThreadsUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): mock_request.side_effect = make_mock_request_impl(course=self.course, text=text) @@ -1512,7 +1538,7 @@ class FollowedThreadsUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin self.assertEqual(response_data["discussion_data"][0]["body"], text) -class EnrollmentTestCase(ModuleStoreTestCase): +class EnrollmentTestCase(ForumsEnableMixin, ModuleStoreTestCase): """ Tests for the behavior of views depending on if the student is enrolled in the course diff --git a/lms/djangoapps/discussion_api/tests/test_api.py b/lms/djangoapps/discussion_api/tests/test_api.py index 57a8fb4085..f90bbee25b 100644 --- a/lms/djangoapps/discussion_api/tests/test_api.py +++ b/lms/djangoapps/discussion_api/tests/test_api.py @@ -49,6 +49,7 @@ from django_comment_common.models import ( FORUM_ROLE_STUDENT, Role, ) +from django_comment_client.tests.utils import ForumsEnableMixin from openedx.core.djangoapps.course_groups.models import CourseUserGroupPartitionGroup from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.lib.exceptions import CourseNotFoundError, PageNotFoundError @@ -85,7 +86,7 @@ def _discussion_disabled_course_for(user): @attr(shard=2) @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetCourseTest(UrlResetMixin, SharedModuleStoreTestCase): +class GetCourseTest(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase): """Test for get_course""" @classmethod @@ -133,7 +134,7 @@ class GetCourseTest(UrlResetMixin, SharedModuleStoreTestCase): @attr(shard=2) @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetCourseTestBlackouts(UrlResetMixin, ModuleStoreTestCase): +class GetCourseTestBlackouts(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): """ Tests of get_course for courses that have blackout dates. """ @@ -177,7 +178,7 @@ class GetCourseTestBlackouts(UrlResetMixin, ModuleStoreTestCase): @attr(shard=2) @mock.patch.dict("django.conf.settings.FEATURES", {"DISABLE_START_DATES": False}) @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): +class GetCourseTopicsTest(ForumsEnableMixin, UrlResetMixin, ModuleStoreTestCase): """Test for get_course_topics""" @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUp(self): @@ -551,7 +552,7 @@ class GetCourseTopicsTest(UrlResetMixin, ModuleStoreTestCase): @attr(shard=2) @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +class GetThreadListTest(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): """Test for get_thread_list""" @classmethod @@ -1006,7 +1007,7 @@ class GetThreadListTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleSto @attr(shard=2) @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) -class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): +class GetCommentListTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModuleStoreTestCase): """Test for get_comment_list""" @classmethod @@ -1442,6 +1443,7 @@ class GetCommentListTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): @disable_signal(api, 'thread_voted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CreateThreadTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -1694,6 +1696,7 @@ class CreateThreadTest( @disable_signal(api, 'comment_voted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CreateCommentTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -1962,6 +1965,7 @@ class CreateCommentTest( @disable_signal(api, 'thread_voted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class UpdateThreadTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -2370,6 +2374,7 @@ class UpdateThreadTest( @disable_signal(api, 'comment_voted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class UpdateCommentTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -2774,6 +2779,7 @@ class UpdateCommentTest( @disable_signal(api, 'thread_deleted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class DeleteThreadTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -2914,6 +2920,7 @@ class DeleteThreadTest( @disable_signal(api, 'comment_deleted') @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class DeleteCommentTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase, @@ -3072,6 +3079,7 @@ class DeleteCommentTest( @ddt.ddt @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class RetrieveThreadTest( + ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase diff --git a/lms/djangoapps/discussion_api/tests/test_serializers.py b/lms/djangoapps/discussion_api/tests/test_serializers.py index a979037e97..0b78746897 100644 --- a/lms/djangoapps/discussion_api/tests/test_serializers.py +++ b/lms/djangoapps/discussion_api/tests/test_serializers.py @@ -31,10 +31,11 @@ from util.testing import UrlResetMixin from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory +from django_comment_client.tests.utils import ForumsEnableMixin @ddt.ddt -class SerializerTestMixin(CommentsServiceMockMixin, UrlResetMixin): +class SerializerTestMixin(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin): @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) def setUpClass(cls): @@ -426,7 +427,12 @@ class CommentSerializerTest(SerializerTestMixin, SharedModuleStoreTestCase): @ddt.ddt -class ThreadSerializerDeserializationTest(CommentsServiceMockMixin, UrlResetMixin, SharedModuleStoreTestCase): +class ThreadSerializerDeserializationTest( + ForumsEnableMixin, + CommentsServiceMockMixin, + UrlResetMixin, + SharedModuleStoreTestCase +): """Tests for ThreadSerializer deserialization.""" @classmethod @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) @@ -629,7 +635,7 @@ class ThreadSerializerDeserializationTest(CommentsServiceMockMixin, UrlResetMixi @ddt.ddt -class CommentSerializerDeserializationTest(CommentsServiceMockMixin, SharedModuleStoreTestCase): +class CommentSerializerDeserializationTest(ForumsEnableMixin, CommentsServiceMockMixin, SharedModuleStoreTestCase): """Tests for ThreadSerializer deserialization.""" @classmethod def setUpClass(cls): diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index ea6444b81f..ab9eeaccef 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -27,13 +27,14 @@ from discussion_api.tests.utils import ( make_minimal_cs_thread, make_paginated_api_response, ProfileImageTestMixin) +from django_comment_client.tests.utils import ForumsEnableMixin from student.tests.factories import CourseEnrollmentFactory, UserFactory from util.testing import UrlResetMixin, PatchMediaTypeMixin from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls, ItemFactory -class DiscussionAPIViewTestMixin(CommentsServiceMockMixin, UrlResetMixin): +class DiscussionAPIViewTestMixin(ForumsEnableMixin, CommentsServiceMockMixin, UrlResetMixin): """ Mixin for common code in tests of Discussion API views. This includes creation of common structures (e.g. a course, user, and enrollment), logging diff --git a/lms/djangoapps/django_comment_client/base/tests.py b/lms/djangoapps/django_comment_client/base/tests.py index b2edcd84b8..236c2e3b8d 100644 --- a/lms/djangoapps/django_comment_client/base/tests.py +++ b/lms/djangoapps/django_comment_client/base/tests.py @@ -17,7 +17,7 @@ from lms.lib.comment_client import Thread from common.test.utils import MockSignalHandlerMixin, disable_signal from django_comment_client.base import views from django_comment_client.tests.group_id import CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, GroupIdAssertionMixin -from django_comment_client.tests.utils import CohortedTestCase +from django_comment_client.tests.utils import CohortedTestCase, ForumsEnableMixin from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_common.models import Role from django_comment_common.utils import seed_permissions_roles, ThreadContext @@ -346,7 +346,13 @@ class ViewsTestCaseMixin(object): @patch('lms.lib.comment_client.utils.requests.request', autospec=True) @disable_signal(views, 'thread_created') @disable_signal(views, 'thread_edited') -class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSetupMixin, ViewsTestCaseMixin): +class ViewsQueryCountTestCase( + ForumsEnableMixin, + UrlResetMixin, + ModuleStoreTestCase, + MockRequestSetupMixin, + ViewsTestCaseMixin +): CREATE_USER = False ENABLED_CACHES = ['default', 'mongo_metadata_inheritance', 'loc_cache'] @@ -392,6 +398,7 @@ class ViewsQueryCountTestCase(UrlResetMixin, ModuleStoreTestCase, MockRequestSet @ddt.ddt @patch('lms.lib.comment_client.utils.requests.request', autospec=True) class ViewsTestCase( + ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase, MockRequestSetupMixin, @@ -1019,7 +1026,7 @@ class ViewsTestCase( @attr(shard=2) @patch("lms.lib.comment_client.utils.requests.request", autospec=True) @disable_signal(views, 'comment_endorsed') -class ViewPermissionsTestCase(UrlResetMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): +class ViewPermissionsTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): @classmethod def setUpClass(cls): @@ -1127,7 +1134,11 @@ class ViewPermissionsTestCase(UrlResetMixin, SharedModuleStoreTestCase, MockRequ @attr(shard=2) -class CreateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): +class CreateThreadUnicodeTestCase( + ForumsEnableMixin, + SharedModuleStoreTestCase, + UnicodeTestMixin, + MockRequestSetupMixin): @classmethod def setUpClass(cls): @@ -1143,6 +1154,9 @@ class CreateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, M cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(CreateThreadUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request,): """ @@ -1164,7 +1178,12 @@ class CreateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, M @attr(shard=2) @disable_signal(views, 'thread_edited') -class UpdateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): +class UpdateThreadUnicodeTestCase( + ForumsEnableMixin, + SharedModuleStoreTestCase, + UnicodeTestMixin, + MockRequestSetupMixin +): @classmethod def setUpClass(cls): @@ -1180,6 +1199,9 @@ class UpdateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, M cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(UpdateThreadUnicodeTestCase, self).setUp() + @patch('django_comment_client.utils.get_discussion_categories_ids', return_value=["test_commentable"]) @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request, mock_get_discussion_id_map): @@ -1202,7 +1224,12 @@ class UpdateThreadUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, M @attr(shard=2) @disable_signal(views, 'comment_created') -class CreateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): +class CreateCommentUnicodeTestCase( + ForumsEnableMixin, + SharedModuleStoreTestCase, + UnicodeTestMixin, + MockRequestSetupMixin +): @classmethod def setUpClass(cls): @@ -1218,6 +1245,9 @@ class CreateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(CreateCommentUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): commentable_id = "non_team_dummy_id" @@ -1245,7 +1275,12 @@ class CreateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, @attr(shard=2) @disable_signal(views, 'comment_edited') -class UpdateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): +class UpdateCommentUnicodeTestCase( + ForumsEnableMixin, + SharedModuleStoreTestCase, + UnicodeTestMixin, + MockRequestSetupMixin +): @classmethod def setUpClass(cls): @@ -1261,6 +1296,9 @@ class UpdateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(UpdateCommentUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): self._set_mock_request_data(mock_request, { @@ -1279,7 +1317,12 @@ class UpdateCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, @attr(shard=2) @disable_signal(views, 'comment_created') -class CreateSubCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixin, MockRequestSetupMixin): +class CreateSubCommentUnicodeTestCase( + ForumsEnableMixin, + SharedModuleStoreTestCase, + UnicodeTestMixin, + MockRequestSetupMixin +): """ Make sure comments under a response can handle unicode. """ @@ -1297,6 +1340,9 @@ class CreateSubCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixi cls.student = UserFactory.create() CourseEnrollmentFactory(user=cls.student, course_id=cls.course.id) + def setUp(self): + super(CreateSubCommentUnicodeTestCase, self).setUp() + @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def _test_unicode_data(self, text, mock_request): """ @@ -1332,7 +1378,7 @@ class CreateSubCommentUnicodeTestCase(SharedModuleStoreTestCase, UnicodeTestMixi @disable_signal(views, 'comment_created') @disable_signal(views, 'comment_voted') @disable_signal(views, 'comment_deleted') -class TeamsPermissionsTestCase(UrlResetMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): +class TeamsPermissionsTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): # Most of the test points use the same ddt data. # args: user, commentable_id, status_code ddt_permissions_args = [ @@ -1599,7 +1645,7 @@ TEAM_COMMENTABLE_ID = 'test-team-discussion' @attr(shard=2) @disable_signal(views, 'comment_created') @ddt.ddt -class ForumEventTestCase(SharedModuleStoreTestCase, MockRequestSetupMixin): +class ForumEventTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): """ Forum actions are expected to launch analytics events. Test these here. """ @@ -1620,6 +1666,9 @@ class ForumEventTestCase(SharedModuleStoreTestCase, MockRequestSetupMixin): cls.student.roles.add(Role.objects.get(name="Student", course_id=cls.course.id)) CourseAccessRoleFactory(course_id=cls.course.id, user=cls.student, role='Wizard') + def setUp(self): + super(ForumEventTestCase, self).setUp() + @patch('eventtracking.tracker.emit') @patch('lms.lib.comment_client.utils.requests.request', autospec=True) def test_thread_event(self, __, mock_emit): @@ -1783,7 +1832,7 @@ class ForumEventTestCase(SharedModuleStoreTestCase, MockRequestSetupMixin): @attr(shard=2) -class UsersEndpointTestCase(SharedModuleStoreTestCase, MockRequestSetupMixin): +class UsersEndpointTestCase(ForumsEnableMixin, SharedModuleStoreTestCase, MockRequestSetupMixin): @classmethod def setUpClass(cls): @@ -1802,6 +1851,9 @@ class UsersEndpointTestCase(SharedModuleStoreTestCase, MockRequestSetupMixin): cls.other_user = UserFactory.create(username="other") CourseEnrollmentFactory(user=cls.other_user, course_id=cls.course.id) + def setUp(self): + super(UsersEndpointTestCase, self).setUp() + def set_post_counts(self, mock_request, threads_count=1, comments_count=1): """ sets up a mock response from the comments service for getting post counts for our other_user diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 690b0972f1..290daafb22 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -3,6 +3,7 @@ import datetime import json import ddt import mock +from mock import patch, Mock from nose.plugins.attrib import attr from pytz import UTC from django.utils.timezone import UTC as django_utc @@ -14,6 +15,8 @@ from edxmako import add_lookup from django_comment_client.tests.factories import RoleFactory from django_comment_client.tests.unicode import UnicodeTestMixin import django_comment_client.utils as utils +from lms.lib.comment_client.utils import perform_request, CommentClientMaintenanceError +from django_comment_common.models import ForumsConfig from courseware.tests.factories import InstructorFactory from courseware.tabs import get_course_tab_list @@ -1417,3 +1420,32 @@ class PermissionsTestCase(ModuleStoreTestCase): # content has no known author del content['user_id'] self.assertFalse(utils.is_content_authored_by(content, user)) + + +class ClientConfigurationTestCase(TestCase): + """Simple test cases to ensure enabling/disabling the use of the comment service works as intended.""" + + def test_disabled(self): + """Ensures that an exception is raised when forums are disabled.""" + config = ForumsConfig.current() + config.enabled = False + config.save() + + with self.assertRaises(CommentClientMaintenanceError): + perform_request('GET', 'http://www.google.com') + + @patch('requests.request') + def test_enabled(self, mock_request): + """Ensures that requests proceed normally when forums are enabled.""" + config = ForumsConfig.current() + config.enabled = True + config.save() + + response = Mock() + response.status_code = 200 + response.json = lambda: {} + + mock_request.return_value = response + + result = perform_request('GET', 'http://www.google.com') + self.assertEqual(result, {}) diff --git a/lms/djangoapps/django_comment_client/tests/utils.py b/lms/djangoapps/django_comment_client/tests/utils.py index eda735dd14..301189b5a0 100644 --- a/lms/djangoapps/django_comment_client/tests/utils.py +++ b/lms/djangoapps/django_comment_client/tests/utils.py @@ -4,7 +4,7 @@ Utilities for tests within the django_comment_client module. from mock import patch from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory -from django_comment_common.models import Role +from django_comment_common.models import Role, ForumsConfig from django_comment_common.utils import seed_permissions_roles from student.tests.factories import CourseEnrollmentFactory, UserFactory from util.testing import UrlResetMixin @@ -12,7 +12,19 @@ from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase -class CohortedTestCase(UrlResetMixin, SharedModuleStoreTestCase): +class ForumsEnableMixin(object): + """ + Ensures that the forums are enabled for a given test class. + """ + def setUp(self): + super(ForumsEnableMixin, self).setUp() + + config = ForumsConfig.current() + config.enabled = True + config.save() + + +class CohortedTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCase): """ Sets up a course with a student, a moderator and their cohorts. """ diff --git a/lms/lib/comment_client/utils.py b/lms/lib/comment_client/utils.py index 25eaaf3ef6..0531574b40 100644 --- a/lms/lib/comment_client/utils.py +++ b/lms/lib/comment_client/utils.py @@ -55,6 +55,10 @@ def perform_request(method, url, data_or_params=None, raw=False, metric_action=None, metric_tags=None, paged_results=False): # To avoid dependency conflict from django_comment_common.models import ForumsConfig + config = ForumsConfig.current() + + if not config.enabled: + raise CommentClientMaintenanceError('service disabled') if metric_tags is None: metric_tags = [] @@ -66,7 +70,7 @@ def perform_request(method, url, data_or_params=None, raw=False, if data_or_params is None: data_or_params = {} headers = { - 'X-Edx-Api-Key': getattr(settings, "COMMENTS_SERVICE_KEY", None), + 'X-Edx-Api-Key': config.api_key, 'Accept-Language': get_language(), } request_id = uuid4() @@ -79,7 +83,6 @@ def perform_request(method, url, data_or_params=None, raw=False, data = None params = merge_dict(data_or_params, request_id_dict) with request_timer(request_id, method, url, metric_tags): - config = ForumsConfig.current() response = requests.request( method, url, @@ -112,7 +115,7 @@ def perform_request(method, url, data_or_params=None, raw=False, data = response.json() except ValueError: raise CommentClientError( - u"Comments service returned invalid JSON for request {request_id}; first 100 characters: '{content}'".format( + u"Invalid JSON response for request {request_id}; first 100 characters: '{content}'".format( request_id=request_id, content=response.text[:100] ) diff --git a/lms/urls.py b/lms/urls.py index 3b10c680e6..fcd6b3282f 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -15,6 +15,7 @@ from openedx.core.djangoapps.auth_exchange.views import LoginWithAccessTokenView from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.djangoapps.programs.models import ProgramsApiConfig from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration +from django_comment_common.models import ForumsConfig from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers # Uncomment the next two lines to enable the admin: @@ -905,6 +906,7 @@ urlpatterns += ( url(r'config/self_paced', ConfigurationModelCurrentAPIView.as_view(model=SelfPacedConfiguration)), url(r'config/programs', ConfigurationModelCurrentAPIView.as_view(model=ProgramsApiConfig)), url(r'config/catalog', ConfigurationModelCurrentAPIView.as_view(model=CatalogIntegration)), + url(r'config/forums', ConfigurationModelCurrentAPIView.as_view(model=ForumsConfig)), ) urlpatterns = patterns(*urlpatterns)