From 146887df1b65b4e06e4e3c820a45fc557980a444 Mon Sep 17 00:00:00 2001 From: stvn Date: Wed, 14 Apr 2021 16:25:16 -0700 Subject: [PATCH] refactor: Remove set_course_discussion_settings helper --- .../django_comment_client/base/tests.py | 16 +++++----- .../django_comment_client/tests/group_id.py | 14 ++++----- .../django_comment_client/tests/test_utils.py | 13 ++++---- .../django_comment_client/tests/utils.py | 17 +++++----- .../discussion/rest_api/serializers.py | 3 +- lms/djangoapps/discussion/views.py | 3 +- .../core/djangoapps/course_groups/models.py | 2 +- .../djangoapps/course_groups/tests/helpers.py | 10 +++--- .../django_comment_common/models.py | 20 ++++++++++++ .../djangoapps/django_comment_common/tests.py | 18 +++++------ .../djangoapps/django_comment_common/utils.py | 31 ------------------- 11 files changed, 68 insertions(+), 79 deletions(-) diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests.py b/lms/djangoapps/discussion/django_comment_client/base/tests.py index e8ab0838aa..9d2c82c7dc 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests.py @@ -47,7 +47,7 @@ from openedx.core.djangoapps.django_comment_common.models import ( from openedx.core.djangoapps.django_comment_common.utils import ( ThreadContext, seed_permissions_roles, - set_course_discussion_settings + get_course_discussion_settings ) from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES from openedx.core.lib.teams_config import TeamsConfig @@ -1425,13 +1425,13 @@ class TeamsPermissionsTestCase(ForumsEnableMixin, UrlResetMixin, SharedModuleSto If dividing by cohorts, create and assign users to a cohort. """ enable_cohorts = True if scheme is CourseDiscussionSettings.COHORT else False - set_course_discussion_settings( - self.course.id, - enable_cohorts=enable_cohorts, - divided_discussions=[], - always_divide_inline_discussions=True, - division_scheme=scheme, - ) + discussion_settings = get_course_discussion_settings(self.course.id) + discussion_settings.update({ + 'enable_cohorts': enable_cohorts, + 'divided_discussions': [], + 'always_divide_inline_discussions': True, + 'division_scheme': scheme, + }) set_course_cohorted(self.course.id, enable_cohorts) @classmethod diff --git a/lms/djangoapps/discussion/django_comment_client/tests/group_id.py b/lms/djangoapps/discussion/django_comment_client/tests/group_id.py index 6acf974d0a..ab1bd4d07c 100644 --- a/lms/djangoapps/discussion/django_comment_client/tests/group_id.py +++ b/lms/djangoapps/discussion/django_comment_client/tests/group_id.py @@ -8,7 +8,7 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory from lms.djangoapps.teams.tests.factories import CourseTeamFactory from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings -from openedx.core.djangoapps.django_comment_common.utils import set_course_discussion_settings +from openedx.core.djangoapps.django_comment_common.utils import get_course_discussion_settings class GroupIdAssertionMixin: @@ -108,12 +108,12 @@ class CohortedTopicGroupIdTestMixin(GroupIdAssertionMixin): def test_cohorted_topic_enrollment_track_invalid_group_id(self, mock_request): CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.AUDIT) CourseModeFactory.create(course_id=self.course.id, mode_slug=CourseMode.VERIFIED) - set_course_discussion_settings( - course_key=self.course.id, - divided_discussions=['cohorted_topic'], - division_scheme=CourseDiscussionSettings.ENROLLMENT_TRACK, - always_divide_inline_discussions=True, - ) + discussion_settings = get_course_discussion_settings(self.course.id) + discussion_settings.update({ + 'divided_discussions': ['cohorted_topic'], + 'division_scheme': CourseDiscussionSettings.ENROLLMENT_TRACK, + 'always_divide_inline_discussions': True, + }) invalid_id = -1000 response = self.call_view(mock_request, "cohorted_topic", self.moderator, invalid_id) # lint-amnesty, pylint: disable=assignment-from-no-return diff --git a/lms/djangoapps/discussion/django_comment_client/tests/test_utils.py b/lms/djangoapps/discussion/django_comment_client/tests/test_utils.py index 19efdcb495..3f99b32503 100644 --- a/lms/djangoapps/discussion/django_comment_client/tests/test_utils.py +++ b/lms/djangoapps/discussion/django_comment_client/tests/test_utils.py @@ -42,7 +42,6 @@ from openedx.core.djangoapps.django_comment_common.models import ( from openedx.core.djangoapps.django_comment_common.utils import ( get_course_discussion_settings, seed_permissions_roles, - set_course_discussion_settings ) from openedx.core.djangoapps.util.testing import ContentGroupTestCase from xmodule.modulestore import ModuleStoreEnum @@ -1697,12 +1696,12 @@ def set_discussion_division_settings( COHORT is the default division_scheme, as no other schemes were supported at the time that the unit tests were originally written. """ - set_course_discussion_settings( - course_key=course_key, - divided_discussions=divided_discussions, - division_scheme=division_scheme, - always_divide_inline_discussions=always_divide_inline_discussions, - ) + discussion_settings = get_course_discussion_settings(course_key) + discussion_settings.update({ + 'divided_discussions': divided_discussions, + 'division_scheme': division_scheme, + 'always_divide_inline_discussions': always_divide_inline_discussions, + }) set_course_cohorted(course_key, enable_cohorts) diff --git a/lms/djangoapps/discussion/django_comment_client/tests/utils.py b/lms/djangoapps/discussion/django_comment_client/tests/utils.py index bc9124197f..3f49f42183 100644 --- a/lms/djangoapps/discussion/django_comment_client/tests/utils.py +++ b/lms/djangoapps/discussion/django_comment_client/tests/utils.py @@ -11,8 +11,8 @@ from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory from openedx.core.djangoapps.django_comment_common.models import ForumsConfig, Role from openedx.core.djangoapps.django_comment_common.utils import ( CourseDiscussionSettings, + get_course_discussion_settings, seed_permissions_roles, - set_course_discussion_settings ) from openedx.core.lib.teams_config import TeamsConfig from xmodule.modulestore import ModuleStoreEnum @@ -108,12 +108,15 @@ def config_course_discussions( """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, - ) + discussion_settings = get_course_discussion_settings(course.id) + discussion_settings.update({ + '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 = {name: {"sort_key": "A", "id": to_id(name)} for name in discussion_topics} diff --git a/lms/djangoapps/discussion/rest_api/serializers.py b/lms/djangoapps/discussion/rest_api/serializers.py index e0cdb8cd41..a1016ae82d 100644 --- a/lms/djangoapps/discussion/rest_api/serializers.py +++ b/lms/djangoapps/discussion/rest_api/serializers.py @@ -37,7 +37,6 @@ from openedx.core.djangoapps.django_comment_common.models import ( Role ) from openedx.core.djangoapps.django_comment_common.utils import get_course_discussion_settings -from openedx.core.djangoapps.django_comment_common.utils import set_course_discussion_settings def get_context(course, request, thread=None): @@ -542,7 +541,7 @@ class DiscussionSettingsSerializer(serializers.Serializer): if not any(field in validated_data for field in self.fields): raise ValidationError('Bad request') try: - set_course_discussion_settings(course_key, **validated_data) + instance.update(validated_data) except ValueError as e: raise ValidationError(str(e)) from e return instance diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index ac50ccdd00..1e227ee668 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -54,7 +54,6 @@ from openedx.core.djangoapps.django_comment_common.models import CourseDiscussio from openedx.core.djangoapps.django_comment_common.utils import ( ThreadContext, get_course_discussion_settings, - set_course_discussion_settings ) from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.features.course_duration_limits.access import generate_course_expired_fragment @@ -941,7 +940,7 @@ def course_discussions_settings_handler(request, course_key_string): try: if settings_to_change: - discussion_settings = set_course_discussion_settings(course_key, **settings_to_change) + discussion_settings.update(settings_to_change) except ValueError as err: # Note: error message not translated because it is not exposed to the user (UI prevents this state). diff --git a/openedx/core/djangoapps/course_groups/models.py b/openedx/core/djangoapps/course_groups/models.py index 58d394586a..26f41d0c86 100644 --- a/openedx/core/djangoapps/course_groups/models.py +++ b/openedx/core/djangoapps/course_groups/models.py @@ -203,7 +203,7 @@ class CourseCohortsSettings(models.Model): @cohorted_discussions.setter def cohorted_discussions(self, value): """ - DEPRECATED-- DO NOT USE. Instead use `CourseDiscussionSettings` via `set_course_discussion_settings`. + DEPRECATED-- DO NOT USE. Instead use `CourseDiscussionSettings.update` """ self._cohorted_discussions = json.dumps(value) diff --git a/openedx/core/djangoapps/course_groups/tests/helpers.py b/openedx/core/djangoapps/course_groups/tests/helpers.py index 3e670034eb..0024fc7c93 100644 --- a/openedx/core/djangoapps/course_groups/tests/helpers.py +++ b/openedx/core/djangoapps/course_groups/tests/helpers.py @@ -10,7 +10,7 @@ from factory.django import DjangoModelFactory from opaque_keys.edx.locator import CourseLocator from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings -from openedx.core.djangoapps.django_comment_common.utils import set_course_discussion_settings +from openedx.core.djangoapps.django_comment_common.utils import get_course_discussion_settings from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore @@ -125,10 +125,10 @@ def config_course_cohorts( """ set_course_cohorted(course.id, is_cohorted) - set_course_discussion_settings( - course.id, - division_scheme=discussion_division_scheme, - ) + discussion_settings = get_course_discussion_settings(course.id) + discussion_settings.update({ + 'division_scheme': discussion_division_scheme, + }) for cohort_name in auto_cohorts: cohort = CohortFactory(course_id=course.id, name=cohort_name) diff --git a/openedx/core/djangoapps/django_comment_common/models.py b/openedx/core/djangoapps/django_comment_common/models.py index 47795da9fe..aa7c1912b1 100644 --- a/openedx/core/djangoapps/django_comment_common/models.py +++ b/openedx/core/djangoapps/django_comment_common/models.py @@ -267,6 +267,26 @@ class CourseDiscussionSettings(models.Model): """ self._divided_discussions = json.dumps(value) + def update(self, validated_data: dict): + """ + Set discussion settings for a course + + Returns: + A CourseDiscussionSettings object + """ + fields = { + 'division_scheme': (str,)[0], + 'always_divide_inline_discussions': bool, + 'divided_discussions': list, + } + for field, field_type in fields.items(): + if field in validated_data: + if not isinstance(validated_data[field], field_type): + raise ValueError(f"Incorrect field type for `{field}`. Type must be `{field_type.__name__}`") + setattr(self, field, validated_data[field]) + self.save() + return self + class DiscussionsIdMapping(models.Model): """ diff --git a/openedx/core/djangoapps/django_comment_common/tests.py b/openedx/core/djangoapps/django_comment_common/tests.py index 5d996d90ce..005d789530 100644 --- a/openedx/core/djangoapps/django_comment_common/tests.py +++ b/openedx/core/djangoapps/django_comment_common/tests.py @@ -10,7 +10,6 @@ from openedx.core.djangoapps.course_groups.cohorts import CourseCohortsSettings from openedx.core.djangoapps.django_comment_common.models import CourseDiscussionSettings, Role from openedx.core.djangoapps.django_comment_common.utils import ( get_course_discussion_settings, - set_course_discussion_settings ) from common.djangoapps.student.models import CourseEnrollment, User from xmodule.modulestore import ModuleStoreEnum @@ -111,13 +110,13 @@ class CourseDiscussionSettingsTest(ModuleStoreTestCase): assert ['foo', 'bar'] == discussion_settings.divided_discussions assert discussion_settings.always_divide_inline_discussions - def test_set_course_discussion_settings(self): - set_course_discussion_settings( - course_key=self.course.id, - divided_discussions=['cohorted_topic'], - division_scheme=CourseDiscussionSettings.ENROLLMENT_TRACK, - always_divide_inline_discussions=True, - ) + def test_update_course_discussion_settings(self): + discussion_settings = get_course_discussion_settings(self.course.id) + discussion_settings.update({ + 'divided_discussions': ['cohorted_topic'], + 'division_scheme': CourseDiscussionSettings.ENROLLMENT_TRACK, + 'always_divide_inline_discussions': True, + }) discussion_settings = get_course_discussion_settings(self.course.id) assert CourseDiscussionSettings.ENROLLMENT_TRACK == discussion_settings.division_scheme assert ['cohorted_topic'] == discussion_settings.divided_discussions @@ -132,8 +131,9 @@ class CourseDiscussionSettingsTest(ModuleStoreTestCase): ] invalid_value = 3.14 + discussion_settings = get_course_discussion_settings(self.course.id) for field in fields: with pytest.raises(ValueError) as value_error: - set_course_discussion_settings(self.course.id, **{field['name']: invalid_value}) + discussion_settings.update({field['name']: invalid_value}) assert str(value_error.value) == exception_msg_template.format(field['name'], field['type'].__name__) diff --git a/openedx/core/djangoapps/django_comment_common/utils.py b/openedx/core/djangoapps/django_comment_common/utils.py index 004479341d..b689043deb 100644 --- a/openedx/core/djangoapps/django_comment_common/utils.py +++ b/openedx/core/djangoapps/django_comment_common/utils.py @@ -134,34 +134,3 @@ def get_course_discussion_settings(course_key): ) return course_discussion_settings - - -def set_course_discussion_settings(course_key, **kwargs): - """ - Set discussion settings for a course. - - Arguments: - course_key: CourseKey - always_divide_inline_discussions (bool): If inline discussions should always be divided. - divided_discussions (list): List of discussion ids. - division_scheme (str): `CourseDiscussionSettings.NONE`, `CourseDiscussionSettings.COHORT`, - or `CourseDiscussionSettings.ENROLLMENT_TRACK` - - Returns: - A CourseDiscussionSettings object. - """ - fields = { - 'division_scheme': (str,)[0], - '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(f"Incorrect field type for `{field}`. Type must be `{field_type.__name__}`") - setattr(course_discussion_settings, field, kwargs[field]) - - course_discussion_settings.save() - return course_discussion_settings