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

refactor: prioritize current site setting for org value #3

Conversation

navinkarkera
Copy link
Member

@navinkarkera navinkarkera commented Oct 3, 2023

Description

If multiple tenant configs are enabled for a given org, fetching value
from site config for the organization should return current site config
value if the org is included in the org_filter.

Private-ref: BB-7927

Testing instructions

Setup

  • Setup master devstack and start lms & cms using make {lms,cms}-up
  • Clone this repo in <devstack_base_dir>/src/
  • Checkout default branch in eox, i.e. open-release/palm.master.
  • Open lms shell using make lms-shell.
  • Run pip install -e /edx/src/eox-tenant in lms shell.
  • Repeat above 2 steps to install eox in cms.
  • Run make {lms,cms}-migrate to create eox tables.
  • Goto eox tenant configs via: http://localhost:18000/admin/eox_tenant/tenantconfig/
  • Add an entry with below details:
    • External key: localhost:18000
    • Lms configs:
        {
            "EDNX_USE_SIGNAL": true,
            "LMS_BASE": "localhost:18000",
            "LMS_ROOT_URL": "http://locahost:18000",
            "SOCIAL_AUTH_EDX_OAUTH2_PUBLIC_URL_ROOT": "http://localhost:18000",
            "course_org_filter": [
                "edX"
            ]
        }
    • studio configs:
        {
            "EDNX_USE_SIGNAL": true,
            "LMS_BASE": "localhost:18000",
            "LMS_ROOT_URL": "http://locahost:18000",
            "SOCIAL_AUTH_EDX_OAUTH2_PUBLIC_URL_ROOT": "http://localhost:18000",
            "course_org_filter": [
                "edX"
            ]
        }
  • Add another entry with below details:
    • External key: lacolhost.com:18000
    • Lms configs:
        {
            "EDNX_USE_SIGNAL": true,
            "LMS_BASE": "lacolhost.com:18000",
            "LMS_ROOT_URL": "http://lacolhost.com:18000",
            "course_org_filter": [
                "edX"
            ]
        }
    • studio configs:
        {
            "EDNX_USE_SIGNAL": true,
            "LMS_BASE": "lacolhost.com:18000",
            "LMS_ROOT_URL": "http://lacolhost.com:18000",
            "SOCIAL_AUTH_EDX_OAUTH2_PUBLIC_URL_ROOT": "http://lacolhost.com:18000",
            "course_org_filter": [
                "edX"
            ]
        }
  • Goto http://localhost:18000/admin/oauth2_provider/application/ and find application with name studio-sso and update its redirect urls to this value: http://localhost:18010/complete/edx-oauth2/ http://localhost:18000/ http://lacolhost.com:18010/complete/edx-oauth2/ http://lacolhost.com:18000/ http://lacolhost.com http://edx.devstack.lms:18000/
  • Add localhost & lacolhost.com with its respective tenant config to routes: http://localhost:18000/admin/eox_tenant/route/
  • Add USE_EOX_TENANT = True setting to lms/envs/private.py and cms/envs/private.py files

Test

Checklist for Merge

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

@navinkarkera navinkarkera marked this pull request as ready for review October 4, 2023 05:15
Copy link
Member

@kaustavb12 kaustavb12 left a comment

Choose a reason for hiding this comment

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

The default implementation of get_value_for_org() both in eox-tenant and in edx-platform, seems pretty faulty to me. It seems the intention was that an individual org will be used for only a single tenant/microsite. If this is true, then our client's use-case seems to be unique, and so there might be difficulty with upstream accepting the current changes.

Also, the current changes works great for url configs, but for some other ones such as ENABLE_PUBLISHERS OR ENABLE_EXTENDED_COURSE_DETAILS, this could potentially lead to inconsistencies in the studio views for the same course in different tenants, if different values are configured for different tenants.

What if we make this inconsistency formal, by gating the changes here behind a flag called something like USE_TENANT_VALUE_FOR_ORG. Is there anyway to do this here ?

eox_tenant/tenant_wise/proxies.py Outdated Show resolved Hide resolved
@navinkarkera
Copy link
Member Author

navinkarkera commented Oct 4, 2023

The default implementation of get_value_for_org() both in eox-tenant and in edx-platform, seems pretty faulty to me. It seems the intention was that an individual org will be used for only a single tenant/microsite. If this is true, then our client's use-case seems to be unique, and so there might be difficulty with upstream accepting the current changes.

Agree, the implementation is not good. I hope that upstream will accept this change as it is better than the current implementation in any case.

Also, the current changes works great for url configs, but for some other ones such as ENABLE_PUBLISHERS OR ENABLE_EXTENDED_COURSE_DETAILS, this could potentially lead to inconsistencies in the studio views for the same course in different tenants, if different values are configured for different tenants.

Actually, the logic should work as expected here but the cache-key will cause issues with boolean default values. I'll see if I can create a unique key for each tenant using tenant external key or LMS_BASE or something similar.

What if we make this inconsistency formal, by gating the changes here behind a flag called something like USE_TENANT_VALUE_FOR_ORG. Is there anyway to do this here ?

Not sure if this is possible or required.

@navinkarkera
Copy link
Member Author

@kaustavb12 Updated the PR to use tenant/microsite external_key for uniquely identifying the org config for current tenant. This should work for all types of values now. See the latest commit for changes.

Copy link
Member

@kaustavb12 kaustavb12 left a comment

Choose a reason for hiding this comment

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

👍

@navinkarkera Great job with the latest changes. The changes in the PR are definitely an improvement over the original implementation.

Left a few small comments. Please consider this approved once those are resolved.

  • I tested this: Tested in devstack
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

eox_tenant/models.py Outdated Show resolved Hide resolved
eox_tenant/models.py Show resolved Hide resolved
eox_tenant/tenant_wise/proxies.py Show resolved Hide resolved
eox_tenant/models.py Outdated Show resolved Hide resolved
eox_tenant/models.py Outdated Show resolved Hide resolved
If multiple tenant configs are enabled for a given org, fetching value
from site config for the organization should return current site config
value if the org is included in the org_filter.
@navinkarkera navinkarkera force-pushed the navin/current-site-priority-for-org-value branch from 4cb7e7d to 0d46626 Compare October 9, 2023 07:35
@navinkarkera navinkarkera merged commit 5022e4a into opencraft-release/nutmeg.2 Oct 9, 2023
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.

2 participants