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

[Akanshu-2u]

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.

[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]

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]

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]

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

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

[Akanshu-2u]

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]

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]

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.

[robrap]

I'm not clear if fixing a subset at a time adds some risk or not. Do we need to ensure changes are consistent for any particular setting, or does the proxy make that moot? Possibly not if someone had the FEATURES dict settings duplicated and out-of-sync, but that might just move from one bad result to a different bad result.

In any case, if no one has a problem with it, can we delay landing this until after the ulmo branch has been cut? After that, there is less risk for 2U, and since this is a 2U development effort, that would be my preference. That said, if this did cause any issues, the known active early deployer would still want to know, so we could still announce in #risky-changes when we decide to merge.

[feanil]

@Akanshu-2u are you planning on continuing to work on this PR or should it get closed for now and you can re-open it when you have time?

[robrap]

I (Robert) had written:

... can we delay landing this until after the ulmo branch has been cut?

@feanil: Now that the ulmo branch has been cut, and since this already was approved by @kdmccormick, I think this can just be merged.

[robrap]

FURTHER UPDATE: I see, someone needs to resolve conflicts. Happy to have @Akanshu-2u take care of that, but this will be a backlog task. Otherwise, anyone who has time could resolve the conflict and merge.
FYI: @feanil

FURTHER FURTHER UPDATE: I did ticket this and added it to our backlog.