-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
other: Redirect to the referrer URL when changing locale. #1679
Conversation
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
Codecov Report
@@ Coverage Diff @@
## master #1679 +/- ##
==========================================
- Coverage 76.67% 76.44% -0.24%
==========================================
Files 55 55
Lines 8091 8037 -54
==========================================
- Hits 6204 6144 -60
- Misses 1887 1893 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
Signed-off-by: tianhe1986 <w1s2j3229@163.com>
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.
Thank you for the PR, left a couple of questions
page_history = Stack(session.get("page_history", [])) | ||
page_history.push(request.referrer) | ||
session["page_history"] = page_history.to_json() | ||
|
||
session["locale"] = locale | ||
refresh() | ||
self.update_redirect() |
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.
The LocaleView.index
is already calling update_redirect
why updating it again? because we need to update it with the referrer?
Also would it make more sense to add the update_redirect
to Superset?
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.
The LocaleView.index is already calling update_redirect why updating it again? because we need to update it with the referrer?
Yes, updating it with the referrer before calling update_redirect
would ensure it redirects back to the referrer page.
Also would it make more sense to add the update_redirect to Superset?
Surely you are right:) It is just a bit tough to add update_redirect
in every related view, and the same case even with
build-in pages, not only Superset.
For example, with the URL "/login/", that is, login
function at subclass of class AuthView
, e.g class AuthDBView, it also does not call the update_redirect
and would redirect to other page while changing locale.
Thank you for the contribution! Would love to get it through. @villebro want to chime in? |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Feel free to reopen it if it's still relevant to you. Thank you |
Signed-off-by: tianhe1986 w1s2j3229@163.com
Description
In most case, users hope just refreshing the current page after changing locale.
However, not all pages call
update_redirect
, turning out it will jump to other page at the top of the page history stack.For example, this issue in apache superset
ADDITIONAL INFORMATION