diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 4b8804ca3802..ab0c73e16c31 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -92,7 +92,6 @@ def B(*args, **kwargs): from openedx.core.djangoapps.user_authn import cookies as user_authn_cookies from openedx.core.djangoapps.user_authn.toggles import is_auto_generated_username_enabled from openedx.core.djangoapps.user_authn.utils import is_safe_login_or_logout_redirect -from openedx.core.djangoapps.user_authn.views.utils import get_auto_generated_username from common.djangoapps.third_party_auth.utils import ( get_associated_user_by_email_response, get_user_from_email, @@ -1010,6 +1009,8 @@ def get_username(strategy, details, backend, user=None, *args, **kwargs): # lin slug_func = lambda val: val if is_auto_generated_username_enabled() and details.get('username') is None: + # Lazy import to avoid circular dependency + from openedx.core.djangoapps.user_authn.views.utils import get_auto_generated_username username = get_auto_generated_username(details) else: if email_as_username and details.get('email'): diff --git a/docs/concepts/extension_points.rst b/docs/concepts/extension_points.rst index a8410fc6c4fa..bcb3c14188ef 100644 --- a/docs/concepts/extension_points.rst +++ b/docs/concepts/extension_points.rst @@ -119,9 +119,16 @@ Here are the different integration points that python plugins can use: - The course home page (the landing page for the course) includes a "Course Tools" section that provides links to "tools" associated with the course. Examples of course tool plugins included in the core are reviews, updates, and bookmarks. See |course_tools.py|_ to learn more. This API may be changing soon with the new Courseware microfrontend implementation. - * - Custom registration form app (``REGISTRATION_EXTENSION_FORM`` Django setting in the LMS) + * - Custom profile extension form app (``PROFILE_EXTENSION_FORM`` Django setting in the LMS) - Trial, Stable - - By default, the registration page for each instance of Open edX has fields that ask for information such as a user’s name, country, and highest level of education completed. You can add custom fields to the registration page for your own Open edX instance. These fields can be different types, including text entry fields and drop-down lists. See `Adding Custom Fields to the Registration Page`_. + - By default, the registration page for each instance of Open edX has fields that ask for information such as a user’s name, country, and highest level of education completed. You can add custom fields to the registration page and user profile for your own Open edX instance. These fields can be different types, including text entry fields and drop-down lists. See `Adding Custom Fields to the Registration Page`_. + + **Important Migration Note:** + + - ``REGISTRATION_EXTENSION_FORM`` (deprecated) continues to work with old behavior: custom fields only for registration, data stored in UserProfile.meta + - ``PROFILE_EXTENSION_FORM`` (new) enables new capabilities: custom fields in registration and account settings, data stored in dedicated model + + Sites using the deprecated setting will maintain backward compatibility. To get the new capabilities, migrate to ``PROFILE_EXTENSION_FORM``. * - Learning Context (``openedx.learning_context``) - Trial, Limited - A "Learning Context" is a course, a library, a program, a blog, an external site, or some other collection of content where learning happens. If you are trying to build a totally new learning experience that's not a type of course, you may need to implement a new learning context. Learning contexts are a new abstraction and are only supported in the nascent openedx_content-based XBlock runtime. Since existing courses use modulestore instead of openedx_content, they are not yet implemented as learning contexts. However, openedx_content-based content libraries are. See |learning_context.py|_ to learn more. diff --git a/lms/envs/common.py b/lms/envs/common.py index 917dd025e96f..39cb2904d511 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2554,8 +2554,33 @@ # Note: If you want to use a model to store the results of the form, you will # need to add the model's app to the ADDL_INSTALLED_APPS array in your # lms.yml file. +# +# REGISTRATION_EXTENSION_FORM is deprecated but will continue to work for backward compatibility. +# Sites using this setting will maintain the old behavior: +# - Data is stored in UserProfile.meta JSON field +# - No ability to update extended fields after registration via account settings API +# +# To get new capabilities (model-based storage), migrate to PROFILE_EXTENSION_FORM. +REGISTRATION_EXTENSION_FORM = None # DEPRECATED: Use PROFILE_EXTENSION_FORM instead -REGISTRATION_EXTENSION_FORM = None +# PROFILE_EXTENSION_FORM is a Django ModelForm class used for extending user profiles +# beyond the default fields. This setting enables new capabilities for profile management: +# - Data is stored in a dedicated model (not just UserProfile.meta) +# - Users can update their extended profile fields via the account settings API +# +# This setting supersedes REGISTRATION_EXTENSION_FORM and provides more accurate naming +# for profile extension functionality. +# +# Example: PROFILE_EXTENSION_FORM = 'myapp.forms.ExtendedProfileForm' +# +# The custom form's model should have: +# - A OneToOneField to User (typically named 'user') +# - Additional fields for extended profile data +# +# MIGRATION NOTE: If you're currently using REGISTRATION_EXTENSION_FORM (deprecated), +# your custom fields will continue working as before (data in meta field). +# To get the new capabilities, migrate to PROFILE_EXTENSION_FORM. +PROFILE_EXTENSION_FORM = None # Identifier included in the User Agent from Open edX mobile apps. MOBILE_APP_USER_AGENT_REGEXES = [ diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index c3fd805a3166..33dc39e74f57 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -4,11 +4,15 @@ """ import datetime +import logging import re +from typing import Optional +from django import forms from django.conf import settings from django.core.exceptions import ObjectDoesNotExist from django.core.validators import ValidationError, validate_email +from django.db import transaction from django.utils.translation import gettext as _ from django.utils.translation import override as override_language from eventtracking import tracker @@ -48,6 +52,8 @@ # pylint: disable=import-error from edx_name_affirmation.name_change_validator import NameChangeValidator +logger = logging.getLogger(__name__) + # Public access point for this function. visible_fields = _visible_fields @@ -107,7 +113,7 @@ def get_account_settings(request, usernames=None, configuration=None, view=None) @helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) -def update_account_settings(requesting_user, update, username=None): +def update_account_settings(requesting_user, update, username=None, extended_profile_form=None): """Update user account information. Note: @@ -120,6 +126,7 @@ def update_account_settings(requesting_user, update, username=None): update (dict): The updated account field values. username (str): Optional username specifying which account should be updated. If not specified, `requesting_user.username` is assumed. + extended_profile_form (Optional[forms.Form]): Optional validated extended profile form instance. Raises: errors.UserNotFound: no user with username `username` exists (or `requesting_user.username` if @@ -176,7 +183,7 @@ def update_account_settings(requesting_user, update, username=None): _update_preferences_if_needed(update, requesting_user, user) _notify_language_proficiencies_update_if_needed(update, user, user_profile, old_language_proficiencies) _store_old_name_if_needed(old_name, user_profile, requesting_user) - _update_extended_profile_if_needed(update, user_profile) + _update_extended_profile_if_needed(update, user_profile, extended_profile_form) _update_state_if_needed(update, user_profile) except PreferenceValidationError as err: @@ -346,16 +353,69 @@ def _notify_language_proficiencies_update_if_needed(data, user, user_profile, ol ) -def _update_extended_profile_if_needed(data, user_profile): - if 'extended_profile' in data: - meta = user_profile.get_meta() - new_extended_profile = data['extended_profile'] - for field in new_extended_profile: - field_name = field['field_name'] - new_value = field['field_value'] - meta[field_name] = new_value - user_profile.set_meta(meta) - user_profile.save() +def _update_extended_profile_if_needed( + data: dict, user_profile: UserProfile, extended_profile_form: Optional[forms.Form] +) -> None: + """ + Update the extended profile information if present in the data. + + This function handles two types of extended profile updates: + 1. Updates the user profile meta fields with extended_profile data + 2. Saves the extended profile form data to the extended profile model if a validated form is provided + + Args: + data (dict): Dictionary containing the update data, may include 'extended_profile' key + user_profile (UserProfile): The UserProfile instance to update + extended_profile_form (Optional[forms.Form]): The validated extended profile form + containing extended profile data, or None if no extended profile form is provided + + Note: + If `extended_profile` is present in data, the function will: + - Extract `field_name` and `field_value` pairs from extended_profile list + - Update the `user_profile.meta` dictionary with new values and save the profile + + If `extended_profile_form` is provided and valid, the function will: + - Save the form data to the extended profile model + - Associate the model instance with the user if it's a new instance + + Both the meta update and the extended profile model save (when present) are performed + within a single database transaction. If either operation fails, the transaction is + rolled back so that no partial updates are persisted. The error is logged and an + AccountUpdateError is raised to the caller. + """ + has_extended_profile_data = "extended_profile" in data + has_extended_profile_form = extended_profile_form is not None + + if not has_extended_profile_data and not has_extended_profile_form: + return + + try: + with transaction.atomic(): + if has_extended_profile_data: + meta = user_profile.get_meta() + new_extended_profile = data["extended_profile"] + for field in new_extended_profile: + field_name = field["field_name"] + new_value = field["field_value"] + meta[field_name] = new_value + user_profile.set_meta(meta) + user_profile.save() + + if has_extended_profile_form: + # Use commit=False to create the model instance in memory without saving to DB yet. + # This allows us to set the user field before persisting, which is necessary because: + # 1. The form validates and creates the instance with form data + # 2. For new profiles, the user field isn't in the form data + # 3. We need to assign the user programmatically before the database save + # 4. If we called save() directly, it would fail with integrity errors for new profiles + extended_profile = extended_profile_form.save(commit=False) + if not hasattr(extended_profile, "user") or extended_profile.user is None: + extended_profile.user = user_profile.user + # Now persist the instance with the user field properly set + extended_profile.save() + except Exception as exc: # pylint: disable=broad-exception-caught + logger.error("Error saving extended profile model", exc_info=True) + raise AccountUpdateError("Error saving extended profile model") from exc def _update_state_if_needed(data, user_profile): diff --git a/openedx/core/djangoapps/user_api/accounts/forms.py b/openedx/core/djangoapps/user_api/accounts/forms.py index 771b6a7ccf39..f59fdc69587b 100644 --- a/openedx/core/djangoapps/user_api/accounts/forms.py +++ b/openedx/core/djangoapps/user_api/accounts/forms.py @@ -2,11 +2,21 @@ Django forms for accounts """ +import logging +from typing import Optional, Tuple from django import forms -from django.core.exceptions import ValidationError +from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.utils.translation import gettext as _ +from common.djangoapps.student.models import User from openedx.core.djangoapps.user_api.accounts.utils import handle_retirement_cancellation +from openedx.core.djangoapps.user_authn.views.registration_form import ( + get_extended_profile_model, + get_registration_extension_form, +) + +logger = logging.getLogger(__name__) class RetirementQueueDeletionForm(forms.Form): @@ -35,3 +45,135 @@ def save(self, retirement): raise ValidationError('Retirement is in the wrong state!') handle_retirement_cancellation(retirement) + + +def extract_extended_profile_fields_data(extended_profile: Optional[list[dict]]) -> Tuple[dict, dict]: + """ + Extract extended profile fields data from extended_profile structure. + + Args: + extended_profile (Optional[list[dict]]): List of field data dictionaries with keys + `field_name` and `field_value` + + Returns: + tuple: A tuple containing (extended_profile_fields_data, field_errors) + - extended_profile_fields_data (dict): Extracted custom fields data + - field_errors (dict): Dictionary of validation errors, if any + """ + field_errors = {} + + if not isinstance(extended_profile, list): + field_errors["extended_profile"] = { + "developer_message": "extended_profile must be a list", + "user_message": _("Invalid extended profile format"), + } + return {}, field_errors + + extended_profile_fields_data = {} + + for field_data in extended_profile: + if not isinstance(field_data, dict): + logger.warning("Invalid field_data structure in extended_profile: %s", field_data) + continue + + field_name = field_data.get("field_name") + field_value = field_data.get("field_value") + + if not field_name: + logger.warning("Missing field_name in extended_profile field_data: %s", field_data) + continue + + extended_profile_fields_data[field_name] = field_value + + return extended_profile_fields_data, field_errors + + +def get_extended_profile_form( + extended_profile_fields_data: dict, + user: User, +) -> Tuple[Optional[forms.Form], dict]: + """ + Get and validate an extended profile form instance. + + Args: + extended_profile_fields_data (dict): Extended profile field data to + populate the form user (User): User instance to associate with the + extended profile + + Returns: + tuple: A tuple containing (extended_profile_form, field_errors) + - extended_profile_form (Optional[forms.Form]): The validated form instance, + or None if no extended profile form is configured, creation fails, + or form validation fails. + - field_errors (dict): Dictionary of validation errors, if any + """ + field_errors, kwargs = {}, {} + extended_profile_model = get_extended_profile_model() + + try: + kwargs["instance"] = extended_profile_model.objects.get(user=user) + except AttributeError: + logger.info("No extended profile model configured") + except ObjectDoesNotExist: + logger.info("No existing extended profile found for user %s, creating new instance", user.username) + + try: + extended_profile_form = get_registration_extension_form(data=extended_profile_fields_data, **kwargs) + except Exception as e: # pylint: disable=broad-exception-caught + logger.error("Unexpected error creating custom form for user %s: %s", user.username, str(e)) + field_errors["extended_profile"] = { + "developer_message": f"Error creating custom form: {str(e)}", + "user_message": _("There was an error processing the extended profile information"), + } + return None, field_errors + + if extended_profile_form is None: + return None, field_errors + + if not extended_profile_form.is_valid(): + logger.info("Extended profile form validation failed with errors: %s", extended_profile_form.errors) + + for field_name, field_errors_list in extended_profile_form.errors.items(): + first_error = field_errors_list[0] if field_errors_list else "Unknown error" + field_errors[field_name] = { + "developer_message": f"Error in extended profile field [{field_name}]: {first_error}", + "user_message": str(first_error), + } + + return None, field_errors + + return extended_profile_form, field_errors + + +def validate_and_get_extended_profile_form( + extended_profile_data: list, user: User +) -> Tuple[Optional[forms.Form], dict]: + """ + Validate and return an extended profile form instance. + + This function orchestrates the extraction and validation of extended profile data. + + Args: + extended_profile_data (list): The raw extended_profile data from the API request + user (User): The user instance for whom the extended profile is being validated + + Returns: + tuple: A tuple containing (validated_form, field_errors) + - validated_form (Optional[forms.Form]): The validated form instance, or None if + validation fails or no extended profile is configured + - field_errors (dict): Dictionary of validation errors, if any + """ + extended_profile_fields_data, field_errors = extract_extended_profile_fields_data(extended_profile_data) + + if field_errors: + return None, field_errors + + if not extended_profile_fields_data: + return None, {} + + extended_profile_form, form_errors = get_extended_profile_form(extended_profile_fields_data, user) + + if form_errors: + field_errors.update(form_errors) + + return extended_profile_form, field_errors diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py index 87db1e65de2d..5af680f1681e 100644 --- a/openedx/core/djangoapps/user_api/accounts/serializers.py +++ b/openedx/core/djangoapps/user_api/accounts/serializers.py @@ -10,6 +10,7 @@ from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import ObjectDoesNotExist +from django.forms.models import model_to_dict from django.urls import reverse from rest_framework import serializers @@ -26,7 +27,9 @@ from openedx.core.djangoapps.user_api.accounts.utils import is_secondary_email_feature_enabled from openedx.core.djangoapps.user_api.models import RetirementState, UserPreference, UserRetirementStatus from openedx.core.djangoapps.user_api.serializers import ReadOnlyFieldsSerializerMixin -from openedx.core.djangoapps.user_authn.views.registration_form import contains_html, contains_url +from openedx.core.djangoapps.user_authn.views.registration_form import ( + contains_html, contains_url, get_extended_profile_model +) from openedx.features.name_affirmation_api.utils import get_name_affirmation_service from . import ( @@ -573,26 +576,52 @@ def validate_new_name(self, new_name): raise serializers.ValidationError('Name cannot contain a URL') -def get_extended_profile(user_profile): +def get_extended_profile(user_profile: UserProfile) -> list[dict[str, str]]: """ - Returns the extended user profile fields stored in user_profile.meta + Retrieve extended user profile fields for API serialization. + + This function extracts custom profile fields that extend beyond the standard + UserProfile model. It prefers data from a custom extended profile model + (when configured), and only uses the `user_profile.meta` JSON field when + no such model is configured. The returned data is filtered to include only + fields specified in the `extended_profile_fields` site configuration. + + The function supports two data sources: + 1. Custom model: If the `PROFILE_EXTENSION_FORM` setting points to a form with a + `Meta.model`, data is retrieved from that model using `model_to_dict()`. If a + model is configured but the user does not yet have a corresponding record, + this function returns an empty mapping for extended profile fields (it does + not fall back to `user_profile.meta` in that case). + 2. Fallback: JSON data stored in `UserProfile.meta` field, used only when no + custom extended profile model is configured. + + Args: + user_profile (UserProfile): The user profile instance to get extended fields from. + + Returns: + list[dict[str, str]]: A list of dictionaries, each containing: + - field_name: The name of the extended profile field + - field_value: The value of the field (converted to string) """ - # pick the keys from the site configuration - extended_profile_field_names = configuration_helpers.get_value('extended_profile_fields', []) + def get_extended_profile_data(): + extended_profile_model = get_extended_profile_model() - try: - extended_profile_fields_data = json.loads(user_profile.meta) - except ValueError: - extended_profile_fields_data = {} + if extended_profile_model: + try: + profile_obj = extended_profile_model.objects.get(user=user_profile.user) + return model_to_dict(profile_obj) + except extended_profile_model.DoesNotExist: + return {} - extended_profile = [] - for field_name in extended_profile_field_names: - extended_profile.append({ - "field_name": field_name, - "field_value": extended_profile_fields_data.get(field_name, "") - }) - return extended_profile + try: + return json.loads(user_profile.meta or "{}") + except (ValueError, TypeError, AttributeError): + return {} + + data = get_extended_profile_data() + field_names = configuration_helpers.get_value("extended_profile_fields", []) + return [{"field_name": name, "field_value": data.get(name, "")} for name in field_names] def get_profile_visibility(user_profile, user, configuration): diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 11b2f800f996..790fb31f3e30 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -597,6 +597,108 @@ def test_get_name_validation_error_too_long(self): result = get_name_validation_error("A" * 256) assert result == "Full name can't be longer than 255 symbols" + def test_update_extended_profile_with_meta_only(self): + """ + Test updating extended profile using only the meta field (legacy behavior) + """ + update_data = { + "extended_profile": [ + {"field_name": "department", "field_value": "Engineering"}, + {"field_name": "title", "field_value": "Software Engineer"}, + ], + "bio": "Updated bio", + } + + update_account_settings(self.user, update_data) + + user_profile = UserProfile.objects.get(user=self.user) + meta = user_profile.get_meta() + self.assertEqual(meta["department"], "Engineering") + self.assertEqual(meta["title"], "Software Engineer") + self.assertEqual(user_profile.bio, "Updated bio") + + def test_update_extended_profile_with_form(self): + """ + Test updating extended profile with a validated form + """ + mock_form = Mock() + mock_instance = Mock() + mock_instance.user = self.user + mock_form.save.return_value = mock_instance + update_data = {"extended_profile": [{"field_name": "department", "field_value": "Engineering"}]} + + update_account_settings(self.user, update_data, extended_profile_form=mock_form) + + mock_form.save.assert_called_once_with(commit=False) + mock_instance.save.assert_called_once() + user_profile = UserProfile.objects.get(user=self.user) + meta = user_profile.get_meta() + self.assertEqual(meta["department"], "Engineering") + + def test_update_extended_profile_with_form_new_instance(self): + """ + Test updating extended profile with a form for a new instance + """ + mock_form = Mock() + mock_instance = Mock() + mock_instance.user = None + mock_form.save.return_value = mock_instance + update_data = {"extended_profile": [{"field_name": "department", "field_value": "Engineering"}]} + + update_account_settings(self.user, update_data, extended_profile_form=mock_form) + + self.assertEqual(mock_instance.user, self.user) + mock_form.save.assert_called_once_with(commit=False) + mock_instance.save.assert_called_once() + + @patch("openedx.core.djangoapps.user_api.accounts.api.logger") + def test_update_extended_profile_form_save_error(self, mock_logger): + """ + Test that errors during form save are logged, cause an AccountUpdateError, and do not leave partial updates + """ + mock_form = Mock() + mock_form.save.side_effect = Exception("Database error") + update_data = {"extended_profile": [{"field_name": "department", "field_value": "Engineering"}]} + + with pytest.raises(AccountUpdateError) as context_manager: + update_account_settings(self.user, update_data, extended_profile_form=mock_form) + + mock_logger.error.assert_called_once() + self.assertIn("Error saving extended profile model", str(mock_logger.error.call_args)) + self.assertIn("Error saving extended profile model", context_manager.value.developer_message) + user_profile = UserProfile.objects.get(user=self.user) + meta = user_profile.get_meta() + # The meta update should also be rolled back, so no extended profile data is persisted. + self.assertNotIn("department", meta) + + def test_update_extended_profile_without_extended_profile_data(self): + """ + Test that update_account_settings works when no extended_profile is provided + """ + update_data = {"bio": "Updated bio"} + + update_account_settings(self.user, update_data) + + user_profile = UserProfile.objects.get(user=self.user) + self.assertEqual(user_profile.bio, "Updated bio") + + def test_update_extended_profile_form_without_data_in_update(self): + """ + Test that form is saved even if extended_profile is not in update data + """ + mock_form = Mock() + mock_instance = Mock() + mock_instance.user = self.user + mock_form.save.return_value = mock_instance + update_data = {"bio": "Updated bio"} + + update_account_settings(self.user, update_data, extended_profile_form=mock_form) + + mock_form.save.assert_called_once_with(commit=False) + mock_instance.save.assert_called_once() + user_profile = UserProfile.objects.get(user=self.user) + self.assertEqual(user_profile.bio, "Updated bio") + @patch('openedx.core.djangoapps.user_api.accounts.image_helpers._PROFILE_IMAGE_SIZES', [50, 10]) @patch.dict( diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_forms.py b/openedx/core/djangoapps/user_api/accounts/tests/test_forms.py new file mode 100644 index 000000000000..c4dd80752642 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_forms.py @@ -0,0 +1,319 @@ +""" +Unit tests for forms in the accounts API +""" + +from unittest.mock import Mock, patch + +from django.core.exceptions import ObjectDoesNotExist +from django.test import TestCase + +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.user_api.accounts.forms import ( + extract_extended_profile_fields_data, + get_extended_profile_form, + validate_and_get_extended_profile_form, +) + + +class TestExtractExtendedProfileFieldsData(TestCase): + """ + Tests for extract_extended_profile_fields_data function + """ + + def test_extract_valid_extended_profile_data(self): + """ + Test extraction of valid extended profile data + """ + extended_profile = [ + {"field_name": "department", "field_value": "Engineering"}, + {"field_name": "title", "field_value": "Software Engineer"}, + ] + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + self.assertEqual(errors, {}) + self.assertEqual(extracted_data, {"department": "Engineering", "title": "Software Engineer"}) + + def test_extract_extended_profile_with_empty_string(self): + """ + Test that empty strings are included + """ + extended_profile = [ + {"field_name": "department", "field_value": ""}, + {"field_name": "title", "field_value": "Engineer"}, + ] + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + self.assertEqual(errors, {}) + self.assertEqual(extracted_data, {"department": "", "title": "Engineer"}) + + def test_extract_extended_profile_not_a_list(self): + """ + Test error when extended_profile is not a list + """ + extended_profile = "not a list" + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + self.assertEqual(extracted_data, {}) + self.assertIn("extended_profile", errors) + self.assertEqual(errors["extended_profile"]["developer_message"], "extended_profile must be a list") + + def test_extract_extended_profile_with_invalid_field_data(self): + """ + Test that invalid field data entries are skipped (logged but not errored) + """ + extended_profile = [ + {"field_name": "department", "field_value": "Engineering"}, + "invalid entry", # Not a dict + {"field_name": "title", "field_value": "Engineer"}, + ] + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + # Invalid entry should be skipped, but valid ones should be extracted + self.assertEqual(errors, {}) + self.assertEqual(extracted_data, {"department": "Engineering", "title": "Engineer"}) + + def test_extract_extended_profile_missing_field_name(self): + """ + Test that entries without field_name are skipped + """ + extended_profile = [ + {"field_name": "department", "field_value": "Engineering"}, + {"field_value": "Engineer"}, # Missing field_name + ] + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + self.assertEqual(errors, {}) + self.assertEqual(extracted_data, {"department": "Engineering"}) + + def test_extract_extended_profile_empty_list(self): + """ + Test that an empty list returns empty data + """ + extended_profile = [] + + extracted_data, errors = extract_extended_profile_fields_data(extended_profile) + + self.assertEqual(errors, {}) + self.assertEqual(extracted_data, {}) + + +class TestGetExtendedProfileForm(TestCase): + """ + Tests for get_extended_profile_form function + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model") + def test_get_extended_profile_form_no_model_configured(self, mock_get_model: Mock): + """ + Test when no extended profile model is configured + """ + mock_get_model.return_value = None + extended_profile_fields_data = {"department": "Engineering"} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) + self.assertEqual(errors, {}) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model") + def test_get_extended_profile_form_model_has_no_objects(self, mock_get_model: Mock): + """ + Test when model doesn't have objects attribute (AttributeError) + """ + mock_get_model.return_value = None + extended_profile_fields_data = {"department": "Engineering"} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) + self.assertEqual(errors, {}) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_registration_extension_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model") + def test_get_extended_profile_form_with_existing_instance(self, mock_get_model: Mock, mock_get_form: Mock): + """ + Test form creation with an existing profile instance + """ + mock_model = Mock() + mock_instance = Mock() + mock_model.objects.get.return_value = mock_instance + mock_get_model.return_value = mock_model + mock_form_instance = Mock() + mock_form_instance.is_valid.return_value = True + mock_get_form.return_value = mock_form_instance + extended_profile_fields_data = {"department": "Engineering"} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertEqual(form, mock_form_instance) + self.assertEqual(errors, {}) + mock_model.objects.get.assert_called_once_with(user=self.user) + mock_get_form.assert_called_once_with(data=extended_profile_fields_data, instance=mock_instance) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_registration_extension_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model") + def test_get_extended_profile_form_without_existing_instance(self, mock_get_model: Mock, mock_get_form: Mock): + """ + Test form creation for a new profile (no existing instance) + """ + mock_model = Mock() + mock_model.DoesNotExist = ObjectDoesNotExist + mock_model.objects.get.side_effect = ObjectDoesNotExist("Profile not found") + mock_get_model.return_value = mock_model + mock_form_instance = Mock() + mock_form_instance.is_valid.return_value = True + mock_get_form.return_value = mock_form_instance + extended_profile_fields_data = {"department": "Engineering"} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertEqual(form, mock_form_instance) + self.assertEqual(errors, {}) + mock_model.objects.get.assert_called_once_with(user=self.user) + mock_get_form.assert_called_once_with(data=extended_profile_fields_data) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_registration_extension_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model") + def test_get_extended_profile_form_validation_errors(self, _: Mock, mock_get_form: Mock): + """ + Test when form validation fails + """ + mock_form_instance = Mock() + mock_form_instance.is_valid.return_value = False + mock_form_instance.errors = {"department": ["This field is required"], "title": ["Invalid value"]} + mock_get_form.return_value = mock_form_instance + extended_profile_fields_data = {} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) + self.assertIn("department", errors) + self.assertIn("title", errors) + self.assertEqual(errors["department"]["user_message"], "This field is required") + self.assertEqual(errors["title"]["user_message"], "Invalid value") + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_registration_extension_form") + def test_get_extended_profile_form_returns_none(self, mock_get_form: Mock): + """ + Test when get_registration_extension_form returns None + """ + mock_get_form.return_value = None + extended_profile_fields_data = {"department": "Engineering"} + + with patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model"): + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) + self.assertEqual(errors, {}) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_registration_extension_form") + def test_get_extended_profile_form_exception_during_creation(self, mock_get_form: Mock): + """ + Test when an unexpected exception occurs during form creation + """ + mock_get_form.side_effect = Exception("Unexpected error") + extended_profile_fields_data = {"department": "Engineering"} + + with patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_model"): + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) + self.assertIn("extended_profile", errors) + self.assertIn("Error creating custom form", errors["extended_profile"]["developer_message"]) + + +class TestValidateAndGetExtendedProfileForm(TestCase): + """ + Tests for validate_and_get_extended_profile_form function + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.extract_extended_profile_fields_data") + def test_validate_with_valid_data(self, mock_extract: Mock, mock_get_form: Mock): + """ + Test successful validation with valid data + """ + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + mock_extract.return_value = ({"department": "Engineering"}, {}) + mock_form = Mock() + mock_get_form.return_value = (mock_form, {}) + + form, errors = validate_and_get_extended_profile_form(extended_profile_data, self.user) + + self.assertEqual(form, mock_form) + self.assertEqual(errors, {}) + mock_extract.assert_called_once_with(extended_profile_data) + mock_get_form.assert_called_once_with({"department": "Engineering"}, self.user) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.extract_extended_profile_fields_data") + def test_validate_with_extraction_errors(self, mock_extract: Mock): + """ + Test when extraction fails + """ + extended_profile_data = "invalid data" + mock_extract.return_value = ({}, {"extended_profile": {"developer_message": "Invalid format"}}) + + form, errors = validate_and_get_extended_profile_form(extended_profile_data, self.user) + + self.assertIsNone(form) + self.assertIn("extended_profile", errors) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.extract_extended_profile_fields_data") + def test_validate_with_empty_data(self, mock_extract: Mock): + """ + Test when extracted data is empty + """ + extended_profile_data = [] + mock_extract.return_value = ({}, {}) + + form, errors = validate_and_get_extended_profile_form(extended_profile_data, self.user) + + self.assertIsNone(form) + self.assertEqual(errors, {}) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.extract_extended_profile_fields_data") + def test_validate_with_form_errors(self, mock_extract: Mock, mock_get_form: Mock): + """ + Test when form validation fails + """ + extended_profile_data = [{"field_name": "department", "field_value": ""}] + mock_extract.return_value = ({"department": ""}, {}) + mock_form = Mock() + form_errors = {"department": {"developer_message": "Required field"}} + mock_get_form.return_value = (mock_form, form_errors) + + form, errors = validate_and_get_extended_profile_form(extended_profile_data, self.user) + + self.assertEqual(form, mock_form) + self.assertIn("department", errors) + + @patch("openedx.core.djangoapps.user_api.accounts.forms.get_extended_profile_form") + @patch("openedx.core.djangoapps.user_api.accounts.forms.extract_extended_profile_fields_data") + def test_validate_merges_errors(self, mock_extract: Mock, mock_get_form: Mock): + """ + Test that extraction and form errors are merged + """ + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + mock_extract.return_value = ({"department": "Engineering"}, {}) + mock_form = Mock() + form_errors = {"title": {"developer_message": "Required field"}} + mock_get_form.return_value = (mock_form, form_errors) + + form, errors = validate_and_get_extended_profile_form(extended_profile_data, self.user) + + self.assertEqual(form, mock_form) + self.assertIn("title", errors) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_serializers.py b/openedx/core/djangoapps/user_api/accounts/tests/test_serializers.py index 34b4cb0b0e0a..cd57d8e6b1fc 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_serializers.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_serializers.py @@ -2,16 +2,17 @@ Test cases to cover Accounts-related serializers of the User API application """ - import logging +from unittest.mock import Mock, patch from django.test import TestCase from django.test.client import RequestFactory +from django.test.utils import override_settings from testfixtures import LogCapture -from openedx.core.djangoapps.user_api.accounts.serializers import UserReadOnlySerializer from common.djangoapps.student.models import UserProfile from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.user_api.accounts.serializers import UserReadOnlySerializer, get_extended_profile LOGGER_NAME = "openedx.core.djangoapps.user_api.accounts.serializers" @@ -52,3 +53,148 @@ def test_user_no_profile(self): assert data['username'] == self.user.username assert data['name'] is None + + +class GetExtendedProfileTest(TestCase): + """ + Tests for get_extended_profile function + """ + + def setUp(self): + super().setUp() + self.user = UserFactory.create() + self.user_profile = UserProfile.objects.get(user=self.user) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_from_model(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test getting extended profile from a custom model + """ + mock_config_helpers.get_value.return_value = ["department", "title", "company"] + mock_model = Mock() + mock_instance = Mock() + mock_instance.department = "Engineering" + mock_instance.title = "Software Engineer" + mock_instance.company = "EdX" + mock_instance.user = self.user + mock_model.objects.get.return_value = mock_instance + mock_get_model.return_value = mock_model + + with patch("openedx.core.djangoapps.user_api.accounts.serializers.model_to_dict") as mock_model_to_dict: + mock_model_to_dict.return_value = { + "department": "Engineering", + "title": "Software Engineer", + "company": "EdX", + "user": self.user.id, + } + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 3) + self.assertIn({"field_name": "department", "field_value": "Engineering"}, result) + self.assertIn({"field_name": "title", "field_value": "Software Engineer"}, result) + self.assertIn({"field_name": "company", "field_value": "EdX"}, result) + + @override_settings(REGISTRATION_EXTENSION_FORM=None) + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + def test_get_extended_profile_model_does_not_exist(self, mock_config_helpers: Mock): + """ + Test fallback to meta field when model instance doesn't exist + """ + mock_config_helpers.get_value.return_value = ["department", "title"] + self.user_profile.set_meta({"department": "Sales", "title": "Manager"}) + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 2) + self.assertIn({"field_name": "department", "field_value": "Sales"}, result) + self.assertIn({"field_name": "title", "field_value": "Manager"}, result) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_no_model_configured(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test fallback to meta field when no model is configured + """ + mock_config_helpers.get_value.return_value = ["department", "title"] + mock_get_model.return_value = None + meta_data = {"department": "Marketing", "title": "Director"} + self.user_profile.set_meta(meta_data) + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 2) + self.assertIn({"field_name": "department", "field_value": "Marketing"}, result) + self.assertIn({"field_name": "title", "field_value": "Director"}, result) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_empty_meta(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test getting extended profile with empty meta field + """ + mock_config_helpers.get_value.return_value = ["department", "title"] + mock_get_model.return_value = None + self.user_profile.meta = "" + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 2) + self.assertIn({"field_name": "department", "field_value": ""}, result) + self.assertIn({"field_name": "title", "field_value": ""}, result) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_invalid_json_in_meta(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test getting extended profile with invalid JSON in meta field + """ + mock_config_helpers.get_value.return_value = ["department", "title"] + mock_get_model.return_value = None + self.user_profile.meta = "invalid json {" + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 2) + self.assertIn({"field_name": "department", "field_value": ""}, result) + self.assertIn({"field_name": "title", "field_value": ""}, result) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_missing_fields(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test getting extended profile when some configured fields are missing + """ + mock_config_helpers.get_value.return_value = ["department", "title", "location"] + mock_get_model.return_value = None + meta_data = {"department": "HR", "title": "Recruiter"} + self.user_profile.set_meta(meta_data) + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 3) + self.assertIn({"field_name": "department", "field_value": "HR"}, result) + self.assertIn({"field_name": "title", "field_value": "Recruiter"}, result) + self.assertIn({"field_name": "location", "field_value": ""}, result) + + @patch("openedx.core.djangoapps.user_api.accounts.serializers.configuration_helpers") + @patch("openedx.core.djangoapps.user_api.accounts.serializers.get_extended_profile_model") + def test_get_extended_profile_no_configured_fields(self, mock_get_model: Mock, mock_config_helpers: Mock): + """ + Test getting extended profile when no fields are configured + """ + mock_config_helpers.get_value.return_value = [] + mock_get_model.return_value = None + meta_data = {"department": "Finance", "title": "Analyst"} + self.user_profile.set_meta(meta_data) + self.user_profile.save() + + result = get_extended_profile(self.user_profile) + + self.assertEqual(len(result), 0) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 45b1773e4f02..0bcd8adfd83e 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -1372,6 +1372,130 @@ def test_update_account_settings_rollback(self, mock_email_change): assert 'm' == data['gender'] +@skip_unless_lms +class TestAccountsAPIExtendedProfile(UserAPITestCase): + """ + Tests for extended profile validation in the Accounts API + """ + + def setUp(self): + super().setUp() + self.url = reverse("accounts_api", kwargs={"username": self.user.username}) + self.client.login(username=self.user.username, password=TEST_PASSWORD) + + @mock.patch("openedx.core.djangoapps.user_api.accounts.views.validate_and_get_extended_profile_form") + def test_patch_account_with_valid_extended_profile(self, mock_validate_form: mock.Mock): + """ + Test that PATCH with valid extended_profile data succeeds + """ + mock_form = mock.Mock() + mock_validate_form.return_value = (mock_form, {}) + extended_profile_data = [ + {"field_name": "department", "field_value": "Engineering"}, + {"field_name": "title", "field_value": "Software Engineer"}, + ] + json_data = {"extended_profile": extended_profile_data, "bio": "Test bio"} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 200) + mock_validate_form.assert_called_once_with(extended_profile_data, self.user) + + @mock.patch("openedx.core.djangoapps.user_api.accounts.views.validate_and_get_extended_profile_form") + def test_patch_account_with_invalid_extended_profile(self, mock_validate_form: mock.Mock): + """ + Test that PATCH with invalid extended_profile data returns 400 + """ + field_errors = { + "department": {"developer_message": "This field is required", "user_message": "This field is required"} + } + mock_validate_form.return_value = (None, field_errors) + extended_profile_data = [{"field_name": "department", "field_value": ""}] + json_data = {"extended_profile": extended_profile_data} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 400) + self.assertIn("field_errors", response.data) + self.assertIn("department", response.data["field_errors"]) + mock_validate_form.assert_called_once_with(extended_profile_data, self.user) + + def test_patch_account_without_extended_profile(self: UserAPITestCase): + """ + Test that PATCH without extended_profile data works normally + """ + json_data = {"bio": "Test bio without extended profile"} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data["bio"], "Test bio without extended profile") + + @mock.patch("openedx.core.djangoapps.user_api.accounts.views.validate_and_get_extended_profile_form") + def test_patch_account_extended_profile_with_empty_list(self, mock_validate_form: mock.Mock): + """ + Test that PATCH with empty extended_profile list works + """ + mock_validate_form.return_value = (None, {}) + json_data = {"extended_profile": [], "bio": "Test bio"} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 200) + mock_validate_form.assert_called_once_with([], self.user) + + @mock.patch("openedx.core.djangoapps.user_api.accounts.views.validate_and_get_extended_profile_form") + def test_patch_account_extended_profile_form_exception(self, mock_validate_form: mock.Mock): + """ + Test that exceptions in form validation return appropriate errors + """ + field_errors = { + "extended_profile": {"developer_message": "Unexpected error", "user_message": "An error occurred"} + } + mock_validate_form.return_value = (None, field_errors) + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + json_data = {"extended_profile": extended_profile_data} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 400) + self.assertIn("field_errors", response.data) + + @mock.patch("openedx.core.djangoapps.user_api.accounts.views.validate_and_get_extended_profile_form") + def test_patch_account_extended_profile_multiple_fields(self, mock_validate_form: mock.Mock): + """ + Test PATCH with multiple extended_profile fields + """ + mock_form = mock.Mock() + mock_validate_form.return_value = (mock_form, {}) + extended_profile_data = [ + {"field_name": "department", "field_value": "Engineering"}, + {"field_name": "title", "field_value": "Software Engineer"}, + {"field_name": "company", "field_value": "EdX"}, + {"field_name": "location", "field_value": "Remote"}, + ] + json_data = {"extended_profile": extended_profile_data} + + response = self.client.patch(self.url, data=json.dumps(json_data), content_type="application/merge-patch+json") + + self.assertEqual(response.status_code, 200) + mock_validate_form.assert_called_once_with(extended_profile_data, self.user) + + def test_patch_account_extended_profile_unauthorized(self): + """ + Test that unauthorized users cannot update extended_profile + """ + self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + json_data = {"extended_profile": extended_profile_data} + + response = self.different_client.patch( + self.url, data=json.dumps(json_data), content_type="application/merge-patch+json" + ) + + self.assertIn(response.status_code, [403, 404]) + + @ddt.ddt class NameChangeViewTests(UserAPITestCase): """ NameChangeView tests """ diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index c3ff6ce7a2f2..de3844e28fc3 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -81,6 +81,7 @@ UserRetirementStatus, ) from .api import get_account_settings, update_account_settings +from .forms import validate_and_get_extended_profile_form from .permissions import ( CanCancelUserRetirement, CanDeactivateUser, @@ -408,9 +409,17 @@ def partial_update(self, request, username): ): return Response({"error": "Too many requests"}, status=status.HTTP_429_TOO_MANY_REQUESTS) + extended_profile_form = None + if "extended_profile" in request.data: + extended_profile_form, form_errors = validate_and_get_extended_profile_form( + request.data["extended_profile"], request.user + ) + if form_errors: + return Response({"field_errors": form_errors}, status=status.HTTP_400_BAD_REQUEST) + try: with transaction.atomic(): - update_account_settings(request.user, request.data, username=username) + update_account_settings(request.user, request.data, username, extended_profile_form) account_settings = get_account_settings(request, [username])[0] except UserNotAuthorized: return Response(status=status.HTTP_403_FORBIDDEN) diff --git a/openedx/core/djangoapps/user_authn/api/helper.py b/openedx/core/djangoapps/user_authn/api/helper.py index 21bd7c278d70..83bb4dc4ab87 100644 --- a/openedx/core/djangoapps/user_authn/api/helper.py +++ b/openedx/core/djangoapps/user_authn/api/helper.py @@ -100,7 +100,8 @@ def get_fields(self): """ Returns the required or optional fields configured in REGISTRATION_EXTRA_FIELDS settings. """ - # Custom form fields can be added via the form set in settings.REGISTRATION_EXTENSION_FORM + # Custom form fields can be added via the form set in settings.PROFILE_EXTENSION_FORM + # (or deprecated settings.REGISTRATION_EXTENSION_FORM) custom_form = get_registration_extension_form() or {} response = {} for field in self.valid_fields: diff --git a/openedx/core/djangoapps/user_authn/views/registration_form.py b/openedx/core/djangoapps/user_authn/views/registration_form.py index 338c6889fe72..c8104bbc3f55 100644 --- a/openedx/core/djangoapps/user_authn/views/registration_form.py +++ b/openedx/core/djangoapps/user_authn/views/registration_form.py @@ -16,6 +16,8 @@ from django.urls import reverse from django.utils.translation import gettext as _ from django_countries import countries +from django.db.models import Model +from typing import Optional, Type from eventtracking import tracker from common.djangoapps import third_party_auth @@ -317,17 +319,96 @@ def clean_country(self): return self.cleaned_data.get("country") -def get_registration_extension_form(*args, **kwargs): +def get_registration_extension_form(*args, **kwargs) -> Optional[forms.Form]: """ - Convenience function for getting the custom form set in settings.REGISTRATION_EXTENSION_FORM. + Convenience function for getting the custom form set in settings.PROFILE_EXTENSION_FORM + or settings.REGISTRATION_EXTENSION_FORM (deprecated). + + Returns an instance of the configured profile extension form. + + The function first checks for PROFILE_EXTENSION_FORM (recommended), then falls back to + REGISTRATION_EXTENSION_FORM for backwards compatibility. When REGISTRATION_EXTENSION_FORM + is used, a deprecation warning is logged. An example form app for this can be found at http://github.com/open-craft/custom-form-app + + Returns: + Form instance or None if no form is configured """ - if not getattr(settings, 'REGISTRATION_EXTENSION_FORM', None): + # Check for the new setting first + setting_value = getattr(settings, "PROFILE_EXTENSION_FORM", None) + setting_name = "PROFILE_EXTENSION_FORM" + + # Fall back to the deprecated setting + if not setting_value: + setting_value = getattr(settings, "REGISTRATION_EXTENSION_FORM", None) + if setting_value: + setting_name = "REGISTRATION_EXTENSION_FORM" + log.warning( + "REGISTRATION_EXTENSION_FORM is deprecated and will be removed in a future release. " + "Please use PROFILE_EXTENSION_FORM instead. Current value: %s", + setting_value, + ) + + if not setting_value: + return None + + try: + module, klass = setting_value.rsplit(".", 1) + module = import_module(module) + return getattr(module, klass)(*args, **kwargs) + except (ValueError, ImportError, AttributeError) as e: + log.error("Could not load form from %s='%s': %s", setting_name, setting_value, str(e)) + return None + + +def get_extended_profile_model() -> Optional[Type[Model]]: + """ + Get the model class for the extended profile form. + + Returns the Django model class associated with the form specified in + the `PROFILE_EXTENSION_FORM` setting. + + IMPORTANT: This function only works with PROFILE_EXTENSION_FORM. If you're using + the deprecated REGISTRATION_EXTENSION_FORM, this will return None to maintain + backward compatibility. The new profile extension capabilities (loading/saving + to a custom model) are only available when using PROFILE_EXTENSION_FORM. + + Migration path: + - Old behavior (REGISTRATION_EXTENSION_FORM): Custom fields only for registration, + data stored in UserProfile.meta field + - New behavior (PROFILE_EXTENSION_FORM): Custom fields for registration and profile, + data stored in dedicated model with ability to load/update via account settings API + + Returns: + Optional[Type[Model]]: The model class if PROFILE_EXTENSION_FORM is configured + and valid, None otherwise (including when using the deprecated + REGISTRATION_EXTENSION_FORM). + + Examples: + # New setting with model support: + # In settings.py: PROFILE_EXTENSION_FORM = 'myapp.forms.ExtendedProfileForm' + model_class = get_extended_profile_model() # Returns the model + + # Deprecated setting - maintains old behavior: + # In settings.py: REGISTRATION_EXTENSION_FORM = 'myapp.forms.ExtendedForm' + model_class = get_extended_profile_model() # Returns None (no model support) + """ + # Only check for the new setting - do NOT fall back to REGISTRATION_EXTENSION_FORM + # This ensures backward compatibility: users of the old setting keep the old behavior + setting_value = getattr(settings, "PROFILE_EXTENSION_FORM", None) + + if not setting_value: + return None + + try: + module_path, klass_name = setting_value.rsplit(".", 1) + module = import_module(module_path) + form_class = getattr(module, klass_name) + return getattr(form_class.Meta, "model", None) + except (ValueError, ImportError, ModuleNotFoundError, AttributeError) as e: + log.warning("Could not load extended profile model from PROFILE_EXTENSION_FORM='%s': %s", setting_value, e) return None - module, klass = settings.REGISTRATION_EXTENSION_FORM.rsplit('.', 1) - module = import_module(module) - return getattr(module, klass)(*args, **kwargs) class RegistrationFormFactory: @@ -504,7 +585,8 @@ def get_registration_form(self, request): form_desc = FormDescription("post", self._get_registration_submit_url(request)) self._apply_third_party_auth_overrides(request, form_desc) - # Custom form fields can be added via the form set in settings.REGISTRATION_EXTENSION_FORM + # Custom form fields can be added via the form set in settings.PROFILE_EXTENSION_FORM + # (or deprecated settings.REGISTRATION_EXTENSION_FORM) custom_form = get_registration_extension_form() if custom_form: custom_form_field_names = [field_name for field_name, field in custom_form.fields.items()] diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_utils.py b/openedx/core/djangoapps/user_authn/views/tests/test_utils.py index 397448005f85..1f348b694bdf 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_utils.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_utils.py @@ -1,11 +1,20 @@ """ Tests for user utils functionality. """ -from django.test import TestCase from datetime import datetime -from openedx.core.djangoapps.user_authn.views.utils import get_auto_generated_username, _get_username_prefix +from typing import Optional +from unittest.mock import Mock, patch + import ddt -from unittest.mock import patch +from django.db.models import Model +from django.test import TestCase +from django.test.utils import override_settings + +from openedx.core.djangoapps.user_authn.views.registration_form import ( + get_extended_profile_model, + get_registration_extension_form, +) +from openedx.core.djangoapps.user_authn.views.utils import _get_username_prefix, get_auto_generated_username @ddt.ddt @@ -77,3 +86,252 @@ def test_get_auto_generated_username_no_prefix(self, mock_datetime, mock_choices username = get_auto_generated_username({}) self.assertEqual(username, expected_username) + + +@ddt.ddt +class TestGetExtendedProfileModel(TestCase): + """ + Tests for `get_extended_profile_model function + """ + + @ddt.data(None, "") + def test_get_extended_profile_model_no_setting_or_empty_string(self, setting_value: Optional[str]): + """ + Test when `PROFILE_EXTENSION_FORM` setting is not configured + """ + with override_settings(PROFILE_EXTENSION_FORM=setting_value): + result = get_extended_profile_model() + + self.assertIsNone(result) + + @override_settings(PROFILE_EXTENSION_FORM="invalid.module.path") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_extended_profile_model_invalid_module(self, mock_logger: Mock): + """ + Test when the module path is invalid + """ + result = get_extended_profile_model() + + self.assertIsNone(result) + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) + + @override_settings(PROFILE_EXTENSION_FORM="django.forms.Form") + def test_get_extended_profile_model_no_meta_class(self): + """ + Test when the form class doesn't have a Meta class + """ + result = get_extended_profile_model() + + self.assertIsNone(result) + + @override_settings(PROFILE_EXTENSION_FORM="invalid_module_path") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_extended_profile_model_malformed_path(self, mock_logger: Mock): + """ + Test when the setting value doesn't have a dot separator + """ + result = get_extended_profile_model() + + self.assertIsNone(result) + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) + + @override_settings(PROFILE_EXTENSION_FORM="myapp.forms.CustomExtendedProfileForm") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + def test_get_extended_profile_model_custom_form(self, mock_import_module: Mock): + """ + Test loading model from a custom extended profile form + """ + mock_model = Mock(spec=Model) + mock_form_class = Mock() + mock_form_class.Meta = Mock() + mock_form_class.Meta.model = mock_model + mock_module = Mock() + mock_module.CustomExtendedProfileForm = mock_form_class + mock_import_module.return_value = mock_module + + result = get_extended_profile_model() + + self.assertEqual(result, mock_model) + mock_import_module.assert_called_once_with("myapp.forms") + + @override_settings(PROFILE_EXTENSION_FORM="myapp.forms.FormWithoutModel") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + def test_get_extended_profile_model_form_without_model(self, mock_import_module: Mock): + """ + Test when form has Meta but no model attribute + """ + # Create a mock form class with Meta but no model + mock_form_class = Mock() + mock_form_class.Meta = Mock(spec=[]) # Meta exists but has no model attribute + # Create a mock module with the form class + mock_module = Mock() + mock_module.FormWithoutModel = mock_form_class + mock_import_module.return_value = mock_module + + result = get_extended_profile_model() + + self.assertIsNone(result) + + @ddt.data( + (ImportError, "Module not found"), + (ModuleNotFoundError, "No module named 'myapp'"), + ) + @ddt.unpack + @override_settings(PROFILE_EXTENSION_FORM="myapp.forms.ExtendedProfileForm") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_extended_profile_model_import_errors( + self, exception_class: type, error_message: str, mock_logger: Mock, mock_import_module: Mock + ): + """ + Test when import_module raises ImportError or ModuleNotFoundError + """ + mock_import_module.side_effect = exception_class(error_message) + + result = get_extended_profile_model() + + self.assertIsNone(result) + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) + + @override_settings(PROFILE_EXTENSION_FORM="myapp.forms.NonExistentForm") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_extended_profile_model_attribute_error(self, mock_logger: Mock, mock_import_module: Mock): + """ + Test when the form class doesn't exist in the module + """ + mock_module = Mock(spec=[]) + mock_import_module.return_value = mock_module + + result = get_extended_profile_model() + + self.assertIsNone(result) + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) + + @override_settings(PROFILE_EXTENSION_FORM=None, REGISTRATION_EXTENSION_FORM="myapp.forms.LegacyForm") + def test_get_extended_profile_model_with_deprecated_setting_returns_none(self): + """ + Test that using REGISTRATION_EXTENSION_FORM returns None (maintains old behavior). + + This ensures backward compatibility: sites using REGISTRATION_EXTENSION_FORM + will NOT get the new model-based profile capabilities. They continue using + the old UserProfile.meta field approach. + """ + result = get_extended_profile_model() + + self.assertIsNone(result) + + +@ddt.ddt +class TestGetRegistrationExtensionForm(TestCase): + """ + Tests for get_registration_extension_form function + """ + + @ddt.data(None, "") + def test_get_registration_extension_form_no_setting(self, setting_value: Optional[str]): + """ + Test when neither PROFILE_EXTENSION_FORM nor REGISTRATION_EXTENSION_FORM is configured + """ + with override_settings( + PROFILE_EXTENSION_FORM=setting_value, + REGISTRATION_EXTENSION_FORM=setting_value + ): + result = get_registration_extension_form() + + self.assertIsNone(result) + + @override_settings(PROFILE_EXTENSION_FORM="myapp.forms.CustomProfileForm") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + def test_get_registration_extension_form_with_new_setting(self, mock_import_module: Mock): + """ + Test loading form from PROFILE_EXTENSION_FORM (new setting) + """ + mock_form_instance = Mock() + mock_form_class = Mock(return_value=mock_form_instance) + mock_module = Mock() + mock_module.CustomProfileForm = mock_form_class + mock_import_module.return_value = mock_module + + result = get_registration_extension_form(data={'field': 'value'}) + + self.assertEqual(result, mock_form_instance) + mock_import_module.assert_called_once_with("myapp.forms") + mock_form_class.assert_called_once_with(data={'field': 'value'}) + + @override_settings( + PROFILE_EXTENSION_FORM="myapp.forms.NewForm", + REGISTRATION_EXTENSION_FORM="myapp.forms.OldForm" + ) + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + def test_get_registration_extension_form_new_setting_precedence(self, mock_import_module: Mock): + """ + Test that PROFILE_EXTENSION_FORM takes precedence over REGISTRATION_EXTENSION_FORM + """ + mock_form_instance = Mock() + mock_form_class = Mock(return_value=mock_form_instance) + mock_module = Mock() + mock_module.NewForm = mock_form_class + mock_import_module.return_value = mock_module + + result = get_registration_extension_form() + + self.assertEqual(result, mock_form_instance) + mock_import_module.assert_called_once_with("myapp.forms") + + @override_settings( + PROFILE_EXTENSION_FORM=None, + REGISTRATION_EXTENSION_FORM="myapp.forms.LegacyForm" + ) + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_registration_extension_form_deprecation_warning(self, mock_logger: Mock, mock_import_module: Mock): + """ + Test that using REGISTRATION_EXTENSION_FORM logs a deprecation warning + """ + mock_form_instance = Mock() + mock_form_class = Mock(return_value=mock_form_instance) + mock_module = Mock() + mock_module.LegacyForm = mock_form_class + mock_import_module.return_value = mock_module + + result = get_registration_extension_form() + + self.assertEqual(result, mock_form_instance) + deprecation_calls = [call for call in mock_logger.warning.call_args_list if "deprecated" in str(call).lower()] + self.assertGreater(len(deprecation_calls), 0, "Expected a deprecation warning to be logged") + warning_message = str(deprecation_calls[0]) + self.assertIn("REGISTRATION_EXTENSION_FORM", warning_message) + self.assertIn("PROFILE_EXTENSION_FORM", warning_message) + + @override_settings(PROFILE_EXTENSION_FORM="invalid.path") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.import_module") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_registration_extension_form_import_error(self, mock_logger: Mock, mock_import_module: Mock): + """ + Test when form import fails + """ + mock_import_module.side_effect = ImportError("Module not found") + + result = get_registration_extension_form() + + self.assertIsNone(result) + error_calls = mock_logger.error.call_args_list + self.assertGreater(len(error_calls), 0, "Expected an error to be logged") + + @override_settings(PROFILE_EXTENSION_FORM="invalid_path_without_dot") + @patch("openedx.core.djangoapps.user_authn.views.registration_form.log") + def test_get_registration_extension_form_malformed_path(self, mock_logger: Mock): + """ + Test when setting value doesn't have proper format (no dot separator) + """ + result = get_registration_extension_form() + + self.assertIsNone(result) + + error_calls = mock_logger.error.call_args_list + self.assertGreater(len(error_calls), 0, "Expected an error to be logged")