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.

Sign up for GitHub

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

Jump to bottom

fix(thumbnails): missing field, logging and new config var #10562

Merged
merged 7 commits into from
Aug 14, 2020
Merged

fix(thumbnails): missing field, logging and new config var #10562

merged 7 commits into from
Aug 14, 2020

Conversation

dpgaspar
Copy link
Member

@dpgaspar dpgaspar commented Aug 10, 2020 •
edited
Loading

SUMMARY

PR with a couple of minor fixes:

  • Add missing thumbnail_url to the list chart API
  • Fixes chart thumbnail logging
  • Introduces new CONFIG key, distinct from the email report key

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API: Removes EMAIL_REPORTS_WEBDRIVER config key

dpgaspar
dpgaspar commented Aug 10, 2020
View reviewed changes
superset/charts/api.py
"table.default_endpoint",
"table.table_name",
"thumbnail_url",
Copy link
Member Author

Choose a reason for hiding this comment

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

sorted and added a new field thumbnail_url

@dpgaspar dpgaspar marked this pull request as ready for review August 10, 2020 13:39
bkyryliuk
bkyryliuk approved these changes Aug 10, 2020
View reviewed changes
superset/config.py Outdated
@@ -797,6 +797,8 @@ class CeleryConfig: # pylint: disable=too-few-public-methods

# Any config options to be passed as-is to the webdriver
WEBDRIVER_CONFIGURATION: Dict[Any, Any] = {}
# The webdriver to use supports "chrome" and "firefox"
WEBDRIVER_TYPE = "firefox"
Copy link
Member

@bkyryliuk bkyryliuk Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

let's use EMAIL_REPORTS_WEBDRIVER or just converge those 2
easy solution would be:

WEBDRIVER_TYPE = EMAIL_REPORTS_WEBDRIVER

or

EMAIL_REPORTS_WEBDRIVER = WEBDRIVER_TYPE

Copy link
Member Author

@dpgaspar dpgaspar Aug 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe took it too far, but I actually removed it now. Tell me if it's ok by you @bkyryliuk

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2020 •
edited
Loading

Codecov Report

Merging #10562 into master will increase coverage by 4.15%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10562      +/-   ##
==========================================
+ Coverage   60.10%   64.26%   +4.15%     
==========================================
  Files         775      775              
  Lines       36528    36529       +1     
  Branches     3459     3459              
==========================================
+ Hits        21955    23474    +1519     
+ Misses      14382    12943    -1439     
+ Partials      191      112      -79
Flag Coverage Δ
#cypress 54.47% <ø> (?)
#javascript 60.48% <ø> (ø)
#python 59.85% <83.33%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
superset/tasks/thumbnails.py 43.33% <0.00%> (ø)
superset/charts/api.py 73.89% <100.00%> (ø)
superset/config.py 90.34% <100.00%> (ø)
superset/tasks/schedules.py 79.52% <100.00%> (ø)
superset/utils/screenshots.py 36.36% <100.00%> (ø)
superset/db_engine_specs/sqlite.py 65.62% <0.00%> (-9.38%) ⬇️
superset/result_set.py 96.69% <0.00%> (-1.66%) ⬇️
superset/sql_lab.py 77.92% <0.00%> (-0.44%) ⬇️
...src/components/FilterableTable/FilterableTable.tsx 81.77% <0.00%> (-0.41%) ⬇️
...rontend/src/SqlLab/components/AceEditorWrapper.tsx 56.98% <0.00%> (+1.07%) ⬆️
... and 147 more

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 db88cec...d453071. Read the comment docs.

mistercrunch
mistercrunch reviewed Aug 10, 2020
View reviewed changes
superset/config.py
@@ -790,7 +790,7 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
# chrome:
# Requires: headless chrome
# Limitations: unable to generate screenshots of elements
EMAIL_REPORTS_WEBDRIVER = "firefox"
WEBDRIVER_TYPE = "firefox"
Copy link
Member

Choose a reason for hiding this comment

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

optional: change default to "chrome" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would, but: https://github.com/apache/incubator-superset/blob/master/docs/installation.rst#email-reports

States that firefox is preferred, is it reasonable to change the default?

Copy link
Member

Choose a reason for hiding this comment

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

I believe there are unresolved rendering errors on Firefox. Might not be applicable for thumbs, but worth mentioning.

@dpgaspar dpgaspar mentioned this pull request Aug 11, 2020
Merged
villebro
villebro approved these changes Aug 11, 2020
View reviewed changes
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

superset/config.py
@@ -790,7 +790,7 @@ class CeleryConfig: # pylint: disable=too-few-public-methods
# chrome:
# Requires: headless chrome
# Limitations: unable to generate screenshots of elements
EMAIL_REPORTS_WEBDRIVER = "firefox"
WEBDRIVER_TYPE = "firefox"
Copy link
Member

Choose a reason for hiding this comment

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

I believe there are unresolved rendering errors on Firefox. Might not be applicable for thumbs, but worth mentioning.

@dpgaspar dpgaspar merged commit 4dd1d1d into apache:master Aug 14, 2020
@dpgaspar dpgaspar deleted the fix/minor-thumbnails branch August 14, 2020 13:12
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
@dpgaspar
fix(thumbnails): missing field, logging and new config var (apache#10562
8f01149 
)

* fix(thumbnails): missing field, logging and new config var

* deprecate EMAIL_REPORTS_WEBDRIVER

* fix after merge and conflicts

* fix tests

* black
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@dpgaspar @auxten
fix(thumbnails): missing field, logging and new config var (apache#10562
4af2d11 
)

* fix(thumbnails): missing field, logging and new config var

* deprecate EMAIL_REPORTS_WEBDRIVER

* fix after merge and conflicts

* fix tests

* black
@octaviancorlade octaviancorlade mentioned this pull request Apr 2, 2021
Merged
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mistercrunch mistercrunch mistercrunch approved these changes

@bkyryliuk bkyryliuk bkyryliuk approved these changes

@villebro villebro villebro approved these changes

@nytai nytai Awaiting requested review from nytai

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.38.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dpgaspar @codecov-commenter @mistercrunch @bkyryliuk @villebro