diff --git a/lms/djangoapps/discussion/toggles.py b/lms/djangoapps/discussion/toggles.py index 204b02f9fd..feea519d45 100644 --- a/lms/djangoapps/discussion/toggles.py +++ b/lms/djangoapps/discussion/toggles.py @@ -1,10 +1,9 @@ """ Discussions feature toggles """ +from openedx.core.djangoapps.discussions.config.waffle import WAFFLE_FLAG_NAMESPACE from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag -WAFFLE_FLAG_NAMESPACE = "discussions" - # .. toggle_name: discussions.enable_discussions_mfe # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False @@ -25,17 +24,6 @@ ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE = CourseWaffleFlag( f'{WAFFLE_FLAG_NAMESPACE}.enable_discussions_mfe_for_everyone', __name__ ) -# .. toggle_name: discussions.enable_new_structure_discussions -# .. toggle_implementation: CourseWaffleFlag -# .. toggle_default: False -# .. toggle_description: Waffle flag to toggle on the new structure for in context discussions -# .. toggle_use_cases: temporary, open_edx -# .. toggle_creation_date: 2022-02-22 -# .. toggle_target_removal_date: 2022-09-22 -ENABLE_NEW_STRUCTURE_DISCUSSIONS = CourseWaffleFlag( - f'{WAFFLE_FLAG_NAMESPACE}.enable_new_structure_discussions', __name__ -) - # .. toggle_name: discussions.enable_learners_tab_in_discussions_mfe # .. toggle_implementation: CourseWaffleFlag # .. toggle_default: False @@ -43,7 +31,6 @@ ENABLE_NEW_STRUCTURE_DISCUSSIONS = CourseWaffleFlag( # .. toggle_use_cases: temporary, open_edx # .. toggle_creation_date: 2022-02-21 # .. toggle_target_removal_date: 2022-05-21 -# lint-amnesty, pylint: disable=line-too-long ENABLE_LEARNERS_TAB_IN_DISCUSSIONS_MFE = CourseWaffleFlag( f'{WAFFLE_FLAG_NAMESPACE}.enable_learners_tab_in_discussions_mfe', __name__ ) diff --git a/openedx/core/djangoapps/discussions/config/waffle.py b/openedx/core/djangoapps/discussions/config/waffle.py index 091839c7f3..1d4c67e9e1 100644 --- a/openedx/core/djangoapps/discussions/config/waffle.py +++ b/openedx/core/djangoapps/discussions/config/waffle.py @@ -2,11 +2,9 @@ This module contains various configuration settings via waffle switches for the discussions app. """ - from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag - -WAFFLE_NAMESPACE = 'discussions' +WAFFLE_FLAG_NAMESPACE = "discussions" # .. toggle_name: discussions.override_discussion_legacy_settings # .. toggle_implementation: CourseWaffleFlag @@ -18,7 +16,7 @@ WAFFLE_NAMESPACE = 'discussions' # .. toggle_warning: When the flag is ON, the discussion settings will be available on legacy experience. # .. toggle_tickets: TNL-8389 OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG = CourseWaffleFlag( - f'{WAFFLE_NAMESPACE}.override_discussion_legacy_settings', __name__ + f"{WAFFLE_FLAG_NAMESPACE}.override_discussion_legacy_settings", __name__ ) @@ -32,5 +30,16 @@ OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG = CourseWaffleFlag( # .. toggle_warning: When the flag is ON, the new experience for Pages and Resources will be enabled. # .. toggle_tickets: TNL-7791 ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND = CourseWaffleFlag( - f'{WAFFLE_NAMESPACE}.pages_and_resources_mfe', __name__ + f"{WAFFLE_FLAG_NAMESPACE}.pages_and_resources_mfe", __name__ +) + +# .. toggle_name: discussions.enable_new_structure_discussions +# .. toggle_implementation: CourseWaffleFlag +# .. toggle_default: False +# .. toggle_description: Waffle flag to toggle on the new structure for in context discussions +# .. toggle_use_cases: temporary, open_edx +# .. toggle_creation_date: 2022-02-22 +# .. toggle_target_removal_date: 2022-12-22 +ENABLE_NEW_STRUCTURE_DISCUSSIONS = CourseWaffleFlag( + f"{WAFFLE_FLAG_NAMESPACE}.enable_new_structure_discussions", __name__ ) diff --git a/openedx/core/djangoapps/discussions/handlers.py b/openedx/core/djangoapps/discussions/handlers.py index ed118691ee..25fa0a1e1a 100644 --- a/openedx/core/djangoapps/discussions/handlers.py +++ b/openedx/core/djangoapps/discussions/handlers.py @@ -9,9 +9,9 @@ from django.db import transaction from openedx_events.learning.data import CourseDiscussionConfigurationData from openedx_events.learning.signals import COURSE_DISCUSSIONS_CHANGED from openedx.core.djangoapps.discussions.models import ( - DEFAULT_PROVIDER_TYPE, DiscussionTopicLink, DiscussionsConfiguration, + get_default_provider_type, ) log = logging.getLogger(__name__) @@ -47,7 +47,7 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration configuration (CourseDiscussionConfigurationData): configuration data for the course """ course_key = configuration.course_key - provider_id = configuration.provider_type or DEFAULT_PROVIDER_TYPE + provider_id = configuration.provider_type or get_default_provider_type() new_topic_map = { (topic_context.usage_key or topic_context.external_id): topic_context for topic_context in configuration.contexts diff --git a/openedx/core/djangoapps/discussions/migrations/0013_auto_20220802_1154.py b/openedx/core/djangoapps/discussions/migrations/0013_auto_20220802_1154.py new file mode 100644 index 0000000000..3108dd27e9 --- /dev/null +++ b/openedx/core/djangoapps/discussions/migrations/0013_auto_20220802_1154.py @@ -0,0 +1,33 @@ +# Generated by Django 3.2.14 on 2022-08-02 11:54 + +from django.db import migrations, models +import openedx.core.djangoapps.discussions.models + + +class Migration(migrations.Migration): + dependencies = [ + ('discussions', '0012_auto_20220511_0827'), + ] + + operations = [ + migrations.AlterField( + model_name='discussionsconfiguration', + name='provider_type', + field=models.CharField( + default=openedx.core.djangoapps.discussions.models.get_default_provider_type, + help_text="The discussion tool/provider's id", + max_length=100, + verbose_name='Discussion provider', + ), + ), + migrations.AlterField( + model_name='historicaldiscussionsconfiguration', + name='provider_type', + field=models.CharField( + default=openedx.core.djangoapps.discussions.models.get_default_provider_type, + help_text="The discussion tool/provider's id", + max_length=100, + verbose_name='Discussion provider', + ), + ), + ] diff --git a/openedx/core/djangoapps/discussions/models.py b/openedx/core/djangoapps/discussions/models.py index 25571b045b..f018cf6afa 100644 --- a/openedx/core/djangoapps/discussions/models.py +++ b/openedx/core/djangoapps/discussions/models.py @@ -20,6 +20,7 @@ from opaque_keys.edx.django.models import LearningContextKeyField, UsageKeyField from opaque_keys.edx.keys import CourseKey from simple_history.models import HistoricalRecords +from openedx.core.djangoapps.discussions.config.waffle import ENABLE_NEW_STRUCTURE_DISCUSSIONS from openedx.core.djangoapps.config_model_utils.models import StackedConfigurationModel from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.course_groups.models import CourseUserGroup @@ -45,10 +46,21 @@ class Provider: OPEN_EDX = 'openedx' -DEFAULT_PROVIDER_TYPE = Provider.LEGACY DEFAULT_CONFIG_ENABLED = True +def get_default_provider_type() -> str: + """ + Returns the default provider type to use for new courses. + Returns: + (str) default provider type to use + """ + if ENABLE_NEW_STRUCTURE_DISCUSSIONS.is_enabled(): + return Provider.OPEN_EDX + else: + return Provider.LEGACY + + class Features(Enum): """ Features to be used/mapped in discussion providers @@ -433,7 +445,7 @@ class DiscussionsConfiguration(TimeStampedModel): max_length=100, verbose_name=_("Discussion provider"), help_text=_("The discussion tool/provider's id"), - default=DEFAULT_PROVIDER_TYPE, + default=get_default_provider_type, ) history = HistoricalRecords() @@ -492,7 +504,7 @@ class DiscussionsConfiguration(TimeStampedModel): configuration = cls( context_key=context_key, enabled=DEFAULT_CONFIG_ENABLED, - provider_type=DEFAULT_PROVIDER_TYPE, + provider_type=get_default_provider_type(), ) return configuration diff --git a/openedx/core/djangoapps/discussions/tests/test_models.py b/openedx/core/djangoapps/discussions/tests/test_models.py index 0609ea5d88..fe85a19404 100644 --- a/openedx/core/djangoapps/discussions/tests/test_models.py +++ b/openedx/core/djangoapps/discussions/tests/test_models.py @@ -3,13 +3,17 @@ Perform basic validation of the models """ from unittest.mock import patch + +import ddt import pytest from django.test import TestCase +from edx_toggles.toggles.testutils import override_waffle_flag from opaque_keys.edx.keys import CourseKey from organizations.models import Organization -from ..models import DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE, Provider +from ..config.waffle import ENABLE_NEW_STRUCTURE_DISCUSSIONS +from ..models import DEFAULT_CONFIG_ENABLED, Provider, get_default_provider_type from ..models import DiscussionsConfiguration from ..models import ProviderFilter @@ -117,6 +121,7 @@ class OrganizationFilterTest(TestCase): assert len(providers) == 1 +@ddt.ddt class DiscussionsConfigurationModelTest(TestCase): """ Perform basic validation on the configuration model @@ -162,7 +167,7 @@ class DiscussionsConfigurationModelTest(TestCase): assert configuration.enabled # by default assert configuration.lti_configuration is None assert len(configuration.plugin_configuration.keys()) == 0 - assert configuration.provider_type == DEFAULT_PROVIDER_TYPE + assert configuration.provider_type == get_default_provider_type() def test_get_with_values(self): """ @@ -216,16 +221,22 @@ class DiscussionsConfigurationModelTest(TestCase): is_enabled = DiscussionsConfiguration.is_enabled(self.course_key_with_values) assert not is_enabled - def test_get_nonexistent_defaults_to_legacy(self): + @ddt.data( + (True, Provider.OPEN_EDX), + (False, Provider.LEGACY), + ) + @ddt.unpack + def test_get_nonexistent_defaults(self, new_structure_enabled, default_provider_type): """ Assert we get default provider "legacy" and default enabled model instance back for nonexistent records """ - configuration = DiscussionsConfiguration.get(self.course_key_without_config) - assert configuration is not None - assert configuration.enabled == DEFAULT_CONFIG_ENABLED - assert configuration.provider_type == DEFAULT_PROVIDER_TYPE - assert not configuration.lti_configuration - assert not configuration.plugin_configuration + with override_waffle_flag(ENABLE_NEW_STRUCTURE_DISCUSSIONS, active=new_structure_enabled): + configuration = DiscussionsConfiguration.get(self.course_key_without_config) + assert configuration is not None + assert configuration.enabled == DEFAULT_CONFIG_ENABLED + assert configuration.provider_type == default_provider_type + assert not configuration.lti_configuration + assert not configuration.plugin_configuration def test_get_defaults(self): """ diff --git a/openedx/core/djangoapps/discussions/tests/test_transformer.py b/openedx/core/djangoapps/discussions/tests/test_transformer.py index 0ae6437207..3b7bd815f6 100644 --- a/openedx/core/djangoapps/discussions/tests/test_transformer.py +++ b/openedx/core/djangoapps/discussions/tests/test_transformer.py @@ -7,7 +7,10 @@ from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from lms.djangoapps.course_blocks.api import get_course_blocks from lms.djangoapps.course_blocks.transformers.tests.helpers import TransformerRegistryTestMixin -from openedx.core.djangoapps.discussions.models import DEFAULT_PROVIDER_TYPE, DiscussionTopicLink +from openedx.core.djangoapps.discussions.models import ( + DiscussionTopicLink, + get_default_provider_type, +) from openedx.core.djangoapps.discussions.transformers import DiscussionsTopicLinkTransformer @@ -39,7 +42,7 @@ class DiscussionsTopicLinkTransformerTestCase(TransformerRegistryTestMixin, Modu context_key=self.course.id, usage_key=self.discussable_unit.location, title=self.discussable_unit.display_name, - provider_id=DEFAULT_PROVIDER_TYPE, + provider_id=get_default_provider_type(), external_id=self.test_topic_id, ) self.non_discussable_unit = ItemFactory.create( diff --git a/openedx/core/djangoapps/discussions/tests/test_views.py b/openedx/core/djangoapps/discussions/tests/test_views.py index b8f560847c..6c0b99c159 100644 --- a/openedx/core/djangoapps/discussions/tests/test_views.py +++ b/openedx/core/djangoapps/discussions/tests/test_views.py @@ -19,9 +19,14 @@ from xmodule.modulestore.tests.factories import CourseFactory from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory -from lms.djangoapps.discussion.toggles import ENABLE_NEW_STRUCTURE_DISCUSSIONS +from ..config.waffle import ENABLE_NEW_STRUCTURE_DISCUSSIONS -from ..models import AVAILABLE_PROVIDER_MAP, DEFAULT_CONFIG_ENABLED, DEFAULT_PROVIDER_TYPE, Provider +from ..models import ( + AVAILABLE_PROVIDER_MAP, + DEFAULT_CONFIG_ENABLED, + Provider, + get_default_provider_type, +) DATA_LEGACY_COHORTS = { 'divided_inline_discussions': [], @@ -233,10 +238,9 @@ class CourseDiscussionRoleAuthorizedTests(ApiTest): assert response.status_code == status.HTTP_200_OK -@ddt.ddt -class DataTest(AuthorizedApiTest): +class DataTestMixin: """ - Check API-data correctness + Mixin for common test methods for testing API responses. """ def get(self): @@ -260,7 +264,7 @@ class DataTest(AuthorizedApiTest): data = response.json() assert response.status_code == self.expected_response_code assert data['enabled'] == DEFAULT_CONFIG_ENABLED - assert data['provider_type'] == DEFAULT_PROVIDER_TYPE + assert data['provider_type'] == get_default_provider_type() assert data['providers']['available']['legacy'] == AVAILABLE_PROVIDER_MAP['legacy'] assert not [ name for name, spec in data['providers']['available'].items() @@ -299,6 +303,49 @@ class DataTest(AuthorizedApiTest): assert response.status_code == self.expected_response_code return response.json() + +@ddt.ddt +class ProviderDataTest(CourseInstructorAuthorizedTest, DataTestMixin): + """ + Tests for provider data when user is not global admin. + """ + + @ddt.data( + # If the legacy provider is selected always show the legacy provider, + # and only show the new one if the toggle is enabled + (Provider.LEGACY, [Provider.LEGACY], Provider.OPEN_EDX, False), + (Provider.LEGACY, [Provider.LEGACY, Provider.OPEN_EDX], 'dummy', True), + # If the new provider is selected show the legacy provider only + # if the new discussions toggle is disabled + (Provider.OPEN_EDX, [Provider.OPEN_EDX, Provider.LEGACY], 'dummy', False), + (Provider.OPEN_EDX, [Provider.OPEN_EDX], Provider.LEGACY, True), + # If some other provider is selected show only legacy provider if the toggle is false + # and only the new provider if the toggle is enabled + (Provider.PIAZZA, [Provider.LEGACY], Provider.OPEN_EDX, False), + (Provider.PIAZZA, [Provider.OPEN_EDX], Provider.LEGACY, True), + ) + @ddt.unpack + def test_available_providers( + self, current_provider, visible_providers, hidden_provider, new_structure_enabled + ): + """ + Tests that providers available depending on the course. + """ + self._configure_lti_discussion_provider(provider=current_provider) + with override_waffle_flag(ENABLE_NEW_STRUCTURE_DISCUSSIONS, new_structure_enabled): + response = self._get() + data = response.json() + for visible_provider in visible_providers: + assert visible_provider in data['providers']['available'].keys() + assert hidden_provider not in data['providers']['available'].keys() + + +@ddt.ddt +class DataTest(AuthorizedApiTest, DataTestMixin): + """ + Check API-data correctness + """ + def test_get_non_configured_provider_for_course(self): """ Tests that if no provider is configured for a course, default configuration @@ -367,34 +414,18 @@ class DataTest(AuthorizedApiTest): # directly in the model should be stored here. assert course.discussions_settings['piazza'] == {'custom_field': 'custom_value'} - @ddt.data( - # If the legacy provider is selected show the legacy provider only - # and hide the new provider even if the mfe is enabled - (Provider.LEGACY, [Provider.LEGACY], Provider.OPEN_EDX, False), - (Provider.LEGACY, [Provider.LEGACY], Provider.OPEN_EDX, True), - # If the new provider is selected show the new provider only - # and hide the legacy provider even if the mfe is disabled - (Provider.OPEN_EDX, [Provider.OPEN_EDX], Provider.LEGACY, False), - (Provider.OPEN_EDX, [Provider.OPEN_EDX], Provider.LEGACY, True), - # If some other provider is selected show the new provider - # if the mfe is enabled - (Provider.PIAZZA, [Provider.LEGACY], Provider.OPEN_EDX, False), - (Provider.PIAZZA, [Provider.OPEN_EDX, Provider.LEGACY], 'dummy', True), - ) + @ddt.data(*itertools.product( + (Provider.LEGACY, Provider.OPEN_EDX, Provider.PIAZZA), + (True, False) + )) @ddt.unpack - def test_available_providers_legacy( - self, current_provider, visible_providers, hidden_provider, new_structure_enabled - ): - """ - Tests that providers available depending on the course. - """ + def test_available_providers_staff(self, current_provider, new_structure_enabled): self._configure_lti_discussion_provider(provider=current_provider) with override_waffle_flag(ENABLE_NEW_STRUCTURE_DISCUSSIONS, new_structure_enabled): response = self._get() data = response.json() - for visible_provider in visible_providers: + for visible_provider in [Provider.OPEN_EDX, Provider.LEGACY]: assert visible_provider in data['providers']['available'].keys() - assert hidden_provider not in data['providers']['available'].keys() @ddt.data( {'enabled': 3}, diff --git a/openedx/core/djangoapps/discussions/views.py b/openedx/core/djangoapps/discussions/views.py index e5185278ed..1045b1b261 100644 --- a/openedx/core/djangoapps/discussions/views.py +++ b/openedx/core/djangoapps/discussions/views.py @@ -11,10 +11,10 @@ from rest_framework.request import Request from rest_framework.response import Response from rest_framework.views import APIView -from lms.djangoapps.discussion.toggles import ENABLE_NEW_STRUCTURE_DISCUSSIONS from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import validate_course_key +from .config.waffle import ENABLE_NEW_STRUCTURE_DISCUSSIONS from .models import AVAILABLE_PROVIDER_MAP, DiscussionsConfiguration, Features, Provider from .permissions import IsStaffOrCourseTeam, check_course_permissions from .serializers import ( @@ -155,15 +155,17 @@ class DiscussionsProvidersView(APIView): """ Handle HTTP/GET requests """ - data = self.get_provider_data(course_key_string) + # Return all providers always, if the user is staff + data = self.get_provider_data(course_key_string, show_all=request.user.is_staff) return Response(data) @staticmethod - def get_provider_data(course_key_string: str) -> Dict: + def get_provider_data(course_key_string: str, show_all: bool = False) -> Dict: """ Get provider data for specified course Args: course_key_string (str): course key string + show_all (bool): don't hide any providers Returns: Dict: course discussion providers @@ -171,18 +173,17 @@ class DiscussionsProvidersView(APIView): course_key = validate_course_key(course_key_string) configuration = DiscussionsConfiguration.get(course_key) hidden_providers = [] - # If the user is currently using the legacy provider, don't show the new provider - # TODO: Allow switching between legacy and new providers - if configuration.provider_type == Provider.LEGACY: - hidden_providers.append(Provider.OPEN_EDX) - # If the user is currently using the new provider, don't show the legacy provider - elif configuration.provider_type == Provider.OPEN_EDX: - hidden_providers.append(Provider.LEGACY) - else: - # If this is a new course, or some other provider is selected, the new provider - # should only show up if the new structure for in context discussions flag is enabled - if not ENABLE_NEW_STRUCTURE_DISCUSSIONS.is_enabled(course_key): - hidden_providers.append(Provider.OPEN_EDX) + + if not show_all: + # If the new style discussions are enabled, then hide the legacy provider unless it's already in use. + if ENABLE_NEW_STRUCTURE_DISCUSSIONS.is_enabled(course_key): + if configuration.provider_type != Provider.LEGACY: + hidden_providers.append(Provider.LEGACY) + # If new discussions is not enabled, hide the new provider + else: + if configuration.provider_type != Provider.OPEN_EDX: + hidden_providers.append(Provider.OPEN_EDX) + serializer = DiscussionsProvidersSerializer( { 'features': [ @@ -214,7 +215,7 @@ class CombinedDiscussionsConfigurationView(DiscussionsConfigurationSettingsView) Handle HTTP/GET requests """ config_data = self.get_configuration_data(request, course_key_string) - provider_data = DiscussionsProvidersView.get_provider_data(course_key_string) + provider_data = DiscussionsProvidersView.get_provider_data(course_key_string, show_all=request.user.is_staff) return Response({ **config_data, "features": provider_data["features"], @@ -229,7 +230,7 @@ class CombinedDiscussionsConfigurationView(DiscussionsConfigurationSettingsView) Handle HTTP/POST requests """ config_data = self.update_configuration_data(request, course_key_string) - provider_data = DiscussionsProvidersView.get_provider_data(course_key_string) + provider_data = DiscussionsProvidersView.get_provider_data(course_key_string, request.user.is_staff) return Response( { **config_data,