refactor: migrated FEATURES dict settings to top-level in core files and fixed related test files. #37389

Akanshu-2u · 2025-09-26T13:00:05Z

Description:

Refactored FEATURES settings to top-level in core files and updated related tests for consistency and maintainability.

Solution:

Modified the SettingDictToggle class to SettingToggle.
Moved FEATURES setting to top level in core file.
Fixed the related settings in test files leaving other unrelated FEATURE references intact.

Note: The unrelated FEATURE references are working fine due to the use of featureproxy , hence for the time being left those unchanged.

Private JIRA Link:

BOMS-200

Reference PR:

#37067

Note:

On investigating how the top-level-settings are working in edx platform , found these few settings are still getting derived from the FEATURES dict.
Planned to move these settings to top level.

feanil

Thanks for the PR @Akanshu-2u, it looks like the right changes but maybe not enough fixes in the tests which may be trying to override the features dict still. Once the tests get fixed up, I think this would be fine to land. Let me know when you've got the tests passing. Until then, I'm gonna mark this as a draft PR.

cms/djangoapps/contentstore/tests/test_course_settings.py

jcapphelix · 2025-10-21T12:12:25Z

cms/djangoapps/contentstore/tests/test_utils.py

        }

-        utils.update_course_details(mock_request, self.course.id, payload, None)
+        utils.update_course_details(mock_request, self.course.id, payload, self.course)


why are we changing the args ? This is un-related to features.

Was it because of tests failing ?

Yes, the CI test was failing with this error AttributeError: 'NoneType' object has no attribute 'entrance_exam_enabled' . So changed the arg to fix this.

I believe this is happening because the setting value for ENTRANCE_EXAMS is now being set to true instead of false which is not the correct expected behavior. So I think the fix needs to be to patch the toggle rather than the item in the features dictionary not to change the test code here.

common/djangoapps/util/tests/test_milestones_helpers.py

jcapphelix

While core file changes are all good.

I have some questions on test cases, mainly :-

Why are some feature flags not removed ?
Is it something that is supposed to be removed later ?

Akanshu-2u · 2025-10-27T04:51:20Z

While core file changes are all good.

I have some questions on test cases, mainly :-

Why are some feature flags not removed ?

Is it something that is supposed to be removed later ?

The settings which where removed from features dict in the core files. Only those are modified in the test files leaving the other feature settings in place as due to featureproxy those will not create any issue.
Secondly, some args and cache changes are mainly due to the failure of the CI checks so have included those to prevent anything from breaking.
The remaining feature instances in the test files can be removed, waiting on confirmation.

jcapphelix

My Queries are all answered.

Good to go from my side.

kdmccormick

Thanks @Akanshu-2u , these changes all look good.

But, I am confused why you chose to update these references in particular. There are many remaining references to FEATURES that we will need to clean up in a future PR.

Were these particular references causing bugs?

Akanshu-2u · 2025-10-27T13:41:54Z

Thanks @Akanshu-2u , these changes all look good.

But, I am confused why you chose to update these references in particular. There are many remaining references to FEATURES that we will need to clean up in a future PR.

Were these particular references causing bugs?

I focused on fixing only the test files that referenced the FEATURES dictionary settings, which had been removed from the core files within this PR.
The settings were causing bugs resulting in test failures so was essential to update those test cases with override-settings.

kdmccormick · 2025-10-27T13:47:20Z

which had been removed from the core files within this PR.

Why did you choose to refactor those settings within this PR?

Akanshu-2u · 2025-10-27T13:50:15Z

which had been removed from the core files within this PR.

Why did you choose to refactor those settings within this PR?

I guess this may help you with the explanation: #37396 (comment)

kdmccormick · 2025-10-27T14:50:43Z

Okay, so you have chosen some remaining references, but not all of them, correct? That is fine, but the description of this PR says that it "moved remaining features dicts settings" and "Removed FEATURES dictionary". These are not true, and it is misleading to have them in the PR description when there are still so many FEATURES references remaining in the platform. Could you please edit your PR and commit message?

Akanshu-2u · 2025-10-28T13:44:47Z

Okay, so you have chosen some remaining references, but not all of them, correct? That is fine, but the description of this PR says that it "moved remaining features dicts settings" and "Removed FEATURES dictionary". These are not true, and it is misleading to have them in the PR description when there are still so many FEATURES references remaining in the platform. Could you please edit your PR and commit message?

Addressed. Modified the PR title and description which shall be more convenient for the changes achieved in this PR.

feanil · 2026-01-14T16:02:08Z

I rebased and resolved conflicts but it looks like some change maybe broke some rate limiting. @Akanshu-2u can you look into that when you have some time?

robrap · 2026-01-14T20:25:20Z

I apologize @feanil, but @Akanshu-2u is on to other things. He created this PR to begin with out of a misunderstanding of what was required of him for handling the DEPR on the 2U-side. Hopefully this PR helps get the work started by someone, but I need him working on some other things right now.

Note: I thought the PR was ready to merge. I didn't realize it would involve additional work.

feanil · 2026-01-14T20:41:31Z

In that case, closing this for now as I don't think we have anyone to work on it and it will probably get stale and full of conflicts pretty regularly given the number of things it touches.

…ict-depr

Akanshu-2u · 2026-01-16T14:48:13Z

In that case, closing this for now as I don't think we have anyone to work on it and it will probably get stale and full of conflicts pretty regularly given the number of things it touches.

Hii @feanil , I’ve rebased the latest master onto my branch and the CI is now passing successfully. Please feel free to review, approve, and merge the PR. Thanks!

feanil

Mostly looks good, just had a couple of questions.

cms/djangoapps/contentstore/tests/test_course_settings.py

feanil · 2026-01-16T15:07:02Z

cms/djangoapps/contentstore/tests/test_utils.py

        }

-        utils.update_course_details(mock_request, self.course.id, payload, None)
+        utils.update_course_details(mock_request, self.course.id, payload, self.course)


I believe this is happening because the setting value for ENTRANCE_EXAMS is now being set to true instead of false which is not the correct expected behavior. So I think the fix needs to be to patch the toggle rather than the item in the features dictionary not to change the test code here.

feanil · 2026-01-16T15:07:59Z

cms/djangoapps/contentstore/tests/test_course_settings.py

    )
    @override_waffle_flag(toggles.LEGACY_STUDIO_SCHEDULE_DETAILS, True)
-    def test_visibility_of_entrance_exam_section(self, feature_flags):
+    @patch.object(milestones_helpers.ENABLE_MILESTONES_APP, 'is_enabled', return_value=False)


Why patch the toggle rather than just using override_setting which is more standard and what you're using in some of the later files?

Implemented the suggestion.

feanil · 2026-01-16T15:08:48Z

cms/djangoapps/contentstore/views/tests/test_library.py


-    @mock.patch.dict('django.conf.settings.FEATURES', {'ENABLE_CONTENT_LIBRARIES': False})
-    def test_with_libraries_disabled(self):
+    @patch.object(toggles.ENABLE_CONTENT_LIBRARIES, 'is_enabled', return_value=False)


I think it's much clearer if you just override the django settings rather than the toggle.

Yes, following the suggestion I added override setting instead of patch toggle.

feanil · 2026-01-16T15:20:39Z

lms/djangoapps/mfe_config_api/tests/test_views.py


    def setUp(self):
        self.mfe_config_api_url = reverse("mfe_config_api:config")
+        cache.clear()


Why was this needed?

This change was needed because CI checks were repeatedly failing due to cached data being retained for the test file. Clearing the cache ensures the tests run cleanly and pass as expected.

…ict-depr

feanil

Looks like one more file needs to be updated and then this is good to land I think.

feanil · 2026-01-20T14:38:52Z

cms/djangoapps/contentstore/tests/test_course_settings.py


-    @patch.dict(settings.FEATURES, {'ENABLE_EXPORT_GIT': True})
-    def test_fetch_giturl_present(self):
+    @patch.object(toggles.EXPORT_GIT, 'is_enabled', return_value=True)


Is there a reason this isn't using override_settings as well?

Changed this to override settings and ensured other files do not require changes for override settings.

…ict-depr

feanil

Changes look great, thanks @Akanshu-2u

Akanshu-2u requested a review from a team as a code owner September 26, 2025 13:00

Akanshu-2u mentioned this pull request Oct 9, 2025

[DEPR]: Support for the FEATURES dictionary in Django settings #37396

Open

feanil reviewed Oct 9, 2025

View reviewed changes

feanil marked this pull request as draft October 9, 2025 14:12

Akanshu-2u force-pushed the BOMS-200-handle-features-setting-dict-depr branch from 1f8e60f to e3f8473 Compare October 10, 2025 15:51

Akanshu-2u marked this pull request as ready for review October 21, 2025 09:44

jcapphelix reviewed Oct 21, 2025

View reviewed changes

cms/djangoapps/contentstore/tests/test_course_settings.py Show resolved Hide resolved

jcapphelix reviewed Oct 21, 2025

View reviewed changes

cms/djangoapps/contentstore/tests/test_course_settings.py Show resolved Hide resolved

jcapphelix reviewed Oct 21, 2025

View reviewed changes

common/djangoapps/util/tests/test_milestones_helpers.py Show resolved Hide resolved

jcapphelix suggested changes Oct 21, 2025

View reviewed changes

Akanshu-2u requested review from farhan, irtazaakram and salman2013 as code owners October 27, 2025 04:30

jcapphelix approved these changes Oct 27, 2025

View reviewed changes

Akanshu-2u requested a review from feanil October 27, 2025 09:42

kdmccormick approved these changes Oct 27, 2025

View reviewed changes

Akanshu-2u requested a review from kdmccormick October 27, 2025 13:42

Akanshu-2u changed the title ~~refactor: moved remaining feature dicts settings into top-level setting~~ Refactored: migrated remaining FEATURES dictionary settings to top-level settings in core files and updated corresponding test files. Oct 28, 2025

Akanshu-2u changed the title ~~refactor: migrated remaining FEATURES dictionary settings to top-level settings in core files and updated corresponding test files.~~ refactor: migrated FEATURES dictionary settings to top-level settings in core files and updated corresponding test files. Oct 28, 2025

Akanshu-2u changed the title ~~refactor: migrated FEATURES dictionary settings to top-level settings in core files and updated corresponding test files.~~ refactor: migrated FEATURES dict to top-level settings in core files and fixed related test files. Oct 28, 2025

Akanshu-2u changed the title ~~refactor: migrated FEATURES dict to top-level settings in core files and fixed related test files.~~ refactor: migrated FEATURES dict settings to top-level in core files and fixed related test files. Oct 28, 2025

Akanshu-2u added 11 commits January 9, 2026 12:26

fix: added configurations for base utl fix

9bed0f2

fix: handled none issue for mfe config

73ef6b0

fix: corrected override settings in test views

5ff3d4f

fix: added getattr defensive technique for view settings

2466cad

fix: reverted views and test_views file

772dc24

fix: added settings in views file

a5710f0

fix: added with patch within functions in test view

a1e6d1f

fix: rearranged the features in default_legacy_config

c2a3b0f

fix: fixing the tests with clearing cache

650da17

fix: reverted test views to verify the CI check

10c429b

fix: added cache clear in mfe config test

a3cad1f

feanil force-pushed the BOMS-200-handle-features-setting-dict-depr branch from 6cfcc78 to a3cad1f Compare January 9, 2026 17:27

feanil closed this Jan 14, 2026

Merge branch 'openedx:master' into BOMS-200-handle-features-setting-d…

385004c

…ict-depr

Akanshu-2u reopened this Jan 16, 2026

feanil reviewed Jan 16, 2026

View reviewed changes

Akanshu-2u added 3 commits January 19, 2026 11:47

Merge branch 'openedx:master' into BOMS-200-handle-features-setting-d…

d0cfbc3

…ict-depr

fix: fixed the patch toggles to override settings

9aee2e2

fix: fixed the lint errors

55a26cd

Akanshu-2u requested a review from feanil January 19, 2026 09:57

feanil reviewed Jan 20, 2026

View reviewed changes

Akanshu-2u added 2 commits January 20, 2026 15:08

fix: changed patch toggle to override settings

bb6ad96

Merge branch 'openedx:master' into BOMS-200-handle-features-setting-d…

70a8370

…ict-depr

Akanshu-2u requested a review from feanil January 20, 2026 16:20

feanil approved these changes Jan 20, 2026

View reviewed changes

feanil merged commit 2d82d90 into openedx:master Jan 20, 2026
65 checks passed

refactor: migrated FEATURES dict settings to top-level in core files and fixed related test files. #37389

refactor: migrated FEATURES dict settings to top-level in core files and fixed related test files. #37389

Uh oh!

Conversation

Akanshu-2u commented Sep 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Solution:

Private JIRA Link:

Reference PR:

Note:

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcapphelix left a comment

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcapphelix left a comment

Choose a reason for hiding this comment

Uh oh!

kdmccormick left a comment

Choose a reason for hiding this comment

Uh oh!

Akanshu-2u commented Oct 27, 2025

Uh oh!

kdmccormick commented Oct 27, 2025

Uh oh!

Akanshu-2u commented Oct 27, 2025

Uh oh!

kdmccormick commented Oct 27, 2025

Uh oh!

Akanshu-2u commented Oct 28, 2025

Uh oh!

feanil commented Jan 14, 2026

Uh oh!

robrap commented Jan 14, 2026

Uh oh!

feanil commented Jan 14, 2026

Uh oh!

Akanshu-2u commented Jan 16, 2026

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feanil left a comment

Akanshu-2u commented Sep 26, 2025 •

edited

Loading

Akanshu-2u commented Oct 27, 2025 •

edited

Loading