From c2326f03995291b0f9032c848db50425bb4ccf2d Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Thu, 25 Aug 2022 11:00:18 +0000 Subject: [PATCH] fix: change conditions for showing new and legacy discussion providers (#30818) The current logic for showing discussion providers makes it hard to switch from the legacy to the new provider. This commit changes the conditions in which different providers are shown, and which provider is used as default. Before this commit, the new provider would be hidden if the legacy provider was in use and vice-versa. So both would only be shown if neither legacy nor the new provider were in use (i.e. an LTI provider was in use). Now, all providers are always displayed to global staff. If the waffle flag for the new provider is set (`discussions.enable_new_structure_discussions`), then new provider is always displayed, and the legacy provider is hidden unless it's currently in use. If flag is not set, then the new provider is always hidden unless it is used by a course. Finally, the default provider now depends on the flag above. If it is set globally, then the default provider is the new provider, otherwise the legacy provider remains the default provider. --- lms/djangoapps/discussion/toggles.py | 15 +--- .../djangoapps/discussions/config/waffle.py | 19 ++-- .../core/djangoapps/discussions/handlers.py | 4 +- .../migrations/0013_auto_20220802_1154.py | 33 +++++++ openedx/core/djangoapps/discussions/models.py | 18 +++- .../discussions/tests/test_models.py | 29 +++++-- .../discussions/tests/test_transformer.py | 7 +- .../discussions/tests/test_views.py | 87 +++++++++++++------ openedx/core/djangoapps/discussions/views.py | 35 ++++---- 9 files changed, 167 insertions(+), 80 deletions(-) create mode 100644 openedx/core/djangoapps/discussions/migrations/0013_auto_20220802_1154.py 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,