fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option #13157

iercan · 2021-02-16T17:21:31Z

SUMMARY

This PR resolves #13097
This improvement make thumbnails and new reports module uses WEBDRIVER_WINDOW option from config.

I defined init function to screenshot object to pass window_size and thumb_size same value for reports so that images will not be shrink . Before It was using harcoded 800x600 value which is defined for thumbnails.

TEST PLAN

Create alert and reports. Images should be sent same size defined with WEBDRIVER_WINDOW

ADDITIONAL INFORMATION

Sync with master

dpgaspar · 2021-02-17T14:54:22Z

@iercan thank you for the fix, I restarted CI jobs. But you need to take care of:
https://github.com/apache/superset/pull/13157/checks?check_run_id=1912529461

you can run locally:

git add -u
pre-commit

iercan · 2021-02-17T17:34:26Z

@dpgaspar I executed the commands on my branch. What it is doing?

dpgaspar · 2021-02-18T08:00:57Z

@iercan it does a bunch of things, https://github.com/apache/superset/blob/master/.pre-commit-config.yaml
the most relevant are black, mypy, isort

Also take a look at:
https://github.com/apache/superset/blob/master/CONTRIBUTING.md

iercan · 2021-02-18T08:10:25Z

@dpgaspar so should I recommit and push my changes? or running pre-commit is just enough?

iercan · 2021-02-18T08:26:13Z

@dpgaspar oh I got it. It reformatted files. I had to run as pre-commit run --all-files

codecov-io · 2021-02-18T08:56:55Z

Codecov Report

Merging #13157 (f4cec93) into master (e8857ba) will increase coverage by 0.41%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13157      +/-   ##
==========================================
+ Coverage   66.88%   67.30%   +0.41%     
==========================================
  Files        1021      497     -524     
  Lines       50015    29410   -20605     
  Branches     4907        0    -4907     
==========================================
- Hits        33455    19794   -13661     
+ Misses      16435     9616    -6819     
+ Partials      125        0     -125

Flag	Coverage Δ
cypress	`?`
hive	`66.60% <ø> (?)`
javascript	`?`
postgres	`66.88% <ø> (?)`
presto	`66.63% <ø> (?)`
python	`67.30% <ø> (+3.21%)`	⬆️
sqlite	`66.58% <ø> (?)`

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

Impacted Files	Coverage Δ
superset/utils/celery.py	`89.65% <0.00%> (-10.35%)`	⬇️
superset/utils/cache.py	`76.34% <0.00%> (-8.77%)`	⬇️
superset/dataframe.py	`91.66% <0.00%> (-8.34%)`	⬇️
superset/db_engine_specs/clickhouse.py	`87.09% <0.00%> (-7.35%)`	⬇️
superset/dashboards/commands/update.py	`83.07% <0.00%> (-5.61%)`	⬇️
superset/utils/decorators.py	`94.44% <0.00%> (-5.56%)`	⬇️
superset/reports/notifications/slack.py	`83.72% <0.00%> (-5.47%)`	⬇️
superset/connectors/sqla/views.py	`62.43% <0.00%> (-4.76%)`	⬇️
superset/db_engine_specs/elasticsearch.py	`90.24% <0.00%> (-4.21%)`	⬇️
superset/viz.py	`56.28% <0.00%> (-2.78%)`	⬇️
... and 539 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 e8857ba...f4cec93. Read the comment docs.

dpgaspar · 2021-02-18T09:27:43Z

superset/config.py

@@ -946,7 +946,7 @@ class CeleryConfig:  # pylint: disable=too-few-public-methods
 WEBDRIVER_TYPE = "firefox"

 # Window size - this will impact the rendering of the data
-WEBDRIVER_WINDOW = {"dashboard": (1600, 2000), "slice": (3000, 1200)}
+WEBDRIVER_WINDOW = {"dashboard": (1600, 3000), "slice": (1280, 960)}


This is probably a breaking change, why do we need it?

In my tests too big slice resolution makes pictures blur too much. Here is result for 3000x1200 and 1280x960

@dpgaspar For the record. There will be no problem with reports because I provided same thumb_size and window_size for reports. But they still get so blur for thumbnails. I can revert this change if you want.

I decided to revert that and provide old harcoded sizes for thumbnails so that caches will not be needed to recalculated again.

nice, better to change your PR description: Additional note: This PR will cause thumbnails recalculate because cache keys depends on window size.

Removed that part

superset/reports/commands/execute.py

superset/utils/screenshots.py

defining defaults in thumbnails.py caused thumbnail caches invalidated. they moved to init.

dpgaspar

@iercan, Although this is a bug, and this PR fixes it, it's also a breaking change. Starting to see the necessity to create 2 new config keys, one for thumbnails and another for reports, so that it's possible to configure both independently

iercan · 2021-03-01T08:47:02Z

@iercan, Although this is a bug, and this PR fixes it, it's also a breaking change. Starting to see the necessity to create 2 new config keys, one for thumbnails and another for reports, so that it's possible to configure both independently

Sure I can add 2 different options. But with final edit I made, I don't think it is a breaking change anymore. I kept thumbnails untouched and just reports will use webdriver window. Isn't WEBDRIVER_WINDOW meant to be used just for reports?

dpgaspar · 2021-03-01T09:19:05Z

@iercan, Although this is a bug, and this PR fixes it, it's also a breaking change. Starting to see the necessity to create 2 new config keys, one for thumbnails and another for reports, so that it's possible to configure both independently

Sure I can add 2 different options. But with final edit I made, I don't think it is a breaking change anymore. I kept thumbnails untouched and just reports will use webdriver window. Isn't WEBDRIVER_WINDOW meant to be used just for reports?

it is, it's sort of a breaking change because the new reports will now change its image size. I think it would be more flexible to have the 2 new config keys. Let the new reports still take the screenshot with the previous size, but making it now possible for everyone to configure it

thoughts?

iercan · 2021-03-01T09:37:27Z

it is, it's sort of a breaking change because the new reports will now change its image size. I think it would be more flexible to have the 2 new config keys. Let the new reports still take the screenshot with the previous size, but making it now possible for everyone to configure it

thoughts?

I think adding 2 different config may get configuration more complicated from point of view of a user. Also I'm not sure thumbnail size should be configurable.

dpgaspar · 2021-03-01T10:06:00Z

it is, it's sort of a breaking change because the new reports will now change its image size. I think it would be more flexible to have the 2 new config keys. Let the new reports still take the screenshot with the previous size, but making it now possible for everyone to configure it
thoughts?

I think adding 2 different config may get configuration more complicated from point of view of a user. Also I'm not sure thumbnail size should be configurable.

Good point, but still seems weird to me that by configuring WEBDRIVER_WINDOW we only end up configuring the new alerts & reports window, while thumbnails is hardcoded. Not explicit nor obvious. But this looks good, and I don't want to stale this fix for long. Thank you for your contribution and patience

…WINDOW option (apache#13157) * fix: thumbnails and reports will be use WEBDRIVER_WINDOW option * changes reformatted * config change reverted. thumbnails sizes changed to original * typo fix * bugfix defining defaults in thumbnails.py caused thumbnail caches invalidated. they moved to init. Co-authored-by: Ibrahim Ercan <ibrahim.ercan@vlmedia.com.tr>

iercan and others added 2 commits February 16, 2021 18:10

Merge pull request #1 from apache/master

e813c22

Sync with master

fix: thumbnails and reports will be use WEBDRIVER_WINDOW option

92ee4fb

pull-request-size bot added the size/M label Feb 16, 2021

dpgaspar self-requested a review February 16, 2021 18:14

junlincc added the new:contributor The author is a new contributor label Feb 17, 2021

changes reformatted

2909ab4

dpgaspar reviewed Feb 18, 2021

View reviewed changes

vnourdin reviewed Feb 18, 2021

View reviewed changes

superset/reports/commands/execute.py Show resolved Hide resolved

superset/utils/screenshots.py Show resolved Hide resolved

Ibrahim Ercan added 2 commits February 18, 2021 14:16

config change reverted. thumbnails sizes changed to original

24d3efb

typo fix

07a028f

iercan changed the title ~~fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option~~ fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option Feb 19, 2021

iercan requested a review from dpgaspar February 22, 2021 20:27

betodealmeida added the v1.1 label Feb 27, 2021

bugfix

f4cec93

defining defaults in thumbnails.py caused thumbnail caches invalidated. they moved to init.

dpgaspar reviewed Mar 1, 2021

View reviewed changes

dpgaspar approved these changes Mar 1, 2021

View reviewed changes

dpgaspar merged commit b04aebf into apache:master Mar 2, 2021

iercan mentioned this pull request Mar 26, 2021

chore: script for changelog #13588

Merged

6 tasks

laxmanraparthi mentioned this pull request Nov 30, 2023

[Snyk] Security upgrade jest from 26.6.3 to 29.0.0 ShareChat/superset#70

Open

matrix-compute mentioned this pull request Dec 14, 2023

[Snyk] Fix for 15 vulnerabilities matrix-compute/superset#13

Open

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 labels Mar 12, 2024

laxmanraparthi mentioned this pull request Mar 15, 2024

[Snyk] Fix for 22 vulnerabilities ShareChat/superset#156

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option #13157

fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option #13157

iercan commented Feb 16, 2021 •

edited

Loading

dpgaspar commented Feb 17, 2021

iercan commented Feb 17, 2021

dpgaspar commented Feb 18, 2021

iercan commented Feb 18, 2021 •

edited

Loading

iercan commented Feb 18, 2021 •

edited

Loading

codecov-io commented Feb 18, 2021 •

edited by codecov bot

Loading

dpgaspar Feb 18, 2021

iercan Feb 18, 2021 •

edited

Loading

iercan Feb 18, 2021

iercan Feb 18, 2021 •

edited

Loading

dpgaspar Mar 1, 2021

iercan Mar 1, 2021

dpgaspar left a comment •

edited

Loading

iercan commented Mar 1, 2021 •

edited

Loading

dpgaspar commented Mar 1, 2021

iercan commented Mar 1, 2021

dpgaspar commented Mar 1, 2021

fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option #13157

fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option #13157

Conversation

iercan commented Feb 16, 2021 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

dpgaspar commented Feb 17, 2021

iercan commented Feb 17, 2021

dpgaspar commented Feb 18, 2021

iercan commented Feb 18, 2021 • edited Loading

iercan commented Feb 18, 2021 • edited Loading

codecov-io commented Feb 18, 2021 • edited by codecov bot Loading

Codecov Report

dpgaspar Feb 18, 2021

Choose a reason for hiding this comment

iercan Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

iercan Feb 18, 2021

Choose a reason for hiding this comment

iercan Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

dpgaspar Mar 1, 2021

Choose a reason for hiding this comment

iercan Mar 1, 2021

Choose a reason for hiding this comment

dpgaspar left a comment • edited Loading

Choose a reason for hiding this comment

iercan commented Mar 1, 2021 • edited Loading

dpgaspar commented Mar 1, 2021

iercan commented Mar 1, 2021

dpgaspar commented Mar 1, 2021

iercan commented Feb 16, 2021 •

edited

Loading

iercan commented Feb 18, 2021 •

edited

Loading

iercan commented Feb 18, 2021 •

edited

Loading

codecov-io commented Feb 18, 2021 •

edited by codecov bot

Loading

iercan Feb 18, 2021 •

edited

Loading

iercan Feb 18, 2021 •

edited

Loading

dpgaspar left a comment •

edited

Loading

iercan commented Mar 1, 2021 •

edited

Loading