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

feat: add pipelines to add custom registrations fields and custom options in account settings #224

Closed
wants to merge 2 commits into from

Conversation

Henrrypg
Copy link
Contributor

@Henrrypg Henrrypg commented Nov 16, 2022

Description

This PR is to add a pipeline for AccountSettingsRenderStarted filter, we added AddCustomOptionsOnAccountSettings pipeline. It implements custom options on account settings context added by EDNX_CUSTOM_REGISTRATION_FIELDS setting.

This pipeline is following https://edunext.atlassian.net/browse/DS-303 ticket and JU-11 Saas migration step.

Testing instructions

Add in your tenant settings:

   "EDNX_CUSTOM_REGISTRATION_FIELDS": [
       {
           "label": "Document Type",
           "name": "type_document",
           "options": [
               "DNI",
               "PASSPORT",
               "ID"
           ],
           "type": "select"
       },
       {
           "label": "Document Number",
           "name": "personal_id",
           "type": "text"
       },
       {
           "label": "Phone number",
           "name": "mobile",
           "type": "text"
       },
       {
           "label": "Address",
           "name": "address",
           "type": "text"
       },
       {
           "label": "Company Name",
           "name": "company",
           "type": "text"
       }
   ],
   "REGISTRATION_EXTRA_FIELDS": {
       "address": "required",
       "company": "optional",
       "country": "hidden",
       "gender": "required",
       "goals": "hidden",
       "honor_code": "hidden",
       "level_of_education": "hidden",
       "mailing_address": "hidden",
       "mobile": "required",
       "personal_id": "required",
       "terms_of_service": "required",
       "type_document": "required",
       "year_of_birth": "hidden"
   },
   "REGISTRATION_FIELD_ORDER": [
       "gender",
       "type_document",
       "personal_id",
       "email",
       "mobile",
       "address",
       "company"
   ],
   "extended_profile_fields": [
       "type_document",
       "personal_id",
       "mobile",
       "address",
       "company"
   ],
   "OPEN_EDX_FILTERS_CONFIG": {
        "org.openedx.learning.student.registration.render.started.v1": {
            "fail_silently": false,
            "pipeline": [
                "openedx.core.djangoapps.user_authn.views.registration_form.AddCustomFieldsBeforeRegistration"
            ]
        },
        "org.openedx.learning.student.settings.render.started.v1": {
                  "fail_silently": false,
                  "pipeline": [
                      "openedx.core.djangoapps.user_api.accounts.settings_views.AddCustomOptionsOnAccountSettings"
                  ]
              }
          }

Now you look at registration form in http://{lms.base}:8000/register

image

in http://{lms.base}:8000/admin/auth/user/

image

In http://{lms.base}:8000/account/settings you should look your custom field types, for this example Type Document as List instead of plain text.

image

You can look full working example of this functionality, screenshots and testing instructions at eduNEXT/edunext-platform#691 PR.

Additional information

Currently we have two PRs in upstream to implements its filter and hook.

Checklist for Merge

  • Tested in a remote environment
  • Upstream openedx-filters and edx-platform PRs was merged
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@Henrrypg Henrrypg self-assigned this Nov 16, 2022
@Henrrypg Henrrypg force-pushed the hpg/add_registration_settings_pipeline branch from b75b2d6 to 50e62fe Compare November 16, 2022 14:42
@MaferMazu
Copy link
Contributor

I think you need to add openedx-filters as a requirement and update the requirements.

On the other hand, I think it's important to test this pipelines. Here you have some examples:

I know having tests probably slows down the migration process; maybe we can add tests after the migration, but sometimes we forget that technical debt. I think we can try to test it, but if it takes too long, we can commit ourselves to do it later.

What do you think @Henrrypg?

@Henrrypg Henrrypg force-pushed the hpg/add_registration_settings_pipeline branch 4 times, most recently from 2533083 to bdf2d4a Compare November 17, 2022 22:21
@Henrrypg
Copy link
Contributor Author

Henrrypg commented Nov 17, 2022

I think you need to add openedx-filters as a requirement and update the requirements.

On the other hand, I think it's important to test this pipelines. Here you have some examples:

I know having tests probably slows down the migration process; maybe we can add tests after the migration, but sometimes we forget that technical debt. I think we can try to test it, but if it takes too long, we can commit ourselves to do it later.

What do you think @Henrrypg?

Hello @MaferMazu

Yes, i made some changes in requirements.

About tests... i was looking and testing some ideas to make it, but to make successful test we need to use openedx-filter and add OPEN_EDX_FILTERS_CONFIG, the problem is: the filters are not currently merged in openedx-filters (openedx/openedx-filters#46) and use more dependencies or mock these filters i think that is work to refactor in the future. What do you think?

@MaferMazu
Copy link
Contributor

I think it's better to retake this after migration. I prefer we merge the things in this order:

I am not too agree with merging this right now.

@Henrrypg
Copy link
Contributor Author

I think it's better to retake this after migration. I prefer we merge the things in this order:

I am not too agree with merging this right now.

Sure @MaferMazu, i think that too, we can wait to the others PRs and then make effort to close this one successful

@Henrrypg Henrrypg marked this pull request as draft November 28, 2022 19:37
@Henrrypg Henrrypg force-pushed the hpg/add_registration_settings_pipeline branch from bdf2d4a to 9750b7c Compare December 1, 2022 17:17
@Henrrypg Henrrypg marked this pull request as ready for review December 1, 2022 17:20
@Henrrypg Henrrypg marked this pull request as draft December 5, 2022 13:21
@MaferMazu
Copy link
Contributor

@Henrrypg, do you think we can continue with this work, or what's next with this? What effort do we need to make to finish this?

@Henrrypg
Copy link
Contributor Author

Hello @MaferMazu My first approach to retake this PR is:

  • Re base in needed
  • Update openedx-filters version to the real one where this filter works v1.2.0
  • do make upgrade
  • set up local installation to test the pipeline (I'm nor sure for which cut was the hook)

@Alec4r
Copy link
Member

Alec4r commented May 10, 2023

Hi @Henrrypg (@MaferMazu), I wanna ask you if you are planning work in this Draft, or maybe can we close it?

@Henrrypg
Copy link
Contributor Author

It shouldn't be closed @Alec4r. This effort is necesary, i'm some out of context with this one and i'd prefer to someone of Dédalo take this PR. But if not, i can update this one in a couple of weeks.

@Asespinel
Copy link
Contributor

@Henrrypg Hi Henry, do you have any updates on this PR? Or maybe we need to review this changes in a future Sprint?

@JuanDavidBuitrago
Copy link
Member

I'm going to continue working on it, it has to be merged in this sprint

@JuanDavidBuitrago JuanDavidBuitrago force-pushed the hpg/add_registration_settings_pipeline branch 2 times, most recently from 0f076aa to ae99c08 Compare July 22, 2023 04:35
@JuanDavidBuitrago JuanDavidBuitrago force-pushed the hpg/add_registration_settings_pipeline branch from ae99c08 to a011a31 Compare July 22, 2023 04:51
@JuanDavidBuitrago
Copy link
Member

I have to convert this PR to draft. The pipeline is not working as the filter and the platform expected. We have to work again in how to remove JU-11 in our custom features list. I created a JIRA card to solve in one of ours sprints.

@JuanDavidBuitrago JuanDavidBuitrago marked this pull request as draft August 3, 2023 21:38
@Asespinel
Copy link
Contributor

@santiagosuarezedunext friendly reminder to check this on future sprints

@luisfelipec95
Copy link
Contributor

I talked with @Henrrypg and this PR lost his context and i will close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants