diff --git a/openedx/core/djangoapps/discussions/serializers.py b/openedx/core/djangoapps/discussions/serializers.py index 2f465ae15d..5f0a35acdc 100644 --- a/openedx/core/djangoapps/discussions/serializers.py +++ b/openedx/core/djangoapps/discussions/serializers.py @@ -326,18 +326,20 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): """ save = False updated_provider_type = validated_data.get('provider_type') or instance.provider_type + course = self._get_course() for key in self.Meta.course_fields: value = validated_data.get(key) + course_value = course.discussions_settings.get(key, None) # Delay loading course till we know something has actually been updated - if value is not None and value != getattr(instance, key): - self._get_course().discussions_settings[key] = value + if value is not None and value != course_value: + course.discussions_settings[key] = value save = True new_plugin_config = validated_data.get('plugin_configuration', None) if new_plugin_config and new_plugin_config != instance.plugin_configuration: save = True # Any fields here that aren't already stored in the course structure # or in other models should be stored here. - self._get_course().discussions_settings[updated_provider_type] = { + course.discussions_settings[updated_provider_type] = { key: value for key, value in new_plugin_config.items() if ( @@ -346,7 +348,7 @@ class DiscussionsConfigurationSerializer(serializers.ModelSerializer): ) } if save: - modulestore().update_item(self._get_course(), self.context['user_id']) + modulestore().update_item(course, self.context['user_id']) return instance diff --git a/openedx/core/djangoapps/discussions/tasks.py b/openedx/core/djangoapps/discussions/tasks.py index 93dbff3fa0..9cbfac57d5 100644 --- a/openedx/core/djangoapps/discussions/tasks.py +++ b/openedx/core/djangoapps/discussions/tasks.py @@ -85,11 +85,10 @@ def update_discussions_settings_from_course(course_key: CourseKey) -> CourseDisc with store.branch_setting(ModuleStoreEnum.Branch.published_only, course_key): course = store.get_course(course_key) - provider = course.discussions_settings.get('provider', provider_type) - enable_in_context = course.discussions_settings.get('enable_in_context', True) - provider_config = course.discussions_settings.get(provider, {}) - unit_level_visibility = course.discussions_settings.get('unit_level_visibility', True) - enable_graded_units = course.discussions_settings.get('enable_graded_units', False) + enable_in_context = discussions_config.enable_in_context + provider_config = discussions_config.plugin_configuration + unit_level_visibility = discussions_config.unit_level_visibility + enable_graded_units = discussions_config.enable_graded_units contexts = [] if supports_in_context: sorted_topics = sorted( @@ -112,7 +111,7 @@ def update_discussions_settings_from_course(course_key: CourseKey) -> CourseDisc enable_in_context=enable_in_context, enable_graded_units=enable_graded_units, unit_level_visibility=unit_level_visibility, - provider_type=provider, + provider_type=provider_type, plugin_configuration=provider_config, contexts=contexts, ) @@ -152,7 +151,11 @@ def update_unit_discussion_state_from_discussion_blocks(course_key: CourseKey, u verticals = store.get_items(course_key, qualifiers={'block_type': 'vertical'}) graded_subsections = { block.location - for block in store.get_items(course_key, qualifies={'block_type': 'sequential'}, settings={'graded': True}) + for block in store.get_items( + course_key, + qualifies={'block_type': 'sequential'}, + settings={'graded': True} + ) } subsections_with_discussions = set() for vertical in verticals: @@ -176,9 +179,9 @@ def update_unit_discussion_state_from_discussion_blocks(course_key: CourseKey, u course.discussions_settings['provider'] = provider course.discussions_settings['enable_graded_units'] = enable_graded_subsections course.discussions_settings['unit_level_visibility'] = True + store.update_item(course, user_id) discussion_config = DiscussionsConfiguration.get(course_key) discussion_config.provider_type = provider discussion_config.enable_graded_units = enable_graded_subsections discussion_config.unit_level_visibility = True - store.update_item(course, user_id) discussion_config.save() diff --git a/openedx/core/djangoapps/discussions/tests/test_tasks.py b/openedx/core/djangoapps/discussions/tests/test_tasks.py index a529aff549..097fe971e9 100644 --- a/openedx/core/djangoapps/discussions/tests/test_tasks.py +++ b/openedx/core/djangoapps/discussions/tests/test_tasks.py @@ -7,7 +7,7 @@ from edx_toggles.toggles.testutils import override_waffle_flag from openedx_events.learning.data import DiscussionTopicContext from openedx.core.djangoapps.discussions.config.waffle import ENABLE_NEW_STRUCTURE_DISCUSSIONS -from openedx.core.djangoapps.discussions.models import Provider +from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, Provider from openedx.core.djangoapps.discussions.tasks import ( update_discussions_settings_from_course, update_unit_discussion_state_from_discussion_blocks, @@ -16,16 +16,57 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +class DiscussionConfigUpdateMixin: + """ + Mixin for common methods used to update course discussion configuration. + """ + + def update_course_field(self, **update): + """ + Update the test course using provided parameters. + """ + for key, value in update.items(): + setattr(self.course, key, value) + self.store.update_item(self.course, self.user.id) + + def update_discussions_settings(self, settings): + """ + Update course discussion settings based on the provided discussion settings. + """ + discussion_config = DiscussionsConfiguration.get(self.course.id) + for key, value in settings.items(): + key = "provider_type" if key == "provider" else key + if value is not None: + setattr(discussion_config, key, value) + discussion_config.save() + self.course.discussions_settings.update(settings) + self.store.update_item(self.course, self.user.id) + + def assert_discussion_settings(self, **settings): + """ + Assert that the provided settings have the provided values in the course's discussion settings. + """ + course = self.store.get_course(self.course.id) + discussion_config = DiscussionsConfiguration.get(self.course.id) + for key, value in settings.items(): + assert course.discussions_settings.get(key, None) == value + key = "provider_type" if key == "provider" else key + assert getattr(discussion_config, key) == value + + @ddt.ddt -@mock.patch('openedx.core.djangoapps.discussions.tasks.DiscussionsConfiguration', mock.Mock()) -class UpdateDiscussionsSettingsFromCourseTestCase(ModuleStoreTestCase): +class UpdateDiscussionsSettingsFromCourseTestCase(ModuleStoreTestCase, DiscussionConfigUpdateMixin): """ Tests for the discussions settings update tasks """ def setUp(self): super().setUp() - self.course = course = CourseFactory.create() + self.course = course = CourseFactory.create( + discussions_settings={ + "provider": Provider.OPEN_EDX + } + ) self.course_key = course_key = self.course.id with self.store.bulk_operations(course_key): self.section = ItemFactory.create(parent=course, category="chapter", display_name="Section") @@ -72,21 +113,9 @@ class UpdateDiscussionsSettingsFromCourseTestCase(ModuleStoreTestCase): category="html", display_name="Graded HTML Module", ) - - def update_course_field(self, **update): - """ - Update the test course using provided parameters. - """ - for key, value in update.items(): - setattr(self.course, key, value) - self.store.update_item(self.course, self.user.id) - - def update_discussions_settings(self, settings): - """ - Update course discussion settings based on the provided discussion settings. - """ - self.course.discussions_settings.update(settings) - self.store.update_item(self.course, self.user.id) + discussion_config = DiscussionsConfiguration.get(course_key) + discussion_config.provider_type = Provider.OPEN_EDX + discussion_config.save() def test_default(self): """ @@ -156,7 +185,7 @@ class UpdateDiscussionsSettingsFromCourseTestCase(ModuleStoreTestCase): @ddt.ddt -class MigrateUnitDiscussionStateFromXBlockTestCase(ModuleStoreTestCase): +class MigrateUnitDiscussionStateFromXBlockTestCase(ModuleStoreTestCase, DiscussionConfigUpdateMixin): """ Tests for the discussions settings update tasks """ @@ -222,21 +251,6 @@ class MigrateUnitDiscussionStateFromXBlockTestCase(ModuleStoreTestCase): discussion_category=f'Category {unit.display_name}', ) - def update_course_field(self, **update): - """ - Update the test course using provided parameters. - """ - for key, value in update.items(): - setattr(self.course, key, value) - self.store.update_item(self.course, self.user.id) - - def update_discussions_settings(self, settings): - """ - Update course discussion settings based on the provided discussion settings. - """ - self.course.discussions_settings.update(settings) - self.store.update_item(self.course, self.user.id) - @mock.patch('openedx.core.djangoapps.discussions.tasks.get_accessible_discussion_xblocks_by_course_id') def test_course_not_using_legacy(self, mock_get_discussion_blocks): self.update_discussions_settings({'provider': 'non-legacy'}) @@ -272,14 +286,6 @@ class MigrateUnitDiscussionStateFromXBlockTestCase(ModuleStoreTestCase): } return discussible, non_discussible - def assert_discussion_settings(self, **settings): - """ - Assert that the provided settings have the provided values in the course's discussion settings. - """ - course = self.store.get_course(self.course.id) - for key, value in settings.items(): - assert course.discussions_settings.get(key, None) == value - def test_without_graded(self): self.add_discussion_block([self.unit_discussible]) update_unit_discussion_state_from_discussion_blocks(self.course.id, self.user.id)