You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/16 17:21:45 UTC

[GitHub] [superset] iercan opened a new pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

iercan opened a new pull request #13157:
URL: https://github.com/apache/superset/pull/13157


   ### SUMMARY
   This PR resolves #13097
   This improvements make thumbnails and new reports module uses WEBDRIVER_WINDOW option from config. I also changed default values to more proper values according to my tests. 
   
   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. 
   
   Additional note: This PR will cause thumbnails recalculate because cache keys depends on window size. 
   
   ### TEST PLAN
   Create alert and reports. Images should send the same size defined with WEBDRIVER_WINDOW
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] 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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (2909ab4) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `8.87%`.
   > The diff coverage is `85.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   - Coverage   66.88%   58.01%   -8.88%     
   ==========================================
     Files        1021      468     -553     
     Lines       50015    15871   -34144     
     Branches     4907     4077     -830     
   ==========================================
   - Hits        33455     9208   -24247     
   + Misses      16435     6663    -9772     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.01% <85.78%> (+7.09%)` | :arrow_up: |
   | javascript | `?` | |
   | python | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <ø> (-62.19%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `50.00% <0.00%> (-2.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.68% <ø> (-80.84%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <ø> (-16.67%)` | :arrow_down: |
   | ... and [977 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...2909ab4](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f4cec93) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `0.31%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   + Coverage   66.88%   67.20%   +0.31%     
   ==========================================
     Files        1021      497     -524     
     Lines       50015    29410   -20605     
     Branches     4907        0    -4907     
   ==========================================
   - Hits        33455    19764   -13691     
   + Misses      16435     9646    -6789     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | hive | `66.60% <ø> (?)` | |
   | javascript | `?` | |
   | postgres | `66.88% <ø> (?)` | |
   | python | `67.20% <ø> (+3.10%)` | :arrow_up: |
   | sqlite | `66.58% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
   | [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
   | [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `82.47% <0.00%> (-6.05%)` | :arrow_down: |
   | [superset/dashboards/commands/update.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy91cGRhdGUucHk=) | `83.07% <0.00%> (-5.61%)` | :arrow_down: |
   | [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
   | [superset/reports/notifications/slack.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrLnB5) | `83.72% <0.00%> (-5.47%)` | :arrow_down: |
   | [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <0.00%> (-4.76%)` | :arrow_down: |
   | [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `90.24% <0.00%> (-4.21%)` | :arrow_down: |
   | ... and [540 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578265749



##########
File path: 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)}

Review comment:
       In my tests too big slice resolution makes pictures blur too much. Here is result for 3000x1200 and 1280x960
   ![image](https://user-images.githubusercontent.com/3406152/108336911-9703f300-71e5-11eb-8e8d-8f59ec777e74.png)
   
   
   ![image](https://user-images.githubusercontent.com/3406152/108337108-d16d9000-71e5-11eb-962a-9fe2497c62e4.png)
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781134168


   @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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578259740



##########
File path: 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)}

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on a change in pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r584521677



##########
File path: 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)}

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f22f1a0) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `2.78%`.
   > The diff coverage is `24.03%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   + Coverage   66.88%   69.67%   +2.78%     
   ==========================================
     Files        1021      797     -224     
     Lines       50015    40522    -9493     
     Branches     4907     4084     -823     
   ==========================================
   - Hits        33455    28235    -5220     
   + Misses      16435    12287    -4148     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `53.36% <24.03%> (+2.43%)` | :arrow_up: |
   | javascript | `?` | |
   | mysql | `80.31% <ø> (?)` | |
   | postgres | `80.35% <ø> (?)` | |
   | python | `80.39% <ø> (+16.30%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | `20.83% <ø> (ø)` | |
   | [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <ø> (-84.00%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/QueryHistory.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS5qc3g=) | `50.00% <0.00%> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.88% <0.00%> (-80.64%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | ... and [798 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787771111


   > @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 keep thumbnails untouched and just reports will use webdriver window. Isn't it WEBDRIVER_WINDOW meant to use just for reports? 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787771111


   > @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 keep thumbnails untouched and just reports will use webdriver window. Isn't WEBDRIVER_WINDOW meant to be used just for reports? 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f4cec93) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `0.41%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            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%)` | :arrow_up: |
   | sqlite | `66.58% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
   | [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
   | [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
   | [superset/dashboards/commands/update.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy91cGRhdGUucHk=) | `83.07% <0.00%> (-5.61%)` | :arrow_down: |
   | [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
   | [superset/reports/notifications/slack.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrLnB5) | `83.72% <0.00%> (-5.47%)` | :arrow_down: |
   | [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <0.00%> (-4.76%)` | :arrow_down: |
   | [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `90.24% <0.00%> (-4.21%)` | :arrow_down: |
   | [superset/viz.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdml6LnB5) | `56.28% <0.00%> (-2.78%)` | :arrow_down: |
   | ... and [539 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] vnourdin commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
vnourdin commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578349478



##########
File path: superset/utils/screenshots.py
##########
@@ -195,12 +195,34 @@ def resize_image(
 class ChartScreenshot(BaseScreenshot):
     thumbnail_type: str = "chart"
     element: str = "chart-container"
-    window_size: WindowSize = (800, 600)
-    thumb_size: WindowSize = (800, 600)
+
+    def __init__(
+        self,
+        url: str,
+        digest: str,
+        window_size: Optional[WindowSize] = None,
+        thumb_size: Optional[WindowSize] = None,
+    ):
+        super().__init__(url, digest)
+        self.window_size = (
+            window_size or current_app.config["WEBDRIVER_WINDOW"]["slice"]
+        )
+        self.thumb_size = thumb_size or (800, 600)

Review comment:
       OK, thanks :ok_hand: 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-780723989


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar merged pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #13157:
URL: https://github.com/apache/superset/pull/13157


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (2909ab4) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `8.46%`.
   > The diff coverage is `85.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   - Coverage   66.88%   58.42%   -8.47%     
   ==========================================
     Files        1021      468     -553     
     Lines       50015    15871   -34144     
     Branches     4907     4077     -830     
   ==========================================
   - Hits        33455     9272   -24183     
   + Misses      16435     6599    -9836     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.42% <85.78%> (+7.49%)` | :arrow_up: |
   | javascript | `?` | |
   | python | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <ø> (-62.19%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `50.00% <0.00%> (-2.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.68% <ø> (-80.84%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `50.00% <ø> (ø)` | |
   | ... and [977 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...2909ab4](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f4cec93) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `0.03%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   + Coverage   66.88%   66.92%   +0.03%     
   ==========================================
     Files        1021      497     -524     
     Lines       50015    29382   -20633     
     Branches     4907        0    -4907     
   ==========================================
   - Hits        33455    19663   -13792     
   + Misses      16435     9719    -6716     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `?` | |
   | postgres | `66.88% <ø> (?)` | |
   | python | `66.92% <ø> (+2.82%)` | :arrow_up: |
   | sqlite | `66.58% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `74.23% <0.00%> (-16.93%)` | :arrow_down: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
   | [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
   | [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.62% <0.00%> (-6.91%)` | :arrow_down: |
   | [superset/dashboards/commands/update.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy91cGRhdGUucHk=) | `83.07% <0.00%> (-5.61%)` | :arrow_down: |
   | [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
   | [superset/reports/notifications/slack.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrLnB5) | `83.72% <0.00%> (-5.47%)` | :arrow_down: |
   | ... and [541 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787807627


    
   > 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. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781150971


   @dpgaspar oh I got it. It reformatted files. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (2909ab4) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `3.05%`.
   > The diff coverage is `85.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   - Coverage   66.88%   63.83%   -3.06%     
   ==========================================
     Files        1021      960      -61     
     Lines       50015    44889    -5126     
     Branches     4907     4077     -830     
   ==========================================
   - Hits        33455    28654    -4801     
   + Misses      16435    16235     -200     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.42% <85.78%> (+7.49%)` | :arrow_up: |
   | javascript | `?` | |
   | python | `66.79% <ø> (+2.69%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <ø> (-62.19%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `50.00% <0.00%> (-2.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.68% <ø> (-80.84%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `50.00% <ø> (ø)` | |
   | ... and [563 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...2909ab4](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r584530650



##########
File path: 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)}

Review comment:
       Removed that part




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578291158



##########
File path: 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)}

Review comment:
       @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. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578265749



##########
File path: 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)}

Review comment:
       In my tests too big slice resolution makes pictures blur too much. Here is result for 3000x1200 and 1280x960
   
   ![image](https://user-images.githubusercontent.com/3406152/108337295-08dc3c80-71e6-11eb-9954-b6bd5ae29969.png)
   
   
   ![image](https://user-images.githubusercontent.com/3406152/108337108-d16d9000-71e5-11eb-962a-9fe2497c62e4.png)
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578337810



##########
File path: superset/utils/screenshots.py
##########
@@ -195,12 +195,34 @@ def resize_image(
 class ChartScreenshot(BaseScreenshot):
     thumbnail_type: str = "chart"
     element: str = "chart-container"
-    window_size: WindowSize = (800, 600)
-    thumb_size: WindowSize = (800, 600)
+
+    def __init__(
+        self,
+        url: str,
+        digest: str,
+        window_size: Optional[WindowSize] = None,
+        thumb_size: Optional[WindowSize] = None,
+    ):
+        super().__init__(url, digest)
+        self.window_size = (
+            window_size or current_app.config["WEBDRIVER_WINDOW"]["slice"]
+        )
+        self.thumb_size = thumb_size or (800, 600)

Review comment:
       It was hardcoded before. I added to init function so that they can be given as different values 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578339256



##########
File path: superset/reports/commands/execute.py
##########
@@ -152,11 +152,19 @@ def _get_screenshot(self) -> ScreenshotData:
         screenshot: Optional[BaseScreenshot] = None
         if self._report_schedule.chart:
             url = self._get_url(standalone="true")
-            screenshot = ChartScreenshot(url, self._report_schedule.chart.digest)
+            screenshot = ChartScreenshot(
+                url,
+                self._report_schedule.chart.digest,
+                window_size=app.config["WEBDRIVER_WINDOW"]["slice"],

Review comment:
       Yeah I used config value as default but maybe in future default value could change thats why I thought it is better to provide it from init function. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (2909ab4) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `11.19%`.
   > The diff coverage is `85.78%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           master   #13157       +/-   ##
   ===========================================
   - Coverage   66.88%   55.69%   -11.20%     
   ===========================================
     Files        1021      468      -553     
     Lines       50015    15871    -34144     
     Branches     4907     4077      -830     
   ===========================================
   - Hits        33455     8839    -24616     
   + Misses      16435     7032     -9403     
   + Partials      125        0      -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `55.69% <85.78%> (+4.76%)` | :arrow_up: |
   | javascript | `?` | |
   | python | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <ø> (-62.19%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `50.00% <0.00%> (-2.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.68% <ø> (-80.84%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <ø> (-16.67%)` | :arrow_down: |
   | ... and [988 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...2909ab4](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (ccf523c) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `2.54%`.
   > The diff coverage is `80.09%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   - Coverage   66.88%   64.34%   -2.55%     
   ==========================================
     Files        1021      960      -61     
     Lines       50015    44919    -5096     
     Branches     4907     4078     -829     
   ==========================================
   - Hits        33455    28901    -4554     
   + Misses      16435    16018     -417     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `58.45% <80.09%> (+7.53%)` | :arrow_up: |
   | javascript | `?` | |
   | python | `67.55% <ø> (+3.46%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/ResultSet.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1Jlc3VsdFNldC50c3g=) | `4.16% <0.00%> (-62.19%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `50.00% <0.00%> (-2.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.68% <ø> (-80.84%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `50.00% <ø> (ø)` | |
   | ... and [539 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781139182


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] vnourdin commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
vnourdin commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578306009



##########
File path: superset/reports/commands/execute.py
##########
@@ -152,11 +152,19 @@ def _get_screenshot(self) -> ScreenshotData:
         screenshot: Optional[BaseScreenshot] = None
         if self._report_schedule.chart:
             url = self._get_url(standalone="true")
-            screenshot = ChartScreenshot(url, self._report_schedule.chart.digest)
+            screenshot = ChartScreenshot(
+                url,
+                self._report_schedule.chart.digest,
+                window_size=app.config["WEBDRIVER_WINDOW"]["slice"],

Review comment:
       Why passing this as a default value, it's already the default value in your `ChartScreenshot` __init__ :shrug:
   Same for DashboardScreenshot.

##########
File path: superset/utils/screenshots.py
##########
@@ -195,12 +195,34 @@ def resize_image(
 class ChartScreenshot(BaseScreenshot):
     thumbnail_type: str = "chart"
     element: str = "chart-container"
-    window_size: WindowSize = (800, 600)
-    thumb_size: WindowSize = (800, 600)
+
+    def __init__(
+        self,
+        url: str,
+        digest: str,
+        window_size: Optional[WindowSize] = None,
+        thumb_size: Optional[WindowSize] = None,
+    ):
+        super().__init__(url, digest)
+        self.window_size = (
+            window_size or current_app.config["WEBDRIVER_WINDOW"]["slice"]
+        )
+        self.thumb_size = thumb_size or (800, 600)

Review comment:
       Why hard-coding this value instead of using the same as `window_size`? I don't know what it's used for, I'll dig maybe you can enlighten me :wink: 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787825096


   > > 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
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-780610622


   @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
   ```
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] dpgaspar commented on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787794730


   > > @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?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f22f1a0) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `4.38%`.
   > The diff coverage is `24.03%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   + Coverage   66.88%   71.27%   +4.38%     
   ==========================================
     Files        1021      801     -220     
     Lines       50015    40769    -9246     
     Branches     4907     4167     -740     
   ==========================================
   - Hits        33455    29058    -4397     
   + Misses      16435    11711    -4724     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `57.59% <24.03%> (+6.67%)` | :arrow_up: |
   | javascript | `?` | |
   | mysql | `80.31% <ø> (?)` | |
   | postgres | `80.35% <ø> (?)` | |
   | python | `80.39% <ø> (+16.30%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `100.00% <ø> (ø)` | |
   | [.../src/SqlLab/components/EstimateQueryCostButton.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0VzdGltYXRlUXVlcnlDb3N0QnV0dG9uLmpzeA==) | `20.83% <ø> (ø)` | |
   | [...end/src/SqlLab/components/ExploreResultsButton.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0V4cGxvcmVSZXN1bHRzQnV0dG9uLmpzeA==) | `8.00% <ø> (-84.00%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/QueryHistory.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5SGlzdG9yeS5qc3g=) | `50.00% <0.00%> (ø)` | |
   | [...frontend/src/SqlLab/components/QueryStateLabel.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5U3RhdGVMYWJlbC5qc3g=) | `40.00% <0.00%> (-60.00%)` | :arrow_down: |
   | [...rset-frontend/src/SqlLab/components/QueryTable.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5VGFibGUuanN4) | `11.11% <ø> (-55.56%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SaveDatasetModal.tsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NhdmVEYXRhc2V0TW9kYWwudHN4) | `18.18% <ø> (-69.32%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SouthPane.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NvdXRoUGFuZS5qc3g=) | `64.10% <ø> (-17.95%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/components/TableElement.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmxlRWxlbWVudC5qc3g=) | `5.88% <0.00%> (-80.64%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `23.80% <0.00%> (+9.92%)` | :arrow_up: |
   | ... and [786 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-787771111


   > @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? 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f4cec93) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **increase** coverage by `0.31%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   + Coverage   66.88%   67.20%   +0.31%     
   ==========================================
     Files        1021      497     -524     
     Lines       50015    29410   -20605     
     Branches     4907        0    -4907     
   ==========================================
   - Hits        33455    19765   -13690     
   + Misses      16435     9645    -6790     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | hive | `66.60% <ø> (?)` | |
   | javascript | `?` | |
   | postgres | `66.88% <ø> (?)` | |
   | python | `67.20% <ø> (+3.11%)` | :arrow_up: |
   | sqlite | `66.58% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
   | [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
   | [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `82.47% <0.00%> (-6.05%)` | :arrow_down: |
   | [superset/dashboards/commands/update.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy91cGRhdGUucHk=) | `83.07% <0.00%> (-5.61%)` | :arrow_down: |
   | [superset/utils/decorators.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `94.44% <0.00%> (-5.56%)` | :arrow_down: |
   | [superset/reports/notifications/slack.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVwb3J0cy9ub3RpZmljYXRpb25zL3NsYWNrLnB5) | `83.72% <0.00%> (-5.47%)` | :arrow_down: |
   | [superset/connectors/sqla/views.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL3ZpZXdzLnB5) | `62.43% <0.00%> (-4.76%)` | :arrow_down: |
   | [superset/db\_engine\_specs/elasticsearch.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2VsYXN0aWNzZWFyY2gucHk=) | `90.24% <0.00%> (-4.21%)` | :arrow_down: |
   | ... and [539 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578333954



##########
File path: 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)}

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

##########
File path: 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)}

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan edited a comment on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781150971


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578265749



##########
File path: 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)}

Review comment:
       In my tests too big slice resolution makes pictures blur too much. Here is result for 3000x1200 and 1280x960
   ![image](https://user-images.githubusercontent.com/3406152/108336911-9703f300-71e5-11eb-8e8d-8f59ec777e74.png)
   ![image](https://user-images.githubusercontent.com/3406152/108337108-d16d9000-71e5-11eb-962a-9fe2497c62e4.png)
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan edited a comment on pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781139182


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] iercan commented on a change in pull request #13157: fix: Report and thumbnails will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
iercan commented on a change in pull request #13157:
URL: https://github.com/apache/superset/pull/13157#discussion_r578337076



##########
File path: superset/utils/screenshots.py
##########
@@ -195,12 +195,34 @@ def resize_image(
 class ChartScreenshot(BaseScreenshot):
     thumbnail_type: str = "chart"
     element: str = "chart-container"
-    window_size: WindowSize = (800, 600)
-    thumb_size: WindowSize = (800, 600)
+
+    def __init__(
+        self,
+        url: str,
+        digest: str,
+        window_size: Optional[WindowSize] = None,
+        thumb_size: Optional[WindowSize] = None,
+    ):
+        super().__init__(url, digest)
+        self.window_size = (
+            window_size or current_app.config["WEBDRIVER_WINDOW"]["slice"]
+        )
+        self.thumb_size = thumb_size or (800, 600)

Review comment:
       If I didn't understand wrong, screenshots taken with resolution of windows_size and resized to thumb_size(maybe to save space I'm not sure why). I provided thumb_size same with window_size for reports so that they will not shrink.  




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] codecov-io edited a comment on pull request #13157: fix(alerts&reports): Alerts & Reports will use values from WEBDRIVER_WINDOW option

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #13157:
URL: https://github.com/apache/superset/pull/13157#issuecomment-781189430


   # [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=h1) Report
   > Merging [#13157](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=desc) (f4cec93) into [master](https://codecov.io/gh/apache/superset/commit/e8857bac04e8d7b7840c4e603820a6bc34c052bd?el=desc) (e8857ba) will **decrease** coverage by `0.30%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/superset/pull/13157/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #13157      +/-   ##
   ==========================================
   - Coverage   66.88%   66.58%   -0.31%     
   ==========================================
     Files        1021      497     -524     
     Lines       50015    29382   -20633     
     Branches     4907        0    -4907     
   ==========================================
   - Hits        33455    19565   -13890     
   + Misses      16435     9817    -6618     
   + Partials      125        0     -125     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `?` | |
   | python | `66.58% <ø> (+2.49%)` | :arrow_up: |
   | sqlite | `66.58% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/databases/commands/create.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL2NyZWF0ZS5weQ==) | `32.65% <0.00%> (-59.19%)` | :arrow_down: |
   | [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `60.34% <0.00%> (-22.12%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `74.23% <0.00%> (-16.93%)` | :arrow_down: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `89.65% <0.00%> (-10.35%)` | :arrow_down: |
   | [superset/utils/cache.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2FjaGUucHk=) | `76.34% <0.00%> (-8.77%)` | :arrow_down: |
   | [superset/dataframe.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWZyYW1lLnB5) | `91.66% <0.00%> (-8.34%)` | :arrow_down: |
   | [superset/databases/commands/update.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `85.71% <0.00%> (-8.17%)` | :arrow_down: |
   | [superset/db\_engine\_specs/clickhouse.py](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2NsaWNraG91c2UucHk=) | `87.09% <0.00%> (-7.35%)` | :arrow_down: |
   | ... and [551 more](https://codecov.io/gh/apache/superset/pull/13157/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=footer). Last update [e8857ba...f4cec93](https://codecov.io/gh/apache/superset/pull/13157?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org