edx-username-changer plugin settings added #424
Conversation
| @@ -38,7 +38,7 @@ Configurations | |||
| -------------- | |||
| To configure this plugin, you need to do the following one step: | |||
|
|
|||
| 1. Add/Enable a feature flag (ENABLE_EDX_USERNAME_CHANGER) into your environment variables (through ``private.py`` in LMS or CMS, depending upon where you are installing the plugin) | |||
| 1. Add/Enable a feature flag (ENABLE_EDX_USERNAME_CHANGER) into your environment variables (through ``private.py`` in LMS) | |||
There was a problem hiding this comment.
We should remove CMS from line 31 as well
There was a problem hiding this comment.
I think we should not if the plugins works well both from LMS and CMS, Check my above comment on BUILD file.
There was a problem hiding this comment.
We should remove cms from entry_points
docs: update licence year
src/edx_username_changer/BUILD
Outdated
| "lms.djangoapp": [ | ||
| "edx_username_changer = edx_username_changer.apps:EdxUsernameChangerConfig", | ||
| ], | ||
| "cms.djangoapp": [ | ||
| "edx_username_changer = edx_username_changer.apps:EdxUsernameChangerConfig", | ||
| ], | ||
| }, |
There was a problem hiding this comment.
This should not be removed, The BUILD file translates to the Setup.py file when the bundle is generated. The older code registers the plugin in both LMS, CMS. My previous comment was about registering singles in Apps.py instead.
In response to https://github.com/mitodl/open-edx-plugins/pull/420/files#r1908801121, I would recommend testing the username changer from original repo or PyPI and see if it allows the username changes both from LMS and CMS, I believe it would. In which case we should not remove CMS from here and mention this accordingly in the Readme.
| @@ -38,7 +38,7 @@ Configurations | |||
| -------------- | |||
| To configure this plugin, you need to do the following one step: | |||
|
|
|||
| 1. Add/Enable a feature flag (ENABLE_EDX_USERNAME_CHANGER) into your environment variables (through ``private.py`` in LMS or CMS, depending upon where you are installing the plugin) | |||
| 1. Add/Enable a feature flag (ENABLE_EDX_USERNAME_CHANGER) into your environment variables (through ``private.py`` in LMS) | |||
There was a problem hiding this comment.
I think we should not if the plugins works well both from LMS and CMS, Check my above comment on BUILD file.
| env_tokens = getattr(settings, "ENV_TOKENS", {}) | ||
|
|
||
| # .. setting_name: ENABLE_EDX_USERNAME_CHANGER | ||
| # .. setting_default: False | ||
| # .. setting_description: Enable/Disable the username changer plugin | ||
|
|
||
| settings.ENABLE_EDX_USERNAME_CHANGER = env_tokens.get( | ||
| "ENABLE_EDX_USERNAME_CHANGER", False | ||
| ) |
There was a problem hiding this comment.
We are using the feature flag from settings.FEATURES. I don't think this plugin will work if the flag is defined here directly in settings instead of FEATURES. The actual usage is done from FEATURES instead. See This.
This makes the settings and their usage inconsistent.
@Anas12091101 I think in your testing you still had the username changer in the features dictionary that's why this would have worked.
There was a problem hiding this comment.
I looked into this and found that ENV_TOKENS are loaded into settings from the lms.env.yml file. We primarily use private.py for setting feature flags. I think we can directly enable the flag with settings.FEATURES["ENABLE_EDX_USERNAME_CHANGER"] = True in our settings instead of using ENV_TOKENS. A similar example can be seen in the ol_openedx_git_auto_export plugin. What do you think, @arslanashraf7?
| # .. setting_name: ENABLE_EDX_USERNAME_CHANGER | ||
| # .. setting_default: False | ||
| # .. setting_description: Enable/Disable the username changer plugin |
There was a problem hiding this comment.
The setting docs look different for feature flags too. An example is ALLOW_PUBLIC_ACCOUNT_CREATION
There was a problem hiding this comment.
LGTM overall. I didn't test the PR myself. I am ok if @Anas12091101 has tested the PR and it works well.
| # .. toggle_use_case: open_edx | ||
| # .. toggle_creation_date: 2025-01-15 | ||
|
|
||
| settings.FEATURES.ENABLE_EDX_USERNAME_CHANGER = env_tokens.get("FEATURES", {}).get( |
There was a problem hiding this comment.
| settings.FEATURES.ENABLE_EDX_USERNAME_CHANGER = env_tokens.get("FEATURES", {}).get( | |
| settings.FEATURES["ENABLE_EDX_USERNAME_CHANGER"] = env_tokens.get("FEATURES", {}).get( |
| """ | ||
| Populate devstack settings | ||
| """ | ||
| settings.FEATURES.ENABLE_EDX_USERNAME_CHANGER = False |
There was a problem hiding this comment.
| settings.FEATURES.ENABLE_EDX_USERNAME_CHANGER = False | |
| settings.FEATURES["ENABLE_EDX_USERNAME_CHANGER"] = False |
What are the relevant tickets?
https://github.com/mitodl/hq/issues/6381
Description (What does it do?)
This PR adds plugin settings for
edx_username_changerand update docs