Add openedx-filters hook to account settings before rendering it context by Henrrypg · Pull Request #31295 · openedx/openedx-platform

Henrrypg · 2022-11-16T02:29:52Z

Description

This PR introduces a new openedx-filters hook:

org.openedx.learning.student.settings.render.started.v1 hook in the platform at openedx/core/djangoapps/user_api/accounts/settings_views.py. This is implemented to hook the account settings context.

Users impacted by the change:

Users: Allow to get customized account settings.

Supporting information

openedx-filter: In this PR #46 is implemented this filter.

Testing instructions

You can have an example of this filter with its hook working in edunext-platform

You also have an example in openedx-filters-samples in this PR: eduNEXT/openedx-filters-samples#1

Deadline

None

Other information

Once accepted, this should be merged only after openedx/openedx-filters#46 is merged and the requirements are updated.

openedx-webhooks · 2022-11-16T02:29:57Z

Thanks for the pull request, @Henrrypg! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

mariajgrimaldi · 2022-12-05T10:22:42Z

Hello! Thanks for your contribution. Please, add a testing example that works for this PR (not for edunext-platform), so it's easier for reviewers. You could use https://github.com/eduNEXT/openedx-filters-samples

Henrrypg · 2022-12-05T21:21:16Z

@mariajgrimaldi As you suggested, i opened a PR in openedx-filters-samples with a testing example. What do you think?

mphilbrick211 · 2022-12-16T20:47:03Z

Hi @Henrrypg! Just seeing if you are planning to re-run the failing checks on this?

CC: @mariajgrimaldi for review.

Henrrypg · 2022-12-19T15:54:39Z

Hello @mphilbrick211, i was working first with openedx/openedx-filters#46, thats why the checks are failling.

mariajgrimaldi · 2022-12-20T11:43:24Z

I was about to test this in conjunction the other PR (openedx-filters PR 46) but then realized this is not going to work. ErrorFilteringContext is not defined as one of the filter exceptions. Also, try to follow this implementation and adding unittests for this :)

mphilbrick211 · 2023-01-23T22:55:43Z

Hi @Henrrypg! Just checking in to see if you are planning to pursue this pull request?

Henrrypg · 2023-01-24T20:46:33Z

Hello @mphilbrick211 @mariajgrimaldi i just pushed a commit with the changes requested by @mariajgrimaldi, unitests and fix the implementation.

mariajgrimaldi · 2023-01-31T13:56:35Z

You're missing one exception. Please, use the other implementations as a guide.

mariajgrimaldi · 2023-01-31T14:16:24Z

I'd use these tests as a template for testing this kind of filters:

test_account_render_filter_executed test_account_render_alternative test_account_redirect test_account_redirect_default test_account_custom_response test_account_render_without_filter_config

Those are precisely the same as here. I know this needs to be documented somewhere -we're working on it- but again, you could use them as a guide.

Henrrypg · 2023-02-17T18:41:11Z

How you suggested @mariajgrimaldi, i took your example filter and made some changes in favor of that. one exception was missing in filters pr but i already added it.

mariajgrimaldi · 2023-02-20T19:43:25Z

I'll check on this again tomorrow! After reviewing openedx/openedx-filters#46

mariajgrimaldi · 2023-02-28T16:05:27Z

@Henrrypg: can you rebase with the base branch? Also, I left a comment here: eduNEXT/openedx-filters-samples#1. It'd be convenient if you fixed it so we can test it easily :)

mariajgrimaldi · 2023-03-01T14:49:06Z

openedx-filters v1.2.0 was released already!

Henrrypg · 2023-03-01T16:19:55Z

@mariajgrimaldi Sure! I'll work on this

Henrrypg · 2023-03-06T14:46:48Z

@mariajgrimaldi I rebased with master and eduNEXT/openedx-filters-samples#1 is already merged :)

mariajgrimaldi · 2023-03-07T18:41:09Z

These are the instructions I followed:

Install the latest openedx-filters package
Install openedx-filters-samples
Add the following settings to your environment:

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.student.settings.render.started.v1": {
        "fail_silently": False,
        "pipeline": [
            "openedx_filters_samples.samples.pipeline.StopAccountSettingsRender"
        ]
    }
}

Deactivate the account MFE (this filter applies just to the legacy account view)
Since the filter redirects to "", you should stay on the current page.

This looks good. The only thing that worries me is our maintenance plan for filters used in legacy pages. cc @felipemontoya

mariajgrimaldi · 2023-03-07T18:42:30Z

@Henrrypg: can you please rebase with master and fix conflicts?

Henrrypg · 2023-03-07T20:26:26Z

Is done @mariajgrimaldi, checks are failing because openedx-filters version in master

mariajgrimaldi · 2023-03-07T20:30:50Z

@Henrrypg: can you bump openedx-filters to 1.2.0? that's the latest release which has the filter you're including

felipemontoya · 2023-03-07T22:11:18Z

The only thing that worries me is our maintenance plan for filters used in legacy pages.

As a user of a filter what I would expect:

for the page is being removed, I am notified for one stable release
when possible, the filter is used in the new replacement page or the API that MFEs call for the same feature. Otherwise a different but equivalent filter exists for the replacement page.

Do you think one or both of those are applicable here?

mariajgrimaldi · 2023-03-08T17:36:19Z

As a user of a filter what I would expect:

for the page is being removed, I am notified for one stable release

when possible, the filter is used in the new replacement page or the API that MFEs call for the same feature. Otherwise a different but equivalent filter exists for the replacement page.

Do you think one or both of those are applicable here?

That sounds good! I will take those ideas into our documentation so it's clear for future implementations. I'm attaching the DEPR ticket for account pages here for better tracking.

I also created an issue in openedx-filters: openedx/openedx-filters#73

mariajgrimaldi · 2023-03-08T18:03:06Z

@Henrrypg: I'll approve once tests pass :). Let me know!

Henrrypg · 2023-03-08T20:41:29Z

@mariajgrimaldi Is ready to test now :)

Henrrypg · 2023-03-09T15:15:36Z

@mariajgrimaldi i just rebased and checks are passing :)

mariajgrimaldi

LGTM

openedx-webhooks · 2023-03-09T17:17:50Z

@Henrrypg 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2023-03-09T18:16:30Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2023-03-09T18:38:38Z

EdX Release Notice: This PR has been deployed to the production environment.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 16, 2022

Henrrypg marked this pull request as ready for review November 18, 2022 14:58

Henrrypg marked this pull request as draft November 22, 2022 16:49

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch from 1c2110d to 0501284 Compare November 30, 2022 17:30

Henrrypg changed the title ~~Add openedx-filters hook to registration before rendering of form description and account settings before rendering it context~~ Add openedx-filters hook to account settings before rendering it context Nov 30, 2022

Henrrypg marked this pull request as ready for review November 30, 2022 17:44

Henrrypg mentioned this pull request Dec 5, 2022

feat: add example for AccountSettingsRenderStarted filter eduNEXT/openedx-filters-samples#1

Merged

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 16, 2022

mariajgrimaldi reviewed Dec 20, 2022

View reviewed changes

openedx-webhooks removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 20, 2022

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch 2 times, most recently from 5760e2c to 8b1c90a Compare January 24, 2023 20:43

mariajgrimaldi self-assigned this Jan 31, 2023

mariajgrimaldi requested changes Jan 31, 2023

View reviewed changes

mariajgrimaldi reviewed Jan 31, 2023

View reviewed changes

mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 1, 2023

mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 21, 2023

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch from 53f81b8 to 87b9b4b Compare March 6, 2023 14:38

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch from 87b9b4b to 5cbceee Compare March 7, 2023 20:21

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch from 4107b3f to bec74b2 Compare March 8, 2023 17:42

Henrrypg added 6 commits March 9, 2023 09:49

feat: add filter hooks before regitration render and settings context

99dd634

fix: add exceptions and test to account settings filter

98333c8

fix: change implementation and add tests

c2523c6

fix: fix tests

b056cbf

chore: upgrade openedx-filters

4a0b6f2

fix: add auth on test filters

776e65b

Henrrypg force-pushed the hpg/add-filters-hook-registrationrenred-settingsrender branch from e8ea4d3 to 776e65b Compare March 9, 2023 14:49

mariajgrimaldi approved these changes Mar 9, 2023

View reviewed changes

mariajgrimaldi merged commit d7ca7c1 into openedx:master Mar 9, 2023

mariajgrimaldi deleted the hpg/add-filters-hook-registrationrenred-settingsrender branch March 10, 2023 13:09

JuanDavidBuitrago mentioned this pull request Jul 21, 2023

feat(JU-11): add openedx-filters hook to account settings before rendering it context (DS-602) eduNEXT/edunext-platform#769

Merged

Uh oh!

Conversation

Henrrypg commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Other information

Uh oh!

openedx-webhooks commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Henrrypg commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mphilbrick211 commented Dec 16, 2022

Uh oh!

Henrrypg commented Dec 19, 2022

Uh oh!

mariajgrimaldi Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphilbrick211 commented Jan 23, 2023

Uh oh!

Henrrypg commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mariajgrimaldi Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mariajgrimaldi Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Henrrypg commented Feb 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi commented Feb 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi commented Feb 28, 2023

Uh oh!

mariajgrimaldi commented Mar 1, 2023

Uh oh!

Henrrypg commented Mar 1, 2023

Uh oh!

Henrrypg commented Mar 6, 2023

Uh oh!

mariajgrimaldi commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi commented Mar 7, 2023

Uh oh!

Henrrypg commented Mar 7, 2023

Uh oh!

mariajgrimaldi commented Mar 7, 2023

Uh oh!

felipemontoya commented Mar 7, 2023

Uh oh!

mariajgrimaldi commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariajgrimaldi commented Mar 8, 2023

Uh oh!

Henrrypg commented Mar 8, 2023

Uh oh!

Henrrypg commented Mar 9, 2023

Uh oh!

mariajgrimaldi left a comment

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Mar 9, 2023

Uh oh!

edx-pipeline-bot commented Mar 9, 2023

Henrrypg commented Nov 16, 2022 •

edited

Loading

openedx-webhooks commented Nov 16, 2022 •

edited

Loading

mariajgrimaldi commented Dec 5, 2022 •

edited

Loading

Henrrypg commented Dec 5, 2022 •

edited

Loading

mariajgrimaldi Dec 20, 2022 •

edited

Loading

Henrrypg commented Jan 24, 2023 •

edited

Loading

Henrrypg commented Feb 17, 2023 •

edited

Loading

mariajgrimaldi commented Feb 20, 2023 •

edited

Loading

mariajgrimaldi commented Mar 7, 2023 •

edited

Loading

mariajgrimaldi commented Mar 8, 2023 •

edited

Loading