Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: change conditions for showing new and legacy discussion providers (#30818)

Browse Source 
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.
This commit is contained in:
Kshitij Sobti
2022-08-25 11:00:18 +00:00
committed by GitHub
parent 0763d97736
commit c2326f0399
9 changed files with 167 additions and 80 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
lms/djangoapps/discussion/toggles.py
View File

@@ -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__
)

19
openedx/core/djangoapps/discussions/config/waffle.py
View File

@@ -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__
)

4
openedx/core/djangoapps/discussions/handlers.py
View File

@@ -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

33
openedx/core/djangoapps/discussions/migrations/0013_auto_20220802_1154.py Normal file
View File

@@ -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',
),
),
]

18
openedx/core/djangoapps/discussions/models.py
View File

@@ -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

29
openedx/core/djangoapps/discussions/tests/test_models.py
View File

@@ -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):
"""

7
openedx/core/djangoapps/discussions/tests/test_transformer.py
View File

@@ -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(

87
openedx/core/djangoapps/discussions/tests/test_views.py
View File

@@ -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},

35
openedx/core/djangoapps/discussions/views.py
View File

@@ -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,