-
Notifications
You must be signed in to change notification settings - Fork 124
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
Allow disable account deletion #817
Allow disable account deletion #817
Conversation
Thanks for the pull request, @JonasBM! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
About the test, i just updated the existing one. If necessary i can create two other tests just for this feature. |
Fixed linting errors. |
Hi @JonasBM! Is this all set for review? |
Hi, @mphilbrick211! Yes. |
Hi @openedx/fed-bom! Could someone please review/merge this for us? Thanks! |
About this solution, i have some questions that i post in the forum, please give a read, especially if this PR is accepted. |
Apologies, @abdullahwaheed - I see you already plan to review - thanks! |
Hi there, @abdullahwaheed. What do we need to merge this PR? |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #817 +/- ##
=======================================
Coverage 46.54% 46.54%
=======================================
Files 122 122
Lines 2544 2548 +4
Branches 665 669 +4
=======================================
+ Hits 1184 1186 +2
- Misses 1284 1286 +2
Partials 76 76
☔ View full report in Codecov by Sentry. |
Fixed a conflict in the merge. |
Hi @abdullahwaheed! Would you be able to enable tests again for this PR? |
@JonasBM could you please resolve conflicts so we can merge this PR |
Done. |
@JonasBM just another suggestion. Could you update Readme accordingly so we have a proper documentation of this flag and its functionality |
Done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
@JonasBM 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
* feat: allow disable account deletion * test: add disable account deletion to test case * style: fix lint errors * docs: Add ENABLE_ACCOUNT_DELETION to README.rst
Referenced issue: There is no way to disable account deletion in the Account MFE #189
Add environment variable
ENABLE_ACCOUNT_DELETION
, to allow the disable of account deletion.To disable account deletion set
ENABLE_ACCOUNT_DELETION="false"
, otherwise it will default to true.Updated JumpNav test to include the no render of account deletion components.