Skip to content
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

docs: add how-to for extending registration form with dynamic fields #1238

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

GlugovGrGlib
Copy link
Member

Description

This PR is adding how-to guide for extending registration form with custom filed in Authn MFE.

Additional details

Closing issue #923.

@GlugovGrGlib GlugovGrGlib requested a review from a team as a code owner April 19, 2024 21:28
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 19, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 19, 2024

Thanks for the pull request, @GlugovGrGlib!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To 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:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-infinity. Tag them in a comment and let them know that your changes are ready for review.

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:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@mphilbrick211 mphilbrick211 added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label May 1, 2024
@mphilbrick211
Copy link

Hi @openedx/2u-vanguards! Just following up on this!

Copy link
Member

@ghassanmas ghassanmas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are the exact steps I had to go through in order to enable an extra field(s).

Note: This below example and cause, is for when the field that is required to be activated is already is this list EXTRA_FIELDS.

The fields that are in EXTRA_FIELDS are already exists as column in userprofile model, but are disbaled, so when adding other field that don't exits, a step to do data migration is required.

Tutor file plugin to set ENABLE_DYNAMIC_REGISTRATION_FIELDS it at LMS/backend:

from tutor import hooks 

hooks.Filters.ENV_PATCHES.add_item(
  ("openedx-lms-common-settings",
  """
ENABLE_DYNAMIC_REGISTRATION_FIELDS = True
  """
  )
)

Then change site_configuration in django admin for:
1- ENABLE_DYNAMIC_REGISTRATION_FIELDS at MFE_CONFIG level
2- Set REGISTRATION_EXTRA_FIELDS (as example in sippest below we flag country and gender as required which are by default hidden, it could also be as optional)

{
   "REGISTRATION_EXTRA_FIELDS": {
       "country": "required",
       "gender": "required"
   },
   "MFE_CONFIG": {
       "ENABLE_DYNAMIC_REGISTRATION_FIELDS": "true"
   }
}

I think this PR as it's handles the case when a new field is to add/enable that doesn't exists in EXTRA_FIELDS.

@mphilbrick211
Copy link

Hi @openedx/vanguards @openedx/vanguards - Friendly ping on this. We'll rerun tests when we confirm if someone can review from your team. Thanks!

docs/how_tos/adding_custom_fields_to_the_registration.rst Outdated Show resolved Hide resolved
Comment on lines 23 to 32
{
"ENABLE_DYNAMIC_REGISTRATION_FIELDS": "true",
"MFE_CONFIG": {
"ENABLE_DYNAMIC_REGISTRATION_FIELDS": "true"
},
"REGISTRATION_EXTRA_FIELDS": {
"country": "required",
"gender": "required"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't so much readable in the preview. Could you please fix it?

- All possible fields can be find in `EXTRA_FIELDS <https://github.com/openedx/edx-platform/blob/a9355852edede9662762847e0d168663083fc816/openedx/core/djangoapps/user_authn/api/helper.py#L20-L39>`_.
- REST API gets cached, if you are in hurry though you can do this command: `tutor local exec redis redis-cli flushall`.
- Or you can wait a few minutes until the cache is invalidated.
- After this, the new fields should appear on the Auth page.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT:
Can we also mention that the required fields will appear on the registration form and optional fields will appear on progressive profiling form?

Comment on lines 45 to 52
.. code-block:: python
ENABLE_DYNAMIC_REGISTRATION_FIELDS = "true"

MFE_CONFIG["ENABLE_DYNAMIC_REGISTRATION_FIELDS"] = "true"

REGISTRATION_EXTRA_FIELDS["country"] = "required"

REGISTRATION_EXTRA_FIELDS["gender"] = "required"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please fix its preview?

Comment on lines 54 to 55
`In total, you must redefine 3 constants using the method that is most preferable for you:` **ENABLE_DYNAMIC_REGISTRATION_FIELD = True, MFE_CONFIG["ENABLE_DYNAMIC_REGISTRATION_FIELDS"] = True, REGISTRATION_EXTRA_FIELDS["field_name"] = "required/optionl/hidden"**.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please write these CONSTS in a code block for better preview.

docs/how_tos/adding_custom_fields_to_the_registration.rst Outdated Show resolved Hide resolved
@syedsajjadkazmii
Copy link
Contributor

Hi @GlugovGrGlib,

Thanks for the contribution. This looks good—just some small thoughts.

@KyryloKireiev KyryloKireiev force-pushed the glugovgrglib/docs_extending_registration_form branch from 39bd8d3 to cf2877b Compare September 6, 2024 09:25
@KyryloKireiev
Copy link

Hi @syedsajjadkazmii!
I tried to fix the problems you mentioned above. Please take a look when you have the opportunity

REGISTRATION_EXTRA_FIELDS["field_name"] = "required/optionl/hidden"


**B. Add fields that do not exist in the user profile model**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @GlugovGrGlib

I was working on an extended registration form for an openedx instance and face some issues to make it work along with the authn MFE. While doing some research on the possible workarounds I came across with this PR.

Just want to know if you have had the opportunity to test the implementation of an extended registration form along with this MFE? Because I found a conflict between the django model/form definitions and the MFE structure and the way the MFE handles the fields definition.

The purpose of this whole comment is to double check if the steps documented in this B section were already proven since I've follow the exact same steps and haven't make it work, so it would be cool if we confirm that the Authn MFE supports the extended forms or if it's a future work needed to be doing in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U waiting for eng review PR is ready for review. Review and merge it, or suggest changes.
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

7 participants