From 20477abcd1f0c835bb9cb632a701be50b91e873d Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Thu, 1 Sep 2022 15:58:44 +0500 Subject: [PATCH] feat: redirect urls to discussions domain (#30924) Co-authored-by: adeel.tajamul --- lms/djangoapps/discussion/tests/test_views.py | 80 ++++++++++++++----- lms/djangoapps/discussion/views.py | 56 +++++++++---- 2 files changed, 101 insertions(+), 35 deletions(-) diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 9e73967dde..332ac6e40f 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -9,6 +9,7 @@ from unittest.mock import ANY, Mock, call, patch import ddt import pytest +from django.conf import settings from django.http import Http404 from django.test.client import Client, RequestFactory from django.test.utils import override_settings @@ -54,7 +55,8 @@ from lms.djangoapps.discussion.django_comment_client.utils import strip_none from lms.djangoapps.discussion.toggles import ( ENABLE_DISCUSSIONS_MFE, ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, - ENABLE_DISCUSSIONS_MFE_BANNER + ENABLE_DISCUSSIONS_MFE_BANNER, + ENABLE_VIEW_MFE_IN_IFRAME ) from lms.djangoapps.discussion.views import _get_discussion_default_topic_id, course_discussions_settings_handler from lms.djangoapps.teams.tests.factories import CourseTeamFactory, CourseTeamMembershipFactory @@ -2272,6 +2274,9 @@ class ThreadViewedEventTestCase(EventTestMixin, ForumsEnableMixin, UrlResetMixin 'openedx.core.djangoapps.django_comment_common.comment_client.utils.perform_request', Mock( return_value={ + "id": "test_thread", + "title": "Title", + "body": "

", "default_sort_key": "date", "upvoted_ids": [], "downvoted_ids": [], @@ -2331,78 +2336,112 @@ class ForumMFETestCase(ForumsEnableMixin, SharedModuleStoreTestCase): assert "discussions-mfe-tab-embed" not in content @override_settings(DISCUSSIONS_MICROFRONTEND_URL="http://test.url") - @ddt.data(*itertools.product(("learner", "staff"), (True, False))) + @ddt.data(*itertools.product(("learner", "staff"), (True, False), (True, False))) @ddt.unpack - def test_redirect_from_legacy_base_url_to_new_experience(self, user_role, toggle_enabled): + def test_redirect_from_legacy_base_url_to_new_experience(self, user_role, toggle_enabled, in_iframe): """ Verify that the requested is redirected to MFE homepage when ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE flag is enabled. Privileged users will be able to view legacy experience. For learners, if ENABLE_DISCUSSIONS_MFE_BANNER flag is enabled, they will be able to use legacy otherwise they will be redirected to MFE. + IF ENABLE_VIEW_IN_IFRAME is disabled then it will redirect to discussions domain """ if user_role == "staff": user = self.staff_user elif user_role == "learner": user = self.user - with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled): + with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled), \ + override_waffle_flag(ENABLE_VIEW_MFE_IN_IFRAME, in_iframe): self.client.login(username=user.username, password='test') url = reverse("forum_form_discussion", args=[self.course.id]) response = self.client.get(url) content = response.content.decode('utf8') - if toggle_enabled: - assert "discussions-mfe-tab-embed" in content + if in_iframe: + if toggle_enabled: + assert "discussions-mfe-tab-embed" in content + else: + assert "discussions-mfe-tab-embed" not in content else: - assert "discussions-mfe-tab-embed" not in content + if toggle_enabled: + assert response.status_code == 302 + expected_url = f"{settings.DISCUSSIONS_MICROFRONTEND_URL}/{str(self.course.id)}" + assert response.url == expected_url + else: + assert response.status_code == 200 @override_settings(DISCUSSIONS_MICROFRONTEND_URL="http://test.url") - @ddt.data(*itertools.product(("learner", "staff"), (True, False))) + @ddt.data(*itertools.product(("learner", "staff"), (True, False), (True, False))) @ddt.unpack - def test_redirect_from_legacy_profile_url_to_new_experience(self, user_role, toggle_enabled): + def test_redirect_from_legacy_profile_url_to_new_experience(self, user_role, toggle_enabled, in_iframe): """ Verify that the requested is redirected to MFE homepage when ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE flag is enabled. This redirect is only for learners and not for privileged users. + IF ENABLE_VIEW_IN_IFRAME is disabled then it will redirect to discussions domain """ if user_role == "staff": user = self.staff_user elif user_role == "learner": user = self.user - with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled): + with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled), \ + override_waffle_flag(ENABLE_VIEW_MFE_IN_IFRAME, in_iframe): self.client.login(username=user.username, password='test') url = reverse("user_profile", args=[self.course.id, user.id]) response = self.client.get(url) content = response.content.decode('utf8') - if toggle_enabled and user == "learner": - assert "discussions-mfe-tab-embed" in content + + if in_iframe: + if toggle_enabled and user == "learner": + assert "discussions-mfe-tab-embed" in content + else: + assert "discussions-mfe-tab-embed" not in content else: - assert "discussions-mfe-tab-embed" not in content + if toggle_enabled: + if user_role == "staff": + assert "discussions-mfe-tab-embed" not in content + else: + assert response.status_code == 302 + expected_url = f"{settings.DISCUSSIONS_MICROFRONTEND_URL}/{str(self.course.id)}/learners" + assert response.url == expected_url + else: + assert "discussions-mfe-tab-embed" not in content @override_settings(DISCUSSIONS_MICROFRONTEND_URL="http://test.url") - @ddt.data(*itertools.product(("learner", "staff"), (True, False))) + @ddt.data(*itertools.product(("learner", "staff"), (True, False), (True, False))) @ddt.unpack - def test_correct_experience_for_single_thread_url_for_everyone_flag(self, user_role, toggle_enabled): + def test_correct_experience_for_single_thread_url_for_everyone_flag(self, user_role, toggle_enabled, in_iframe): """ Verify that the correct experience is shown based on the MFE toggle for everyone for Legacy single thread url + IF ENABLE_VIEW_IN_IFRAME is disabled then it will redirect to discussions domain """ if user_role == "staff": user = self.staff_user elif user_role == "learner": user = self.user - with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled): + with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, toggle_enabled), \ + override_waffle_flag(ENABLE_VIEW_MFE_IN_IFRAME, in_iframe): self.client.login(username=user.username, password='test') url = reverse("single_thread", args=[self.course.id, "test_discussion", "test_thread"]) response = self.client.get(url) content = response.content.decode('utf8') - if toggle_enabled and user == "learner": - assert "discussions-mfe-tab-embed" in content + if in_iframe: + if toggle_enabled and user == "learner": + assert "discussions-mfe-tab-embed" in content + else: + assert "discussions-mfe-tab-embed" not in content else: - assert "discussions-mfe-tab-embed" not in content + if toggle_enabled: + assert response.status_code == 302 + expected_url = f"{settings.DISCUSSIONS_MICROFRONTEND_URL}/{str(self.course.id)}/posts/test_thread" + assert response.url == expected_url + else: + assert "discussions-mfe-tab-embed" not in content @override_settings(DISCUSSIONS_MICROFRONTEND_URL="http://test.url") @ddt.data(*itertools.product(("legacy", "new"), (True, False))) @@ -2412,7 +2451,8 @@ class ForumMFETestCase(ForumsEnableMixin, SharedModuleStoreTestCase): Verify that the correct experience is shown based on the MFE toggle for everyone for Legacy single thread url """ - with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, True): + with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE, True), \ + override_waffle_flag(ENABLE_VIEW_MFE_IN_IFRAME, True): with override_waffle_flag(ENABLE_DISCUSSIONS_MFE_BANNER, toggle_enabled): self.client.login(username=self.user.username, password='test') url = reverse("forum_form_discussion", args=[self.course.id]) diff --git a/lms/djangoapps/discussion/views.py b/lms/djangoapps/discussion/views.py index de46629458..93d49f6486 100644 --- a/lms/djangoapps/discussion/views.py +++ b/lms/djangoapps/discussion/views.py @@ -269,6 +269,29 @@ def inline_discussion(request, course_key, discussion_id): }) +def redirect_forum_url_to_new_mfe(request, course_id): + """ + Returns the redirect link when user opens default discussion homepage + """ + course_key = CourseKey.from_string(course_id) + discussions_mfe_enabled_for_everyone = ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE.is_enabled(course_key) + view_mfe_in_iframe = ENABLE_VIEW_MFE_IN_IFRAME.is_enabled(course_key) + privileged_user = is_privileged_user(course_key, request.user) + + redirect_url = None + if discussions_mfe_enabled_for_everyone and (not view_mfe_in_iframe): + mfe_base_url = settings.DISCUSSIONS_MICROFRONTEND_URL + redirect_url = f"{mfe_base_url}/{str(course_key)}" + elif discussions_mfe_enabled_for_everyone and (not privileged_user): + discussion_experience = request.GET.get('discussions_experience', None) + banner_enabled = ENABLE_DISCUSSIONS_MFE_BANNER.is_enabled(course_key) + redirect_to_mfe = (discussion_experience is None) or (not banner_enabled) + if redirect_to_mfe and discussion_experience == "legacy": + mfe_context = _discussions_mfe_context(request.GET, course_key, True, False, False) + redirect_url = mfe_context['mfe_url'] + return redirect_url + + @cache_control(no_cache=True, no_store=True, must_revalidate=True) @login_required @use_bulk_ops @@ -307,34 +330,33 @@ def forum_form_discussion(request, course_key): 'corrected_text': query_params['corrected_text'], }) else: - discussions_mfe_enabled_for_everyone = ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE.is_enabled(course_key) - privileged_user = is_privileged_user(course_key, request.user) - if discussions_mfe_enabled_for_everyone and (not privileged_user): - discussion_experience = request.GET.get('discussions_experience', None) - banner_enabled = ENABLE_DISCUSSIONS_MFE_BANNER.is_enabled(course_key) - redirect_to_mfe = (discussion_experience is None) or (not banner_enabled) - if redirect_to_mfe and discussion_experience == "legacy": - mfe_context = _discussions_mfe_context(request.GET, course_key, True, False, False) - return redirect(mfe_context['mfe_url']) + redirect_url = redirect_forum_url_to_new_mfe(request, str(course.id)) + if redirect_url: + return redirect(redirect_url) course_id = str(course.id) tab_view = CourseTabView() return tab_view.get(request, course_id, 'discussion') -def redirect_url_to_new_mfe(request, course_id, thread_id): +def redirect_thread_url_to_new_mfe(request, course_id, thread_id): """ Returns MFE url of the thread if the user is not privileged """ course_key = CourseKey.from_string(course_id) discussions_mfe_enabled_for_everyone = ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE.is_enabled(course_key) - if discussions_mfe_enabled_for_everyone and (not is_privileged_user(course_key, request.user)): + view_mfe_in_iframe = ENABLE_VIEW_MFE_IN_IFRAME.is_enabled(course_key) + redirect_url = None + if discussions_mfe_enabled_for_everyone and (not view_mfe_in_iframe): + mfe_base_url = settings.DISCUSSIONS_MICROFRONTEND_URL + if thread_id: + redirect_url = f"{mfe_base_url}/{str(course_key)}/posts/{thread_id}" + elif discussions_mfe_enabled_for_everyone and (not is_privileged_user(course_key, request.user)): discussion_experience = request.GET.get('discussions_experience', None) if (discussion_experience is None) and (thread_id is not None): mfe_context = _discussions_mfe_context(request.GET, course_key, True, False, False) redirect_url = f"{mfe_context['mfe_url']}#posts/{thread_id}" - return redirect_url - return None + return redirect_url @require_GET @@ -386,7 +408,7 @@ def single_thread(request, course_key, discussion_id, thread_id): 'annotated_content_info': annotated_content_info, }) else: - redirect_url = redirect_url_to_new_mfe(request, str(course.id), thread_id) + redirect_url = redirect_thread_url_to_new_mfe(request, str(course.id), thread_id) if redirect_url: return redirect(redirect_url) @@ -650,8 +672,12 @@ def user_profile(request, course_key, user_id): }) else: discussions_mfe_enabled_for_everyone = ENABLE_DISCUSSIONS_MFE_FOR_EVERYONE.is_enabled(course_key) + view_mfe_in_iframe = ENABLE_VIEW_MFE_IN_IFRAME.is_enabled(course_key) privileged_user = is_privileged_user(course_key, request.user) - if discussions_mfe_enabled_for_everyone and (not privileged_user): + if discussions_mfe_enabled_for_everyone and (not view_mfe_in_iframe): + mfe_base_url = settings.DISCUSSIONS_MICROFRONTEND_URL + return redirect(f"{mfe_base_url}/{str(course_key)}/learners") + elif discussions_mfe_enabled_for_everyone and (not privileged_user): mfe_context = _discussions_mfe_context(request.GET, course_key, True, False, False) return redirect(mfe_context['mfe_url'])