From 7037c8d27bee3ba150970822f9440d945a216a58 Mon Sep 17 00:00:00 2001 From: Nathan Sprenkle Date: Wed, 7 Feb 2024 10:27:57 -0500 Subject: [PATCH] feat: remove rollout percentage code for learner home (#34198) This code allowed us to control rollout but is no longer needed. --- common/djangoapps/student/tests/test_views.py | 6 +-- common/djangoapps/student/views/dashboard.py | 4 +- lms/djangoapps/learner_home/test_waffle.py | 41 ++++++------------- lms/djangoapps/learner_home/waffle.py | 25 +++-------- lms/envs/common.py | 2 - 5 files changed, 22 insertions(+), 56 deletions(-) diff --git a/common/djangoapps/student/tests/test_views.py b/common/djangoapps/student/tests/test_views.py index 1230f8320a..16bab13b90 100644 --- a/common/djangoapps/student/tests/test_views.py +++ b/common/djangoapps/student/tests/test_views.py @@ -235,12 +235,12 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin, response = self.client.get(self.path) self.assertRedirects(response, reverse('account_settings')) - @patch('common.djangoapps.student.views.dashboard.should_redirect_to_learner_home_mfe') - def test_redirect_to_learner_home(self, mock_should_redirect_to_learner_home_mfe): + @patch('common.djangoapps.student.views.dashboard.learner_home_mfe_enabled') + def test_redirect_to_learner_home(self, mock_learner_home_mfe_enabled): """ if learner home mfe is enabled, redirect to learner home mfe """ - mock_should_redirect_to_learner_home_mfe.return_value = True + mock_learner_home_mfe_enabled.return_value = True response = self.client.get(self.path) self.assertRedirects(response, settings.LEARNER_HOME_MICROFRONTEND_URL, fetch_redirect_response=False) diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py index 0e73c3c257..115003921a 100644 --- a/common/djangoapps/student/views/dashboard.py +++ b/common/djangoapps/student/views/dashboard.py @@ -30,7 +30,7 @@ from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_st from common.djangoapps.entitlements.models import CourseEntitlement from lms.djangoapps.commerce.utils import EcommerceService from lms.djangoapps.courseware.access import has_access -from lms.djangoapps.learner_home.waffle import should_redirect_to_learner_home_mfe +from lms.djangoapps.learner_home.waffle import learner_home_mfe_enabled from lms.djangoapps.experiments.utils import get_dashboard_course_info, get_experiment_user_metadata_context from lms.djangoapps.verify_student.services import IDVerificationService from openedx.core.djangoapps.catalog.utils import ( @@ -521,7 +521,7 @@ def student_dashboard(request): # lint-amnesty, pylint: disable=too-many-statem if not UserProfile.objects.filter(user=user).exists(): return redirect(reverse('account_settings')) - if should_redirect_to_learner_home_mfe(user): + if learner_home_mfe_enabled(): return redirect(settings.LEARNER_HOME_MICROFRONTEND_URL) platform_name = configuration_helpers.get_value("platform_name", settings.PLATFORM_NAME) diff --git a/lms/djangoapps/learner_home/test_waffle.py b/lms/djangoapps/learner_home/test_waffle.py index c9b4ee281d..9d3f19f29a 100644 --- a/lms/djangoapps/learner_home/test_waffle.py +++ b/lms/djangoapps/learner_home/test_waffle.py @@ -5,17 +5,15 @@ Tests for toggles, where there is logic beyond enable/disable. from unittest.mock import patch import ddt -from django.test import override_settings - from common.djangoapps.student.tests.factories import UserFactory -from lms.djangoapps.learner_home.waffle import should_redirect_to_learner_home_mfe +from lms.djangoapps.learner_home.waffle import learner_home_mfe_enabled from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @ddt.ddt -class TestLearnerHomeRedirect(SharedModuleStoreTestCase): +class TestLearnerHomeWaffle(SharedModuleStoreTestCase): """ - Tests for should_redirect_to_learner_home, used for experimental rollout. + Tests for learner_home_mfe_enabled """ def setUp(self): @@ -24,31 +22,16 @@ class TestLearnerHomeRedirect(SharedModuleStoreTestCase): # Set up a user for testing self.user = UserFactory + @ddt.data(True, False) @patch("lms.djangoapps.learner_home.waffle.ENABLE_LEARNER_HOME_MFE") - def test_should_redirect_to_learner_home_disabled(self, mock_enable_learner_home): - # Given Learner Home MFE feature is not enabled - mock_enable_learner_home.is_enabled.return_value = False - - # When I check if I should redirect - redirect_choice = should_redirect_to_learner_home_mfe(self.user) - - # Then I never redirect - self.assertFalse(redirect_choice) - - @ddt.data((0, True), (50, False), (100, True)) - @ddt.unpack - @patch("lms.djangoapps.learner_home.waffle.ENABLE_LEARNER_HOME_MFE") - @override_settings(LEARNER_HOME_MFE_REDIRECT_PERCENTAGE=50) - def test_should_redirect_to_learner_home_enabled( - self, user_id, expect_redirect, mock_enable_learner_home + def test_learner_home_mfe_enabled( + self, is_waffle_enabled, mock_enable_learner_home ): - # Given Learner Home MFE feature is enabled - mock_enable_learner_home.is_enabled.return_value = True - self.user.id = user_id + # Given Learner Home MFE feature is / not enabled + mock_enable_learner_home.is_enabled.return_value = is_waffle_enabled - # When I check if I should redirect - redirect_choice = should_redirect_to_learner_home_mfe(self.user) + # When I check if the feature is enabled + is_learner_home_enabled = learner_home_mfe_enabled() - # Then I redirect based on configuration - # (currently user ID % 100 < redirect percentage) - self.assertEqual(expect_redirect, redirect_choice) + # Then I respects waffle setting. + self.assertEqual(is_learner_home_enabled, is_waffle_enabled) diff --git a/lms/djangoapps/learner_home/waffle.py b/lms/djangoapps/learner_home/waffle.py index 5f09e3a9c0..bce7717c65 100644 --- a/lms/djangoapps/learner_home/waffle.py +++ b/lms/djangoapps/learner_home/waffle.py @@ -1,8 +1,6 @@ """ Configuration for features of Learner Home """ -from django.conf import settings - from edx_toggles.toggles import WaffleFlag from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -23,25 +21,12 @@ ENABLE_LEARNER_HOME_MFE = WaffleFlag( ) -def should_redirect_to_learner_home_mfe(user): +def learner_home_mfe_enabled(): """ - Redirect a percentage of learners to Learner Home for experimentation. - - Percentage is based on the LEARNER_HOME_MFE_REDIRECT_PERCENTAGE setting. + Determine if Learner Home MFE is enabled, replacing student_dashboard """ - is_learning_mfe_enabled = configuration_helpers.get_value( - "ENABLE_LEARNER_HOME_MFE", ENABLE_LEARNER_HOME_MFE.is_enabled() + return configuration_helpers.get_value( + "ENABLE_LEARNER_HOME_MFE", + ENABLE_LEARNER_HOME_MFE.is_enabled(), ) - - learning_mfe_redirect_percent = configuration_helpers.get_value( - "LEARNER_HOME_MFE_REDIRECT_PERCENTAGE", - settings.LEARNER_HOME_MFE_REDIRECT_PERCENTAGE, - ) - - # Redirect when 1) Learner Home MFE is enabled and 2) a user falls into the - # target range for experimental rollout. - if is_learning_mfe_enabled and user.id % 100 < learning_mfe_redirect_percent: - return True - - return False diff --git a/lms/envs/common.py b/lms/envs/common.py index f14ec39627..2650198145 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -5064,8 +5064,6 @@ ENABLE_DYNAMIC_REGISTRATION_FIELDS = False # .. toggle_tickets: https://2u-internal.atlassian.net/browse/VAN-1797 ENFORCE_SESSION_EMAIL_MATCH = False -LEARNER_HOME_MFE_REDIRECT_PERCENTAGE = 0 - ############### Settings for the ace_common plugin ################# # Note that all settings are actually defined by the plugin # pylint: disable=wrong-import-position