Skip to content

fix: Report screenshot cut off on left and right sides#22614

Merged
lyndsiWilliams merged 1 commit into
masterfrom
lyndsi/fix-report-screenshot
Jan 17, 2023
Merged

fix: Report screenshot cut off on left and right sides#22614
lyndsiWilliams merged 1 commit into
masterfrom
lyndsi/fix-report-screenshot

Conversation

@lyndsiWilliams
Copy link
Copy Markdown
Member

SUMMARY

This PR fixes report screenshot width. Screenshots were being cut off on the left and right. I fixed this by having the screenshot capture by class standalone and removing the filter bar when standalone=3.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

reportBefore

AFTER:

reportAfter

TESTING INSTRUCTIONS

  • Add a report and set the notification method to email
    • Set it to send every minute for quicker testing
  • Check your email
  • Ensure that the report is properly bordered on the left and right

ADDITIONAL INFORMATION

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 6, 2023

Codecov Report

Merging #22614 (8b4cd90) into master (db20180) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #22614      +/-   ##
==========================================
+ Coverage   67.00%   67.03%   +0.03%     
==========================================
  Files        1859     1859              
  Lines       71036    71037       +1     
  Branches     7766     7767       +1     
==========================================
+ Hits        47597    47623      +26     
+ Misses      21417    21386      -31     
- Partials     2022     2028       +6     
Flag Coverage Δ
hive 52.47% <100.00%> (ø)
javascript 53.89% <100.00%> (+0.03%) ⬆️
mysql 78.01% <100.00%> (?)
postgres 78.08% <100.00%> (ø)
presto 52.37% <100.00%> (ø)
python 81.35% <100.00%> (+0.03%) ⬆️
sqlite 76.46% <100.00%> (ø)
unit 51.47% <100.00%> (ø)

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

Impacted Files Coverage Δ
...d/components/DashboardBuilder/DashboardBuilder.tsx 73.83% <100.00%> (+0.24%) ⬆️
superset/utils/screenshots.py 48.69% <100.00%> (ø)
superset/views/core.py 74.98% <0.00%> (+0.22%) ⬆️
superset/models/core.py 90.47% <0.00%> (+0.64%) ⬆️
superset/db_engine_specs/mysql.py 98.80% <0.00%> (+4.76%) ⬆️
superset/common/utils/dataframe_utils.py 95.23% <0.00%> (+4.76%) ⬆️
...board/components/nativeFilters/FilterBar/index.tsx 58.88% <0.00%> (+8.88%) ⬆️
...rd/components/nativeFilters/FilterBar/keyValue.tsx 40.74% <0.00%> (+22.22%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@eschutho
Copy link
Copy Markdown
Member

eschutho commented Jan 6, 2023

/testenv up

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jan 6, 2023

@eschutho Ephemeral environment spinning up at http://54.185.134.130:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@lyndsiWilliams lyndsiWilliams merged commit 0807875 into master Jan 17, 2023
@lyndsiWilliams lyndsiWilliams deleted the lyndsi/fix-report-screenshot branch January 17, 2023 18:08
@github-actions
Copy link
Copy Markdown
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@villebro
Copy link
Copy Markdown
Member

villebro commented Jan 24, 2023

@lyndsiWilliams I believe this PR has caused a regression for dashboard thumbnails. When rendering dashboard thumbnails, I'm now getting the following timeout error:

Caching dashboard: http://localhost:8088/superset/dashboard/10/
Processing url for thumbnail: b59b6650f84da2dfa80e5c6a10ba69bc
Init selenium driver
Selenium timed out requesting url http://localhost:8088/superset/dashboard/10/
Traceback (most recent call last):
  File "/Users/.../superset/utils/webdriver.py", line 177, in get_screenshot
    element = WebDriverWait(driver, self._screenshot_locate_wait).until(
  File "/Users/.../venv38/lib/python3.8/site-packages/selenium/webdriver/support/wait.py", line 80, in until
    raise TimeoutException(message, screen, stacktrace)
selenium.common.exceptions.TimeoutException: Message:

In this PR we're assuming that dashboards are always being rendered with the standalone flag, but apparently that's only true for alerts & reports. See the logic for dashboard reports (See standalone being set to DashboardStandaloneMode.REPORT.value):

return get_url_path(
"Superset.dashboard",
user_friendly=user_friendly,
dashboard_id_or_slug=self._report_schedule.dashboard_id,
standalone=DashboardStandaloneMode.REPORT.value,
force=force,
**kwargs,
)

Contrast that with dashboard thumbnails (notice the missing standalone kwarg):
url = get_url_path("Superset.dashboard", dashboard_id_or_slug=dashboard.id)

It might be a good idea to create a util for generating the chart and dashboard URLs to ensure we're always using the same logic for alerts, reports and thumbs. Thoughts?

AnushaErrabelli added a commit to preset-io/superset that referenced this pull request Jan 30, 2023
@eschutho
Copy link
Copy Markdown
Member

eschutho commented Feb 8, 2023

It might be a good idea to create a util for generating the chart and dashboard URLs to ensure we're always using the same logic for alerts, reports and thumbs. Thoughts?

Thanks for catching this error @villebro! I took your suggestion in #23003 and wound up moving the standalone logic closer to the screenshot classes that define the element to capture. I'd love to get your thoughts on that approach.

@geido geido mentioned this pull request Mar 31, 2023
9 tasks
@mistercrunch mistercrunch added the 🚢 2.1.3 First shipped in 2.1.3 label Feb 18, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 First shipped in 2.1.0 and removed 🚢 2.1.3 First shipped in 2.1.3 labels Mar 13, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 2.1.0 First shipped in 2.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants