Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: parameterize titles correctly #14509

Merged
merged 1 commit into from
May 6, 2021
Merged

fix: parameterize titles correctly #14509

merged 1 commit into from
May 6, 2021

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented May 6, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented May 6, 2021 •
edited
Loading

Codecov Report

Merging #14509 (ec3f068) into master (1df9384) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14509      +/-   ##
==========================================
- Coverage   77.15%   77.06%   -0.09%     
==========================================
  Files         958      958              
  Lines       48241    48241              
  Branches     5636     5636              
==========================================
- Hits        37219    37178      -41     
- Misses      10821    10862      +41     
  Partials      201      201
Flag Coverage Δ
hive 80.80% <ø> (ø)
javascript 72.01% <ø> (ø)
mysql 81.06% <ø> (ø)
postgres 81.09% <ø> (ø)
presto ?
python 81.47% <ø> (-0.16%) ⬇️
sqlite 80.70% <ø> (ø)

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

Impacted Files Coverage Δ
superset/views/core.py 75.51% <ø> (ø)
superset/db_engine_specs/presto.py 84.42% <0.00%> (-5.90%) ⬇️
superset/connectors/sqla/models.py 88.61% <0.00%> (-1.46%) ⬇️
superset/models/core.py 88.85% <0.00%> (-0.28%) ⬇️

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 1df9384...ec3f068. Read the comment docs.

nytai
nytai reviewed May 6, 2021
View reviewed changes
superset/views/core.py
@@ -837,7 +837,7 @@ def explore( # pylint: disable=too-many-locals,too-many-return-statements,too-m
bootstrap_data, default=utils.pessimistic_json_iso_dttm_ser
),
entry="explore",
title=title,
title=title.__str__(),
Copy link
Member

Choose a reason for hiding this comment

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

wait, so title is a instance of LazyString? Seems like it should just be a string here

Copy link
Member Author

Choose a reason for hiding this comment

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

Title is an instance of lazystring in certain cases. The call to __str__() evaluates it to a string.

Copy link
Member

Choose a reason for hiding this comment

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

i see now, it's title = _("Explore - %(table)s", table=table_name) that's the problematic code path

Copy link
Member

Choose a reason for hiding this comment

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

any reason to use title.__str__() over str(title)?

@suddjian suddjian merged commit 52dbe31 into master May 6, 2021
@suddjian suddjian deleted the fix/title-text branch May 6, 2021 22:39
@villebro villebro added the v1.2 label May 7, 2021
amitmiran137 pushed a commit that referenced this pull request May 7, 2021
@suddjian
fix: parameterize titles correctly (#14509)
c2d2478 
(cherry picked from commit 52dbe31)
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Sep 8, 2021
@suddjian @cccs-RyanS
fix: parameterize titles correctly (apache#14509)
b73ba12 
(cherry picked from commit 52dbe31)
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@suddjian
fix: parameterize titles correctly (apache#14509)
e56387c
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@suddjian
fix: parameterize titles correctly (apache#14509)
3cfc0e5
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@suddjian
fix: parameterize titles correctly (apache#14509)
1ea3e80
@mistercrunch mistercrunch added 🍒 1.2.0 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@mistercrunch mistercrunch mistercrunch left review comments

@nytai nytai nytai approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io size/XS v1.2 🍒 1.2.0 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@suddjian @mistercrunch @nytai @villebro