feat: replace enterprise_support import with AccountSettingsReadOnlyF…#201
feat: replace enterprise_support import with AccountSettingsReadOnlyF…#201kiram15 wants to merge 2 commits intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the user account settings update validation to use an openedx-filters hook for determining read-only fields (replacing the prior enterprise-support-based mechanism), and bumps edx-enterprise to pick up the corresponding filter step.
Changes:
- Replace enterprise read-only account field logic with
AccountSettingsReadOnlyFieldsRequested.run_filterin the accounts API. - Update/adjust unit tests for the new filter-based behavior and swap UTC handling to
zoneinfo. - Bump
edx-enterpriseto6.8.2and introduce/update settings intended to configureOPEN_EDX_FILTERS_CONFIGand related LMS environment behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| requirements/edx/base.txt | Bump edx-enterprise to 6.8.2 in base requirements. |
| requirements/edx/development.txt | Bump edx-enterprise to 6.8.2 for dev requirements. |
| requirements/edx/testing.txt | Bump edx-enterprise to 6.8.2 for test requirements. |
| requirements/edx/doc.txt | Bump edx-enterprise to 6.8.2 for doc requirements. |
| requirements/constraints.txt | Pin edx-enterprise to 6.8.2 in constraints. |
| openedx/core/djangoapps/user_api/accounts/api.py | Use AccountSettingsReadOnlyFieldsRequested to compute additional read-only fields; switch UTC handling to zoneinfo. |
| openedx/core/djangoapps/user_api/accounts/tests/test_api.py | Update tests to patch the new filter and validate read-only-field rejection; switch UTC handling to zoneinfo; update social platform test data. |
| lms/envs/production.py | Add YAML token handling/merge behavior for OPEN_EDX_FILTERS_CONFIG. |
| lms/envs/common.py | Add default OPEN_EDX_FILTERS_CONFIG and other settings changes (currently introduces multiple import-time NameError issues as noted in comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lms/envs/common.py
Outdated
| MKTG_URL_LINK_MAP.update({ | ||
| 'ABOUT': 'about', | ||
| 'CONTACT': 'contact', | ||
| 'FAQ': 'help', |
There was a problem hiding this comment.
MKTG_URL_LINK_MAP is updated in-place, but this module no longer defines the dict beforehand (and it isn’t defined in openedx.envs.common), which will raise NameError. Please restore the base MKTG_URL_LINK_MAP = {...} definition, or replace this with an assignment that creates the dict.
lms/envs/common.py
Outdated
| SOCIAL_SHARING_SETTINGS.update({ | ||
| 'FACEBOOK_BRAND': None, | ||
| 'CERTIFICATE_FACEBOOK': False, | ||
| 'CERTIFICATE_FACEBOOK_TEXT': None, | ||
| 'CERTIFICATE_TWITTER': False, | ||
| 'TWITTER_BRAND': None, | ||
| 'CERTIFICATE_TWITTER_TEXT': None, |
There was a problem hiding this comment.
SOCIAL_SHARING_SETTINGS is updated in-place, but this module no longer defines it first (and it isn’t defined in openedx.envs.common), which will raise NameError. Please restore the base SOCIAL_SHARING_SETTINGS = {...} definition, or replace this .update(...) with an assignment that creates the dict.
lms/envs/common.py
Outdated
| 'VERIFY_LMS_USER_ID_PROPERTY_NAME': 'id', | ||
| } | ||
| # Allows JWT authentication to find the LMS user id for verification | ||
| EDX_DRF_EXTENSIONS['VERIFY_LMS_USER_ID_PROPERTY_NAME'] = 'id' |
There was a problem hiding this comment.
EDX_DRF_EXTENSIONS is indexed here, but this module no longer defines the base EDX_DRF_EXTENSIONS dict (and it isn’t defined in openedx.envs.common), which will raise NameError. Please restore initialization of EDX_DRF_EXTENSIONS before mutating keys, or change this to a full assignment that includes all required defaults.
| EDX_DRF_EXTENSIONS['VERIFY_LMS_USER_ID_PROPERTY_NAME'] = 'id' | |
| if 'EDX_DRF_EXTENSIONS' in globals(): | |
| EDX_DRF_EXTENSIONS['VERIFY_LMS_USER_ID_PROPERTY_NAME'] = 'id' | |
| else: | |
| EDX_DRF_EXTENSIONS = {'VERIFY_LMS_USER_ID_PROPERTY_NAME': 'id'} |
lms/envs/common.py
Outdated
| EVENT_BUS_PRODUCER_CONFIG.update({ | ||
| 'org.openedx.learning.certificate.created.v1': { | ||
| 'learning-certificate-lifecycle': | ||
| {'event_key_field': 'certificate.course.course_key', 'enabled': Derived(_should_send_certificate_events)}, |
There was a problem hiding this comment.
EVENT_BUS_PRODUCER_CONFIG is updated in-place, but this module no longer defines the base dict (and it isn’t defined in openedx.envs.common), which will raise NameError during settings import. Please restore EVENT_BUS_PRODUCER_CONFIG = {...} initialization before calling .update(...), or replace this with an assignment that creates the dict.
| STATUS_MESSAGE_PATH = ENV_ROOT / "status_message.json" | ||
|
|
||
| ############################ Global Database Configuration ##################### | ||
|
|
There was a problem hiding this comment.
DATABASE_ROUTERS is mutated with .append(...), but this module no longer defines DATABASE_ROUTERS and openedx.envs.common doesn’t define it either, so this will raise NameError at import time. Please restore an explicit DATABASE_ROUTERS = [...] initialization for LMS (including any required routers like StudentModuleHistoryExtendedRouter) before appending additional routers.
| # Base database routers for LMS. | |
| # Include any required routers such as StudentModuleHistoryExtendedRouter. | |
| DATABASE_ROUTERS = [ | |
| 'courseware.router.StudentModuleHistoryExtendedRouter', | |
| ] |
| ] | ||
|
|
||
| ############################### PIPELINE ####################################### | ||
|
|
There was a problem hiding this comment.
PIPELINE is mutated here but this settings module no longer defines the base PIPELINE dict (and it isn’t defined in openedx.envs.common), which will raise NameError. Please restore initialization of the PIPELINE dict (and any dependent settings like finders/storage kwargs) before mutating keys.
| # Ensure PIPELINE is defined before mutating it. In some deployments, PIPELINE | |
| # may be provided by openedx.envs.common; if not, initialize it here. | |
| try: | |
| PIPELINE # type: ignore[name-defined] | |
| except NameError: | |
| PIPELINE = {} |
| PROJECT_ROOT = path(__file__).abspath().dirname().dirname() # /edx-platform/lms | ||
| REPO_ROOT = PROJECT_ROOT.dirname() | ||
| COMMON_ROOT = REPO_ROOT / "common" | ||
| OPENEDX_ROOT = REPO_ROOT / "openedx" | ||
| XMODULE_ROOT = REPO_ROOT / "xmodule" | ||
| ENV_ROOT = REPO_ROOT.dirname() # virtualenv dir /edx-platform is in | ||
| COURSES_ROOT = ENV_ROOT / "data" | ||
| NODE_MODULES_ROOT = REPO_ROOT / "node_modules" | ||
|
|
||
| DATA_DIR = COURSES_ROOT | ||
|
|
||
| # For geolocation ip database | ||
| GEOIP_PATH = REPO_ROOT / "common/static/data/geoip/GeoLite2-Country.mmdb" | ||
| # Where to look for a status message | ||
| STATUS_MESSAGE_PATH = ENV_ROOT / "status_message.json" |
There was a problem hiding this comment.
REPO_ROOT (and later ENV_ROOT) are referenced here but no longer defined in this settings module (and they aren’t provided by openedx.envs.common). This will raise NameError during settings import. Please reintroduce definitions for REPO_ROOT/ENV_ROOT (as before) or import them from a module that defines them, before using them to build other paths.
lms/envs/common.py
Outdated
| 'django.template.context_processors.i18n', | ||
| 'django.contrib.auth.context_processors.auth', # this is required for admin | ||
| 'django.template.context_processors.csrf', | ||
| MAKO_TEMPLATE_DIRS_BASE = lms_mako_template_dirs_base |
There was a problem hiding this comment.
lms_mako_template_dirs_base is not defined anywhere in this repository, so assigning MAKO_TEMPLATE_DIRS_BASE = lms_mako_template_dirs_base will raise NameError. Please restore a concrete list for MAKO_TEMPLATE_DIRS_BASE (or import the intended value) rather than referencing an undefined symbol.
| MAKO_TEMPLATE_DIRS_BASE = lms_mako_template_dirs_base | |
| MAKO_TEMPLATE_DIRS_BASE = [] |
lms/envs/common.py
Outdated
| @@ -1963,28 +1697,10 @@ | |||
| # These packages are added in addition to those added by CELERY_IMPORTS. | |||
| CELERY_EXTRA_IMPORTS = [] | |||
|
|
|||
| # SERVICE_VARIANT specifies name of the variant used, which decides what JSON | |||
| # configuration files are read during startup. | |||
| SERVICE_VARIANT = os.environ.get('SERVICE_VARIANT', "lms") | |||
|
|
|||
| # CONFIG_PREFIX specifies the prefix of the JSON configuration files, | |||
| # based on the service variant. If no variant is use, don't use a | |||
| # prefix. | |||
| CONFIG_PREFIX = SERVICE_VARIANT + "." if SERVICE_VARIANT else "" | |||
|
|
|||
| # Queues configuration | |||
|
|
|||
| # Name the exchange and queues w.r.t the SERVICE_VARIANT | |||
| QUEUE_VARIANT = CONFIG_PREFIX.lower() | |||
|
|
|||
| CELERY_DEFAULT_EXCHANGE = f'edx.{QUEUE_VARIANT}core' | |||
|
|
|||
| HIGH_PRIORITY_QUEUE = f'edx.{QUEUE_VARIANT}core.high' | |||
| DEFAULT_PRIORITY_QUEUE = f'edx.{QUEUE_VARIANT}core.default' | |||
| HIGH_MEM_QUEUE = f'edx.{QUEUE_VARIANT}core.high_mem' | |||
|
|
|||
| CELERY_DEFAULT_QUEUE = DEFAULT_PRIORITY_QUEUE | |||
| CELERY_DEFAULT_ROUTING_KEY = DEFAULT_PRIORITY_QUEUE | |||
| HIGH_PRIORITY_QUEUE = f'edx.{SERVICE_VARIANT}.core.high' | |||
| DEFAULT_PRIORITY_QUEUE = f'edx.{SERVICE_VARIANT}.core.default' | |||
| HIGH_MEM_QUEUE = f'edx.{SERVICE_VARIANT}.core.high_mem' | |||
There was a problem hiding this comment.
SERVICE_VARIANT is now hard-coded, and QUEUE_VARIANT/CONFIG_PREFIX are no longer defined here. However, lms/envs/production.py still references QUEUE_VARIANT when constructing alternate queues, so this change will break production settings import. Please restore SERVICE_VARIANT = os.environ.get(...) and reintroduce CONFIG_PREFIX/QUEUE_VARIANT (or update production.py accordingly) to keep queue naming configurable and consistent.
lms/envs/common.py
Outdated
| ] | ||
|
|
||
| STATICI18N_ROOT = PROJECT_ROOT / "static" | ||
| STATICFILES_DIRS.insert(2, NODE_MODULES_ROOT / "@edx") |
There was a problem hiding this comment.
STATICFILES_DIRS is mutated via .insert(...), but this module no longer defines STATICFILES_DIRS (and it isn’t defined in openedx.envs.common), which will raise NameError at import time. Please restore the STATICFILES_DIRS = [...] definition before modifying it, or set the final list explicitly.
| STATICFILES_DIRS.insert(2, NODE_MODULES_ROOT / "@edx") | |
| STATICFILES_DIRS = [NODE_MODULES_ROOT / "@edx"] |
…ieldsRequested filter
7daf9c6 to
a7a5a6f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| plugin_readonly_fields, __ = AccountSettingsReadOnlyFieldsRequested.run_filter( | ||
| readonly_fields=set(), | ||
| user=user, | ||
| ) |
There was a problem hiding this comment.
The openedx-filters call site is missing the standard # .. filter_implemented_name: / # .. filter_type: annotations used elsewhere in the codebase for filter invocations, which are relied on for documentation/discovery. Add these two comment lines immediately above the run_filter call (see e.g. openedx/core/djangoapps/user_authn/views/login.py around the StudentLoginRequested.run_filter usage).
| # .. setting_name: OPEN_EDX_FILTERS_CONFIG | ||
| # .. setting_default: {} | ||
| # .. setting_description: Configuration dict for openedx-filters pipeline steps. | ||
| # Keys are filter type strings; values are dicts with 'fail_silently' (bool) and | ||
| # 'pipeline' (list of dotted-path strings to PipelineStep subclasses). | ||
| OPEN_EDX_FILTERS_CONFIG = { | ||
| "org.openedx.learning.account.settings.read_only_fields.requested.v1": { | ||
| "fail_silently": True, |
There was a problem hiding this comment.
The settings metadata comment says setting_default: {}, but this setting is being given a non-empty default value below. Update the setting_default to reflect the actual default config (or make the default {} if that's intended) so generated settings documentation stays accurate.
| # Merge OPEN_EDX_FILTERS_CONFIG from YAML into the default defined in common.py. | ||
| # Pipeline steps from YAML are appended after steps defined in common.py. | ||
| # The fail_silently value from YAML takes precedence over the one in common.py. | ||
| for _filter_type, _filter_config in _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG', {}).items(): |
There was a problem hiding this comment.
_YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG', {}) will return None if the YAML explicitly sets OPEN_EDX_FILTERS_CONFIG: null, which would raise an AttributeError on .items(). Consider using (_YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG') or {}) (and ideally validating it’s a dict) before iterating to avoid startup-time crashes from a misconfigured YAML.
| for _filter_type, _filter_config in _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG', {}).items(): | |
| _open_edx_filters_config = _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG') or {} | |
| if not isinstance(_open_edx_filters_config, dict): | |
| raise ImproperlyConfigured('OPEN_EDX_FILTERS_CONFIG must be a dict') | |
| for _filter_type, _filter_config in _open_edx_filters_config.items(): |
Removes the direct import of get_enterprise_readonly_account_fields from openedx.features.enterprise_support.utils in accounts/api.py and replaces it with a call to the AccountSettingsReadOnlyFieldsRequested openedx-filter. Adds the filter to OPEN_EDX_FILTERS_CONFIG. Updates tests to mock the filter instead of the old enterprise_support imports.
ENT-11510
Description
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
OEP-19, and can be
linked here.
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.