diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index d682adab1742..f2892857d3e0 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -102,7 +102,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 . import provider @@ -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 55cdf3d93d2c..cfc2c38fd689 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 3889d3b2b7d9..3ec166552166 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2630,8 +2630,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 9a6da6ab44d2..51728e5555ce 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -4,12 +4,15 @@ """ import datetime +import logging import re from zoneinfo import ZoneInfo +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 DatabaseError, IntegrityError, transaction from django.utils.translation import gettext as _ from django.utils.translation import override as override_language from eventtracking import tracker @@ -41,6 +44,7 @@ from openedx.core.lib.api.view_utils import add_serializer_errors from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed +from .forms import validate_and_get_extended_profile_form from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer, UserReadOnlySerializer, _visible_fields name_affirmation_installed = is_name_affirmation_installed() @@ -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 @@ -165,6 +171,12 @@ def update_account_settings(requesting_user, update, username=None): old_name = _validate_name_change(user_profile, update, field_errors) old_language_proficiencies = _get_old_language_proficiencies_if_updating(user_profile, update) + extended_profile_data = update.get("extended_profile") if "extended_profile" in update else None + extended_profile_form = None + if extended_profile_data is not None: + extended_profile_form, ext_profile_errors = validate_and_get_extended_profile_form(extended_profile_data, user) + field_errors.update(ext_profile_errors) + if field_errors: raise errors.AccountValidationError(field_errors) @@ -176,7 +188,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: @@ -352,17 +364,81 @@ 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: forms.Form | None +) -> 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 (forms.Form | None): 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 ValidationError as exc: + raise AccountUpdateError( + developer_message=f"Extended profile validation failed: {str(exc)}", + user_message=_("The extended profile information could not be saved due to validation errors."), + ) from exc + except IntegrityError as exc: + raise AccountUpdateError( + developer_message=f"Extended profile integrity error: {str(exc)}", + user_message=_("The extended profile information could not be saved. Please check for duplicate values."), + ) from exc + except DatabaseError as exc: + raise AccountUpdateError( + developer_message=f"Database error saving extended profile: {str(exc)}", + user_message=_("The extended profile information could not be saved due to a system error."), + ) from exc def _update_state_if_needed(data, user_profile): # If the country was changed to something other than US, remove the state. diff --git a/openedx/core/djangoapps/user_api/accounts/forms.py b/openedx/core/djangoapps/user_api/accounts/forms.py index 95a635a5154a..336eb85f9cf9 100644 --- a/openedx/core/djangoapps/user_api/accounts/forms.py +++ b/openedx/core/djangoapps/user_api/accounts/forms.py @@ -2,11 +2,20 @@ Django forms for accounts """ +import logging 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 +44,136 @@ def save(self, retirement): raise ValidationError('Retirement is in the wrong state!') handle_retirement_cancellation(retirement) + + +def extract_extended_profile_fields_data(extended_profile: list[dict] | None) -> tuple[dict, dict]: + """ + Extract extended profile fields data from extended_profile structure. + + Args: + extended_profile (list[dict] | None): 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[forms.Form | None, 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 (forms.Form | None): 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[forms.Form | None, 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 (forms.Form | None): 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 a23ba772986e..38391e4aa45a 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 @@ -25,7 +26,11 @@ 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 ( @@ -566,26 +571,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 db3f6b078118..beee82b9f2d1 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -13,6 +13,8 @@ from django.conf import settings from django.contrib.auth.hashers import make_password from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.core.exceptions import ValidationError +from django.db import DatabaseError, IntegrityError from django.http import HttpResponse from django.test import TestCase from django.test.client import RequestFactory @@ -30,6 +32,7 @@ from lms.djangoapps.certificates.data import CertificateStatuses from openedx.core.djangoapps.ace_common.tests.mixins import EmailTemplateTagMixin from openedx.core.djangoapps.embargo.models import Country, GlobalRestrictedCountry +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY from openedx.core.djangoapps.user_api.accounts.api import ( get_account_settings, @@ -164,6 +167,30 @@ def test_update_username_provided(self): with pytest.raises(UserNotAuthorized): update_account_settings(self.different_user, {"name": "Pluto"}, username=self.user.username) + @with_site_configuration(configuration={"extended_profile_fields": ["department"]}) + def test_update_username_provided_with_extended_profile(self): + """Test that extended profile is saved when username is provided to update_account_settings.""" + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + + update_account_settings(self.user, {"extended_profile": extended_profile_data, "name": "Donald Duck"}) + account_settings = get_account_settings(self.default_request)[0] + self.assertEqual(extended_profile_data, account_settings["extended_profile"]) # noqa: PT009 + self.assertEqual("Donald Duck", account_settings["name"]) # noqa: PT009 + + update_account_settings( + self.user, {"extended_profile": extended_profile_data, "name": "Mickey Mouse"}, username=self.user.username + ) + account_settings = get_account_settings(self.default_request)[0] + self.assertEqual(extended_profile_data, account_settings["extended_profile"]) # noqa: PT009 + self.assertEqual("Mickey Mouse", account_settings["name"]) # noqa: PT009 + + with pytest.raises(UserNotAuthorized): + update_account_settings( + self.different_user, + {"extended_profile": extended_profile_data, "name": "Pluto"}, + username=self.user.username, + ) + def test_update_non_existent_user(self): with pytest.raises(UserNotAuthorized): update_account_settings(self.user, {}, username="does_not_exist") @@ -504,10 +531,10 @@ def test_add_account_recovery(self): assert account_recovery.secondary_email == test_email def test_change_country_removes_state(self): - ''' + """ Test that changing the country (to something other than a country with states) removes the state - ''' + """ # First set the country and state update_account_settings(self.user, {"country": UserProfile.COUNTRY_WITH_STATES, "state": "MA"}) account_settings = get_account_settings(self.default_request)[0] @@ -543,6 +570,101 @@ 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") # noqa: PT009 + self.assertEqual(meta["title"], "Software Engineer") # noqa: PT009 + self.assertEqual(user_profile.bio, "Updated bio") # noqa: PT009 + + @patch("openedx.core.djangoapps.user_api.accounts.api.validate_and_get_extended_profile_form") + def test_update_extended_profile_with_form(self, mock_validate_and_get_form): + """ + Test updating extended profile with a validated form + """ + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + mock_form = Mock(save=Mock(return_value=Mock(user=self.user))) + mock_validate_and_get_form.return_value = (mock_form, {}) + + update_account_settings(self.user, {"extended_profile": extended_profile_data}) + + mock_validate_and_get_form.assert_called_once_with(extended_profile_data, self.user) + mock_form.save.assert_called_once_with(commit=False) + mock_form.save.return_value.save.assert_called_once() + meta = UserProfile.objects.get(user=self.user).get_meta() + self.assertEqual(meta["department"], "Engineering") # noqa: PT009 + + @patch("openedx.core.djangoapps.user_api.accounts.api.validate_and_get_extended_profile_form") + def test_update_extended_profile_with_form_new_instance(self, mock_validate_and_get_form): + """ + Test updating extended profile with a form for a new instance + """ + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + mock_instance = Mock(user=None) + mock_form = Mock(save=Mock(return_value=mock_instance)) + mock_validate_and_get_form.return_value = (mock_form, {}) + + update_account_settings(self.user, {"extended_profile": extended_profile_data}) + + mock_validate_and_get_form.assert_called_once_with(extended_profile_data, self.user) + mock_form.save.assert_called_once_with(commit=False) + self.assertEqual(mock_instance.user, self.user) # noqa: PT009 + mock_instance.save.assert_called_once() + + @patch("openedx.core.djangoapps.user_api.accounts.api.validate_and_get_extended_profile_form") + @ddt.data( + (ValidationError("Invalid field value"), "Extended profile validation failed"), + (IntegrityError("Duplicate entry"), "Extended profile integrity error"), + (DatabaseError("Connection lost"), "Database error saving extended profile"), + ) + @ddt.unpack + def test_update_extended_profile_form_save_error(self, exception, expected_dev_msg, mock_validate_and_get_form): + """ + Test that errors during form save cause an AccountUpdateError with appropriate messages, + and do not leave partial updates. + """ + extended_profile_data = [{"field_name": "department", "field_value": "Engineering"}] + mock_form = Mock() + mock_form.save.side_effect = exception + mock_validate_and_get_form.return_value = (mock_form, {}) + + with pytest.raises(AccountUpdateError) as context_manager: + update_account_settings(self.user, {"extended_profile": extended_profile_data}) + + self.assertIn(expected_dev_msg, context_manager.value.developer_message) # noqa: PT009 + self.assertIsNotNone(context_manager.value.user_message) # noqa: PT009 + + mock_validate_and_get_form.assert_called_once_with(extended_profile_data, self.user) + mock_form.save.assert_called_once_with(commit=False) + + # The meta update is in the same transaction, it should be rolled back. + meta = UserProfile.objects.get(user=self.user).get_meta() + self.assertNotIn("department", meta) # noqa: PT009 + + 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") # noqa: PT009 + @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..bc2c4c0acd1a --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_forms.py @@ -0,0 +1,323 @@ +""" +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, {}) # noqa: PT009 + self.assertEqual(extracted_data, {"department": "Engineering", "title": "Software Engineer"}) # noqa: PT009 + + 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, {}) # noqa: PT009 + self.assertEqual(extracted_data, {"department": "", "title": "Engineer"}) # noqa: PT009 + + 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, {}) # noqa: PT009 + self.assertIn("extended_profile", errors) # noqa: PT009 + self.assertEqual( # noqa: PT009 + 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, {}) # noqa: PT009 + self.assertEqual(extracted_data, {"department": "Engineering", "title": "Engineer"}) # noqa: PT009 + + 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, {}) # noqa: PT009 + self.assertEqual(extracted_data, {"department": "Engineering"}) # noqa: PT009 + + 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, {}) # noqa: PT009 + self.assertEqual(extracted_data, {}) # noqa: PT009 + + +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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + + @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_model = Mock(spec=[]) + mock_get_model.return_value = mock_model + extended_profile_fields_data = {"department": "Engineering"} + + form, errors = get_extended_profile_form(extended_profile_fields_data, self.user) + + self.assertIsNone(form) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + + @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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + 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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + 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_get_model: 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) + + mock_get_model.assert_called_once() + self.assertIsNone(form) # noqa: PT009 + self.assertIn("department", errors) # noqa: PT009 + self.assertIn("title", errors) # noqa: PT009 + self.assertEqual(errors["department"]["user_message"], "This field is required") # noqa: PT009 + self.assertEqual(errors["title"]["user_message"], "Invalid value") # noqa: PT009 + + @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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn("extended_profile", errors) # noqa: PT009 + self.assertIn("Error creating custom form", errors["extended_profile"]["developer_message"]) # noqa: PT009 + + +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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + 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) # noqa: PT009 + self.assertIn("extended_profile", errors) # noqa: PT009 + + @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) # noqa: PT009 + self.assertEqual(errors, {}) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn("department", errors) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn("title", errors) # noqa: PT009 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 d77ef74c234d..020fde73513e 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 common.djangoapps.student.models import UserProfile from common.djangoapps.student.tests.factories import UserFactory -from openedx.core.djangoapps.user_api.accounts.serializers import UserReadOnlySerializer +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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": "Engineering"}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": "Software Engineer"}, result) # noqa: PT009 + self.assertIn({"field_name": "company", "field_value": "EdX"}, result) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": "Sales"}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": "Manager"}, result) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": "Marketing"}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": "Director"}, result) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": ""}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": ""}, result) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": ""}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": ""}, result) # noqa: PT009 + + @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) # noqa: PT009 + self.assertIn({"field_name": "department", "field_value": "HR"}, result) # noqa: PT009 + self.assertIn({"field_name": "title", "field_value": "Recruiter"}, result) # noqa: PT009 + self.assertIn({"field_name": "location", "field_value": ""}, result) # noqa: PT009 + + @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) # noqa: PT009 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 194c2f30a316..f3f37c2f1469 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.api.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) # noqa: PT009 + mock_validate_form.assert_called_once_with(extended_profile_data, self.user) + + @mock.patch("openedx.core.djangoapps.user_api.accounts.api.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) # noqa: PT009 + self.assertIn("field_errors", response.data) # noqa: PT009 + self.assertIn("department", response.data["field_errors"]) # noqa: PT009 + 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) # noqa: PT009 + self.assertEqual(response.data["bio"], "Test bio without extended profile") # noqa: PT009 + + @mock.patch("openedx.core.djangoapps.user_api.accounts.api.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) # noqa: PT009 + mock_validate_form.assert_called_once_with([], self.user) # noqa: PT009 + + @mock.patch("openedx.core.djangoapps.user_api.accounts.api.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) # noqa: PT009 + self.assertIn("field_errors", response.data) # noqa: PT009 + + @mock.patch("openedx.core.djangoapps.user_api.accounts.api.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) # noqa: PT009 + 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]) # noqa: PT009 + + @ddt.ddt class NameChangeViewTests(UserAPITestCase): """ NameChangeView tests """ diff --git a/openedx/core/djangoapps/user_authn/api/helper.py b/openedx/core/djangoapps/user_authn/api/helper.py index 156b964db7a6..9e4c8104b0ff 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 7c84b224f518..0171367aa027 100644 --- a/openedx/core/djangoapps/user_authn/views/registration_form.py +++ b/openedx/core/djangoapps/user_authn/views/registration_form.py @@ -12,6 +12,7 @@ from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.exceptions import ImproperlyConfigured from django.core.validators import RegexValidator, ValidationError, slug_re +from django.db.models import Model from django.forms import widgets from django.urls import reverse from django.utils.translation import gettext as _ @@ -316,17 +317,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) -> forms.Form | None: """ - 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() -> type[Model] | None: + """ + 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: + type[Model] | None: 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, 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: @@ -503,7 +583,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 50f98dd2ab7d..e9fb7f71a81c 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_utils.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_utils.py @@ -1,12 +1,19 @@ """ Tests for user utils functionality. """ + from datetime import datetime -from unittest.mock import patch +from unittest.mock import Mock, patch import ddt +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 @@ -17,21 +24,21 @@ class TestGenerateUsername(TestCase): """ @ddt.data( - ({'first_name': 'John', 'last_name': 'Doe'}, "JD"), - ({'name': 'Jane Smith'}, "JS"), - ({'name': 'Jane'}, "J"), - ({'name': 'John Doe Smith'}, "JD") + ({"first_name": "John", "last_name": "Doe"}, "JD"), + ({"name": "Jane Smith"}, "JS"), + ({"name": "Jane"}, "J"), + ({"name": "John Doe Smith"}, "JD"), ) @ddt.unpack def test_generate_username_from_data(self, data, expected_initials): """ Test get_auto_generated_username function. """ - random_string = 'XYZA' + random_string = "XYZA" current_year_month = f"_{datetime.now().year % 100}{datetime.now().month:02d}_" - with patch('openedx.core.djangoapps.user_authn.views.utils.random.choices') as mock_choices: - mock_choices.return_value = ['X', 'Y', 'Z', 'A'] + with patch("openedx.core.djangoapps.user_authn.views.utils.random.choices") as mock_choices: + mock_choices.return_value = ["X", "Y", "Z", "A"] username = get_auto_generated_username(data) @@ -39,19 +46,19 @@ def test_generate_username_from_data(self, data, expected_initials): self.assertEqual(username, expected_username) # noqa: PT009 @ddt.data( - ({'first_name': 'John', 'last_name': 'Doe'}, "JD"), - ({'name': 'Jane Smith'}, "JS"), - ({'name': 'Jane'}, "J"), - ({'name': 'John Doe Smith'}, "JD"), - ({'first_name': 'John Doe', 'last_name': 'Smith'}, "JD"), + ({"first_name": "John", "last_name": "Doe"}, "JD"), + ({"name": "Jane Smith"}, "JS"), + ({"name": "Jane"}, "J"), + ({"name": "John Doe Smith"}, "JD"), + ({"first_name": "John Doe", "last_name": "Smith"}, "JD"), ({}, None), - ({'first_name': '', 'last_name': ''}, None), - ({'name': ''}, None), - ({'name': '='}, None), - ({'name': '@'}, None), - ({'first_name': '阿提亚', 'last_name': '阿提亚'}, "AT"), - ({'first_name': 'أحمد', 'last_name': 'محمد'}, "HM"), - ({'name': 'أحمد محمد'}, "HM"), + ({"first_name": "", "last_name": ""}, None), + ({"name": ""}, None), + ({"name": "="}, None), + ({"name": "@"}, None), + ({"first_name": "阿提亚", "last_name": "阿提亚"}, "AT"), + ({"first_name": "أحمد", "last_name": "محمد"}, "HM"), + ({"name": "أحمد محمد"}, "HM"), ) @ddt.unpack def test_get_username_prefix(self, data, expected_initials): @@ -61,21 +68,258 @@ def test_get_username_prefix(self, data, expected_initials): username_prefix = _get_username_prefix(data) self.assertEqual(username_prefix, expected_initials) # noqa: PT009 - @patch('openedx.core.djangoapps.user_authn.views.utils._get_username_prefix') - @patch('openedx.core.djangoapps.user_authn.views.utils.random.choices') - @patch('openedx.core.djangoapps.user_authn.views.utils.datetime') + @patch("openedx.core.djangoapps.user_authn.views.utils._get_username_prefix") + @patch("openedx.core.djangoapps.user_authn.views.utils.random.choices") + @patch("openedx.core.djangoapps.user_authn.views.utils.datetime") def test_get_auto_generated_username_no_prefix(self, mock_datetime, mock_choices, mock_get_username_prefix): """ Test get_auto_generated_username function when no name data is provided. """ mock_datetime.now.return_value.strftime.return_value = f"{datetime.now().year % 100} {datetime.now().month:02d}" - mock_choices.return_value = ['X', 'Y', 'Z', 'A'] # Fixed random string for testing + mock_choices.return_value = ["X", "Y", "Z", "A"] # Fixed random string for testing mock_get_username_prefix.return_value = None current_year_month = f"{datetime.now().year % 100}{datetime.now().month:02d}_" - random_string = 'XYZA' + random_string = "XYZA" expected_username = current_year_month + random_string username = get_auto_generated_username({}) self.assertEqual(username, expected_username) # noqa: PT009 + + +@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: str | None): + """ + 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) # noqa: PT009 + + @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) # noqa: PT009 + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) # noqa: PT009 + + @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) # noqa: PT009 + + @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) # noqa: PT009 + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) # noqa: PT009 + + @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) # noqa: PT009 + 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) # noqa: PT009 + + @ddt.data((ImportError, "Module not found")) + @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) # noqa: PT009 + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) # noqa: PT009 + + @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) # noqa: PT009 + mock_logger.warning.assert_called_once() + self.assertIn("Could not load extended profile model", str(mock_logger.warning.call_args)) # noqa: PT009 + + @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) # noqa: PT009 + + +@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: str | None): + """ + 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) # noqa: PT009 + + @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) # noqa: PT009 + 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) # noqa: PT009 + 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) # noqa: PT009 + 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") # noqa: PT009 + warning_message = str(deprecation_calls[0]) + self.assertIn("REGISTRATION_EXTENSION_FORM", warning_message) # noqa: PT009 + self.assertIn("PROFILE_EXTENSION_FORM", warning_message) # noqa: PT009 + + @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) # noqa: PT009 + error_calls = mock_logger.error.call_args_list + self.assertGreater(len(error_calls), 0, "Expected an error to be logged") # noqa: PT009 + + @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) # noqa: PT009 + + error_calls = mock_logger.error.call_args_list + self.assertGreater(len(error_calls), 0, "Expected an error to be logged") # noqa: PT009