From 346257dadf32d71554c2f4c9073fcffb29b82465 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 29 Sep 2022 12:16:03 +0000 Subject: [PATCH] fix: discussion topic links not created in some cases (#31032) This commit attempts to fix cases where dicussion topic links aren't created during a provider change. It does so by eliminating areas where there could be desynchronisation between the configuration the course configuration in Mogo and the discussion config in MySQL. The topic creation code now uses the database version of the config which is more recent. --- .../djangoapps/discussions/serializers.py | 10 +- openedx/core/djangoapps/discussions/tasks.py | 19 ++-- .../discussions/tests/test_tasks.py | 92 ++++++++++--------- 3 files changed, 66 insertions(+), 55 deletions(-) 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)