Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

edx-username-changer plugin settings added #424

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

edx-username-changer plugin settings added #424

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a286cb
feat: edx-username-changer plugin settings added
marslanabdulrauf Jan 10, 2025
55df2a2
fix: remove CMS entrypoint
marslanabdulrauf Jan 13, 2025
066348e
fix: build file, entrypoint and plugin settings updated
marslanabdulrauf Jan 15, 2025
e1074b1
docs: FEATURE flag setting added for lms.env.yml file
marslanabdulrauf Jan 15, 2025
fc69052
fix: FEATURES setting fixed
marslanabdulrauf Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/edx_username_changer/BUILD
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove cms from entry_points

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
python_sources(
name="edx_username_changer_source",
dependencies=["src/edx_username_changer/settings:edx_username_changer_settings"],
)

python_distribution(
Expand Down
0 src/edx_username_changer/LICENCE → src/edx_username_changer/LICENCE.txt
View file
Open in desktop
marslanabdulrauf marked this conversation as resolved.
Show resolved Hide resolved
File renamed without changes.
2 changes: 1 addition & 1 deletion src/edx_username_changer/README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove CMS from line 31 as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should not if the plugins works well both from LMS and CMS, Check my above comment on BUILD file.


.. code-block::

Expand Down
1 change: 1 addition & 0 deletions src/edx_username_changer/settings/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
python_sources(name="edx_username_changer_settings")
18 changes: 18 additions & 0 deletions src/edx_username_changer/settings/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# noqa: INP001

"""Settings to provide to edX"""


def plugin_settings(settings):
"""
Populate common settings
"""
env_tokens = getattr(settings, "ENV_TOKENS", {})

# .. setting_name: ENABLE_EDX_USERNAME_CHANGER
# .. setting_default: False
# .. setting_description: Enable/Disable the username changer plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setting docs look different for feature flags too. An example is ALLOW_PUBLIC_ACCOUNT_CREATION


settings.ENABLE_EDX_USERNAME_CHANGER = env_tokens.get(
"ENABLE_EDX_USERNAME_CHANGER", False
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Anas12091101 Anas12091101 Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me

10 changes: 10 additions & 0 deletions src/edx_username_changer/settings/devstack.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# noqa: INP001

"""Settings to provide to edX"""


def plugin_settings(settings):
"""
Populate devstack settings
"""
settings.ENABLE_EDX_USERNAME_CHANGER = False
Loading