From 7b1f402199133c30591e67dded93d1ad359bea6b Mon Sep 17 00:00:00 2001 From: Attiya Ishaque Date: Wed, 15 Jun 2022 19:16:33 +0500 Subject: [PATCH] feat: [VAN-953] Update MFE context API (#30516) --- .../core/djangoapps/user_authn/api/helper.py | 19 ++-- .../api/tests/test_optional_fields.py | 106 ------------------ .../user_authn/api/tests/test_views.py | 82 +++++++++++++- .../core/djangoapps/user_authn/api/urls.py | 2 - .../core/djangoapps/user_authn/api/views.py | 70 ++++-------- 5 files changed, 107 insertions(+), 172 deletions(-) delete mode 100644 openedx/core/djangoapps/user_authn/api/tests/test_optional_fields.py diff --git a/openedx/core/djangoapps/user_authn/api/helper.py b/openedx/core/djangoapps/user_authn/api/helper.py index e5f8092f1d..4186d2d716 100644 --- a/openedx/core/djangoapps/user_authn/api/helper.py +++ b/openedx/core/djangoapps/user_authn/api/helper.py @@ -15,8 +15,6 @@ class RegistrationFieldsContext(APIView): """ Registration Fields View used by optional and required fields view. """ - # allow to define custom set of required/optional/hidden fields via configuration - FIELD_TYPE = 'hidden' EXTRA_FIELDS = [ 'confirm_email', @@ -58,8 +56,9 @@ class RegistrationFieldsContext(APIView): return field_order - def __init__(self): + def __init__(self, field_type='required'): super().__init__() + self.field_type = field_type self._fields_setting = copy.deepcopy(configuration_helpers.get_value('REGISTRATION_EXTRA_FIELDS')) if not self._fields_setting: self._fields_setting = copy.deepcopy(settings.REGISTRATION_EXTRA_FIELDS) @@ -70,18 +69,18 @@ class RegistrationFieldsContext(APIView): ordered_extra_fields.remove('year_of_birth') self.valid_fields = [ - field for field in ordered_extra_fields if self._fields_setting.get(field) == self.FIELD_TYPE + field for field in ordered_extra_fields if self._fields_setting.get(field) == self.field_type ] custom_form = get_registration_extension_form() if custom_form: for field_name, field in custom_form.fields.items(): - # If the FIELD_TYPE is required make sure the custom field is required in the form and if the - # FIELD_TYPE is optional only add field if it is not required. This is to make sure field is + # If the field_type is required make sure the custom field is required in the form and if the + # field_type is optional only add field if it is not required. This is to make sure field is # added only once either on Registration Page or Progressive Profiling page. if ( - field.required and self.FIELD_TYPE == 'required' or - not field.required and self.FIELD_TYPE == 'optional' + field.required and self.field_type == 'required' or + not field.required and self.field_type == 'optional' ): self.valid_fields.append(field_name) @@ -95,11 +94,11 @@ class RegistrationFieldsContext(APIView): for field in self.valid_fields: if custom_form and field in custom_form.fields: response[field] = form_fields.add_extension_form_field( - field, custom_form, custom_form.fields[field], self.FIELD_TYPE + field, custom_form, custom_form.fields[field], self.field_type ) else: field_handler = getattr(form_fields, f'add_{field}_field', None) if field_handler: - response[field] = field_handler(self.FIELD_TYPE == 'required') + response[field] = field_handler(self.field_type == 'required') return response diff --git a/openedx/core/djangoapps/user_authn/api/tests/test_optional_fields.py b/openedx/core/djangoapps/user_authn/api/tests/test_optional_fields.py deleted file mode 100644 index 412a2c9364..0000000000 --- a/openedx/core/djangoapps/user_authn/api/tests/test_optional_fields.py +++ /dev/null @@ -1,106 +0,0 @@ -""" -Tests for OptionalFieldsData View -""" -from django.conf import settings -from django.test.utils import override_settings -from django.urls import reverse -from rest_framework import status -from rest_framework.test import APITestCase - -from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration -from openedx.core.djangolib.testing.utils import skip_unless_lms -from common.djangoapps.student.tests.factories import UserFactory - - -@skip_unless_lms -class OptionalFieldsDataViewTest(APITestCase): - """ - Tests for the end-point that returns optional fields. - """ - - def setUp(self): - super().setUp() - - self.user = UserFactory.create(username='test_user', password='password123') - self.client.force_authenticate(user=self.user) - self.url = reverse('optional_fields') - - def test_unauthenticated_request_is_forbidden(self): - """ - Test that unauthenticated user should not be able to access the endpoint. - """ - self.client.logout() - response = self.client.get(self.url) - assert response.status_code == status.HTTP_401_UNAUTHORIZED - - @override_settings(REGISTRATION_EXTRA_FIELDS={"goals": "required", "level_of_education": "required"}) - def test_optional_fields_not_configured(self): - """ - Test that when no optional fields are configured in REGISTRATION_EXTRA_FIELDS - settings, then API returns proper response. - """ - response = self.client.get(self.url) - assert response.status_code == status.HTTP_400_BAD_REQUEST - assert response.data.get('error_code') == 'optional_fields_configured_incorrectly' - - @override_settings(REGISTRATION_EXTRA_FIELDS={"new_field_with_no_description": "optional", "goals": "optional"}) - def test_optional_field_has_no_description(self): - """ - Test that if a new optional field is added to REGISTRATION_EXTRA_FIELDS without - adding field description then that field is omitted from the final response. - """ - expected_response = { - 'goals': { - 'name': 'goals', - 'type': 'textarea', - 'label': "Tell us why you're interested in {platform_name}".format( - platform_name=settings.PLATFORM_NAME - ), - 'error_message': '', - } - } - response = self.client.get(self.url) - assert response.status_code == status.HTTP_200_OK - assert response.data.get('fields') == expected_response - - @with_site_configuration( - configuration={ - 'EXTRA_FIELD_OPTIONS': {'profession': ['Software Engineer', 'Teacher', 'Other']} - } - ) - @override_settings(REGISTRATION_EXTRA_FIELDS={'profession': 'optional', 'specialty': 'optional'}) - def test_configurable_select_option_fields(self): - """ - Test that if optional fields have configurable options present in EXTRA_FIELD_OPTIONS, - they are returned in response as "select" fields otherwise as "text" field. - """ - expected_response = { - 'profession': { - 'name': 'profession', - 'label': 'Profession', - 'error_message': '', - 'type': 'select', - 'options': [('software engineer', 'Software Engineer'), ('teacher', 'Teacher'), ('other', 'Other')], - }, - 'specialty': { - 'name': 'specialty', - 'label': 'Specialty', - 'error_message': '', - 'type': 'text', - } - } - response = self.client.get(self.url) - assert response.status_code == status.HTTP_200_OK - assert response.data.get('fields') == expected_response - - @override_settings( - REGISTRATION_EXTRA_FIELDS={'goals': 'optional', 'specialty': 'optional'}, - REGISTRATION_FIELD_ORDER=['specialty', 'goals'], - ) - def test_field_order(self): - """ - Test that order of fields - """ - response = self.client.get(self.url) - assert response.status_code == status.HTTP_200_OK - assert list(response.data['fields'].keys()) == ['specialty', 'goals'] diff --git a/openedx/core/djangoapps/user_authn/api/tests/test_views.py b/openedx/core/djangoapps/user_authn/api/tests/test_views.py index dd2c24f099..c7aff51071 100644 --- a/openedx/core/djangoapps/user_authn/api/tests/test_views.py +++ b/openedx/core/djangoapps/user_authn/api/tests/test_views.py @@ -16,6 +16,7 @@ from common.djangoapps.student.models import Registration from common.djangoapps.student.tests.factories import UserFactory from common.djangoapps.third_party_auth import pipeline from common.djangoapps.third_party_auth.tests.testutil import ThirdPartyAuthTestMixin, simulate_running_pipeline +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangoapps.geoinfo.api import country_code_from_ip from openedx.core.djangoapps.user_api.tests.test_views import UserAPITestCase from openedx.core.djangolib.testing.utils import skip_unless_lms @@ -34,6 +35,7 @@ class MFEContextViewTest(ThirdPartyAuthTestMixin, APITestCase): """ super().setUp() + self.user = UserFactory.create(username='test_user', password='password123') self.url = reverse('mfe_context') self.query_params = {'next': '/dashboard'} @@ -103,6 +105,7 @@ class MFEContextViewTest(ThirdPartyAuthTestMixin, APITestCase): 'countryCode': self.country_code }, 'registration_fields': {}, + 'optional_fields': {}, } @patch.dict(settings.FEATURES, {'ENABLE_THIRD_PARTY_AUTH': False}) @@ -194,7 +197,8 @@ class MFEContextViewTest(ThirdPartyAuthTestMixin, APITestCase): Test that when no required fields are configured in REGISTRATION_EXTRA_FIELDS settings, then API returns proper response. """ - response = self.client.get(self.url) + self.query_params.update({'is_registered': True}) + response = self.client.get(self.url, self.query_params) assert response.status_code == status.HTTP_200_OK assert response.data['registration_fields']['fields'] == {} @@ -203,13 +207,83 @@ class MFEContextViewTest(ThirdPartyAuthTestMixin, APITestCase): REGISTRATION_EXTRA_FIELDS={'state': 'required', 'last_name': 'required', 'first_name': 'required'}, REGISTRATION_FIELD_ORDER=['first_name', 'last_name', 'state'], ) - def test_field_order(self): + def test_required_field_order(self): """ - Test that order of fields + Test that order of required fields + """ + self.query_params.update({'is_registered': True}) + response = self.client.get(self.url, self.query_params) + assert response.status_code == status.HTTP_200_OK + assert list(response.data['registration_fields']['fields'].keys()) == ['first_name', 'last_name', 'state'] + + @override_settings( + ENABLE_DYNAMIC_REGISTRATION_FIELDS=True, + REGISTRATION_EXTRA_FIELDS={"new_field_with_no_description": "optional", "goals": "optional"} + ) + def test_optional_field_has_no_description(self): + """ + Test that if a new optional field is added to REGISTRATION_EXTRA_FIELDS without + adding field description then that field is omitted from the final response. + """ + expected_response = { + 'goals': { + 'name': 'goals', + 'type': 'textarea', + 'label': "Tell us why you're interested in {platform_name}".format( + platform_name=settings.PLATFORM_NAME + ), + 'error_message': '', + } + } + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + assert response.data['optional_fields']['fields'] == expected_response + + @with_site_configuration( + configuration={ + 'EXTRA_FIELD_OPTIONS': {'profession': ['Software Engineer', 'Teacher', 'Other']} + } + ) + @override_settings( + ENABLE_DYNAMIC_REGISTRATION_FIELDS=True, + REGISTRATION_EXTRA_FIELDS={'profession': 'optional', 'specialty': 'optional'} + ) + def test_configurable_select_option_fields(self): + """ + Test that if optional fields have configurable options present in EXTRA_FIELD_OPTIONS, + they are returned in response as "select" fields otherwise as "text" field. + """ + expected_response = { + 'profession': { + 'name': 'profession', + 'label': 'Profession', + 'error_message': '', + 'type': 'select', + 'options': [('software engineer', 'Software Engineer'), ('teacher', 'Teacher'), ('other', 'Other')], + }, + 'specialty': { + 'name': 'specialty', + 'label': 'Specialty', + 'error_message': '', + 'type': 'text', + } + } + response = self.client.get(self.url) + assert response.status_code == status.HTTP_200_OK + assert response.data['optional_fields']['fields'] == expected_response + + @override_settings( + ENABLE_DYNAMIC_REGISTRATION_FIELDS=True, + REGISTRATION_EXTRA_FIELDS={'goals': 'optional', 'specialty': 'optional'}, + REGISTRATION_FIELD_ORDER=['specialty', 'goals'], + ) + def test_optional_field_order(self): + """ + Test that order of optional fields """ response = self.client.get(self.url) assert response.status_code == status.HTTP_200_OK - assert list(response.data['registration_fields']['fields'].keys()) == ['first_name', 'last_name', 'state'] + assert list(response.data['optional_fields']['fields'].keys()) == ['specialty', 'goals'] @skip_unless_lms diff --git a/openedx/core/djangoapps/user_authn/api/urls.py b/openedx/core/djangoapps/user_authn/api/urls.py index c4ac9dc57c..212249d1c8 100644 --- a/openedx/core/djangoapps/user_authn/api/urls.py +++ b/openedx/core/djangoapps/user_authn/api/urls.py @@ -5,7 +5,6 @@ from django.urls import path from openedx.core.djangoapps.user_authn.api.views import ( MFEContextView, SendAccountActivationEmail, - OptionalFieldsView, ) urlpatterns = [ path('third_party_auth_context', MFEContextView.as_view(), name='third_party_auth_context'), @@ -13,5 +12,4 @@ urlpatterns = [ path('send_account_activation_email', SendAccountActivationEmail.as_view(), name='send_account_activation_email' ), - path('optional_fields', OptionalFieldsView.as_view(), name='optional_fields'), ] diff --git a/openedx/core/djangoapps/user_authn/api/views.py b/openedx/core/djangoapps/user_authn/api/views.py index 6c9176dcca..93f37a1a26 100644 --- a/openedx/core/djangoapps/user_authn/api/views.py +++ b/openedx/core/djangoapps/user_authn/api/views.py @@ -4,12 +4,10 @@ Authn API Views from django.conf import settings -from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from rest_framework import status from rest_framework.response import Response -from rest_framework.throttling import AnonRateThrottle, UserRateThrottle +from rest_framework.throttling import AnonRateThrottle from rest_framework.views import APIView -from rest_framework.authentication import SessionAuthentication from rest_framework.permissions import IsAuthenticated from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser @@ -18,7 +16,6 @@ from common.djangoapps.student.views import compose_and_send_activation_email from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_authn.api.helper import RegistrationFieldsContext from openedx.core.djangoapps.user_authn.views.utils import get_mfe_context -from openedx.core.lib.api.authentication import BearerAuthentication class MFEContextThrottle(AnonRateThrottle): @@ -28,17 +25,18 @@ class MFEContextThrottle(AnonRateThrottle): rate = settings.LOGISTRATION_API_RATELIMIT -class MFEContextView(RegistrationFieldsContext): +class MFEContextView(APIView): """ - API to get third party auth providers, user country code and the currently running pipeline. + API to get third party auth providers, optional fields, required fields, + user country code and the currently running pipeline. """ - FIELD_TYPE = 'required' throttle_classes = [MFEContextThrottle] def get(self, request, **kwargs): # lint-amnesty, pylint: disable=unused-argument """ Returns - dynamic registration fields + - dynamic optional fields - the context for third party auth providers - user country code - the currently running pipeline. @@ -48,22 +46,31 @@ class MFEContextView(RegistrationFieldsContext): is currently running. tpa_hint (string): An override flag that will return a matching provider as long as its configuration has been enabled + is_register_page (boolen): Determine the call is from register or login page """ request_params = request.GET redirect_to = get_next_url_for_login_page(request) third_party_auth_hint = request_params.get('tpa_hint') - + is_register_page = request_params.get('is_registered') context = { 'context_data': get_mfe_context(request, redirect_to, third_party_auth_hint), 'registration_fields': {}, + 'optional_fields': {}, } if settings.ENABLE_DYNAMIC_REGISTRATION_FIELDS: - registration_fields = self._get_fields() - context['registration_fields'].update({ - 'fields': registration_fields, - 'extended_profile': configuration_helpers.get_value('extended_profile_fields', []), - }) + if is_register_page: + registration_fields = RegistrationFieldsContext()._get_fields() # pylint: disable=protected-access + context['registration_fields'].update({ + 'fields': registration_fields, + 'extended_profile': configuration_helpers.get_value('extended_profile_fields', []), + }) + optional_fields = RegistrationFieldsContext('optional')._get_fields() # pylint: disable=protected-access + if optional_fields: + context['optional_fields'].update({ + 'fields': optional_fields, + 'extended_profile': configuration_helpers.get_value('extended_profile_fields', []), + }) return Response( status=status.HTTP_200_OK, @@ -95,40 +102,3 @@ class SendAccountActivationEmail(APIView): return Response( status=status.HTTP_500_INTERNAL_SERVER_ERROR ) - - -class OptionalFieldsThrottle(UserRateThrottle): - """ - Setting rate limit for OptionalFieldsData API - """ - rate = settings.OPTIONAL_FIELD_API_RATELIMIT - - -class OptionalFieldsView(RegistrationFieldsContext): - """ - Construct Registration forms and associated fields. - """ - FIELD_TYPE = 'optional' - - throttle_classes = [OptionalFieldsThrottle] - authentication_classes = (JwtAuthentication, BearerAuthentication, SessionAuthentication,) - permission_classes = (IsAuthenticated,) - - def get(self, request): # lint-amnesty, pylint: disable=unused-argument - """ - Returns Optional fields to be shown on Progressive Profiling page - """ - response = self._get_fields() - if not self.valid_fields or not response: - return Response( - status=status.HTTP_400_BAD_REQUEST, - data={'error_code': f'{self.FIELD_TYPE}_fields_configured_incorrectly'} - ) - - return Response( - status=status.HTTP_200_OK, - data={ - 'fields': response, - 'extended_profile': configuration_helpers.get_value('extended_profile_fields', []), - }, - )