-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
LOGOUT_REDIRECT_URL to be used for logout redirection #28765
Conversation
Thanks for the pull request, @asadiqbal08! I've created OSPR-6051 to keep track of it in JIRA, where we prioritize reviews. 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. |
two quick things @asadiqbal08
I don't think this is quite following the best practices for feature toggles, but I would need to find a reference on that. |
@asadiqbal08 Thank you for your contribution. Please let me know once it is ready for our review. |
b7c6cef
to
1378d6a
Compare
Ok, technically this isn't a feature toggle, but the recommendations for documenting settings are similar: https://code-annotations.readthedocs.io/en/latest/contrib/how_to/documenting_django_settings.html |
0dc4332
to
a720d61
Compare
@pdpinch I addressed your feedback, anything else ? |
@natabene Can you please initiate the review process ? |
@asadiqbal08 Sure, I will ask for this to be reviewed. |
jenkins run all |
@asadiqbal08 how do I test this in devstack? Where should I set the value for |
I think this needs a rebase, in order to pass tests. |
a720d61
to
0f94d24
Compare
Your PR has finished running tests. There were no failures. |
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.
Hello @asadiqbal08, thank you for your contribution!
👍 once all builds are green.
@asadiqbal08 Please try rebasing so that remaining checks run. |
0f94d24
to
c9852e6
Compare
If redirect_url is not specified in the logout URL (/logout) then the user is by default redirected to edX's dashboard (/dashboard). Introduced a new variable named LOGOUT_REDIRECT_URL which can be used as default redirection-url instead of dashboard.
c9852e6
to
1a86301
Compare
@asadiqbal08 We have not heard from you in a while, so I will close this request for now. Please feel free to reopen, should you decide to pursue this again. |
@asadiqbal08 Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
If redirect_url is not specified in the logout URL (/logout) then the user is by default redirected to edX's dashboard (/dashboard). Introduced a new variable named
LOGOUT_REDIRECT_URL
which can be used as default redirection-url instead of dashboard.Supporting information
issue-271
Deadline
Aim to get this merged before October 9