-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: use extended profile model in account settings #37119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 23 commits
6233b2d
6ff0412
30d8cb7
a898336
5747b74
ad36b40
62174ec
575ffbb
bf54af1
4343e62
74f103d
966dc40
62608df
21a6bbb
666a442
176cd20
9d2a40f
fe47add
e620c7f
287bc44
4fb774b
350b9c1
cecbb7c
d862b7c
53a9b59
a0c077a
8379ee2
4c5750d
aaa1327
0307656
1636f4b
cc2fed5
50f9c7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,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: Optional[list]) -> Tuple[dict, dict]: | ||
| """ | ||
| Extract extended profile fields data from extended_profile structure. | ||
|
|
||
| Args: | ||
| extended_profile (Optional[list]): 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Entries where
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can see, the current behavior accepts any value, including |
||
|
|
||
| if field_value is not None: | ||
| 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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The docstring says "The validated form instance", but this function can return an invalid form when
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that’s correct. I updated the docstring to make it clearer. Commit: a0c077a |
||
| 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 or creation fails | ||
| - field_errors (dict): Dictionary of validation errors, if any | ||
| """ | ||
| field_errors = {} | ||
|
|
||
| try: | ||
|
||
| extended_profile_model = get_extended_profile_model() | ||
| except ImportError as e: | ||
| logger.warning("Extended profile model not available: %s", str(e)) | ||
| return None, field_errors | ||
|
|
||
| kwargs = {} | ||
|
|
||
| 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), | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This returns the invalid form to the caller when validation fails (see above on the docstring issue). Since the caller uses
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally agree, changes applied! Commit: 4c5750d |
||
|
|
||
| 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 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors during the model save are caught and logged but not re-raised. Because the meta write (
user_profile.save()above) already committed outside this savepoint, a failure here leaves meta updated but the model record stale — a silent partial save.This appears intentional given
test_update_extended_profile_form_save_errorexplicitly asserts the meta update still succeeds when the form save fails. If so, the docstring's Note should be updated to say so explicitly, e.g. "If the model save fails, the error is logged and the meta update is preserved (not rolled back)."This also has the side effect that the meta values and the values in the model will get out of sync. Since we've now updated the code to read only from the model, this could end up hiding information we had intended to expose to the user. I don't necessarily want us to update the extended profile model on read but perhaps it wolud be good to merge the data from meta and the extended model on read (prioritizing the value in the model) to not lose data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best approach is to include both operations within the same transaction so there’s no risk of the values getting out of sync. Commit: aaa1327