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

Proposed backport for nutmeg Open edX Filters last batch #187

Closed
mariajgrimaldi opened this issue Jun 14, 2022 · 10 comments
Closed

Proposed backport for nutmeg Open edX Filters last batch #187

mariajgrimaldi opened this issue Jun 14, 2022 · 10 comments
Assignees
Milestone

Comments

@mariajgrimaldi
Copy link
Member

mariajgrimaldi commented Jun 14, 2022

Hello everyone!

Description

I'm opening this issue as a proposal for a backport to the latest release nutmeg, consisting of a few filters integrations that didn't make the nutmeg cut (for a variety of reasons):

Add filter before certificate creation starts

(cherry picked from commit openedx/edx-platform@e8fa890)

Add cohort change filter before moving users from cohorts

(cherry picked from commit openedx/edx-platform@465e5c0)

Add filter before certificate rendering process starts

(cherry picked from commit openedx/edx-platform@7f974d1)

Add filter before course dashboard rendering process starts

(cherry picked from commit openedx/edx-platform@895a649)

Add filter before course about rendering process starts

(cherry picked from commit openedx/edx-platform@ccfa0b4)

Integrate cohort assignment filter definition to cohort model

(cherry picked from commit openedx/edx-platform@ec69659)

This is the PR with those commits targeting nutmeg.

We are eager to know if these changes that the BTR wg would consider acceptable. I hope this is the right channel to start a discussion.

Rationale

Some weeks back, the BTR group and @felipemontoya discussed having this functionality in nutmeg, given that we were not going to be able to meet the deadline of the cut -for multiple reasons, mainly the feedback process of each implementation-. So we're pushing this forward since this functionality is vital for the platform extension.

Consequences

If a filter is not configured, then it acts like a no operation. So the platform can still operate as usual.

@felipemontoya
Copy link
Member

I'd like to add some of the previous reasons why we advocated to port this back to nutmeg.
The reason for the hooks framework is to generally allow for more maintainable extensions of the platform and reduce the necessity for some many forks out there.

Given the recent presentation of this framework in the conference and the hype we tried to build around it, we would like for devs to get their hands on this as soon as reasonable. Asking them to target master is not reasonable and waiting for olive seems like a missed opportunity for all the features that could already be built on top of a stronger foundation. Given that this change does not change anything in the data scheme and that ignoring it does not change anything in the platform's behaviour, we think is a good candidate for backporting.

@felipemontoya
Copy link
Member

@BbrSofiane would it make sense to tag this as "nutmeg release" ?

@BbrSofiane
Copy link
Member

Yes, I will assign it to the Nutmeg.2 Milestone.

We can also cherry-pick the change in tutor to make it available before the official release of Nutmeg.2 (9th of August).

@felipemontoya
Copy link
Member

Thanks @BbrSofiane, having this in nutmeg.2 would be great

@mariajgrimaldi
Copy link
Member Author

mariajgrimaldi commented Aug 5, 2022

Hello! After the last (august 1st) BTR meeting, @NeOneSoft and @jfavellar90 reached out to me to know the extent of reviewing this issue; after explaining, they agreed to help us move forward. We had a meeting where we tested all six filters and we documented each step:

First, we set up the environment with the branch MJG/backport-filters rebased on the latest Nutmeg release tag (nutmeg.master). Then, we installed the required packages:

  1. Latest release of the openedx-filters package 0.7.0, this library has all the six definitions we include here.
    pip install openedx-filters==0.7.0
  2. Master branch of the library where we implemented all steps definitions:
    pip install git+https://github.com/eduNEXT/openedx-filters-samples.git@master#egg=openedx_filters_samples
    And we tested each filter one by one with the following configuration:
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"
        ]
    },
}

& we started the testing. Here a detailed explanation:

  1. Certificate creation filter: after creating a course & setting up its certificates, we generated one certificate. Since the filter step -check implementation here- was implemented to stop the generation process, bad request 400 was returned to the learning MFE:
    cert-creation-stop-step

  2. Certificate render filter: we removed the configuration for the certificate creation and generated a certificate. Since the filter step was implemented -check implementation here- to stop the rendering process & render an invalid certificate:
    certificate-render-stopped

  3. Dashboard render filter: we then went to the dashboard. And since the filter step -check implementation here- was implemented to stop the generation process & render the 404 not found template:
    dashboard-filter-render-404

  4. Course about render filter: then we went to a course about an existent course. And since the filter step -check implementation here- was implemented to stop the rendering of the course about replacing the fragment with the 404 not found template:
    course-about-stopped

  5. Cohort assignment render filter: as an instructor of the course, we went to the instructor panel. Since the filter step -check implementation here- was implemented to stop the cohort assignment of a user:
    cohort-assignment-stop

  6. Cohort change render filter: as an instructor of the course, we went to the instructor panel. Since the filter step -check implementation here- was implemented to stop the cohort change of a user from a cohort to another:
    cohort-change-stop

Since the implementation of these PR(s) were already reviewed & supervised when targeting master, the objective of this exercise was to test the filter's functionality in the nutmeg context.

What do you think? @BbrSofiane

@BbrSofiane BbrSofiane moved this from To do to In progress in Build-Test-Release Working Group Aug 8, 2022
@BbrSofiane
Copy link
Member

BbrSofiane commented Aug 8, 2022

Hey, @mariajgrimaldi looks good to me!

I didn't know we would have filters that can alter the render cycles at this stage. That's pretty cool!

Can you use the dashboard filter to redirect to say a dashboard MFE?

@mariajgrimaldi
Copy link
Member Author

mariajgrimaldi commented Aug 8, 2022

Yes @BbrSofiane, you could use a filer like this:

class RedirectDifferentDashboard(PipelineStep):

    def run_filter(self, context, template_name):
        raise DashboardRenderStarted.RedirectToPage(
            "Redirecting to the dashboard MFE.",
            redirect_to="https://mfe-dashboard.com",
        )

And this configuration:

OPEN_EDX_FILTERS_CONFIG = {
    "org.openedx.learning.dashboard.render.started.v1": {
        "fail_silently": False,
        "pipeline": [
            "path.to.plugin.RedirectDifferentDashboard",
        ]
    },
}

To modify the dashboard rendering workflow

github-actions bot added a commit to openedx/edx-platform that referenced this issue Aug 8, 2022
<!--

🌰🌰
🌰🌰🌰🌰         🌰 Note: the Nutmeg master branch has been created.  Please consider whether your change
    🌰🌰🌰🌰     should also be applied to Nutmeg. If so, make another pull request against the
🌰🌰🌰🌰         open-release/nutmeg.master branch, or ping @nedbat for help or questions.
🌰🌰

Please give your pull request a short but descriptive title.
Use conventional commits to separate and summarize commits logically:
https://open-edx-proposals.readthedocs.io/en/latest/oep-0051-bp-conventional-commits.html

Use this template as a guide. Omit sections that don't apply. You may link to information rather than copy it.
More details about the template are at openedx/open-edx-proposals#180
(link will be updated when that document merges)
-->

## 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.
@BbrSofiane
Copy link
Member

I merged the PR. Thank you @NeOneSoft @jfavellar90 and @mariajgrimaldi

Repository owner moved this from 🏗 In Progress to ✅ Done in Build-Test-Release Working Group Aug 8, 2022
@mariajgrimaldi
Copy link
Member Author

Thank you guys! ❤️

@NeOneSoft
Copy link

Thank you all!!! @BbrSofiane @mariajgrimaldi @jfavellar90 !!!

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

No branches or pull requests

4 participants