From d59be9949eba1c6d0490f40f76c66f5d4b4f822f Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Sat, 21 Feb 2015 20:06:59 +0500 Subject: [PATCH] Use cohort settings from CourseCohortSettings. TNL-1258 --- common/lib/xmodule/xmodule/course_module.py | 10 ++ .../lib/xmodule/xmodule/tests/test_import.py | 3 + .../acceptance/tests/discussion/helpers.py | 12 +- .../django_comment_client/forum/tests.py | 6 +- .../django_comment_client/tests/test_utils.py | 37 ++++-- lms/djangoapps/django_comment_client/utils.py | 21 ++-- lms/djangoapps/instructor/tests/test_api.py | 5 +- lms/djangoapps/instructor/views/api.py | 3 +- lms/djangoapps/instructor/views/legacy.py | 3 + .../instructor_task/tasks_helper.py | 7 +- .../legacy_instructor_dashboard.html | 2 +- .../core/djangoapps/course_groups/cohorts.py | 45 ++++---- .../djangoapps/course_groups/tests/helpers.py | 69 +++++++++++- .../course_groups/tests/test_cohorts.py | 105 +++++++++--------- .../tests/test_partition_scheme.py | 2 +- .../course_groups/tests/test_views.py | 28 ++--- 16 files changed, 227 insertions(+), 131 deletions(-) diff --git a/common/lib/xmodule/xmodule/course_module.py b/common/lib/xmodule/xmodule/course_module.py index 085aca92dc..9d1e3ee99b 100644 --- a/common/lib/xmodule/xmodule/course_module.py +++ b/common/lib/xmodule/xmodule/course_module.py @@ -1046,6 +1046,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): def is_cohorted(self): """ Return whether the course is cohorted. + + Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ config = self.cohort_config if config is None: @@ -1057,6 +1059,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): def auto_cohort(self): """ Return whether the course is auto-cohorted. + + Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ if not self.is_cohorted: return False @@ -1070,6 +1074,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): Return the list of groups to put students into. Returns [] if not specified. Returns specified list even if is_cohorted and/or auto_cohort are false. + + Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ if self.cohort_config is None: return [] @@ -1090,6 +1096,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): Return the set of discussions that is explicitly cohorted. It may be the empty set. Note that all inline discussions are automatically cohorted based on the course's is_cohorted setting. + + Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ config = self.cohort_config if config is None: @@ -1103,6 +1111,8 @@ class CourseDescriptor(CourseFields, SequenceDescriptor): This allow to change the default behavior of inline discussions cohorting. By setting this to False, all inline discussions are non-cohorted unless their ids are specified in cohorted_discussions. + + Note: No longer used. See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ config = self.cohort_config if config is None: diff --git a/common/lib/xmodule/xmodule/tests/test_import.py b/common/lib/xmodule/xmodule/tests/test_import.py index c70e1f2742..f185186459 100644 --- a/common/lib/xmodule/xmodule/tests/test_import.py +++ b/common/lib/xmodule/xmodule/tests/test_import.py @@ -643,6 +643,9 @@ class ImportTestCase(BaseCourseTestCase): def test_cohort_config(self): """ Check that cohort config parsing works right. + + Note: The cohort config on the CourseModule is no longer used. + See openedx.core.djangoapps.course_groups.models.CourseCohortSettings. """ modulestore = XMLModuleStore(DATA_DIR, source_dirs=['toy']) diff --git a/common/test/acceptance/tests/discussion/helpers.py b/common/test/acceptance/tests/discussion/helpers.py index 3f4c04e6bc..a5fba485b0 100644 --- a/common/test/acceptance/tests/discussion/helpers.py +++ b/common/test/acceptance/tests/discussion/helpers.py @@ -60,13 +60,11 @@ class CohortTestMixin(object): """ Disables cohorting for the current course fixture. """ - course_fixture._update_xblock(course_fixture._course_location, { - "metadata": { - u"cohort_config": { - "cohorted": False - }, - }, - }) + url = LMS_BASE_URL + "/courses/" + course_fixture._course_key + '/cohorts/settings' # pylint: disable=protected-access + data = json.dumps({'is_cohorted': False}) + response = course_fixture.session.post(url, data=data, headers=course_fixture.headers) + self.assertTrue(response.ok, "Failed to disable cohorts") + def add_manual_cohort(self, course_fixture, cohort_name): """ diff --git a/lms/djangoapps/django_comment_client/forum/tests.py b/lms/djangoapps/django_comment_client/forum/tests.py index 7242c12879..d99657d009 100644 --- a/lms/djangoapps/django_comment_client/forum/tests.py +++ b/lms/djangoapps/django_comment_client/forum/tests.py @@ -315,9 +315,9 @@ class SingleThreadQueryCountTestCase(ModuleStoreTestCase): MODULESTORE = TEST_DATA_MONGO_MODULESTORE @ddt.data( - # old mongo with cache: number of responses plus 17. TODO: O(n)! - (ModuleStoreEnum.Type.mongo, 1, 23, 18), - (ModuleStoreEnum.Type.mongo, 50, 366, 67), + # old mongo with cache: 15 + (ModuleStoreEnum.Type.mongo, 1, 21, 15), + (ModuleStoreEnum.Type.mongo, 50, 315, 15), # split mongo: 3 queries, regardless of thread response size. (ModuleStoreEnum.Type.split, 1, 3, 3), (ModuleStoreEnum.Type.split, 50, 3, 3), diff --git a/lms/djangoapps/django_comment_client/tests/test_utils.py b/lms/djangoapps/django_comment_client/tests/test_utils.py index 79d4cadf0e..1f99a364ff 100644 --- a/lms/djangoapps/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/django_comment_client/tests/test_utils.py @@ -1,18 +1,24 @@ # -*- coding: utf-8 -*- from datetime import datetime import json +import mock from pytz import UTC +from django_comment_client.tests.factories import RoleFactory +from django_comment_client.tests.unicode import UnicodeTestMixin +import django_comment_client.utils as utils from django.core.urlresolvers import reverse from django.test import TestCase from edxmako import add_lookup -import mock from django_comment_client.tests.factories import RoleFactory from django_comment_client.tests.unicode import UnicodeTestMixin from django_comment_client.tests.utils import ContentGroupTestCase import django_comment_client.utils as utils + +from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings from student.tests.factories import UserFactory, CourseEnrollmentFactory +from xmodule.modulestore.tests.django_utils import TEST_DATA_MOCK_MODULESTORE from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -218,20 +224,35 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): check_cohorted_topics([]) # default (empty) cohort config - self.course.cohort_config = {"cohorted": False, "cohorted_discussions": []} + set_course_cohort_settings(course_key=self.course.id, is_cohorted=False, cohorted_discussions=[]) check_cohorted_topics([]) - self.course.cohort_config = {"cohorted": True, "cohorted_discussions": []} + set_course_cohort_settings(course_key=self.course.id, is_cohorted=True, cohorted_discussions=[]) check_cohorted_topics([]) - self.course.cohort_config = {"cohorted": True, "cohorted_discussions": ["Topic_B", "Topic_C"]} + set_course_cohort_settings( + course_key=self.course.id, + is_cohorted=True, + cohorted_discussions=["Topic_B", "Topic_C"], + always_cohort_inline_discussions=False, + ) check_cohorted_topics(["Topic_B", "Topic_C"]) - self.course.cohort_config = {"cohorted": True, "cohorted_discussions": ["Topic_A", "Some_Other_Topic"]} + set_course_cohort_settings( + course_key=self.course.id, + is_cohorted=True, + cohorted_discussions=["Topic_A", "Some_Other_Topic"], + always_cohort_inline_discussions=False, + ) check_cohorted_topics(["Topic_A"]) # unlikely case, but make sure it works. - self.course.cohort_config = {"cohorted": False, "cohorted_discussions": ["Topic_A"]} + set_course_cohort_settings( + course_key=self.course.id, + is_cohorted=False, + cohorted_discussions=["Topic_A"], + always_cohort_inline_discussions=False, + ) check_cohorted_topics([]) def test_single_inline(self): @@ -352,11 +373,11 @@ class CategoryMapTestCase(CategoryMapTestMixin, ModuleStoreTestCase): check_cohorted(False) # explicitly disabled cohorting - self.course.cohort_config = {"cohorted": False} + set_course_cohort_settings(course_key=self.course.id, is_cohorted=False) check_cohorted(False) # explicitly enabled cohorting - self.course.cohort_config = {"cohorted": True} + set_course_cohort_settings(course_key=self.course.id, is_cohorted=True) check_cohorted(True) def test_tree_with_duplicate_targets(self): diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 0a139850fa..3a91daef15 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -19,8 +19,9 @@ from django_comment_client.permissions import check_permissions_by_view, cached_ from edxmako import lookup_template from courseware.access import has_access -from openedx.core.djangoapps.course_groups.cohorts import get_cohort_by_id, get_cohort_id, is_commentable_cohorted, \ - is_course_cohorted +from openedx.core.djangoapps.course_groups.cohorts import ( + get_course_cohort_settings, get_cohort_by_id, get_cohort_id, is_commentable_cohorted, is_course_cohorted +) from openedx.core.djangoapps.course_groups.models import CourseUserGroup @@ -154,8 +155,7 @@ def get_discussion_category_map(course, user): modules = get_accessible_discussion_modules(course, user) - is_course_cohorted = course.is_cohorted - cohorted_discussion_ids = course.cohorted_discussions + course_cohort_settings = get_course_cohort_settings(course.id) for module in modules: id = module.discussion_id @@ -209,16 +209,19 @@ def get_discussion_category_map(course, user): node[level]["entries"][title] = {"id": entry["id"], "sort_key": entry["sort_key"], "start_date": entry["start_date"], - "is_cohorted": is_course_cohorted} + "is_cohorted": course_cohort_settings.is_cohorted} # TODO. BUG! : course location is not unique across multiple course runs! # (I think Kevin already noticed this) Need to send course_id with requests, store it # in the backend. for topic, entry in course.discussion_topics.items(): - category_map['entries'][topic] = {"id": entry["id"], - "sort_key": entry.get("sort_key", topic), - "start_date": datetime.now(UTC()), - "is_cohorted": is_course_cohorted and entry["id"] in cohorted_discussion_ids} + category_map['entries'][topic] = { + "id": entry["id"], + "sort_key": entry.get("sort_key", topic), + "start_date": datetime.now(UTC()), + "is_cohorted": (course_cohort_settings.is_cohorted and + entry["id"] in course_cohort_settings.cohorted_discussions) + } _sort_map_entries(category_map, course.discussion_sort_alpha) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 24eeac62c1..77624cfdd8 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -58,6 +58,8 @@ from instructor.views.api import generate_unique_password from instructor.views.api import _split_input_list, common_exceptions_400 from instructor_task.api_helper import AlreadyRunningError +from openedx.core.djangoapps.course_groups.cohorts import set_course_cohort_settings + from .test_tools import msk_from_problem_urlname from ..views.tools import get_extended_due @@ -2002,8 +2004,7 @@ class TestInstructorAPILevelsDataDump(ModuleStoreTestCase, LoginEnrollmentTestCa cohorted, and does not when the course is not cohorted. """ url = reverse('get_students_features', kwargs={'course_id': unicode(self.course.id)}) - self.course.cohort_config = {'cohorted': is_cohorted} - self.store.update_item(self.course, self.instructor.id) + set_course_cohort_settings(self.course.id, is_cohorted=is_cohorted) response = self.client.get(url, {}) res_json = json.loads(response.content) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index b33abce283..8d5247c1c3 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -104,6 +104,7 @@ from .tools import ( from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey from opaque_keys import InvalidKeyError +from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted log = logging.getLogger(__name__) @@ -1002,7 +1003,7 @@ def get_students_features(request, course_id, csv=False): # pylint: disable=red 'goals': _('Goals'), } - if course.is_cohorted: + if is_course_cohorted(course.id): # Translators: 'Cohort' refers to a group of students within a course. query_features.append('cohort') query_features_names['cohort'] = _('Cohort') diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index 09ecfe05d3..4f208cb343 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -56,6 +56,8 @@ from django.utils.translation import ugettext as _ from microsite_configuration import microsite from opaque_keys.edx.locations import i4xEncoder +from openedx.core.djangoapps.course_groups.cohorts import is_course_cohorted + log = logging.getLogger(__name__) @@ -451,6 +453,7 @@ def instructor_dashboard(request, course_id): context = { 'course': course, + 'course_is_cohorted': is_course_cohorted(course.id), 'staff_access': True, 'admin_access': request.user.is_staff, 'instructor_access': instructor_access, diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index de89f61330..92087df572 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -34,7 +34,7 @@ from lms.djangoapps.lms_xblock.runtime import LmsPartitionService from openedx.core.djangoapps.course_groups.cohorts import get_cohort from openedx.core.djangoapps.course_groups.models import CourseUserGroup from opaque_keys.edx.keys import UsageKey -from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort, is_course_cohorted from student.models import CourseEnrollment @@ -578,7 +578,8 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, TASK_LOG.info(u'%s, Task type: %s, Starting task execution', task_info_string, action_name) course = get_course_by_id(course_id) - cohorts_header = ['Cohort Name'] if course.is_cohorted else [] + course_is_cohorted = is_course_cohorted(course.id) + cohorts_header = ['Cohort Name'] if course_is_cohorted else [] experiment_partitions = get_split_user_partitions(course.user_partitions) group_configs_header = [u'Experiment Group ({})'.format(partition.name) for partition in experiment_partitions] @@ -632,7 +633,7 @@ def upload_grades_csv(_xmodule_instance_args, _entry_id, course_id, _task_input, } cohorts_group_name = [] - if course.is_cohorted: + if course_is_cohorted: group = get_cohort(student, course_id, assign=False) cohorts_group_name.append(group.name if group else '') diff --git a/lms/templates/courseware/legacy_instructor_dashboard.html b/lms/templates/courseware/legacy_instructor_dashboard.html index 9fbe0b3fff..e80a7ee2df 100644 --- a/lms/templates/courseware/legacy_instructor_dashboard.html +++ b/lms/templates/courseware/legacy_instructor_dashboard.html @@ -372,7 +372,7 @@ function goto( mode) %if modeflag.get('Manage Groups'): %if instructor_access: - %if course.is_cohorted: + %if course_is_cohorted:
${_("To manage beta tester roles and cohorts, visit the Membership section of the Instructor Dashboard.")}
%else:${_("To manage beta tester roles, visit the Membership section of the Instructor Dashboard.")}
diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 982b359fd0..ffc520825b 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -71,12 +71,9 @@ def _cohort_membership_changed(sender, **kwargs): tracker.emit(event_name, event) -# A 'default cohort' is an auto-cohort that is automatically created for a course if no auto_cohort_groups have been -# specified. It is intended to be used in a cohorted-course for users who have yet to be assigned to a cohort. -# Note 1: If an administrator chooses to configure a cohort with the same name, the said cohort will be used as -# the "default cohort". -# Note 2: If auto_cohort_groups are configured after the 'default cohort' has been created and populated, the -# stagnant 'default cohort' will still remain (now as a manual cohort) with its previously assigned students. +# A 'default cohort' is an auto-cohort that is automatically created for a course if no cohort with automatic +# assignment have been specified. It is intended to be used in a cohorted-course for users who have yet to be assigned +# to a cohort. # Translation Note: We are NOT translating this string since it is the constant identifier for the "default group" # and needed across product boundaries. DEFAULT_COHORT_NAME = "Default Group" @@ -110,7 +107,7 @@ def is_course_cohorted(course_key): Raises: Http404 if the course doesn't exist. """ - return courses.get_course_by_id(course_key).is_cohorted + return get_course_cohort_settings(course_key).is_cohorted def get_cohort_id(user, course_key): @@ -135,18 +132,19 @@ def is_commentable_cohorted(course_key, commentable_id): Http404 if the course doesn't exist. """ course = courses.get_course_by_id(course_key) + course_cohort_settings = get_course_cohort_settings(course_key) - if not course.is_cohorted: + if not course_cohort_settings.is_cohorted: # this is the easy case :) ans = False elif ( commentable_id in course.top_level_discussion_topic_ids or - course.always_cohort_inline_discussions is False + course_cohort_settings.always_cohort_inline_discussions is False ): # top level discussions have to be manually configured as cohorted # (default is not). # Same thing for inline discussions if the default is explicitly set to False in settings - ans = commentable_id in course.cohorted_discussions + ans = commentable_id in course_cohort_settings.cohorted_discussions else: # inline discussions are cohorted by default ans = True @@ -162,13 +160,13 @@ def get_cohorted_commentables(course_key): Given a course_key return a set of strings representing cohorted commentables. """ - course = courses.get_course_by_id(course_key) + course_cohort_settings = get_course_cohort_settings(course_key) - if not course.is_cohorted: + if not course_cohort_settings.is_cohorted: # this is the easy case :) ans = set() else: - ans = course.cohorted_discussions + ans = set(course_cohort_settings.cohorted_discussions) return ans @@ -193,12 +191,10 @@ def get_cohort(user, course_key, assign=True): """ # First check whether the course is cohorted (users shouldn't be in a cohort # in non-cohorted courses, but settings can change after course starts) - try: - course = courses.get_course_by_id(course_key) - except Http404: - raise ValueError("Invalid course_key") + course = courses.get_course(course_key) + course_cohort_settings = get_course_cohort_settings(course.id) - if not course.is_cohorted: + if not course_cohort_settings.is_cohorted: return None try: @@ -232,9 +228,8 @@ def migrate_cohort_settings(course): Migrate all the cohort settings associated with this course from modulestore to mysql. After that we will never touch modulestore for any cohort related settings. """ - course_id = course.location.course_key cohort_settings, created = CourseCohortsSettings.objects.get_or_create( - course_id=course_id, + course_id=course.id, defaults={ 'is_cohorted': course.is_cohorted, 'cohorted_discussions': list(course.cohorted_discussions), @@ -246,14 +241,14 @@ def migrate_cohort_settings(course): if created: # Update the manual cohorts already present in CourseUserGroup manual_cohorts = CourseUserGroup.objects.filter( - course_id=course_id, + course_id=course.id, group_type=CourseUserGroup.COHORT ).exclude(name__in=course.auto_cohort_groups) for cohort in manual_cohorts: CourseCohort.create(course_user_group=cohort) for group_name in course.auto_cohort_groups: - CourseCohort.create(cohort_name=group_name, course_id=course_id, assignment_type=CourseCohort.RANDOM) + CourseCohort.create(cohort_name=group_name, course_id=course.id, assignment_type=CourseCohort.RANDOM) return cohort_settings @@ -454,7 +449,7 @@ def set_course_cohort_settings(course_key, **kwargs): A CourseCohortSettings object. Raises: - ValueError if course_key is invalid. + Http404 if course_key is invalid. """ fields = {'is_cohorted': bool, 'always_cohort_inline_discussions': bool, 'cohorted_discussions': list} course_cohort_settings = get_course_cohort_settings(course_key) @@ -478,11 +473,11 @@ def get_course_cohort_settings(course_key): A CourseCohortSettings object. Raises: - ValueError if course_key is invalid. + Http404 if course_key is invalid. """ try: course_cohort_settings = CourseCohortsSettings.objects.get(course_id=course_key) except CourseCohortsSettings.DoesNotExist: - course = courses.get_course(course_key) + course = courses.get_course_by_id(course_key) course_cohort_settings = migrate_cohort_settings(course) return course_cohort_settings diff --git a/openedx/core/djangoapps/course_groups/tests/helpers.py b/openedx/core/djangoapps/course_groups/tests/helpers.py index 4964adada9..f14e250aac 100644 --- a/openedx/core/djangoapps/course_groups/tests/helpers.py +++ b/openedx/core/djangoapps/course_groups/tests/helpers.py @@ -4,13 +4,15 @@ Helper methods for testing cohorts. import factory from factory import post_generation, Sequence from factory.django import DjangoModelFactory +import json + from opaque_keys.edx.locations import SlashSeparatedCourseKey from xmodule.modulestore.django import modulestore from xmodule.modulestore import ModuleStoreEnum -from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings - import json +from ..cohorts import get_course_cohort_settings, set_course_cohort_settings +from ..models import CourseUserGroup, CourseCohort, CourseCohortsSettings class CohortFactory(DjangoModelFactory): @@ -67,7 +69,7 @@ def topic_name_to_id(course, name): ) -def config_course_cohorts( +def config_course_cohorts_legacy( course, discussions, cohorted, @@ -77,7 +79,11 @@ def config_course_cohorts( ): """ Given a course with no discussion set up, add the discussions and set - the cohort config appropriately. + the cohort config on the course descriptor. + + Since cohort settings are now stored in models.CourseCohortSettings, + this is only used for testing data migration from the CourseDescriptor + to the table. Arguments: course: CourseDescriptor @@ -105,7 +111,6 @@ def config_course_cohorts( 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 @@ -119,3 +124,57 @@ def config_course_cohorts( modulestore().update_item(course, ModuleStoreEnum.UserID.test) except NotImplementedError: pass + + +def config_course_cohorts( + course, + is_cohorted, + auto_cohorts=[], + manual_cohorts=[], + discussion_topics=[], + cohorted_discussions=[], + always_cohort_inline_discussions=True # pylint: disable=invalid-name +): + """ + Set discussions and configure cohorts for a course. + + Arguments: + course: CourseDescriptor + is_cohorted (bool): Is the course cohorted? + auto_cohorts (list): Names of auto cohorts to create. + manual_cohorts (list): Names of manual cohorts to create. + discussion_topics (list): Discussion topic names. Picks ids and + sort_keys automatically. + cohorted_discussions: Discussion topics to cohort. Converts the + list to use the same ids as discussion topic names. + always_cohort_inline_discussions (bool): Whether inline discussions + should be cohorted by default. + + Returns: + Nothing -- modifies course in place. + """ + def to_id(name): + return topic_name_to_id(course, name) + + set_course_cohort_settings( + course.id, + is_cohorted = is_cohorted, + cohorted_discussions = [to_id(name) for name in cohorted_discussions], + always_cohort_inline_discussions = always_cohort_inline_discussions + ) + + for cohort_name in auto_cohorts: + cohort = CohortFactory(course_id=course.id, name=cohort_name) + CourseCohortFactory(course_user_group=cohort, assignment_type=CourseCohort.RANDOM) + + for cohort_name in manual_cohorts: + cohort = CohortFactory(course_id=course.id, name=cohort_name) + CourseCohortFactory(course_user_group=cohort, assignment_type=CourseCohort.MANUAL) + + 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 diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index 0b704aa7ef..9985fd5964 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -19,7 +19,8 @@ from xmodule.modulestore.tests.django_utils import TEST_DATA_MIXED_TOY_MODULESTO from ..models import CourseUserGroup, CourseCohort, CourseUserGroupPartitionGroup from .. import cohorts from ..tests.helpers import ( - topic_name_to_id, config_course_cohorts, CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory + topic_name_to_id, config_course_cohorts, config_course_cohorts_legacy, + CohortFactory, CourseCohortFactory, CourseCohortSettingsFactory ) @patch("openedx.core.djangoapps.course_groups.cohorts.tracker") @@ -146,12 +147,10 @@ class TestCohorts(ModuleStoreTestCase): Make sure cohorts.is_course_cohorted() correctly reports if a course is cohorted or not. """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) self.assertFalse(cohorts.is_course_cohorted(course.id)) - config_course_cohorts(course, [], cohorted=True) + config_course_cohorts(course, is_cohorted=True) - self.assertTrue(course.is_cohorted) self.assertTrue(cohorts.is_course_cohorted(course.id)) # Make sure we get a Http404 if there's no course @@ -164,12 +163,12 @@ class TestCohorts(ModuleStoreTestCase): invalid course key. """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) user = UserFactory(username="test", email="a@b.com") self.assertIsNone(cohorts.get_cohort_id(user, course.id)) - config_course_cohorts(course, discussions=[], cohorted=True) + config_course_cohorts(course, is_cohorted=True) cohort = CohortFactory(course_id=course.id, name="TestCohort") cohort.users.add(user) self.assertEqual(cohorts.get_cohort_id(user, course.id), cohort.id) @@ -221,7 +220,7 @@ class TestCohorts(ModuleStoreTestCase): """ course = modulestore().get_course(self.toy_course_key) self.assertEqual(course.id, self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) user = UserFactory(username="test", email="a@b.com") other_user = UserFactory(username="test2", email="a2@b.com") @@ -237,7 +236,7 @@ class TestCohorts(ModuleStoreTestCase): ) # Make the course cohorted... - config_course_cohorts(course, discussions=[], cohorted=True) + config_course_cohorts(course, is_cohorted=True) self.assertEquals( cohorts.get_cohort(user, course.id).id, @@ -256,16 +255,15 @@ class TestCohorts(ModuleStoreTestCase): assigned to a user instead of assigning/creating a group automatically """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) user = UserFactory(username="test", email="a@b.com") # Add an auto_cohort_group to the course... config_course_cohorts( course, - discussions=[], - cohorted=True, - auto_cohort_groups=["AutoGroup"] + is_cohorted=True, + auto_cohorts=["AutoGroup"] ) # get_cohort should return None as no group is assigned to user @@ -274,13 +272,13 @@ class TestCohorts(ModuleStoreTestCase): # get_cohort should return a group for user self.assertEquals(cohorts.get_cohort(user, course.id).name, "AutoGroup") - def test_cohorting_with_auto_cohort_groups(self): + def test_cohorting_with_auto_cohorts(self): """ - Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups. + Make sure cohorts.get_cohort() does the right thing. If there are auto cohort groups then a user should be assigned one. """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) user1 = UserFactory(username="test", email="a@b.com") user2 = UserFactory(username="test2", email="a2@b.com") @@ -293,9 +291,8 @@ class TestCohorts(ModuleStoreTestCase): # Add an auto_cohort_group to the course... config_course_cohorts( course, - discussions=[], - cohorted=True, - auto_cohort_groups=["AutoGroup"] + is_cohorted=True, + auto_cohorts=["AutoGroup"] ) self.assertEquals(cohorts.get_cohort(user1, course.id).id, cohort.id, "user1 should stay put") @@ -315,16 +312,15 @@ class TestCohorts(ModuleStoreTestCase): # Add an auto_cohort_group to the course... config_course_cohorts( course, - discussions=[], - cohorted=True, - auto_cohort_groups=["AutoGroup"] + is_cohorted=True, + auto_cohorts=["AutoGroup"] ) self.assertEquals(cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should be auto-cohorted") # Now set the auto_cohort_group to something different # This will have no effect on lms side as we are already done with migrations - config_course_cohorts( + config_course_cohorts_legacy( course, discussions=[], cohorted=True, @@ -339,15 +335,15 @@ class TestCohorts(ModuleStoreTestCase): cohorts.get_cohort(user1, course.id).name, "AutoGroup", "user1 should still be in originally placed cohort" ) - def test_cohorting_with_no_auto_cohort_groups(self): + def test_cohorting_with_no_auto_cohorts(self): """ - Make sure cohorts.get_cohort() does the right thing with auto_cohort_groups. - If there are not auto cohort groups then a user should be assigned to Default Cohort Group. + Make sure cohorts.get_cohort() does the right thing. + If there are not auto cohorts then a user should be assigned to Default Cohort Group. Also verifies that cohort config changes on studio/moduletore side will not be reflected on lms after the migrations are done. """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) user1 = UserFactory(username="test", email="a@b.com") user2 = UserFactory(username="test2", email="a2@b.com") @@ -355,9 +351,8 @@ class TestCohorts(ModuleStoreTestCase): # Make the auto_cohort_group list empty config_course_cohorts( course, - discussions=[], - cohorted=True, - auto_cohort_groups=[] + is_cohorted=True, + auto_cohorts=[] ) self.assertEquals( @@ -368,7 +363,7 @@ class TestCohorts(ModuleStoreTestCase): # Add an auto_cohort_group to the course # This will have no effect on lms side as we are already done with migrations - config_course_cohorts( + config_course_cohorts_legacy( course, discussions=[], cohorted=True, @@ -393,11 +388,11 @@ class TestCohorts(ModuleStoreTestCase): Make sure cohorts.get_cohort() randomizes properly. """ course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) groups = ["group_{0}".format(n) for n in range(5)] config_course_cohorts( - course, discussions=[], cohorted=True, auto_cohort_groups=groups + course, is_cohorted=True, auto_cohorts=groups ) # Assign 100 users to cohorts @@ -423,7 +418,7 @@ class TestCohorts(ModuleStoreTestCase): Tests get_course_cohorts returns an empty list when no cohorts exist. """ course = modulestore().get_course(self.toy_course_key) - config_course_cohorts(course, [], cohorted=True) + config_course_cohorts(course, is_cohorted=True) self.assertEqual([], cohorts.get_course_cohorts(course)) def test_get_course_cohorts(self): @@ -432,8 +427,9 @@ class TestCohorts(ModuleStoreTestCase): """ course = modulestore().get_course(self.toy_course_key) config_course_cohorts( - course, [], cohorted=True, - auto_cohort_groups=["AutoGroup1", "AutoGroup2"] + course, + is_cohorted=True, + auto_cohorts=["AutoGroup1", "AutoGroup2"] ) # add manual cohorts to course 1 @@ -445,7 +441,7 @@ class TestCohorts(ModuleStoreTestCase): def test_is_commentable_cohorted(self): course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) def to_id(name): return topic_name_to_id(course, name) @@ -457,7 +453,7 @@ class TestCohorts(ModuleStoreTestCase): ) # not cohorted - config_course_cohorts(course, ["General", "Feedback"], cohorted=False) + config_course_cohorts(course, is_cohorted=False, discussion_topics=["General", "Feedback"]) self.assertFalse( cohorts.is_commentable_cohorted(course.id, to_id("General")), @@ -465,9 +461,9 @@ class TestCohorts(ModuleStoreTestCase): ) # cohorted, but top level topics aren't - config_course_cohorts(course, ["General", "Feedback"], cohorted=True) + config_course_cohorts(course, is_cohorted=True, discussion_topics=["General", "Feedback"]) - self.assertTrue(course.is_cohorted) + self.assertTrue(cohorts.is_course_cohorted(course.id)) self.assertFalse( cohorts.is_commentable_cohorted(course.id, to_id("General")), "Course is cohorted, but 'General' isn't." @@ -475,12 +471,13 @@ class TestCohorts(ModuleStoreTestCase): # cohorted, including "Feedback" top-level topics aren't config_course_cohorts( - course, ["General", "Feedback"], - cohorted=True, + course, + is_cohorted=True, + discussion_topics= ["General", "Feedback"], cohorted_discussions=["Feedback"] ) - self.assertTrue(course.is_cohorted) + self.assertTrue(cohorts.is_course_cohorted(course.id)) self.assertFalse( cohorts.is_commentable_cohorted(course.id, to_id("General")), "Course is cohorted, but 'General' isn't." @@ -492,14 +489,15 @@ class TestCohorts(ModuleStoreTestCase): def test_is_commentable_cohorted_inline_discussion(self): course = modulestore().get_course(self.toy_course_key) - self.assertFalse(course.is_cohorted) + self.assertFalse(cohorts.is_course_cohorted(course.id)) def to_id(name): # pylint: disable=missing-docstring return topic_name_to_id(course, name) config_course_cohorts( - course, ["General", "Feedback"], - cohorted=True, + course, + is_cohorted=True, + discussion_topics =["General", "Feedback"], cohorted_discussions=["Feedback", "random_inline"] ) self.assertTrue( @@ -510,8 +508,9 @@ class TestCohorts(ModuleStoreTestCase): # if always_cohort_inline_discussions is set to False, non-top-level discussion are always # non cohorted unless they are explicitly set in cohorted_discussions config_course_cohorts( - course, ["General", "Feedback"], - cohorted=True, + course, + is_cohorted=True, + discussion_topics=["General", "Feedback"], cohorted_discussions=["Feedback", "random_inline"], always_cohort_inline_discussions=False ) @@ -538,12 +537,13 @@ class TestCohorts(ModuleStoreTestCase): self.assertEqual(cohorts.get_cohorted_commentables(course.id), set()) - config_course_cohorts(course, [], cohorted=True) + config_course_cohorts(course, is_cohorted=True) self.assertEqual(cohorts.get_cohorted_commentables(course.id), set()) config_course_cohorts( - course, ["General", "Feedback"], - cohorted=True, + course, + is_cohorted=True, + discussion_topics=["General", "Feedback"], cohorted_discussions=["Feedback"] ) self.assertItemsEqual( @@ -552,8 +552,9 @@ class TestCohorts(ModuleStoreTestCase): ) config_course_cohorts( - course, ["General", "Feedback"], - cohorted=True, + course, + is_cohorted=True, + discussion_topics=["General", "Feedback"], cohorted_discussions=["General", "Feedback"] ) self.assertItemsEqual( diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py index 5a33de1e1b..599e31fabe 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py @@ -41,7 +41,7 @@ class TestCohortPartitionScheme(ModuleStoreTestCase): self.course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") self.course = modulestore().get_course(self.course_key) - config_course_cohorts(self.course, [], cohorted=True) + config_course_cohorts(self.course, is_cohorted=True) self.groups = [Group(10, 'Group 10'), Group(20, 'Group 20')] self.user_partition = UserPartition( diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py index f3ee3d21ae..95bec032c0 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_views.py +++ b/openedx/core/djangoapps/course_groups/tests/test_views.py @@ -25,7 +25,9 @@ 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, CohortFactory, CourseCohortFactory, topic_name_to_id +from .helpers import ( + config_course_cohorts, config_course_cohorts_legacy, CohortFactory, CourseCohortFactory, topic_name_to_id +) class CohortViewsTestCase(ModuleStoreTestCase): @@ -136,7 +138,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): Verify that course_cohort_settings_handler is working for HTTP GET. """ cohorted_discussions = ['Topic A', 'Topic B'] - config_course_cohorts(self.course, [], cohorted=True, cohorted_discussions=cohorted_discussions) + config_course_cohorts(self.course, is_cohorted=True, cohorted_discussions=cohorted_discussions) response = self.get_handler(self.course, handler=course_cohort_settings_handler) response['cohorted_discussions'].sort() @@ -155,7 +157,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): """ Verify that course_cohort_settings_handler is working for HTTP POST. """ - config_course_cohorts(self.course, [], cohorted=True) + config_course_cohorts(self.course, is_cohorted=True) response = self.get_handler(self.course, handler=course_cohort_settings_handler) @@ -176,7 +178,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): """ Verify that course_cohort_settings_handler return HTTP 400 if required data field is missing from post data. """ - config_course_cohorts(self.course, [], cohorted=True) + config_course_cohorts(self.course, is_cohorted=True) response = self.put_handler(self.course, expected_response_code=400, handler=course_cohort_settings_handler) self.assertEqual("Bad Request", response.get("error")) @@ -185,7 +187,7 @@ class CourseCohortSettingsHandlerTestCase(CohortViewsTestCase): """ Verify that course_cohort_settings_handler return HTTP 400 if field data type is incorrect. """ - config_course_cohorts(self.course, [], cohorted=True) + config_course_cohorts(self.course, is_cohorted=True) response = self.put_handler( self.course, @@ -268,23 +270,21 @@ class CohortHandlerTestCase(CohortViewsTestCase): """ Verify that auto cohorts are included in the response. """ - config_course_cohorts(self.course, [], cohorted=True, - auto_cohort_groups=["AutoGroup1", "AutoGroup2"]) + config_course_cohorts(self.course, is_cohorted=True, auto_cohorts=["AutoGroup1", "AutoGroup2"]) - # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated. + # Will create manual cohorts cohort1, cohort2, and cohort3. self._create_cohorts() - # Get the cohorts from the course, which will cause auto cohorts to be created. actual_cohorts = self.get_handler(self.course) # Get references to the created auto cohorts. auto_cohort_1 = get_cohort_by_name(self.course.id, "AutoGroup1") auto_cohort_2 = get_cohort_by_name(self.course.id, "AutoGroup2") expected_cohorts = [ + CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CourseCohort.RANDOM), + CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CourseCohort.RANDOM), CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CourseCohort.MANUAL), CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CourseCohort.MANUAL), CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CourseCohort.MANUAL), CohortHandlerTestCase.create_expected_cohort(self.cohort4, 2, CourseCohort.RANDOM), - CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CourseCohort.RANDOM), - CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CourseCohort.RANDOM), ] self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts) @@ -295,8 +295,8 @@ class CohortHandlerTestCase(CohortViewsTestCase): # verify the default cohort is not created when the course is not cohorted self.verify_lists_expected_cohorts([]) - # create a cohorted course without any auto_cohort_groups - config_course_cohorts(self.course, [], cohorted=True) + # create a cohorted course without any auto_cohorts + config_course_cohorts(self.course, is_cohorted=True) # verify the default cohort is not yet created until a user is assigned self.verify_lists_expected_cohorts([]) @@ -320,7 +320,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(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