[BD-32] feat: add filter before dashboard rendering process starts #29994

mariajgrimaldi · 2022-03-01T19:58:16Z

Description

As part of the Hooks Extension Framework implementation plan, this PR adds a filter before the dashboard rendering process starts.

Supporting information

Hooks Extension Framework OEP-50
ADR(s) on:
Open edX Filters naming and versioning: about how to identify filters and manage its versions
Open edX Filters configuration: how to configure filters
Open edX Filters tooling: what to use to run filters
Open edX Filters payload: input arguments for filters

Testing instructions

Install openedx-filters library:

pip install openedx-filters==0.6.0

Implement your pipeline steps in your favorite plugin. We created some as illustration in openedx-filters-samples. We'll be using those in this example.
Install openedx-filters-samples

pip install -e git+https://github.com/eduNEXT/openedx-filters-samples.git@master#egg=openedx_filters_samples

Configure your filters:
With this configuration, you won't be able to:

Render the dashboard. Instead, an error will be raised or a different dashboard template will be rendered (if you define its path here)

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

And with this one, you'll be able to filter course enrollments from the dashboard.

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

If you want to redirect to your custom dashboard, then:

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

If you want to return a custom response, then:

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

openedx-webhooks · 2022-03-01T19:58:21Z

Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1118 to keep track of it in Jira. More details are on the BD-32 project page.

When this pull request is ready, tag your edX technical lead.

felipemontoya

This changeset is working flawlessly. All the controls from PR#29996 are here and working as expected.

Great work @mariajgrimaldi. I think we can squash this and be ready for another reviewer.

I think that @MichaelRoytman could lend us a hand with a review from edX and maybe @pomegranited could bring some community eyes into this. Does any of you think that you could look at this?

pomegranited · 2022-04-05T05:14:17Z

@felipemontoya @mariajgrimaldi Cooool... I'd love a chance to play with your hooks extension framework!

But it'll likely be a couple weeks before I've got time to look at it, and I bet you can get it merged before then :) But I'll keep an eye on it and put in my 2c if there's time.

openedx-webhooks · 2022-04-05T13:25:07Z

Thanks for the pull request, @mariajgrimaldi! I've created BLENDED-1195 to keep track of it in Jira. More details are on the BD-32 project page.

When this pull request is ready, tag your edX technical lead.

felipemontoya · 2022-04-06T20:22:35Z

Thanks Jill, we'll be happy to have your feedback when time permits.

We would still like to get this over the line before the cut, so I'll just throw a long shot and see if @ormsbee or @justinhynes could give a look at this.

pomegranited

These content hooks are really powerful but so clean and simple.. it's a pleasure to play around with them and see them extended.

@mariajgrimaldi CC @felipemontoya I've suggested some changes, but the concept and implementation are great!

pomegranited · 2022-04-12T07:01:38Z

common/djangoapps/student/views/dashboard.py

+    except DashboardRenderStarted.RenderAlternativeDashboard as exc:
+        response = render_to_response(exc.dashboard_template, exc.template_context)
+    except DashboardRenderStarted.RedirectToPage as exc:
+        response = HttpResponseRedirect(exc.redirect_to or 'account_settings')


<lms_root>/account_settings 404s.. shouldn't this be:

Suggested change

response = HttpResponseRedirect(exc.redirect_to or 'account_settings')

response = HttpResponseRedirect(exc.redirect_to or reverse('account_settings'))

I absolutely missed this. Thank you so much!

pomegranited · 2022-04-12T07:02:49Z

common/djangoapps/student/views/dashboard.py

+
+
+class DashboardRenderNotAllowed(DashboardException):
+    pass


I don't see DashboardRenderNotAllowed or DashboardException used anywhere.. should these be removed?

Thanks for noting. I thought we could do something with this, but we just ignored it. I'll remove it!

Thanks @mariajgrimaldi ! Feel free to ping me for a 2nd review when you're ready?

Nudge @mariajgrimaldi CC @felipemontoya -- do you have an estimate for when you'll be able to address my comments?
Just planning my week, and want to make sure I allow time for a 2nd review. Thanks!

I'm so sorry for the delayed response; I got caught up with other stuff. I'll take some time this week to address your comments!

No worries @mariajgrimaldi, thanks for getting back to me :)

pomegranited · 2022-04-12T07:20:20Z

common/djangoapps/student/views/dashboard.py

+    try:
+        # .. filter_implemented_name: DashboardRenderStarted
+        # .. filter_type: org.openedx.learning.dashboard.render.started.v1
+        context, dashboard_template = DashboardRenderStarted.run_filter(
+            context=context, template_name=dashboard_template,
+        )
+    except DashboardRenderStarted.RenderAlternativeDashboard as exc:
+        response = render_to_response(exc.dashboard_template, exc.template_context)


It looks like, instead of raising DashboardRenderStarted.RenderAlternativeDashboard, a dashboard render filter could return something like this and surreptitiously change the template and/or context:

return { "context": context, "template_name": "alternative_template.html", }

It's cleaner if everything is handled using the exception mechanism, even changes to the context dict:

Suggested change

try:

# .. filter_implemented_name: DashboardRenderStarted

# .. filter_type: org.openedx.learning.dashboard.render.started.v1

context, dashboard_template = DashboardRenderStarted.run_filter(

context=context, template_name=dashboard_template,

)

except DashboardRenderStarted.RenderAlternativeDashboard as exc:

response = render_to_response(exc.dashboard_template, exc.template_context)

try:

# .. filter_implemented_name: DashboardRenderStarted

# .. filter_type: org.openedx.learning.dashboard.render.started.v1

DashboardRenderStarted.run_filter(

context=context, template_name=dashboard_template,

)

except DashboardRenderStarted.RenderAlternativeDashboard as exc:

response = render_to_response(exc.dashboard_template, exc.template_context)

This design was intended as the following:
Happy path -> modify return values to render a valid dashboard but modified
Unhappy path -> modify the return values through exceptions
For clarity we could change:
RenderAlternativeDashboard
For:
RenderInvalidDashboard?
Does this make sense? @pomegranited @felipemontoya

That naming change would make things clearer @mariajgrimaldi , good suggestion. Also some docs to explain the expected usage?

We mentioned it in the OEP-50:

Filters on the other hand are passed data and are expected to return something. This something will be used by the platform code. The most common case is to be passed data and return the same of somewhat changed, but similar data. Other common case would be to receive data and raise an exception in accordance with the definition of the hook. Most likely to completely halt the process that would happen after the hook.

But it would be useful to add it in a straightforward way to the opened-filters documentation.

Again, thank you for the great suggestions! 😄

Ah cool, I haven't read all the OEPs :) I'd be fine with just referencing the relevant ones in your openedx-filters README, and then leave it to the users of the library to read up on the details there. No need to re-write what's already been written and reviewed :)

This is working great now :) But we need a small update to

https://github.com/eduNEXT/openedx-filters-samples/blob/master/openedx_filters_samples/samples/pipeline.py#L525 -- needs to raise DashboardRenderStarted.RenderInvalidDashboard.

pomegranited · 2022-04-12T07:25:49Z

common/djangoapps/student/tests/test_filters.py

+        """
+        response = self.client.get(self.dashboard_url)
+
+        self.assertEqual(status.HTTP_302_FOUND, response.status_code)


Should also test that the redirect is headed to the expected location, both when redirect_to is set on the exception, and when it isn't (and falls back to redirecting to the account settings).

Great catch! I'll add that check

Yay thank you @mariajgrimaldi :) Could you also add a check that's like TestRedirectDashboardPageStep, but without the redirect_to value set, so we can see the default redirection to account_settings?

pomegranited · 2022-04-12T07:36:44Z

common/djangoapps/student/tests/test_filters.py

+        """
+        response = self.client.get(self.dashboard_url)
+
+        self.assertContains(response, "This is a custom response.")


Can go even harder here:

Suggested change

self.assertContains(response, "This is a custom response.")

self.assertEqual("This is a custom response.", response.content.decode("utf-8"))

Thank you! :)

mariajgrimaldi · 2022-04-26T12:39:13Z

So many good suggestions @pomegranited! Thank you so much. I'll be committing your suggestions this week.

mariajgrimaldi · 2022-05-05T21:50:42Z

Hello there @pomegranited! I think I got all of your suggestions! Again, thanks for such valuable insight 🥳

pomegranited · 2022-05-09T05:36:21Z

👍 Thank you for your changes @mariajgrimaldi ! This is working really smoothly now.

I tested this on my devstack using the PR testing instructions and some edge cases noticed when reading the code.
I read through the code
~~I checked for accessibility issues~~ - N/A
~~Includes documentation~~ - N/A -- see relevant OEPs

* Add dashboard filter to dashboard student's view * Add tests/docs for filter's integration

openedx-webhooks · 2022-05-09T18:58:13Z

@mariajgrimaldi 🎉 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 · 2022-05-09T21:25:56Z

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

edx-pipeline-bot · 2022-05-09T22:32:08Z

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

@nedbat

## Description Backport filters that didn't make it to nutmeg release: **Add filter before certificate creation starts** (cherry picked from commit e8fa890) **Add cohort change filter before moving users from cohorts** (cherry picked from commit 465e5c0) **Add filter before certificate rendering process starts** (cherry picked from commit 7f974d1) **Add filter before course dashboard rendering process starts** (cherry picked from commit 895a649) **Add filter before course about rendering process starts** (cherry picked from commit ccfa0b4) **Integrate cohort assignment filter definition to cohort model** (cherry picked from commit ec69659) ## Supporting information Refer to the BTR wg github issue for the rationale behind this PR: openedx/wg-build-test-release#187 ## Testing instructions 1. Install the needed library release: `openedx-filters==0.7.0` 2. Install the samples library: `pip install git+https://github.com/eduNEXT/openedx-filters-samples.git@master#egg=openedx_filters_samples` 3. Then, configure each filter. If you want to test all the filters simultaneously, use this configuration and try to do each operation the filter is related to; the filter sample step will stop the operation. ``` OPEN_EDX_FILTERS_CONFIG = { "org.openedx.learning.certificate.creation.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCertificateCreation" ] }, "org.openedx.learning.cohort.change.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCohortChange" ] }, "org.openedx.learning.certificate.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeCertificate", ] }, "org.openedx.learning.dashboard.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeDashboard", ] }, "org.openedx.learning.course_about.render.started.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.RenderAlternativeCourseAbout", ] }, "org.openedx.learning.cohort.assignment.requested.v1": { "fail_silently": False, "pipeline": [ "openedx_filters_samples.samples.pipeline.StopCohortAssignment" ] }, } ``` Please, for detailed instructions on how to test each filter, refer to each of these PR(s): Filter for certificate creation: #29949 Filter for cohort change: #29964 Filter for certificate rendering: #29976 Filter for dashboard rendering: #29994 Filter for course about rendering: #29996 Filter for cohort assignment: #30431 ## Deadline For the next nutmeg release.

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 1, 2022

mariajgrimaldi changed the title ~~[WIP] [BD-32] feat: feat: add filter before dashboard rendering process starts~~ [WIP] [BD-32] feat: add filter before dashboard rendering process starts Mar 1, 2022

mariajgrimaldi changed the title ~~[WIP] [BD-32] feat: add filter before dashboard rendering process starts~~ [BD-32] feat: add filter before dashboard rendering process starts Mar 1, 2022

mariajgrimaldi marked this pull request as ready for review March 1, 2022 20:55

openedx-webhooks added needs triage waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 1, 2022

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from df0dddc to f4e433e Compare March 2, 2022 12:23

mariajgrimaldi mentioned this pull request Mar 2, 2022

[BD-32] feat: add 2nd batch of Open edX Filters #29840

Closed

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch 3 times, most recently from 87b6f84 to 30a58a7 Compare April 4, 2022 15:45

felipemontoya approved these changes Apr 5, 2022

View reviewed changes

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from 30a58a7 to 784ba2e Compare April 5, 2022 13:25

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from 784ba2e to c259ca7 Compare April 7, 2022 14:04

pomegranited requested changes Apr 12, 2022

View reviewed changes

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from f648338 to 36797ab Compare April 28, 2022 22:35

mariajgrimaldi mentioned this pull request May 4, 2022

refactor: change dashboard/course about exceptions naming for clarity openedx/openedx-filters#33

Merged

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch 2 times, most recently from acf101a to 202aed9 Compare May 5, 2022 00:03

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from e43fa27 to e736ae3 Compare May 9, 2022 16:03

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch 3 times, most recently from 086bf60 to 22cd7de Compare May 9, 2022 18:30

feat: add filter before course dashboard rendering process starts

895a649

* Add dashboard filter to dashboard student's view * Add tests/docs for filter's integration

mariajgrimaldi force-pushed the MJG/dashboard-render-filter branch from 22cd7de to 895a649 Compare May 9, 2022 18:40

mariajgrimaldi merged commit 5985b6a into openedx:master May 9, 2022

openedx-webhooks added merged and removed needs triage labels May 9, 2022

mariajgrimaldi mentioned this pull request Jun 14, 2022

[BD-32] feat: Open edX Filters backport #30583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BD-32] feat: add filter before dashboard rendering process starts #29994

[BD-32] feat: add filter before dashboard rendering process starts #29994

mariajgrimaldi commented Mar 1, 2022 •

edited

Loading

openedx-webhooks commented Mar 1, 2022 •

edited

Loading

felipemontoya left a comment

pomegranited commented Apr 5, 2022

openedx-webhooks commented Apr 5, 2022

felipemontoya commented Apr 6, 2022

pomegranited left a comment

pomegranited Apr 12, 2022

mariajgrimaldi Apr 12, 2022

pomegranited Apr 12, 2022

mariajgrimaldi Apr 12, 2022

pomegranited Apr 18, 2022

pomegranited Apr 26, 2022

mariajgrimaldi Apr 26, 2022

pomegranited Apr 26, 2022

pomegranited Apr 12, 2022

mariajgrimaldi Apr 26, 2022

pomegranited Apr 26, 2022

mariajgrimaldi Apr 28, 2022

pomegranited Apr 29, 2022

pomegranited May 9, 2022

pomegranited Apr 12, 2022

mariajgrimaldi Apr 26, 2022

pomegranited Apr 29, 2022

pomegranited Apr 12, 2022

mariajgrimaldi Apr 26, 2022

mariajgrimaldi commented Apr 26, 2022

mariajgrimaldi commented May 5, 2022

pomegranited commented May 9, 2022

openedx-webhooks commented May 9, 2022

edx-pipeline-bot commented May 9, 2022

edx-pipeline-bot commented May 9, 2022

	response = HttpResponseRedirect(exc.redirect_to or 'account_settings')
	response = HttpResponseRedirect(exc.redirect_to or reverse('account_settings'))

	self.assertContains(response, "This is a custom response.")
	self.assertEqual("This is a custom response.", response.content.decode("utf-8"))

[BD-32] feat: add filter before dashboard rendering process starts #29994

[BD-32] feat: add filter before dashboard rendering process starts #29994

Conversation

mariajgrimaldi commented Mar 1, 2022 • edited Loading

Description

Supporting information

Testing instructions

openedx-webhooks commented Mar 1, 2022 • edited Loading

felipemontoya left a comment

Choose a reason for hiding this comment

pomegranited commented Apr 5, 2022

openedx-webhooks commented Apr 5, 2022

felipemontoya commented Apr 6, 2022

pomegranited left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariajgrimaldi commented Apr 26, 2022

mariajgrimaldi commented May 5, 2022

pomegranited commented May 9, 2022

openedx-webhooks commented May 9, 2022

edx-pipeline-bot commented May 9, 2022

edx-pipeline-bot commented May 9, 2022

mariajgrimaldi commented Mar 1, 2022 •

edited

Loading

openedx-webhooks commented Mar 1, 2022 •

edited

Loading