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.

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 added #420

Merged
merged 9 commits into from
Jan 9, 2025

Conversation

marslanabdulrauf
Copy link
Contributor

What are the relevant tickets?

https://github.com/mitodl/hq/issues/6381#event-15804990298

Description (What does it do?)

This PR adds edx-username-changer plugin

How can this be tested?

Please follow the README

Copy link
Contributor

@Anas12091101 Anas12091101 left a comment

Choose a reason for hiding this comment

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

LGTM 👍 , Just small changes in readme file

src/edx_username_changer/README.rst Outdated Show resolved Hide resolved
src/edx_username_changer/README.rst Outdated Show resolved Hide resolved
src/edx_username_changer/README.rst Outdated Show resolved Hide resolved
src/edx_username_changer/BUILD Show resolved Hide resolved
@marslanabdulrauf marslanabdulrauf merged commit 5496c24 into main Jan 9, 2025
8 checks passed
@marslanabdulrauf marslanabdulrauf deleted the marslan/6381-edx-username-changer branch January 9, 2025 12:25
Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

I wish I had reviewed it earlier before this PR was merged but I had a couple of thoughts on this.

@@ -0,0 +1,28 @@
Copyright (C) 2022 MIT Open Learning
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Copyright (C) 2022 MIT Open Learning
Copyright (C) 2025 MIT Open Learning


plugin_app = {
PluginSignals.CONFIG: {
ProjectType.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 are adding the LMS project type only, but the documentation mentions that it can be used from CMS and LMS both. I believe anything under this would be registered in LMS.

"""
Pre-save signal handler of User model to store changed fields to be used later
"""
if settings.FEATURES.get("ENABLE_EDX_USERNAME_CHANGER"):
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 have settings files like other applications to specify flags that are not part of open edx but the plugin itself. An example can be seen in https://github.com/mitodl/open-edx-plugins/tree/main/src/ol_openedx_chat/settings

Copy link
Contributor

Choose a reason for hiding this comment

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

We generally use the LICENSE files with file extension e.g. .txt. I believe open edx uses the same format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I checked recently updated plugins e.g., ol_social_auth and openedx_companion_auth and they had without .txt.

I will update it.

Comment on lines +6 to +10
Version Compatibility
---------------------

It only supports koa and latest releases of Open edX.

Copy link
Contributor

Choose a reason for hiding this comment

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

How did we decide it? We've had different updates in the package e.g. Python, Django, etc. I think this should specify the version compatibility map or something we do in rapid response plugin where we specify which plugin version is compatible with which edX release.

Comment on lines +42 to +45

.. code-block::
FEATURES["ENABLE_EDX_USERNAME_CHANGER"] = True
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not rendered properly
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants