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

other: Redirect to the referrer URL when changing locale. #1679

Closed

Conversation

tianhe1986
Copy link

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

  • Has associated issue:
  • Is CRUD MVC related.
  • Is Auth, RBAC security related.
  • Changes the security db schema.
  • Introduces new feature
  • Removes existing feature

Signed-off-by: tianhe1986 <w1s2j3229@163.com>
@tianhe1986 tianhe1986 changed the title Redirect to the referrer URL when changing locale. other: Redirect to the referrer URL when changing locale. Aug 20, 2021
@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #1679 (f3703fa) into master (fdf5784) will decrease coverage by 0.23%.
The diff coverage is 100.00%.

❗ Current head f3703fa differs from pull request most recent head bad9fb5. Consider uploading reports for the commit bad9fb5 to get more accurate results
Impacted file tree graph

@@            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     
Flag Coverage Δ
python 76.44% <100.00%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flask_appbuilder/babel/views.py 94.73% <100.00%> (+1.87%) ⬆️
flask_appbuilder/models/sqla/filters.py 76.66% <0.00%> (-4.18%) ⬇️
flask_appbuilder/views.py 65.26% <0.00%> (-0.85%) ⬇️
flask_appbuilder/models/base.py 73.74% <0.00%> (-0.30%) ⬇️
flask_appbuilder/security/views.py 61.74% <0.00%> (-0.28%) ⬇️
flask_appbuilder/exceptions.py 100.00% <0.00%> (ø)
flask_appbuilder/validators.py 100.00% <0.00%> (ø)
flask_appbuilder/security/forms.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eba517a...bad9fb5. Read the comment docs.

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>
Copy link
Owner

@dpgaspar dpgaspar left a 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()
Copy link
Owner

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?

Copy link
Author

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.

@junlincc
Copy link

Thank you for the contribution! Would love to get it through. @villebro want to chime in?

@stale
Copy link

stale bot commented Mar 30, 2022

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

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

Successfully merging this pull request may close these issues.

3 participants