feat: migrate callers off FEATURES-as-dict access - #38772
Merged
Conversation
feanil
force-pushed
the
feanil/fix-deprecation-warnings
branch
from
June 16, 2026 18:52
1b8c4e8 to
7f5dbc2
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
3 times, most recently
from
June 17, 2026 14:27
9ffdb02 to
f9359f9
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
June 23, 2026 18:42
60dd592 to
4ecd09f
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
June 26, 2026 19:30
858d762 to
346382f
Compare
This was referenced Jun 26, 2026
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
June 26, 2026 19:59
346382f to
520006a
Compare
kdmccormick
approved these changes
Jul 8, 2026
kdmccormick
left a comment
kdmccormick
left a comment
Member
There was a problem hiding this comment.
looks good once rebased & green.
As I mentioned before I dislike all the new getattr invocations, but I've come around that it's the right way to push through this FEATURES refactor. I wrote up a followup ticket which we can put into the config simplification epic somewhere post-FEATURES-refactor #38861
Comment on lines
11
to
+12
| TPA_FEATURES_KEY = 'ENABLE_THIRD_PARTY_AUTH' | ||
| TPA_FEATURE_ENABLED = TPA_FEATURES_KEY in settings.FEATURES | ||
| TPA_FEATURE_ENABLED = hasattr(settings, TPA_FEATURES_KEY) |
Member
There was a problem hiding this comment.
not your problem to fix, but 🤮🤮🤮
feanil
force-pushed
the
feanil/features-dict-bridge
branch
from
July 9, 2026 13:57
3a686d6 to
1299ef1
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
July 9, 2026 14:03
520006a to
20636c5
Compare
feanil
force-pushed
the
feanil/features-dict-migration
branch
4 times, most recently
from
July 10, 2026 13:58
df5644a to
7009266
Compare
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Production reads use getattr(settings, ...); tests use @override_settings(ENABLE_CREATOR_GROUP=...). Mixed-flag patches keep the other flag in patch.dict until its own migration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Production reads use getattr(settings, ...); tests use @override_settings(CERTIFICATES_HTML_VIEW=...). - Drop now-unused FEATURES_WITH_CERTS_ENABLED/_DISABLED helpers. - FEATURES_WITH_CUSTOM_CERTS_ENABLED keeps CUSTOM_CERTIFICATE_TEMPLATES_ENABLED until that flag migrates. - Mixed-flag patches keep the other flag in patch.dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feanil
force-pushed
the
feanil/features-dict-migration
branch
2 times, most recently
from
July 10, 2026 21:05
113517b to
873ee59
Compare
…s-dict Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Flat setting was already being assigned to via the instructor plugin's plugin_settings hook (settings.MAX_ENROLLMENT_INSTR_BUTTONS = 200) — only the readers were still going through settings.FEATURES. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
July 13, 2026 12:55
1f85e3b to
4c7f254
Compare
feanil
marked this pull request as ready for review
July 13, 2026 12:56
feanil
requested review from
a team,
farhan,
irtazaakram and
salman2013
as code owners
July 13, 2026 12:56
feanil
enabled auto-merge (rebase)
July 13, 2026 12:58
…as-dict Flat setting was already being assigned in the instructor plugin's plugin_settings hook — only the two readers were still going through settings.FEATURES.get. Also fixes the annotation that read FEATURES['ENABLE_CERTIFICATES_INSTRUCTOR_MANAGE] (missing closing quote, wrong location). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…dict Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
feanil
force-pushed
the
feanil/features-dict-migration
branch
from
July 13, 2026 13:33
4c7f254 to
a14b33f
Compare
This was referenced Jul 13, 2026
This was referenced Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Second PR in the FEATURES-as-dict migration stack. #38818 (the bridge) is merged.
Per-flag
refactor:commits that move callers offsettings.FEATURES['X']/patch.dict(settings.FEATURES, …)and ontosettings.X/@override_settings(X=…). The underlying flat settings already exist inlms/envs/common.py,cms/envs/common.py, and the sharedopenedx/envs/common.py— the env files were previously flattened, so this PR only touches call sites (plus a few flag definitions that needed to move into the shared file).settings.Xvsgetattr(settings, 'X', <default>)Most reads drop the
getattrfallback in favor of a baresettings.X— but not all. The rule is: a baresettings.Xrequires that the setting be defined in thecommon.pyfile of every process that can reach the read.openedx/envs/common.py(shared, imported by both LMS and CMS) — use baresettings.Xat every caller.lms/envs/common.py— use baresettings.Xonly if the caller is exclusively reachable from the LMS process. If the caller lives undercommon/djangoapps/oropenedx/core/and can be pulled in by CMS test collection or CMS request handling, keepgetattr(settings, 'X', <default>)so CMS doesn'tAttributeError.cms/envs/common.py— the mirror rule. Baresettings.Xat CMS-only callers;getattrat shared/LMS-reachable callers. This deliberately keeps Studio-only concepts (e.g.ENABLE_CREATOR_GROUP,DISABLE_COURSE_CREATION) from leaking into LMS settings... toggle_name:/.. setting_name:annotation) in the smallest common file that covers all callers, then dropsgetattrat the call sites.hasattr(settings, 'X')is used in a few module-load contexts (test util constants) where we specifically want "is the setting defined" rather than "what is its value" — safe against a missing setting on either process.Stack: