From 99dd634255b8fce3b403a57c2dfb2e157c2c83ad Mon Sep 17 00:00:00 2001 From: henrrypg Date: Tue, 15 Nov 2022 19:37:20 -0500 Subject: [PATCH 1/6] feat: add filter hooks before regitration render and settings context --- .../core/djangoapps/user_api/accounts/settings_views.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index ae7bca301444..12b74a66b77b 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -8,12 +8,14 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required +from django.core.exceptions import ImproperlyConfigured from django.shortcuts import redirect from django.urls import reverse from django.utils.translation import gettext as _ from django.views.decorators.http import require_http_methods from django_countries import countries +from openedx_filters.learning.filters import AccountSettingsRenderStarted from common.djangoapps import third_party_auth from common.djangoapps.edxmako.shortcuts import render_to_response from common.djangoapps.student.models import UserProfile @@ -72,6 +74,12 @@ def account_settings(request): return redirect(url) context = account_settings_context(request) + + try: + context = AccountSettingsRenderStarted().run_filter(context=context) + except AccountSettingsRenderStarted.ErrorFilteringContext as exc: + raise ImproperlyConfigured(f'Pipeline configuration error: {exc}') from exc + return render_to_response('student_account/account_settings.html', context) From 98333c84d3e5c1d322ff14bc4223421df37d12e2 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Tue, 24 Jan 2023 14:48:04 -0500 Subject: [PATCH 2/6] fix: add exceptions and test to account settings filter --- .../user_api/accounts/settings_views.py | 10 +- .../user_api/accounts/tests/test_filters.py | 126 ++++++++++++++++++ 2 files changed, 133 insertions(+), 3 deletions(-) create mode 100644 openedx/core/djangoapps/user_api/accounts/tests/test_filters.py diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index 12b74a66b77b..8a818ce8bcf9 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -8,7 +8,7 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required -from django.core.exceptions import ImproperlyConfigured +from django.http import HttpResponseRedirect from django.shortcuts import redirect from django.urls import reverse from django.utils.translation import gettext as _ @@ -76,9 +76,13 @@ def account_settings(request): context = account_settings_context(request) try: + # .. filter_implemented_name: AccountSettingsRenderStarted + # .. filter_type: org.openedx.learning.student.settings.render.started.v1 context = AccountSettingsRenderStarted().run_filter(context=context) - except AccountSettingsRenderStarted.ErrorFilteringContext as exc: - raise ImproperlyConfigured(f'Pipeline configuration error: {exc}') from exc + except AccountSettingsRenderStarted.RedirectToPage as exc: + return HttpResponseRedirect(exc.redirect_to) + except AccountSettingsRenderStarted.RenderCustomResponse as exc: + return exc.response return render_to_response('student_account/account_settings.html', context) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py new file mode 100644 index 000000000000..481a4be8bd8d --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py @@ -0,0 +1,126 @@ +""" +Test that various filters are fired for views in the certificates app. +""" +from django.http import HttpResponse +from django.test import override_settings +from openedx_filters import PipelineStep +from openedx_filters.learning.filters import AccountSettingsRenderStarted +from rest_framework import status +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase + + +class TestRedirectToPageStep(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, context): # pylint: disable=arguments-differ + """Pipeline step that redirects to dashboard before rendering the account settings page.""" + + raise AccountSettingsRenderStarted.RedirectToPage( + "You can't access this page, redirecting to dashboard.", + redirect_to="/dashboard", + ) + + +class TestRenderCustomResponse(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, context): # pylint: disable=arguments-differ + """Pipeline step that returns a custom response when rendering the account settings page.""" + response = HttpResponse("Here's the text of the web page.") + raise AccountSettingsRenderStarted.RenderCustomResponse( + "You can't access this page.", + response=response, + ) + +class TestAccountSettingsRenderPipelineStep(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, context): # pylint: disable=arguments-differ + """Pipeline step that returns a custom response when rendering the account settings page.""" + context += { + 'test': 'test', + } + return context + +class TestAccountSettingsFilters(SharedModuleStoreTestCase): + """ + Tests for the Open edX Filters associated with the account settings proccess. + + This class guarantees that the following filters are triggered during the user's account settings rendering: + + - AccountSettingsRenderStarted + """ + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.student.settings.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestAccountSettingsRenderPipelineStep", + ], + "fail_silently": False, + }, + }, + ) + def test_account_settings_render_pipeline_step(self): + """ + Test whether the account settings filter is triggered before the user's + account settings page is rendered. + + Expected result: + - AccountSettingsRenderStarted is triggered and executes TestAccountSettingsRenderPipelineStep + """ + response = self.client.get('/account/settings') + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.context['test'], 'test') + + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.student.settings.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRenderCustomResponse", + ], + "fail_silently": False, + }, + }, + ) + def test_account_settings_render_custom_response(self): + """ + Test whether the account settings filter is triggered before the user's + account settings page is rendered. + + Expected result: + - AccountSettingsRenderStarted is triggered and executes TestRenderCustomResponse + """ + response = self.client.get('/account/settings') + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.content, b"Here's the text of the web page.") + + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.student.settings.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRedirectToPageStep", + ], + "fail_silently": False, + }, + }, + ) + def test_account_settings_redirect_to_page(self): + """ + Test whether the account settings filter is triggered before the user's + account settings page is rendered. + + Expected result: + - AccountSettingsRenderStarted is triggered and executes TestRedirectToPageStep + """ + response = self.client.get('/account/settings') + self.assertEqual(response.status_code, status.HTTP_302_FOUND) + self.assertEqual(response.url, '/dashboard') From c2523c66ebff060ddd8511c72401279c094dac63 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Tue, 14 Feb 2023 17:43:47 -0500 Subject: [PATCH 3/6] fix: change implementation and add tests --- .../user_api/accounts/settings_views.py | 16 +- .../user_api/accounts/tests/test_filters.py | 145 +++++++++++++++--- 2 files changed, 138 insertions(+), 23 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index 8a818ce8bcf9..506991f78a45 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -75,16 +75,24 @@ def account_settings(request): context = account_settings_context(request) + account_settings_template = 'student_account/account_settings.html' + try: # .. filter_implemented_name: AccountSettingsRenderStarted # .. filter_type: org.openedx.learning.student.settings.render.started.v1 - context = AccountSettingsRenderStarted().run_filter(context=context) + context, account_settings_template = AccountSettingsRenderStarted.run_filter( + context=context, account_settings_template=account_settings_template, + ) + except AccountSettingsRenderStarted.RenderInvalidAccountSettings as exc: + response = render_to_response(exc.account_settings_template, exc.template_context) except AccountSettingsRenderStarted.RedirectToPage as exc: - return HttpResponseRedirect(exc.redirect_to) + response = HttpResponseRedirect(exc.redirect_to or reverse('dashboard')) except AccountSettingsRenderStarted.RenderCustomResponse as exc: - return exc.response + response = exc.response + else: + response = render_to_response(account_settings_template, context) - return render_to_response('student_account/account_settings.html', context) + return response def account_settings_context(request): diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py index 481a4be8bd8d..359020af7c70 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py @@ -3,23 +3,62 @@ """ from django.http import HttpResponse from django.test import override_settings +from django.urls import reverse from openedx_filters import PipelineStep from openedx_filters.learning.filters import AccountSettingsRenderStarted from rest_framework import status from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from openedx.core.djangolib.testing.utils import skip_unless_lms -class TestRedirectToPageStep(PipelineStep): + +class TestRenderInvalidAccountSettings(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, context, template_name): # pylint: disable=arguments-differ + """ + Pipeline step that stops the course about render process. + """ + raise AccountSettingsRenderStarted.RenderInvalidAccountSettings( + "You can't access the account settings page.", + account_settings_template="static_templates/server-error.html", + ) + + +class TestRedirectToPage(PipelineStep): """ Utility class used when getting steps for pipeline. """ - def run_filter(self, context): # pylint: disable=arguments-differ - """Pipeline step that redirects to dashboard before rendering the account settings page.""" + def run_filter(self, context, template_name): # pylint: disable=arguments-differ + """ + Pipeline step that redirects to dashboard before rendering the account settings page. + When raising RedirectToPage, this filter uses a redirect_to field handled by + the course about view that redirects to that URL. + """ raise AccountSettingsRenderStarted.RedirectToPage( "You can't access this page, redirecting to dashboard.", - redirect_to="/dashboard", + redirect_to="/courses", + ) + + +class TestRedirectToDefaultPage(PipelineStep): + """ + Utility class used when getting steps for pipeline. + """ + + def run_filter(self, context, template_name): # pylint: disable=arguments-differ + """ + Pipeline step that redirects to dashboard before rendering the account settings page. + + When raising RedirectToPage, this filter uses a redirect_to field handled by + the course about view that redirects to that URL. + """ + raise AccountSettingsRenderStarted.RedirectToPage( + "You can't access this page, redirecting to dashboard." ) @@ -28,7 +67,7 @@ class TestRenderCustomResponse(PipelineStep): Utility class used when getting steps for pipeline. """ - def run_filter(self, context): # pylint: disable=arguments-differ + def run_filter(self, context, template_name): # pylint: disable=arguments-differ """Pipeline step that returns a custom response when rendering the account settings page.""" response = HttpResponse("Here's the text of the web page.") raise AccountSettingsRenderStarted.RenderCustomResponse( @@ -36,18 +75,23 @@ def run_filter(self, context): # pylint: disable=arguments-differ response=response, ) -class TestAccountSettingsRenderPipelineStep(PipelineStep): + +class TestAccountSettingsRender(PipelineStep): """ Utility class used when getting steps for pipeline. """ - def run_filter(self, context): # pylint: disable=arguments-differ + def run_filter(self, context, template_name): # pylint: disable=arguments-differ """Pipeline step that returns a custom response when rendering the account settings page.""" context += { 'test': 'test', } - return context + return { + 'context': context, template_name: template_name, + } + +@skip_unless_lms class TestAccountSettingsFilters(SharedModuleStoreTestCase): """ Tests for the Open edX Filters associated with the account settings proccess. @@ -56,29 +100,54 @@ class TestAccountSettingsFilters(SharedModuleStoreTestCase): - AccountSettingsRenderStarted """ + def setUp(self): # pylint: disable=arguments-differ + super().setUp() + self.account_settings_url = '/account/settings' @override_settings( OPEN_EDX_FILTERS_CONFIG={ "org.openedx.learning.student.settings.render.started.v1": { "pipeline": [ - "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestAccountSettingsRenderPipelineStep", + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestAccountSettingsRender", ], "fail_silently": False, }, }, ) - def test_account_settings_render_pipeline_step(self): + def test_account_settings_render_filter_executed(self): """ Test whether the account settings filter is triggered before the user's account settings page is rendered. Expected result: - - AccountSettingsRenderStarted is triggered and executes TestAccountSettingsRenderPipelineStep + - AccountSettingsRenderStarted is triggered and executes TestAccountSettingsRender """ - response = self.client.get('/account/settings') + response = self.client.get(self.account_settings_url) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(response.context['test'], 'test') + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.student.settings.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRenderInvalidAccountSettings", # pylint: disable=line-too-long + ], + "fail_silently": False, + }, + }, + PLATFORM_NAME="My site", + ) + def test_account_settings_render_alternative(self): + """ + Test whether the account settings filter is triggered before the user's + account settings page is rendered. + + Expected result: + - AccountSettingsRenderStarted is triggered and executes TestRenderInvalidAccountSettings # pylint: disable=line-too-long + """ + response = self.client.get(self.account_settings_url) + + self.assertContains(response, "There has been a 500 error on the My site servers") @override_settings( OPEN_EDX_FILTERS_CONFIG={ @@ -98,16 +167,15 @@ def test_account_settings_render_custom_response(self): Expected result: - AccountSettingsRenderStarted is triggered and executes TestRenderCustomResponse """ - response = self.client.get('/account/settings') - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.content, b"Here's the text of the web page.") + response = self.client.get(self.account_settings_url) + self.assertEqual(response.content, b"Here's the text of the web page.") @override_settings( OPEN_EDX_FILTERS_CONFIG={ "org.openedx.learning.student.settings.render.started.v1": { "pipeline": [ - "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRedirectToPageStep", + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRedirectToPage", ], "fail_silently": False, }, @@ -119,8 +187,47 @@ def test_account_settings_redirect_to_page(self): account settings page is rendered. Expected result: - - AccountSettingsRenderStarted is triggered and executes TestRedirectToPageStep + - AccountSettingsRenderStarted is triggered and executes TestRedirectToPage """ - response = self.client.get('/account/settings') + response = self.client.get(self.account_settings_url) + self.assertEqual(response.status_code, status.HTTP_302_FOUND) - self.assertEqual(response.url, '/dashboard') + self.assertEqual('/courses', response.url) + + @override_settings( + OPEN_EDX_FILTERS_CONFIG={ + "org.openedx.learning.student.settings.render.started.v1": { + "pipeline": [ + "openedx.core.djangoapps.user_api.accounts.tests.test_filters.TestRedirectToDefaultPage", + ], + "fail_silently": False, + }, + }, + ) + def test_account_settings_redirect_default(self): + """ + Test whether the account settings filter is triggered before the user's + account settings page is rendered. + + Expected result: + - AccountSettingsRenderStarted is triggered and executes TestRedirectToDefaultPage + """ + response = self.client.get(self.account_settings_url) + + self.assertEqual(response.status_code, status.HTTP_302_FOUND) + self.assertEqual(f"{reverse('dashboard')}", response.url) + +@override_settings(OPEN_EDX_FILTERS_CONFIG={}) + def test_account_settings_render_without_filter_config(self): + """ + Test whether the course about filter is triggered before the course about + render without affecting its execution flow. + + Expected result: + - AccountSettingsRenderStarted executes a noop (empty pipeline). Without any + modification comparing it with the effects of TestAccountSettingsRender. + - The view response is HTTP_200_OK. + """ + response = self.client.get(self.course_about_url) + + self.assertNotContains(response, "View About Page in studio", status_code=200) From b056cbfaa22b07640e0d10390868952803a10c80 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Thu, 16 Feb 2023 14:34:12 -0500 Subject: [PATCH 4/6] fix: fix tests --- .../djangoapps/user_api/accounts/settings_views.py | 2 +- .../user_api/accounts/tests/test_filters.py | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index 506991f78a45..002695d4e33f 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -81,7 +81,7 @@ def account_settings(request): # .. filter_implemented_name: AccountSettingsRenderStarted # .. filter_type: org.openedx.learning.student.settings.render.started.v1 context, account_settings_template = AccountSettingsRenderStarted.run_filter( - context=context, account_settings_template=account_settings_template, + context=context, template_name=account_settings_template, ) except AccountSettingsRenderStarted.RenderInvalidAccountSettings as exc: response = render_to_response(exc.account_settings_template, exc.template_context) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py index 359020af7c70..072e5dc51d2a 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py @@ -83,11 +83,9 @@ class TestAccountSettingsRender(PipelineStep): def run_filter(self, context, template_name): # pylint: disable=arguments-differ """Pipeline step that returns a custom response when rendering the account settings page.""" - context += { - 'test': 'test', - } + template_name = 'static_templates/about.html' return { - 'context': context, template_name: template_name, + "context": context, "template_name": template_name, } @@ -124,7 +122,7 @@ def test_account_settings_render_filter_executed(self): """ response = self.client.get(self.account_settings_url) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.context['test'], 'test') + self.assertContains(response, "This page left intentionally blank. Feel free to add your own content.") @override_settings( OPEN_EDX_FILTERS_CONFIG={ @@ -217,7 +215,7 @@ def test_account_settings_redirect_default(self): self.assertEqual(response.status_code, status.HTTP_302_FOUND) self.assertEqual(f"{reverse('dashboard')}", response.url) -@override_settings(OPEN_EDX_FILTERS_CONFIG={}) + @override_settings(OPEN_EDX_FILTERS_CONFIG={}) def test_account_settings_render_without_filter_config(self): """ Test whether the course about filter is triggered before the course about @@ -230,4 +228,4 @@ def test_account_settings_render_without_filter_config(self): """ response = self.client.get(self.course_about_url) - self.assertNotContains(response, "View About Page in studio", status_code=200) + self.assertNotContains(response, "This page left intentionally blank. Feel free to add your own content.") From 4a0b6f287463a99880daa3ebacc88a0f1844557e Mon Sep 17 00:00:00 2001 From: henrrypg Date: Tue, 7 Mar 2023 15:37:30 -0500 Subject: [PATCH 5/6] chore: upgrade openedx-filters --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/testing.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index fdc1833d422d..44017df949c9 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -761,7 +761,7 @@ openedx-events==5.1.0 # via # -r requirements/edx/base.in # edx-event-bus-kafka -openedx-filters==1.1.0 +openedx-filters==1.2.0 # via # -r requirements/edx/base.in # lti-consumer-xblock diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 6e463b93430a..d7556a7435ee 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1021,7 +1021,7 @@ openedx-events==5.1.0 # via # -r requirements/edx/testing.txt # edx-event-bus-kafka -openedx-filters==1.1.0 +openedx-filters==1.2.0 # via # -r requirements/edx/testing.txt # lti-consumer-xblock diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 029d356dc910..d7ecf9e95593 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -969,7 +969,7 @@ openedx-events==5.1.0 # via # -r requirements/edx/base.txt # edx-event-bus-kafka -openedx-filters==1.1.0 +openedx-filters==1.2.0 # via # -r requirements/edx/base.txt # lti-consumer-xblock From 776e65b94d004616d711092146411507d578dfd8 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Wed, 8 Mar 2023 14:26:40 -0500 Subject: [PATCH 6/6] fix: add auth on test filters --- .../user_api/accounts/tests/test_filters.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py index 072e5dc51d2a..782549aea0b5 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_filters.py @@ -10,6 +10,7 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from openedx.core.djangolib.testing.utils import skip_unless_lms +from common.djangoapps.student.tests.factories import UserFactory class TestRenderInvalidAccountSettings(PipelineStep): @@ -100,6 +101,15 @@ class TestAccountSettingsFilters(SharedModuleStoreTestCase): """ def setUp(self): # pylint: disable=arguments-differ super().setUp() + self.user = UserFactory.create( + username="somestudent", + first_name="Student", + last_name="Person", + email="robot@robot.org", + is_active=True, + password="password", + ) + self.client.login(username=self.user.username, password="password") self.account_settings_url = '/account/settings' @override_settings( @@ -226,6 +236,6 @@ def test_account_settings_render_without_filter_config(self): modification comparing it with the effects of TestAccountSettingsRender. - The view response is HTTP_200_OK. """ - response = self.client.get(self.course_about_url) + response = self.client.get(self.account_settings_url) self.assertNotContains(response, "This page left intentionally blank. Feel free to add your own content.")