-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: prioritize current site setting for org value #3
Conversation
There was a problem hiding this 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 ?
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.
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.
Not sure if this is possible or required. |
@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. |
There was a problem hiding this 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.
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.
4cb7e7d
to
0d46626
Compare
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-7927Testing instructions
Setup
make {lms,cms}-up
open-release/palm.master
.make lms-shell
.pip install -e /edx/src/eox-tenant
in lms shell.make {lms,cms}-migrate
to create eox tables.localhost:18000
lacolhost.com:18000
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/
USE_EOX_TENANT = True
setting tolms/envs/private.py
andcms/envs/private.py
filesTest
View Live
button, both the sites should take you to lacohost.com lms. The first value from any site having this org is returned which is lacolhost.com in this case.make {lms,cms}-restart
.View Live
button in localhost:18010, it should show its lms url, i.e. http://localhost:18000/courses/... and the button in lacolhost.com:18010 should show http://lacolhost.com:18000/courses/...Checklist for Merge