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: adds FilterTenantAwareLinksFromStudio in filters pipeline #219

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

Conversation

jignaciopm
Copy link
Contributor

@jignaciopm jignaciopm commented Oct 1, 2024

Description

This PR adds a custom filter pipeline to get LMS_ROOT_URL value for org from tenant site.

The idea is including some changes in openedx-filters and edx-platform repositories to anyone implement the filter function, and one clear example to use is this custom pipeline on eox-tenant plugin.

Dependencies:

Testing instructions

  1. Install the plugin with the changes in openedx/edx-platform and openedx/openedx-filters branches. Use the Tutor instructions to install the eox-tenant django plugin with the changes in this branch.

  2. Create a Tenant and import a certificate course (You need to create a tenant for Studio too). Go on to create sites: http://local.edly.io:8000/admin/eox_tenant/tenantconfig/

  3. Add the following configurations to your configuration site and Studio tenant.

"OPEN_EDX_FILTERS_CONFIG": {
                "org.openedx.learning.course_about.page.url.requested.v1": {
                    "fail_silently": false,
                    "pipeline": [
                        "eox_tenant.filters.pipeline.OrgAwareCourseAboutPageURL"
                    ]
                },
                "org.openedx.course_authoring.lms.page.url.requested.v1": {
                    "fail_silently": false,
                    "pipeline": [
                        "eox_tenant.filters.pipeline.OrgAwareLMSURLStudio"
                    ]
                }
            }

or too you can add settings in /env/apps/openedx/settings/cms/development.py

  1. Go to Settings-> Schedule & Details, click on the “Course Summary Page” link, which should direct you to the URL of the specific tenant
    Captura de pantalla de 2024-10-01 23-39-30

  2. Verify that the link of the "Invite your students" button in the body refers to the URL of the specific tenant

  3. Also check for a link to a course summary below the “course overview” text box.
    Captura de pantalla de 2024-10-01 23-40-55

  4. Go to Content -> Files & uploads, verify the absolute links of any asset (In the Web button), which should lead to the URL of the specific tenant

Example Tenant Config

{
    "EDNX_USE_SIGNAL": true,
    "LMS_BASE": "tenant-a.local.edly.io:8000",
    "LMS_ROOT_URL": "tenant-a.local.edly.io:8000",
    "PLATFORM_NAME": "TEST-THEMING",
    "SITE_NAME": "tenant-a.local.edly.io",
    "THEME_OPTIONS": {
        "scripts": {
            "/login": [
                {
                    "content": "alert('This is a test for the inline script');",
                    "type": "inline"
                }
            ]
        },
        "theme": {
            "grandparent": "test-3",
            "name": "test-1",
            "parent": "test-2"
        }
    },
    "course_org_filter": [
        "DEMO"
    ]
}

Additional information

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation
  • Rebased master/main
  • Squashed commits

@mariajgrimaldi
Copy link
Member

Please, review these changes related to my last comment here: 08b320f

@jignaciopm
Copy link
Contributor Author

@mariajgrimaldi about your comment #219 (comment) Don't you think the name should be FilterOrgAwareLMSURLStudio instead of just OrgAwareLMSURLStudio? 🤔

eox_tenant/filters/README.rst Outdated Show resolved Hide resolved
eox_tenant/filters/pipeline.py Outdated Show resolved Hide resolved
eox_tenant/filters/test/test_pipeline.py Outdated Show resolved Hide resolved
LMS_ROOT_URL="https://lms-base"
)
@mock.patch('eox_tenant.filters.pipeline.configuration_helpers')
def test_org_aware_lms_url_studio(self, configuration_helpers_mock):
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about using a more explicit name like: test_get_lms_url_based_for_org. Also, can we consider the case when the filter is not configured?

Copy link
Member

Choose a reason for hiding this comment

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

Consider these comments for the other test suite as well.

Copy link
Contributor Author

@jignaciopm jignaciopm Oct 8, 2024

Choose a reason for hiding this comment

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

@mariajgrimaldi I'm thinking that a test is not necessary for the case when the filter is not configured, because we are always testing run_filter. I'm think is not necessary @override_settings.OPEN_EDX_FILTERS_CONFIG. In other words, the tests work without having that configured.

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

Successfully merging this pull request may close these issues.

3 participants