diff --git a/common/djangoapps/django_comment_common/tests.py b/common/djangoapps/django_comment_common/tests.py index d5db1189f0..482e23dce2 100644 --- a/common/djangoapps/django_comment_common/tests.py +++ b/common/djangoapps/django_comment_common/tests.py @@ -120,7 +120,7 @@ class CourseDiscussionSettingsTest(ModuleStoreTestCase): def test_invalid_data_types(self): exception_msg_template = "Incorrect field type for `{}`. Type must be `{}`" fields = [ - {'name': 'division_scheme', 'type': str}, + {'name': 'division_scheme', 'type': basestring}, {'name': 'always_divide_inline_discussions', 'type': bool}, {'name': 'divided_discussions', 'type': list} ] diff --git a/common/djangoapps/django_comment_common/utils.py b/common/djangoapps/django_comment_common/utils.py index 5886131e0d..7c6699da88 100644 --- a/common/djangoapps/django_comment_common/utils.py +++ b/common/djangoapps/django_comment_common/utils.py @@ -130,12 +130,13 @@ def set_course_discussion_settings(course_key, **kwargs): Returns: A CourseDiscussionSettings object. """ - fields = {'division_scheme': str, 'always_divide_inline_discussions': bool, 'divided_discussions': list} + fields = {'division_scheme': basestring, 'always_divide_inline_discussions': bool, 'divided_discussions': list} course_discussion_settings = get_course_discussion_settings(course_key) for field, field_type in fields.items(): if field in kwargs: if not isinstance(kwargs[field], field_type): raise ValueError("Incorrect field type for `{}`. Type must be `{}`".format(field, field_type.__name__)) setattr(course_discussion_settings, field, kwargs[field]) - course_discussion_settings.save() + + course_discussion_settings.save() return course_discussion_settings diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index c5c3f4092e..1f727c321d 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/common/test/acceptance/pages/lms/instructor_dashboard.py @@ -807,19 +807,19 @@ class DiscussionManagementSection(PageObject): Returns the ID of the selected discussion division scheme ("NOT_DIVIDED_SCHEME", "COHORT_SCHEME", or "ENROLLMENT_TRACK_SCHEME)". """ - return self.q(css=self._bounded_selector('.division-scheme:checked')).first.attrs('id')[0] + return self.q(css=self._bounded_selector('.division-scheme:checked')).first.attrs('value')[0] def select_division_scheme(self, scheme): """ Selects the radio button associated with the specified division scheme. """ - self.q(css=self._bounded_selector("input#%s" % scheme)).first.click() + self.q(css=self._bounded_selector("input.%s" % scheme)).first.click() def division_scheme_visible(self, scheme): """ Returns whether or not the specified scheme is visible as an option. """ - return self.q(css=self._bounded_selector("input#%s" % scheme)).visible + return self.q(css=self._bounded_selector("input.%s" % scheme)).visible class MembershipPageAutoEnrollSection(PageObject): diff --git a/common/test/acceptance/tests/discussion/test_discussion_management.py b/common/test/acceptance/tests/discussion/test_discussion_management.py index 8ae8552453..6d53fcd7d7 100644 --- a/common/test/acceptance/tests/discussion/test_discussion_management.py +++ b/common/test/acceptance/tests/discussion/test_discussion_management.py @@ -99,7 +99,7 @@ class BaseDividedDiscussionTest(UniqueCourseTest, CohortTestMixin): Verify that the save confirmation message for the specified portion of the page is visible. """ confirmation_message = self.discussion_management_page.get_divide_discussions_message(key=key) - self.assertEqual("Your changes have been saved.", confirmation_message) + self.assertIn("Your changes have been saved.", confirmation_message) @attr(shard=6) diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 800919ceb3..da3579bc7c 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -1,6 +1,7 @@ import json import logging +from datetime import datetime import ddt from django.core.urlresolvers import reverse from django.http import Http404 @@ -9,20 +10,27 @@ from django.test.utils import override_settings from django.utils import translation from lms.lib.comment_client.utils import CommentClientPaginatedResult +from course_modes.models import CourseMode +from course_modes.tests.factories import CourseModeFactory from django_comment_common.utils import ThreadContext -from django_comment_common.models import ForumsConfig +from django_comment_common.models import ForumsConfig, CourseDiscussionSettings from django_comment_client.permissions import get_team +from django_comment_client.tests.utils import config_course_discussions from django_comment_client.tests.group_id import ( GroupIdAssertionMixin, CohortedTopicGroupIdTestMixin, NonCohortedTopicGroupIdTestMixin, ) +from django_comment_client.constants import TYPE_ENTRY, TYPE_SUBCATEGORY from django_comment_client.tests.unicode import UnicodeTestMixin -from django_comment_client.tests.utils import CohortedTestCase, ForumsEnableMixin +from django_comment_client.tests.utils import CohortedTestCase, ForumsEnableMixin, topic_name_to_id from django_comment_client.utils import strip_none from lms.djangoapps.discussion import views +from lms.djangoapps.discussion.views import course_discussions_settings_handler from student.tests.factories import UserFactory, CourseEnrollmentFactory from util.testing import UrlResetMixin +from openedx.core.djangoapps.course_groups.tests.helpers import config_course_cohorts +from openedx.core.djangoapps.course_groups.tests.test_views import CohortViewsTestCase from openedx.core.djangoapps.util.testing import ContentGroupTestCase from openedx.features.enterprise_support.tests.mixins.enterprise import EnterpriseTestConsentRequired from xmodule.modulestore import ModuleStoreEnum @@ -1626,3 +1634,252 @@ class EnterpriseConsentTestCase(EnterpriseTestConsentRequired, ForumsEnableMixin kwargs=dict(course_id=course_id, discussion_id=self.discussion_id, thread_id=thread_id)), ): self.verify_consent_required(self.client, url) + + +class DividedDiscussionsTestCase(CohortViewsTestCase): + + def create_divided_discussions(self): + """ + Set up a divided discussion in the system, complete with all the fixings + """ + divided_inline_discussions = ['Topic A'] + divided_course_wide_discussions = ["Topic B"] + divided_discussions = divided_inline_discussions + divided_course_wide_discussions + + # inline discussion + ItemFactory.create( + parent_location=self.course.location, + category="discussion", + discussion_id=topic_name_to_id(self.course, "Topic A"), + discussion_category="Chapter", + discussion_target="Discussion", + start=datetime.now() + ) + # course-wide discussion + discussion_topics = { + "Topic B": {"id": "Topic B"}, + } + + config_course_cohorts( + self.course, + is_cohorted=True, + ) + + config_course_discussions( + self.course, + discussion_topics=discussion_topics, + divided_discussions=divided_discussions + ) + return divided_inline_discussions, divided_course_wide_discussions + + +class CourseDiscussionTopicsTestCase(DividedDiscussionsTestCase): + """ + Tests the `divide_discussion_topics` view. + """ + + def test_non_staff(self): + """ + Verify that we cannot access divide_discussion_topics if we're a non-staff user. + """ + self._verify_non_staff_cannot_access(views.discussion_topics, "GET", [unicode(self.course.id)]) + + def test_get_discussion_topics(self): + """ + Verify that discussion_topics is working for HTTP GET. + """ + # create inline & course-wide discussion to verify the different map. + self.create_divided_discussions() + + response = self.get_handler(self.course, handler=views.discussion_topics) + start_date = response['inline_discussions']['subcategories']['Chapter']['start_date'] + expected_response = { + "course_wide_discussions": { + 'children': [['Topic B', TYPE_ENTRY]], + 'entries': { + 'Topic B': { + 'sort_key': 'A', + 'is_divided': True, + 'id': topic_name_to_id(self.course, "Topic B"), + 'start_date': response['course_wide_discussions']['entries']['Topic B']['start_date'] + } + } + }, + "inline_discussions": { + 'subcategories': { + 'Chapter': { + 'subcategories': {}, + 'children': [['Discussion', TYPE_ENTRY]], + 'entries': { + 'Discussion': { + 'sort_key': None, + 'is_divided': True, + 'id': topic_name_to_id(self.course, "Topic A"), + 'start_date': start_date + } + }, + 'sort_key': 'Chapter', + 'start_date': start_date + } + }, + 'children': [['Chapter', TYPE_SUBCATEGORY]] + } + } + self.assertEqual(response, expected_response) + + +class CourseDiscussionsHandlerTestCase(DividedDiscussionsTestCase): + """ + Tests the course_discussion_settings_handler + """ + def get_expected_response(self): + """ + Returns the static response dict. + """ + return { + u'always_divide_inline_discussions': False, + u'divided_inline_discussions': [], + u'divided_course_wide_discussions': [], + u'id': 1, + u'division_scheme': u'cohort', + u'available_division_schemes': [u'cohort'] + } + + def test_non_staff(self): + """ + Verify that we cannot access course_discussions_settings_handler if we're a non-staff user. + """ + self._verify_non_staff_cannot_access( + course_discussions_settings_handler, "GET", [unicode(self.course.id)] + ) + self._verify_non_staff_cannot_access( + course_discussions_settings_handler, "PATCH", [unicode(self.course.id)] + ) + + def test_update_always_divide_inline_discussion_settings(self): + """ + Verify that course_discussions_settings_handler is working for always_divide_inline_discussions via HTTP PATCH. + """ + config_course_cohorts(self.course, is_cohorted=True) + + response = self.get_handler(self.course, handler=course_discussions_settings_handler) + + expected_response = self.get_expected_response() + + self.assertEqual(response, expected_response) + + expected_response['always_divide_inline_discussions'] = True + response = self.patch_handler( + self.course, data=expected_response, handler=course_discussions_settings_handler + ) + + self.assertEqual(response, expected_response) + + def test_update_course_wide_discussion_settings(self): + """ + Verify that course_discussions_settings_handler is working for divided_course_wide_discussions via HTTP PATCH. + """ + # course-wide discussion + discussion_topics = { + "Topic B": {"id": "Topic B"}, + } + + config_course_cohorts(self.course, is_cohorted=True) + config_course_discussions(self.course, discussion_topics=discussion_topics) + + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + + expected_response = self.get_expected_response() + self.assertEqual(response, expected_response) + + expected_response['divided_course_wide_discussions'] = [topic_name_to_id(self.course, "Topic B")] + response = self.patch_handler( + self.course, data=expected_response, handler=views.course_discussions_settings_handler + ) + + self.assertEqual(response, expected_response) + + def test_update_inline_discussion_settings(self): + """ + Verify that course_discussions_settings_handler is working for divided_inline_discussions via HTTP PATCH. + """ + config_course_cohorts(self.course, is_cohorted=True) + + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + + expected_response = self.get_expected_response() + self.assertEqual(response, expected_response) + + now = datetime.now() + # inline discussion + ItemFactory.create( + parent_location=self.course.location, + category="discussion", + discussion_id="Topic_A", + discussion_category="Chapter", + discussion_target="Discussion", + start=now + ) + + expected_response['divided_inline_discussions'] = ["Topic_A"] + response = self.patch_handler( + self.course, data=expected_response, handler=views.course_discussions_settings_handler + ) + + self.assertEqual(response, expected_response) + + def test_get_settings(self): + """ + Verify that course_discussions_settings_handler is working for HTTP GET. + """ + divided_inline_discussions, divided_course_wide_discussions = self.create_divided_discussions() + + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + expected_response = self.get_expected_response() + + expected_response['divided_inline_discussions'] = [topic_name_to_id(self.course, name) + for name in divided_inline_discussions] + expected_response['divided_course_wide_discussions'] = [topic_name_to_id(self.course, name) + for name in divided_course_wide_discussions] + + self.assertEqual(response, expected_response) + + def test_update_settings_with_invalid_field_data_type(self): + """ + Verify that course_discussions_settings_handler return HTTP 400 if field data type is incorrect. + """ + config_course_cohorts(self.course, is_cohorted=True) + + response = self.patch_handler( + self.course, + data={'always_divide_inline_discussions': ''}, + expected_response_code=400, + handler=views.course_discussions_settings_handler + ) + self.assertEqual( + "Incorrect field type for `{}`. Type must be `{}`".format('always_divide_inline_discussions', bool.__name__), + response.get("error") + ) + + def test_available_schemes(self): + # Cohorts disabled, single enrollment mode. + config_course_cohorts(self.course, is_cohorted=False) + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + expected_response = self.get_expected_response() + expected_response['available_division_schemes'] = [] + self.assertEqual(response, expected_response) + + # Add 2 enrollment modes + CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.AUDIT) + CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.VERIFIED) + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + expected_response['available_division_schemes'] = [CourseDiscussionSettings.ENROLLMENT_TRACK] + self.assertEqual(response, expected_response) + + # Enable cohorts + config_course_cohorts(self.course, is_cohorted=True) + response = self.get_handler(self.course, handler=views.course_discussions_settings_handler) + expected_response['available_division_schemes'] = [ + CourseDiscussionSettings.COHORT, CourseDiscussionSettings.ENROLLMENT_TRACK + ] + self.assertEqual(response, expected_response) diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index 12ac563375..8ea4280657 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -24,6 +24,9 @@ try: except ImportError: newrelic = None # pylint: disable=invalid-name +from django.views.decorators.csrf import ensure_csrf_cookie +from django.views.decorators.http import require_http_methods + from rest_framework import status from web_fragments.fragment import Fragment @@ -36,10 +39,12 @@ from courseware.access import has_access from student.models import CourseEnrollment from xmodule.modulestore.django import modulestore -from django_comment_common.utils import ThreadContext, get_course_discussion_settings +from django_comment_common.utils import ThreadContext, get_course_discussion_settings, set_course_discussion_settings +from django_comment_client.constants import TYPE_ENTRY from django_comment_client.permissions import has_permission, get_team from django_comment_client.utils import ( + available_division_schemes, merge_dict, extract, strip_none, @@ -57,6 +62,8 @@ import lms.lib.comment_client as cc from opaque_keys.edx.keys import CourseKey from contextlib import contextmanager +from util.json_request import expect_json, JsonResponse + THREADS_PER_PAGE = 20 INLINE_THREADS_PER_PAGE = 20 @@ -684,3 +691,167 @@ class DiscussionBoardFragmentView(EdxFragmentView): return self.get_css_dependencies('style-discussion-main-rtl') else: return self.get_css_dependencies('style-discussion-main') + + +@expect_json +@login_required +def discussion_topics(request, course_key_string): + """ + The handler for divided discussion categories requests. + This will raise 404 if user is not staff. + + Returns the JSON representation of discussion topics w.r.t categories for the course. + + Example: + >>> example = { + >>> "course_wide_discussions": { + >>> "entries": { + >>> "General": { + >>> "sort_key": "General", + >>> "is_divided": True, + >>> "id": "i4x-edx-eiorguegnru-course-foobarbaz" + >>> } + >>> } + >>> "children": ["General", "entry"] + >>> }, + >>> "inline_discussions" : { + >>> "subcategories": { + >>> "Getting Started": { + >>> "subcategories": {}, + >>> "children": [ + >>> ["Working with Videos", "entry"], + >>> ["Videos on edX", "entry"] + >>> ], + >>> "entries": { + >>> "Working with Videos": { + >>> "sort_key": None, + >>> "is_divided": False, + >>> "id": "d9f970a42067413cbb633f81cfb12604" + >>> }, + >>> "Videos on edX": { + >>> "sort_key": None, + >>> "is_divided": False, + >>> "id": "98d8feb5971041a085512ae22b398613" + >>> } + >>> } + >>> }, + >>> "children": ["Getting Started", "subcategory"] + >>> }, + >>> } + >>> } + """ + course_key = CourseKey.from_string(course_key_string) + course = get_course_with_access(request.user, 'staff', course_key) + + discussion_topics = {} + discussion_category_map = utils.get_discussion_category_map( + course, request.user, divided_only_if_explicit=True, exclude_unstarted=False + ) + + # We extract the data for the course wide discussions from the category map. + course_wide_entries = discussion_category_map.pop('entries') + + course_wide_children = [] + inline_children = [] + + for name, c_type in discussion_category_map['children']: + if name in course_wide_entries and c_type == TYPE_ENTRY: + course_wide_children.append([name, c_type]) + else: + inline_children.append([name, c_type]) + + discussion_topics['course_wide_discussions'] = { + 'entries': course_wide_entries, + 'children': course_wide_children + } + + discussion_category_map['children'] = inline_children + discussion_topics['inline_discussions'] = discussion_category_map + + return JsonResponse(discussion_topics) + + +@require_http_methods(("GET", "PATCH")) +@ensure_csrf_cookie +@expect_json +@login_required +def course_discussions_settings_handler(request, course_key_string): + """ + The restful handler for divided discussion setting requests. Requires JSON. + This will raise 404 if user is not staff. + GET + Returns the JSON representation of divided discussion settings for the course. + PATCH + Updates the divided discussion settings for the course. Returns the JSON representation of updated settings. + """ + course_key = CourseKey.from_string(course_key_string) + course = get_course_with_access(request.user, 'staff', course_key) + discussion_settings = get_course_discussion_settings(course_key) + + if request.method == 'PATCH': + divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions( + course, discussion_settings + ) + + settings_to_change = {} + + if 'divided_course_wide_discussions' in request.json or 'divided_inline_discussions' in request.json: + divided_course_wide_discussions = request.json.get( + 'divided_course_wide_discussions', divided_course_wide_discussions + ) + divided_inline_discussions = request.json.get( + 'divided_inline_discussions', divided_inline_discussions + ) + settings_to_change['divided_discussions'] = divided_course_wide_discussions + divided_inline_discussions + + if 'always_divide_inline_discussions' in request.json: + settings_to_change['always_divide_inline_discussions'] = request.json.get( + 'always_divide_inline_discussions' + ) + if 'division_scheme' in request.json: + settings_to_change['division_scheme'] = request.json.get( + 'division_scheme' + ) + + if not settings_to_change: + return JsonResponse({"error": unicode("Bad Request")}, 400) + + try: + if settings_to_change: + discussion_settings = set_course_discussion_settings(course_key, **settings_to_change) + + except ValueError as err: + # Note: error message not translated because it is not exposed to the user (UI prevents this state). + return JsonResponse({"error": unicode(err)}, 400) + + divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions( + course, discussion_settings + ) + + return JsonResponse({ + 'id': discussion_settings.id, + 'divided_inline_discussions': divided_inline_discussions, + 'divided_course_wide_discussions': divided_course_wide_discussions, + 'always_divide_inline_discussions': discussion_settings.always_divide_inline_discussions, + 'division_scheme': discussion_settings.division_scheme, + 'available_division_schemes': available_division_schemes(course_key) + }) + + +def get_divided_discussions(course, discussion_settings): + """ + Returns the course-wide and inline divided discussion ids separately. + """ + divided_course_wide_discussions = [] + divided_inline_discussions = [] + + course_wide_discussions = [topic['id'] for __, topic in course.discussion_topics.items()] + all_discussions = utils.get_discussion_categories_ids(course, None, include_all=True) + + for divided_discussion_id in discussion_settings.divided_discussions: + if divided_discussion_id in course_wide_discussions: + divided_course_wide_discussions.append(divided_discussion_id) + elif divided_discussion_id in all_discussions: + divided_inline_discussions.append(divided_discussion_id) + + return divided_course_wide_discussions, divided_inline_discussions diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 26ce771cba..33855765c5 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -14,6 +14,7 @@ from edxmako import add_lookup from django_comment_client.tests.factories import RoleFactory from django_comment_client.tests.unicode import UnicodeTestMixin +from django_comment_client.tests.utils import topic_name_to_id, config_course_discussions from django_comment_client.constants import TYPE_ENTRY, TYPE_SUBCATEGORY import django_comment_client.utils as utils from lms.lib.comment_client.utils import perform_request, CommentClientMaintenanceError @@ -26,7 +27,7 @@ from courseware.tests.factories import InstructorFactory from courseware.tabs import get_course_tab_list from openedx.core.djangoapps.course_groups import cohorts from openedx.core.djangoapps.course_groups.cohorts import set_course_cohorted -from openedx.core.djangoapps.course_groups.tests.helpers import config_course_cohorts, config_course_discussions, topic_name_to_id, CohortFactory +from openedx.core.djangoapps.course_groups.tests.helpers import config_course_cohorts, CohortFactory from student.tests.factories import UserFactory, AdminFactory, CourseEnrollmentFactory from openedx.core.djangoapps.content.course_structures.models import CourseStructure from openedx.core.djangoapps.util.testing import ContentGroupTestCase @@ -1489,7 +1490,7 @@ class GroupIdForUserTestCase(ModuleStoreTestCase): @attr(shard=1) class CourseDiscussionDivisionEnabledTestCase(ModuleStoreTestCase): - """ Test the course_discussion_division_enabled method. """ + """ Test the course_discussion_division_enabled and available_division_schemes methods. """ def setUp(self): super(CourseDiscussionDivisionEnabledTestCase, self).setUp() @@ -1504,6 +1505,7 @@ class CourseDiscussionDivisionEnabledTestCase(ModuleStoreTestCase): def test_discussion_division_disabled(self): course_discussion_settings = get_course_discussion_settings(self.course.id) self.assertFalse(utils.course_discussion_division_enabled(course_discussion_settings)) + self.assertEqual([], utils.available_division_schemes(self.course.id)) def test_discussion_division_by_cohort(self): set_discussion_division_settings( @@ -1511,11 +1513,13 @@ class CourseDiscussionDivisionEnabledTestCase(ModuleStoreTestCase): ) # Because cohorts are disabled, discussion division is not enabled. self.assertFalse(utils.course_discussion_division_enabled(get_course_discussion_settings(self.course.id))) + self.assertEqual([], utils.available_division_schemes(self.course.id)) # Now enable cohorts, which will cause discussions to be divided. set_discussion_division_settings( self.course.id, enable_cohorts=True, division_scheme=CourseDiscussionSettings.COHORT ) self.assertTrue(utils.course_discussion_division_enabled(get_course_discussion_settings(self.course.id))) + self.assertEqual([CourseDiscussionSettings.COHORT], utils.available_division_schemes(self.course.id)) def test_discussion_division_by_enrollment_track(self): set_discussion_division_settings( @@ -1523,10 +1527,12 @@ class CourseDiscussionDivisionEnabledTestCase(ModuleStoreTestCase): ) # Only a single enrollment track exists, so discussion division is not enabled. self.assertFalse(utils.course_discussion_division_enabled(get_course_discussion_settings(self.course.id))) + self.assertEqual([], utils.available_division_schemes(self.course.id)) # Now create a second CourseMode, which will cause discussions to be divided. CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.VERIFIED) self.assertTrue(utils.course_discussion_division_enabled(get_course_discussion_settings(self.course.id))) + self.assertEqual([CourseDiscussionSettings.ENROLLMENT_TRACK], utils.available_division_schemes(self.course.id)) @attr(shard=1) diff --git a/lms/djangoapps/django_comment_client/tests/utils.py b/lms/djangoapps/django_comment_client/tests/utils.py index 301189b5a0..5406ff828e 100644 --- a/lms/djangoapps/django_comment_client/tests/utils.py +++ b/lms/djangoapps/django_comment_client/tests/utils.py @@ -5,9 +5,11 @@ from mock import patch from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from django_comment_common.models import Role, ForumsConfig -from django_comment_common.utils import seed_permissions_roles +from django_comment_common.utils import seed_permissions_roles, set_course_discussion_settings, CourseDiscussionSettings from student.tests.factories import CourseEnrollmentFactory, UserFactory from util.testing import UrlResetMixin +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -63,3 +65,58 @@ class CohortedTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleStoreTestCa course_id=self.course.id, users=[self.moderator] ) + + +# pylint: disable=dangerous-default-value +def config_course_discussions( + course, + discussion_topics={}, + divided_discussions=[], + always_divide_inline_discussions=False +): + """ + Set discussions and configure divided discussions for a course. + + Arguments: + course: CourseDescriptor + discussion_topics (Dict): Discussion topic names. Picks ids and + sort_keys automatically. + divided_discussions: Discussion topics to divide. Converts the + list to use the same ids as discussion topic names. + always_divide_inline_discussions (bool): Whether inline discussions + should be divided by default. + + Returns: + Nothing -- modifies course in place. + """ + + def to_id(name): + """Convert name to id.""" + return topic_name_to_id(course, name) + + set_course_discussion_settings( + course.id, + divided_discussions=[to_id(name) for name in divided_discussions], + always_divide_inline_discussions=always_divide_inline_discussions, + division_scheme=CourseDiscussionSettings.COHORT, + ) + + course.discussion_topics = dict((name, {"sort_key": "A", "id": to_id(name)}) + for name in discussion_topics) + try: + # Not implemented for XMLModulestore, which is used by test_cohorts. + modulestore().update_item(course, ModuleStoreEnum.UserID.test) + except NotImplementedError: + pass + + +def topic_name_to_id(course, name): + """ + Given a discussion topic name, return an id for that name (includes + course and url_name). + """ + return "{course}_{run}_{name}".format( + course=course.location.course, + run=course.url_name, + name=name + ) diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index f513ac7d27..01ec2710d0 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -860,6 +860,24 @@ def course_discussion_division_enabled(course_discussion_settings): return _get_course_division_scheme(course_discussion_settings) != CourseDiscussionSettings.NONE +def available_division_schemes(course_key): + """ + Returns a list of possible discussion division schemes for this course. + This takes into account if cohorts are enabled and if there are multiple + enrollment tracks. If no schemes are available, returns an empty list. + Args: + course_key: CourseKey + + Returns: list of possible division schemes (for example, CourseDiscussionSettings.COHORT) + """ + available_schemes = [] + if is_course_cohorted(course_key): + available_schemes.append(CourseDiscussionSettings.COHORT) + if len(_get_enrollment_track_groups(course_key)) > 1: + available_schemes.append(CourseDiscussionSettings.ENROLLMENT_TRACK) + return available_schemes + + def _get_course_division_scheme(course_discussion_settings): division_scheme = course_discussion_settings.division_scheme if ( diff --git a/lms/static/js/discussions_management/models/course_discussions_settings.js b/lms/static/js/discussions_management/models/course_discussions_settings.js index 6eb50f0d5f..cd0b64b2fe 100644 --- a/lms/static/js/discussions_management/models/course_discussions_settings.js +++ b/lms/static/js/discussions_management/models/course_discussions_settings.js @@ -6,7 +6,8 @@ defaults: { divided_inline_discussions: [], divided_course_wide_discussions: [], - always_divide_inline_discussions: false + always_divide_inline_discussions: false, + division_scheme: 'none' } }); return CourseDiscussionsSettingsModel; diff --git a/lms/static/js/discussions_management/views/discussions.js b/lms/static/js/discussions_management/views/discussions.js index a784ee74ac..9576d3ebd6 100644 --- a/lms/static/js/discussions_management/views/discussions.js +++ b/lms/static/js/discussions_management/views/discussions.js @@ -3,11 +3,24 @@ define(['jquery', 'underscore', 'backbone', 'gettext', 'js/discussions_management/views/divided_discussions_inline', 'js/discussions_management/views/divided_discussions_course_wide', - 'edx-ui-toolkit/js/utils/html-utils' + 'edx-ui-toolkit/js/utils/html-utils', + 'edx-ui-toolkit/js/utils/string-utils', + 'js/models/notification', + 'js/views/notification' ], - function($, _, Backbone, gettext, InlineDiscussionsView, CourseWideDiscussionsView, HtmlUtils) { + function($, _, Backbone, gettext, InlineDiscussionsView, CourseWideDiscussionsView, HtmlUtils, StringUtils) { + /* global NotificationModel, NotificationView */ + + var hiddenClass = 'is-hidden'; + var cohort = 'cohort'; + var none = 'none'; + var enrollmentTrack = 'enrollment_track'; + var DiscussionsView = Backbone.View.extend({ + events: { + 'click .division-scheme': 'divisionSchemeChanged' + }, initialize: function(options) { this.template = HtmlUtils.template($('#discussions-tpl').text()); @@ -16,16 +29,145 @@ }, render: function() { - HtmlUtils.setHtml(this.$el, this.template({})); - this.showDiscussionTopics(); + var numberAvailableSchemes = this.discussionSettings.attributes.available_division_schemes.length; + HtmlUtils.setHtml(this.$el, this.template({ + availableSchemes: this.getDivisionSchemeData(this.discussionSettings.attributes.division_scheme), // eslint-disable-line max-len + layoutClass: numberAvailableSchemes === 1 ? 'two-column-layout' : 'three-column-layout' + })); + this.updateTopicVisibility(this.getSelectedScheme(), this.getTopicNav()); + this.renderTopics(); return this; }, + getDivisionSchemeData: function(selectedScheme) { + return [ + { + key: none, + displayName: gettext('Not divided'), + descriptiveText: gettext('Discussions are unified; all learners interact with posts from other learners, regardless of the group they are in.'), // eslint-disable-line max-len + selected: selectedScheme === none, + enabled: true // always leave none enabled + }, + { + key: enrollmentTrack, + displayName: gettext('Enrollment Tracks'), + descriptiveText: gettext('Use enrollment tracks as the basis for dividing discussions. All learners, regardless of their enrollment track, see the same discussion topics, but within divided topics, only learners who are in the same enrollment track see and respond to each others’ posts.'), // eslint-disable-line max-len + selected: selectedScheme === enrollmentTrack, + enabled: this.isSchemeAvailable(enrollmentTrack) || selectedScheme === enrollmentTrack + }, + { + key: cohort, + displayName: gettext('Cohorts'), + descriptiveText: gettext('Use cohorts as the basis for dividing discussions. All learners, regardless of cohort, see the same discussion topics, but within divided topics, only members of the same cohort see and respond to each others’ posts. '), // eslint-disable-line max-len + selected: selectedScheme === cohort, + enabled: this.isSchemeAvailable(cohort) || selectedScheme === cohort + } + + ]; + }, + + isSchemeAvailable: function(scheme) { + return this.discussionSettings.attributes.available_division_schemes.indexOf(scheme) !== -1; + }, + + showMessage: function(message, type) { + var model = new NotificationModel({type: type || 'confirmation', title: message}); + this.removeNotification(); + this.notification = new NotificationView({ + model: model + }); + this.$('.division-scheme-container').prepend(this.notification.$el); + this.notification.render(); + }, + + removeNotification: function() { + if (this.notification) { + this.notification.remove(); + } + }, + + getSelectedScheme: function() { + return this.$('input[name="division-scheme"]:checked').val(); + }, + + getTopicNav: function() { + return this.$('.topic-division-nav'); + }, + + divisionSchemeChanged: function() { + var selectedScheme = this.getSelectedScheme(), + topicNav = this.getTopicNav(), + fieldData = { + division_scheme: selectedScheme + }; + + this.updateTopicVisibility(selectedScheme, topicNav); + this.saveDivisionScheme(topicNav, fieldData, selectedScheme); + }, + + saveDivisionScheme: function($element, fieldData, selectedScheme) { + var self = this, + discussionSettingsModel = this.discussionSettings, + showErrorMessage, + details = ''; + + this.removeNotification(); + showErrorMessage = function(message) { + self.showMessage(message, 'error'); + }; + + discussionSettingsModel.save( + fieldData, {patch: true, wait: true} + ).done(function() { + switch (selectedScheme) { + case none: + details = gettext('Discussion topics in the course are not divided.'); + break; + case enrollmentTrack: + details = gettext('Any divided discussion topics are divided based on enrollment track.'); // eslint-disable-line max-len + break; + case cohort: + details = gettext('Any divided discussion topics are divided based on cohort.'); + break; + default: + break; + } + self.showMessage( + StringUtils.interpolate( + gettext('Your changes have been saved. {details}'), + {details: details}, + true + ) + ); + }).fail(function(result) { + var errorMessage = null, + jsonResponse; + try { + jsonResponse = JSON.parse(result.responseText); + errorMessage = jsonResponse.error; + } catch (e) { + // Ignore the exception and show the default error message instead. + } + if (!errorMessage) { + errorMessage = gettext('We have encountered an error. Refresh your browser and then try again.'); // eslint-disable-line max-len + } + showErrorMessage(errorMessage); + }); + }, + + updateTopicVisibility: function(selectedScheme, topicNav) { + if (selectedScheme === none) { + topicNav.addClass(hiddenClass); + } else { + topicNav.removeClass(hiddenClass); + } + }, + getSectionCss: function(section) { return ".instructor-nav .nav-item [data-section='" + section + "']"; }, - showDiscussionTopics: function() { + renderTopics: function() { var dividedDiscussionsElement = this.$('.discussions-nav'); if (!this.CourseWideDiscussionsView) { this.CourseWideDiscussionsView = new CourseWideDiscussionsView({ diff --git a/lms/static/js/spec/groups/views/discussions_spec.js b/lms/static/js/spec/groups/views/discussions_spec.js index 6a11910755..627c78f096 100644 --- a/lms/static/js/spec/groups/views/discussions_spec.js +++ b/lms/static/js/spec/groups/views/discussions_spec.js @@ -20,22 +20,26 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers createMockDiscussionsSettingsJson = function(dividedInlineDiscussions, dividedCourseWideDiscussions, - alwaysDivideInlineDiscussions) { + alwaysDivideInlineDiscussions, + availableDivisionSchemes) { return { id: 0, divided_inline_discussions: dividedInlineDiscussions || [], divided_course_wide_discussions: dividedCourseWideDiscussions || [], - always_divide_inline_discussions: alwaysDivideInlineDiscussions || false + always_divide_inline_discussions: alwaysDivideInlineDiscussions || false, + available_division_schemes: availableDivisionSchemes || ['cohort'] }; }; createMockDiscussionsSettings = function(dividedInlineDiscussions, dividedCourseWideDiscussions, - alwaysDivideInlineDiscussions) { + alwaysDivideInlineDiscussions, + availableDivisionSchemes) { return new CourseDiscussionsSettingsModel( createMockDiscussionsSettingsJson(dividedInlineDiscussions, dividedCourseWideDiscussions, - alwaysDivideInlineDiscussions) + alwaysDivideInlineDiscussions, + availableDivisionSchemes) ); }; @@ -132,14 +136,14 @@ define(['backbone', 'jquery', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers expect($courseWideDiscussionsForm.text()). toContain('Course-Wide Discussion Topics'); expect($courseWideDiscussionsForm.text()). - toContain('Select the course-wide discussion topics that you want to divide by cohort.'); + toContain('Select the course-wide discussion topics that you want to divide.'); // Should see the inline discussions form and its content expect($inlineDiscussionsForm.length).toBe(1); expect($inlineDiscussionsForm.text()). toContain('Content-Specific Discussion Topics'); expect($inlineDiscussionsForm.text()). - toContain('Specify whether content-specific discussion topics are divided by cohort.'); + toContain('Specify whether content-specific discussion topics are divided.'); }; beforeEach(function() { diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index d01e2e7d62..1439e0d95f 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -1241,6 +1241,36 @@ // -------------------- .instructor-dashboard-wrapper-2 section.idash-section#discussions_management { + .division-scheme-container { + // See https://css-tricks.com/snippets/css/a-guide-to-flexbox/ + display: flex; + flex-direction: column; + justify-content: space-between; + + .division-scheme { + font-size: 18px; + } + + .division-scheme-item { + padding-left: 1%; + padding-right: 1%; + float: left; + } + + .three-column-layout { + max-width: 33%; + } + + .two-column-layout { + max-width: 50%; + } + + .field-message { + font-size: 13px; + } + } + + // cohort management .form-submit { @include idashbutton($uxpl-blue-base); @include font-size(14); diff --git a/lms/templates/instructor/instructor_dashboard_2/discussions.underscore b/lms/templates/instructor/instructor_dashboard_2/discussions.underscore index 444788c395..2e0ceb83b3 100644 --- a/lms/templates/instructor/instructor_dashboard_2/discussions.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/discussions.underscore @@ -1,5 +1,26 @@
-
-
+
<%- gettext('Specify whether discussion topics are divided') %>
+
+
+ <% for (var i = 0; i < availableSchemes.length; i++) { %> +
+ +

<%- availableSchemes[i].descriptiveText %>

+
+ <% } %> +
+
+
+
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/divided-discussions-course-wide.underscore b/lms/templates/instructor/instructor_dashboard_2/divided-discussions-course-wide.underscore index f359bf75b4..262a536d4c 100644 --- a/lms/templates/instructor/instructor_dashboard_2/divided-discussions-course-wide.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/divided-discussions-course-wide.underscore @@ -1,11 +1,10 @@ -

<%- gettext('Specify whether discussion topics are divided by cohort') %>

<%- gettext('Course-Wide Discussion Topics') %>

-

<%- gettext('Select the course-wide discussion topics that you want to divide by cohort.') %>

+

<%- gettext('Select the course-wide discussion topics that you want to divide.') %>

    <%= HtmlUtils.ensureHtml(courseWideTopicsHtml) %>
diff --git a/lms/templates/instructor/instructor_dashboard_2/divided-discussions-inline.underscore b/lms/templates/instructor/instructor_dashboard_2/divided-discussions-inline.underscore index b6c9e129bf..f7e0990bdf 100644 --- a/lms/templates/instructor/instructor_dashboard_2/divided-discussions-inline.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/divided-discussions-inline.underscore @@ -6,17 +6,17 @@

<%- gettext('Content-Specific Discussion Topics') %>

-

<%- gettext('Specify whether content-specific discussion topics are divided by cohort.') %>

+

<%- gettext('Specify whether content-specific discussion topics are divided.') %>

diff --git a/lms/urls.py b/lms/urls.py index 5946f8af74..8e9b64774d 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -509,7 +509,7 @@ urlpatterns += ( r'^courses/{}/discussions/settings$'.format( settings.COURSE_KEY_PATTERN, ), - 'openedx.core.djangoapps.course_groups.views.course_discussions_settings_handler', + 'lms.djangoapps.discussion.views.course_discussions_settings_handler', name='course_discussions_settings', ), @@ -560,7 +560,7 @@ urlpatterns += ( r'^courses/{}/discussion/topics$'.format( settings.COURSE_KEY_PATTERN, ), - 'openedx.core.djangoapps.course_groups.views.discussion_topics', + 'lms.djangoapps.discussion.views.discussion_topics', name='discussion_topics', ), url( diff --git a/openedx/core/djangoapps/course_groups/tests/helpers.py b/openedx/core/djangoapps/course_groups/tests/helpers.py index d22e5cb7bc..9dd84b64ad 100644 --- a/openedx/core/djangoapps/course_groups/tests/helpers.py +++ b/openedx/core/djangoapps/course_groups/tests/helpers.py @@ -63,25 +63,10 @@ class CourseCohortSettingsFactory(DjangoModelFactory): always_cohort_inline_discussions = False -def topic_name_to_id(course, name): - """ - Given a discussion topic name, return an id for that name (includes - course and url_name). - """ - return "{course}_{run}_{name}".format( - course=course.location.course, - run=course.url_name, - name=name - ) - - def config_course_cohorts_legacy( course, - discussions, cohorted, - cohorted_discussions=None, - auto_cohort_groups=None, - always_cohort_inline_discussions=None + auto_cohort_groups=None ): """ Given a course with no discussion set up, add the discussions and set @@ -93,39 +78,19 @@ def config_course_cohorts_legacy( Arguments: course: CourseDescriptor - discussions: list of topic names strings. Picks ids and sort_keys - automatically. cohorted: bool. - cohorted_discussions: optional list of topic names. If specified, - converts them to use the same ids as topic names. auto_cohort_groups: optional list of strings (names of groups to put students into). Returns: Nothing -- modifies course in place. """ - def to_id(name): - """ - Helper method to convert a discussion topic name to a database identifier - """ - return topic_name_to_id(course, name) - - topics = dict((name, {"sort_key": "A", - "id": to_id(name)}) - for name in discussions) - - course.discussion_topics = topics + course.discussion_topics = {} config = {"cohorted": cohorted} - if cohorted_discussions is not None: - config["cohorted_discussions"] = [to_id(name) - for name in cohorted_discussions] if auto_cohort_groups is not None: config["auto_cohort_groups"] = auto_cohort_groups - if always_cohort_inline_discussions is not None: - config["always_cohort_inline_discussions"] = always_cohort_inline_discussions - course.cohort_config = config try: @@ -135,53 +100,11 @@ def config_course_cohorts_legacy( pass -# pylint: disable=dangerous-default-value -def config_course_discussions( - course, - discussion_topics={}, - divided_discussions=[], - always_divide_inline_discussions=False -): - """ - Set discussions and configure divided discussions for a course. - - Arguments: - course: CourseDescriptor - discussion_topics (Dict): Discussion topic names. Picks ids and - sort_keys automatically. - divided_discussions: Discussion topics to divide. Converts the - list to use the same ids as discussion topic names. - always_divide_inline_discussions (bool): Whether inline discussions - should be divided by default. - - Returns: - Nothing -- modifies course in place. - """ - - def to_id(name): - """Convert name to id.""" - return topic_name_to_id(course, name) - - set_course_discussion_settings( - course.id, - divided_discussions=[to_id(name) for name in divided_discussions], - always_divide_inline_discussions=always_divide_inline_discussions, - division_scheme=CourseDiscussionSettings.COHORT, - ) - - course.discussion_topics = dict((name, {"sort_key": "A", "id": to_id(name)}) - for name in discussion_topics) - try: - # Not implemented for XMLModulestore, which is used by test_cohorts. - modulestore().update_item(course, ModuleStoreEnum.UserID.test) - except NotImplementedError: - pass - - # pylint: disable=dangerous-default-value def config_course_cohorts( course, is_cohorted, + discussion_division_scheme=CourseDiscussionSettings.COHORT, auto_cohorts=[], manual_cohorts=[], ): @@ -191,6 +114,8 @@ def config_course_cohorts( Arguments: course: CourseDescriptor is_cohorted (bool): Is the course cohorted? + discussion_division_scheme (String): the division scheme for discussions. Default is + CourseDiscussionSettings.COHORT. auto_cohorts (list): Names of auto cohorts to create. manual_cohorts (list): Names of manual cohorts to create. @@ -201,7 +126,7 @@ def config_course_cohorts( set_course_cohorted(course.id, is_cohorted) set_course_discussion_settings( course.id, - division_scheme=CourseDiscussionSettings.COHORT, + division_scheme=discussion_division_scheme, ) for cohort_name in auto_cohorts: diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index dd589f56b6..722a5a1235 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -350,7 +350,6 @@ class TestCohorts(ModuleStoreTestCase): # This will have no effect on lms side as we are already done with migrations config_course_cohorts_legacy( course, - discussions=[], cohorted=True, auto_cohort_groups=["OtherGroup"] ) @@ -393,7 +392,6 @@ class TestCohorts(ModuleStoreTestCase): # This will have no effect on lms side as we are already done with migrations config_course_cohorts_legacy( course, - discussions=[], cohorted=True, auto_cohort_groups=["AutoGroup"] ) diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py index 14ef5acfee..432c1aa5af 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_views.py +++ b/openedx/core/djangoapps/course_groups/tests/test_views.py @@ -6,36 +6,34 @@ Tests for course group views import json from collections import namedtuple -from datetime import datetime from nose.plugins.attrib import attr from django.contrib.auth.models import User from django.http import Http404 from django.test.client import RequestFactory +from django_comment_common.models import CourseDiscussionSettings +from django_comment_common.utils import get_course_discussion_settings from student.models import CourseEnrollment from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from opaque_keys.edx.locations import SlashSeparatedCourseKey -from xmodule.modulestore.tests.factories import ItemFactory -from lms.djangoapps.django_comment_client.constants import TYPE_ENTRY, TYPE_SUBCATEGORY -from openedx.core.djangolib.testing.utils import skip_unless_lms from ..models import CourseUserGroup, CourseCohort from ..views import ( - course_cohort_settings_handler, course_discussions_settings_handler, + course_cohort_settings_handler, cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort, - link_cohort_to_partition_group, discussion_topics + link_cohort_to_partition_group, ) from ..cohorts import ( get_cohort, get_cohort_by_name, get_cohort_by_id, DEFAULT_COHORT_NAME, get_group_info_for_cohort ) from .helpers import ( - config_course_cohorts, config_course_discussions, config_course_cohorts_legacy, CohortFactory, CourseCohortFactory, topic_name_to_id + config_course_cohorts, config_course_cohorts_legacy, CohortFactory, CourseCohortFactory ) @@ -101,40 +99,6 @@ class CohortViewsTestCase(ModuleStoreTestCase): view_args.insert(0, request) self.assertRaises(Http404, view, *view_args) - def create_divided_discussions(self): - """ - Set up a divided discussion in the system, complete with all the fixings - """ - divided_inline_discussions = ['Topic A'] - divided_course_wide_discussions = ["Topic B"] - divided_discussions = divided_inline_discussions + divided_course_wide_discussions - - # inline discussion - ItemFactory.create( - parent_location=self.course.location, - category="discussion", - discussion_id=topic_name_to_id(self.course, "Topic A"), - discussion_category="Chapter", - discussion_target="Discussion", - start=datetime.now() - ) - # course-wide discussion - discussion_topics = { - "Topic B": {"id": "Topic B"}, - } - - config_course_cohorts( - self.course, - is_cohorted=True, - ) - - config_course_discussions( - self.course, - discussion_topics=discussion_topics, - divided_discussions=divided_discussions - ) - return divided_inline_discussions, divided_course_wide_discussions - def get_handler(self, course, cohort=None, expected_response_code=200, handler=cohort_handler): """ Call a GET on `handler` for a given `course` and return its response as a dict. @@ -183,129 +147,6 @@ class CohortViewsTestCase(ModuleStoreTestCase): return json.loads(response.content) -@attr(shard=2) -class CourseDiscussionsHandlerTestCase(CohortViewsTestCase): - """ - Tests the course_discussion_settings_handler - """ - def get_expected_response(self): - """ - Returns the static response dict. - """ - return { - u'always_divide_inline_discussions': False, - u'divided_inline_discussions': [], - u'divided_course_wide_discussions': [], - u'id': 1 - } - - def test_non_staff(self): - """ - Verify that we cannot access course_discussions_settings_handler if we're a non-staff user. - """ - self._verify_non_staff_cannot_access(course_discussions_settings_handler, "GET", [unicode(self.course.id)]) - self._verify_non_staff_cannot_access(course_discussions_settings_handler, "PATCH", [unicode(self.course.id)]) - - def test_update_always_divide_inline_discussion_settings(self): - """ - Verify that course_discussions_settings_handler is working for always_divide_inline_discussions via HTTP PATCH. - """ - config_course_cohorts(self.course, is_cohorted=True) - - response = self.get_handler(self.course, handler=course_discussions_settings_handler) - - expected_response = self.get_expected_response() - - self.assertEqual(response, expected_response) - - expected_response['always_divide_inline_discussions'] = True - response = self.patch_handler(self.course, data=expected_response, handler=course_discussions_settings_handler) - - self.assertEqual(response, expected_response) - - def test_update_course_wide_discussion_settings(self): - """ - Verify that course_discussions_settings_handler is working for divided_course_wide_discussions via HTTP PATCH. - """ - # course-wide discussion - discussion_topics = { - "Topic B": {"id": "Topic B"}, - } - - config_course_cohorts(self.course, is_cohorted=True) - config_course_discussions(self.course, discussion_topics=discussion_topics) - - response = self.get_handler(self.course, handler=course_discussions_settings_handler) - - expected_response = self.get_expected_response() - self.assertEqual(response, expected_response) - - expected_response['divided_course_wide_discussions'] = [topic_name_to_id(self.course, "Topic B")] - response = self.patch_handler(self.course, data=expected_response, handler=course_discussions_settings_handler) - - self.assertEqual(response, expected_response) - - def test_update_inline_discussion_settings(self): - """ - Verify that course_discussions_settings_handler is working for divided_inline_discussions via HTTP PATCH. - """ - config_course_cohorts(self.course, is_cohorted=True) - - response = self.get_handler(self.course, handler=course_discussions_settings_handler) - - expected_response = self.get_expected_response() - self.assertEqual(response, expected_response) - - now = datetime.now() - # inline discussion - ItemFactory.create( - parent_location=self.course.location, - category="discussion", - discussion_id="Topic_A", - discussion_category="Chapter", - discussion_target="Discussion", - start=now - ) - - expected_response['divided_inline_discussions'] = ["Topic_A"] - response = self.patch_handler(self.course, data=expected_response, handler=course_discussions_settings_handler) - - self.assertEqual(response, expected_response) - - def test_get_settings(self): - """ - Verify that course_discussions_settings_handler is working for HTTP GET. - """ - divided_inline_discussions, divided_course_wide_discussions = self.create_divided_discussions() - - response = self.get_handler(self.course, handler=course_discussions_settings_handler) - expected_response = self.get_expected_response() - - expected_response['divided_inline_discussions'] = [topic_name_to_id(self.course, name) - for name in divided_inline_discussions] - expected_response['divided_course_wide_discussions'] = [topic_name_to_id(self.course, name) - for name in divided_course_wide_discussions] - - self.assertEqual(response, expected_response) - - def test_update_settings_with_invalid_field_data_type(self): - """ - Verify that course_discussions_settings_handler return HTTP 400 if field data type is incorrect. - """ - config_course_cohorts(self.course, is_cohorted=True) - - response = self.patch_handler( - self.course, - data={'always_divide_inline_discussions': ''}, - expected_response_code=400, - handler=course_discussions_settings_handler - ) - self.assertEqual( - "Incorrect field type for `{}`. Type must be `{}`".format('always_divide_inline_discussions', bool.__name__), - response.get("error") - ) - - @attr(shard=2) class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): """ @@ -345,6 +186,30 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): self.assertEqual(response, expected_response) + def test_enabling_cohorts_does_not_change_division_scheme(self): + """ + Verify that enabling cohorts on a course does not automatically set the discussion division_scheme + to cohort. + """ + config_course_cohorts(self.course, is_cohorted=False, discussion_division_scheme=CourseDiscussionSettings.NONE) + + response = self.get_handler(self.course, handler=course_cohort_settings_handler) + + expected_response = self.get_expected_response() + expected_response['is_cohorted'] = False + self.assertEqual(response, expected_response) + self.assertEqual( + CourseDiscussionSettings.NONE, get_course_discussion_settings(self.course.id).division_scheme + ) + + expected_response['is_cohorted'] = True + response = self.patch_handler(self.course, data=expected_response, handler=course_cohort_settings_handler) + + self.assertEqual(response, expected_response) + self.assertEqual( + CourseDiscussionSettings.NONE, get_course_discussion_settings(self.course.id).division_scheme + ) + def test_update_settings_with_missing_field(self): """ Verify that course_cohort_settings_handler return HTTP 400 if required data field is missing from post data. @@ -492,7 +357,7 @@ class CohortHandlerTestCase(CohortViewsTestCase): # set auto_cohort_groups # these cohort config will have not effect on lms side as we are already done with migrations - config_course_cohorts_legacy(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"]) + config_course_cohorts_legacy(self.course, cohorted=True, auto_cohort_groups=["AutoGroup"]) # We should expect the DoesNotExist exception because above cohort config have # no effect on lms side so as a result there will be no AutoGroup cohort present @@ -1270,60 +1135,3 @@ class RemoveUserFromCohortTestCase(CohortViewsTestCase): cohort = CohortFactory(course_id=self.course.id, users=[user]) response_dict = self.request_remove_user_from_cohort(user.username, cohort) self.verify_removed_user_from_cohort(user.username, response_dict, cohort) - - -@attr(shard=2) -@skip_unless_lms -class CourseDividedDiscussionTopicsTestCase(CohortViewsTestCase): - """ - Tests the `divide_discussion_topics` view. - """ - - def test_non_staff(self): - """ - Verify that we cannot access divide_discussion_topics if we're a non-staff user. - """ - self._verify_non_staff_cannot_access(discussion_topics, "GET", [unicode(self.course.id)]) - - def test_get_discussion_topics(self): - """ - Verify that divide_discussion_topics is working for HTTP GET. - """ - # create inline & course-wide discussion to verify the different map. - self.create_divided_discussions() - - response = self.get_handler(self.course, handler=discussion_topics) - start_date = response['inline_discussions']['subcategories']['Chapter']['start_date'] - expected_response = { - "course_wide_discussions": { - 'children': [['Topic B', TYPE_ENTRY]], - 'entries': { - 'Topic B': { - 'sort_key': 'A', - 'is_divided': True, - 'id': topic_name_to_id(self.course, "Topic B"), - 'start_date': response['course_wide_discussions']['entries']['Topic B']['start_date'] - } - } - }, - "inline_discussions": { - 'subcategories': { - 'Chapter': { - 'subcategories': {}, - 'children': [['Discussion', TYPE_ENTRY]], - 'entries': { - 'Discussion': { - 'sort_key': None, - 'is_divided': True, - 'id': topic_name_to_id(self.course, "Topic A"), - 'start_date': start_date - } - }, - 'sort_key': 'Chapter', - 'start_date': start_date - } - }, - 'children': [['Chapter', TYPE_SUBCATEGORY]] - } - } - self.assertEqual(response, expected_response) diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index d955254ae0..ee43f47b6c 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -15,11 +15,7 @@ from django.http import Http404, HttpResponseBadRequest from django.utils.translation import ugettext from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_http_methods, require_POST -from django_comment_common.models import CourseDiscussionSettings -from django_comment_common.utils import get_course_discussion_settings, set_course_discussion_settings from edxmako.shortcuts import render_to_response -from lms.djangoapps.django_comment_client.constants import TYPE_ENTRY -from lms.djangoapps.django_comment_client.utils import get_discussion_categories_ids, get_discussion_category_map from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from util.json_request import JsonResponse, expect_json @@ -74,22 +70,6 @@ def _get_course_cohort_settings_representation(cohort_id, is_cohorted): } -def _get_course_discussion_settings_representation(course, course_discussion_settings): - """ - Returns a JSON representation of a course discussion settings. - """ - divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions( - course, course_discussion_settings - ) - - return { - 'id': course_discussion_settings.id, - 'divided_inline_discussions': divided_inline_discussions, - 'divided_course_wide_discussions': divided_course_wide_discussions, - 'always_divide_inline_discussions': course_discussion_settings.always_divide_inline_discussions, - } - - def _get_cohort_representation(cohort, course): """ Returns a JSON representation of a cohort. @@ -107,80 +87,6 @@ def _get_cohort_representation(cohort, course): } -def get_divided_discussions(course, discussion_settings): - """ - Returns the course-wide and inline divided discussion ids separately. - """ - divided_course_wide_discussions = [] - divided_inline_discussions = [] - - course_wide_discussions = [topic['id'] for __, topic in course.discussion_topics.items()] - all_discussions = get_discussion_categories_ids(course, None, include_all=True) - - for divided_discussion_id in discussion_settings.divided_discussions: - if divided_discussion_id in course_wide_discussions: - divided_course_wide_discussions.append(divided_discussion_id) - elif divided_discussion_id in all_discussions: - divided_inline_discussions.append(divided_discussion_id) - - return divided_course_wide_discussions, divided_inline_discussions - - -@require_http_methods(("GET", "PATCH")) -@ensure_csrf_cookie -@expect_json -@login_required -def course_discussions_settings_handler(request, course_key_string): - """ - The restful handler for divided discussion setting requests. Requires JSON. - This will raise 404 if user is not staff. - GET - Returns the JSON representation of divided discussion settings for the course. - PATCH - Updates the divided discussion settings for the course. Returns the JSON representation of updated settings. - """ - course_key = CourseKey.from_string(course_key_string) - course = get_course_with_access(request.user, 'staff', course_key) - discussion_settings = get_course_discussion_settings(course_key) - - if request.method == 'PATCH': - divided_course_wide_discussions, divided_inline_discussions = get_divided_discussions( - course, discussion_settings - ) - - settings_to_change = {} - - if 'divided_course_wide_discussions' in request.json or 'divided_inline_discussions' in request.json: - divided_course_wide_discussions = request.json.get( - 'divided_course_wide_discussions', divided_course_wide_discussions - ) - divided_inline_discussions = request.json.get( - 'divided_inline_discussions', divided_inline_discussions - ) - settings_to_change['divided_discussions'] = divided_course_wide_discussions + divided_inline_discussions - - if 'always_divide_inline_discussions' in request.json: - settings_to_change['always_divide_inline_discussions'] = request.json.get( - 'always_divide_inline_discussions' - ) - - if not settings_to_change: - return JsonResponse({"error": unicode("Bad Request")}, 400) - - try: - if settings_to_change: - discussion_settings = set_course_discussion_settings(course_key, **settings_to_change) - - except ValueError as err: - # Note: error message not translated because it is not exposed to the user (UI prevents this state). - return JsonResponse({"error": unicode(err)}, 400) - - return JsonResponse(_get_course_discussion_settings_representation( - course, - discussion_settings - )) - - @require_http_methods(("GET", "PATCH")) @ensure_csrf_cookie @expect_json @@ -196,7 +102,7 @@ def course_cohort_settings_handler(request, course_key_string): """ course_key = CourseKey.from_string(course_key_string) # Although this course data is not used this method will return 404 is user is not staff - course = get_course_with_access(request.user, 'staff', course_key) + get_course_with_access(request.user, 'staff', course_key) if request.method == 'PATCH': if 'is_cohorted' not in request.json: @@ -205,9 +111,6 @@ def course_cohort_settings_handler(request, course_key_string): is_cohorted = request.json.get('is_cohorted') try: cohorts.set_course_cohorted(course_key, is_cohorted) - scheme = CourseDiscussionSettings.COHORT if is_cohorted else CourseDiscussionSettings.NONE - scheme_settings = {'division_scheme': scheme} - set_course_discussion_settings(course_key, **scheme_settings) except ValueError as err: # Note: error message not translated because it is not exposed to the user (UI prevents this state). return JsonResponse({"error": unicode(err)}, 400) @@ -464,81 +367,3 @@ def debug_cohort_mgmt(request, course_key_string): kwargs={'course_key': course_key.to_deprecated_string()} )} return render_to_response('/course_groups/debug.html', context) - - -@expect_json -@login_required -def discussion_topics(request, course_key_string): - """ - The handler for divided discussion categories requests. - This will raise 404 if user is not staff. - - Returns the JSON representation of discussion topics w.r.t categories for the course. - - Example: - >>> example = { - >>> "course_wide_discussions": { - >>> "entries": { - >>> "General": { - >>> "sort_key": "General", - >>> "is_divided": True, - >>> "id": "i4x-edx-eiorguegnru-course-foobarbaz" - >>> } - >>> } - >>> "children": ["General", "entry"] - >>> }, - >>> "inline_discussions" : { - >>> "subcategories": { - >>> "Getting Started": { - >>> "subcategories": {}, - >>> "children": [ - >>> ["Working with Videos", "entry"], - >>> ["Videos on edX", "entry"] - >>> ], - >>> "entries": { - >>> "Working with Videos": { - >>> "sort_key": None, - >>> "is_divided": False, - >>> "id": "d9f970a42067413cbb633f81cfb12604" - >>> }, - >>> "Videos on edX": { - >>> "sort_key": None, - >>> "is_divided": False, - >>> "id": "98d8feb5971041a085512ae22b398613" - >>> } - >>> } - >>> }, - >>> "children": ["Getting Started", "subcategory"] - >>> }, - >>> } - >>> } - """ - course_key = CourseKey.from_string(course_key_string) - course = get_course_with_access(request.user, 'staff', course_key) - - discussion_topics = {} - discussion_category_map = get_discussion_category_map( - course, request.user, divided_only_if_explicit=True, exclude_unstarted=False - ) - - # We extract the data for the course wide discussions from the category map. - course_wide_entries = discussion_category_map.pop('entries') - - course_wide_children = [] - inline_children = [] - - for name, c_type in discussion_category_map['children']: - if name in course_wide_entries and c_type == TYPE_ENTRY: - course_wide_children.append([name, c_type]) - else: - inline_children.append([name, c_type]) - - discussion_topics['course_wide_discussions'] = { - 'entries': course_wide_entries, - 'children': course_wide_children - } - - discussion_category_map['children'] = inline_children - discussion_topics['inline_discussions'] = discussion_category_map - - return JsonResponse(discussion_topics)