feat: use extended profile model in account settings#37119
feat: use extended profile model in account settings#37119BryanttV wants to merge 23 commits intoopenedx:masterfrom
Conversation
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Thanks for the pull request, @BryanttV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
12561c1 to
dc1cee2
Compare
dc1cee2 to
04cdfb7
Compare
dae3705 to
065faee
Compare
efortish
left a comment
There was a problem hiding this comment.
Thank you so much for the contribution @BryanttV
Everything looks good, I followed your instructions step by step and it went great!
Can we add some edge-case tests suit for the new features in api.py and registration_form.py?
065faee to
06c8680
Compare
|
Thanks for the review, @efortish! I will be working on the unit tests for this functionality. |
| extended_profile_model = extended_profile_form.save(commit=False) | ||
| if not hasattr(extended_profile_model, "user") or extended_profile_model.user is None: | ||
| extended_profile_model.user = user_profile.user | ||
| extended_profile_model.save() |
There was a problem hiding this comment.
Is there a reason why do we have to do this double step save for the extended profile? Is it because L531 should raise errors before attaching the user? Wouldn't be safer to try save within an atomic transaction?
There was a problem hiding this comment.
We can indeed use an atomic transaction, but we need to include both save operations.
The first one (with commit=False) creates the model instance without saving it to the database. The second one actually saves it to the database, including the user field if the instance doesn’t already have an assigned user. If we try to save the model without a user, it will fail.
Something similar is made here:
https://github.com/openedx/edx-platform/blob/7b953a5310ccd51d24207b70a17844ce3c83e664/common/djangoapps/student/helpers.py#L704-L709
I think we can use an atomic transaction and add a comment explaining why we need both save operations.
| _send_email_change_requests_if_needed(update, user) | ||
|
|
||
|
|
||
| def _get_and_validate_extended_profile_form(update: dict, user, field_errors: dict) -> Optional[forms.Form]: |
There was a problem hiding this comment.
Not sure if update is the best name for this field, can we try something more descriptive? Also are we not typing user on purpose?
| def _get_and_validate_extended_profile_form(update: dict, user, field_errors: dict) -> Optional[forms.Form]: | |
| def _get_and_validate_extended_profile_form(updated_data: dict, user, field_errors: dict) -> Optional[forms.Form]: |
| try: | ||
| extended_profile_model = get_extended_profile_model() | ||
|
|
||
| kwargs = {} | ||
| if not extended_profile_model: | ||
| logger.info("No extended profile model configured") | ||
| else: | ||
| try: | ||
| kwargs["instance"] = extended_profile_model.objects.get(user=user) | ||
| except ObjectDoesNotExist: | ||
| logger.info("No existing extended profile found for user %s, creating new instance", user.username) | ||
|
|
||
| extended_profile_form = get_registration_extension_form(data=extended_profile_fields_data, **kwargs) | ||
|
|
||
| return extended_profile_form | ||
|
|
||
| except ImportError as e: | ||
| logger.warning("Extended profile model not available: %s", str(e)) | ||
| return None | ||
| 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 |
There was a problem hiding this comment.
I think we can redude the level of nastiness here, here's a suggestion - haven't tested it though:
try:
extended_profile_model = get_extended_profile_model()
except ImportError:
logger.warning("Extended profile model not available: %s", str(e))
return None
kwargs = {}
try:
kwargs["instance"] = extended_profile_model.objects.get(user=user)
except AttributeError: # Kind of ugly
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:
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
return extended_profile_form
| 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), | ||
| } |
There was a problem hiding this comment.
I think we can reduce 1 level of nestiness here by:
| 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), | |
| } | |
| if extended_profile_form.is_valid(): | |
| return # Nothing to do here, the 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), | |
| } |
| def get_extended_profile_data(): | ||
| extended_profile_model = get_extended_profile_model() | ||
|
|
||
| # pick the keys from the site configuration | ||
| extended_profile_field_names = configuration_helpers.get_value('extended_profile_fields', []) | ||
| if extended_profile_model: | ||
| try: | ||
| profile_obj = extended_profile_model.objects.get(user=user_profile.user) | ||
| return model_to_dict(profile_obj) | ||
| except (AttributeError, extended_profile_model.DoesNotExist): | ||
| return {} | ||
|
|
||
| try: | ||
| extended_profile_fields_data = json.loads(user_profile.meta) | ||
| except ValueError: | ||
| extended_profile_fields_data = {} | ||
| try: | ||
| return json.loads(user_profile.meta or "{}") | ||
| except (ValueError, TypeError, AttributeError): | ||
| return {} |
There was a problem hiding this comment.
This section made me question something: is the profile meta and the extended profile model out of date at some point? Should they be in sync?
There was a problem hiding this comment.
Every time we update the extended profile fields, they will be updated in both the model and the meta, so they should always be synchronized.
| _send_email_change_requests_if_needed(update, user) | ||
|
|
||
|
|
||
| def _get_and_validate_extended_profile_form(update: dict, user, field_errors: dict) -> Optional[forms.Form]: |
There was a problem hiding this comment.
I'm not super happy about having form specific code in api.py. For that matter, I'm also not super happy about having form validators in api.py. That seems like form and view and validator code to me. api.py should be dealing with abstract data models, in my opinion.
If you can point to prior art where we do this, especially recent prior art, I'm willing to change my mind about that, but this really seems like, well...not dependency inversion, but dependency reversal. Your form.py should import from the API, not vice versa.
There was a problem hiding this comment.
Hello @deborahgu, thanks for your comment! I made some changes according to your suggestions. Could you check again, please?
06c8680 to
4e25200
Compare
f234f96 to
b6b08c9
Compare
f15cab3 to
750e00a
Compare
feanil
left a comment
There was a problem hiding this comment.
The approach makes sense to me. I did a quick AI guided review which left the above comment with some stuff to cleanup but higher level I have a bigger suggestion.
This will change the behavior in the case where someone is already using the REGISTRATION_EXTENSION_FORM but has different data in their meta field than in the registration form, since one this code is enabled the system will not load the meta information and only load the information they have in their registration extra fields.
I wonder if it makes sense to introduce a new setting PROFILE_EXTENSION_FORM which does everything the REGISTRATION_EXTENSION_FORM does but also all the new capabilities you're adding. We can also create a DEPR for the REGISTRATION_EXTENSION_FORM as this would become redundant.
What do you think of this approach? The setting name would also become more clear and accurate which is a refactor that we'll want to do eventually anyway so maybe makes sense to do it from the beginning.
|
Hi @feanil, thanks for the thoughtful feedback! I agree with your approach. The new name is clearer and more accurate. For now, we can introduce the new setting and start the DEPR process by creating the corresponding ticket. I’ll begin working on that. Regarding the AI-guided review, I’m not able to find the comment you mentioned about the code cleanup. Could you please point me to it? |
f30ed36 to
b9b26cc
Compare
b9b26cc to
cc79b5d
Compare
…PROFILE_EXTENSION_FORM setting
cc79b5d to
350b9c1
Compare
9ea2a62 to
cecbb7c
Compare
Description
This PR enhances the extended profile fields functionality, allowing use the custom extended profile model in the account settings.
Currently, when we have extended profile fields associated with the
REGISTRATION_EXTENSION_FORMsetting, it works in this way:metafield of theUserProfilemodel, but are also stored in the custom model associated with the form. Here is the code.metafield of theUserProfilemodel, but not in/from the custom model.This PR introduces the possibility of storing those custom additional fields in the custom model when we update the Account Settings, maintaining parity and consistency similar to the registration process.
Testing instructions
Create a Tutor environment.
Create a mount of edx-platform with the changes in this PR.
Add the following property to the LMS Django Admin > Site Configurations >
local.openedx.io:8000{ "extended_profile_fields": [ "nickname", "interests", "wants_newsletter", "favorite_language" ] }Install this app that includes the custom extra fields mentioned above: https://github.com/bryanttv/custom-extra-fields. You can use this setting in your
config.ymlCreate a tutor inline plugin and enable it with
tutor plugins enable custom-settingsRun
tutor dev launchCreate a user in the platform.
Go to
{lms_domain}/api/user/v1/accounts/{username}in your browser. Be sure to use theapplication/merge-patch+jsonMedia type in the DRF UI.Test Cases
1. No Form (
REGISTRATION_EXTENSION_FORM = None) and no extended profile fieldsThe response includes the
extended_profilekey as an empty list. This list cannot be updated because no fields are defined in theextended_profile_fieldssetting, and there is no extended form either.{ "extended_profile": [], }2. No Form (
REGISTRATION_EXTENSION_FORM = None) and with extended profile fieldsThe fields defined in the
extended_profile_fieldssetting are returned and can be updated. However, these fields do not have any validation, meaning any value can be assigned to them.{ "extended_profile": [ { "field_name": "nickname", "field_value": true }, { "field_name": "interests", "field_value": 0 }, { "field_name": "wants_newsletter", "field_value": "of course" }, { "field_name": "favorite_language", "field_value": "spanish" } ] }3. With Form (
REGISTRATION_EXTENSION_FORM = "myapp.forms.ExtendedForm") and no extended profile fieldsFields defined in the form will be displayed in the legacy registration form if they are marked as required. However, if the
extended_profile_fieldsproperty in the Site Configuration is not defined, no fields will be returned from the endpoint.Still, the defined fields can be updated and will be validated according to the logic in the form.
{ "extended_profile": [], }4. With Form (
REGISTRATION_EXTENSION_FORM = "myapp.forms.ExtendedForm") and with extended profile fieldsWhen both settings are defined, the fields listed in
extended_profile_fieldsare returned, each with its corresponding validation as defined in the form.{ "extended_profile": [ { "field_name": "nickname", "field_value": "codewanderer" }, { "field_name": "interests", "field_value": "Programming" }, { "field_name": "wants_newsletter", "field_value": true }, { "field_name": "favorite_language", "field_value": "python" } ] }Deadline
None
Demo
extended-profile-fields-in-account-settings.mp4