Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

fix: Unable to download the Dashboard as image in case there's an image added through Markdown #20362

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

Users can add Markdown to their Dashboards, which allow to include images using <img src="$link">. However, if the Dashboard has an image added this way, the Download as image functionality will fail.

The problem is that, depending on where do we source the image from, we could run into CORS issues.
That, however, show not make the image generation fail.

The library that we use, dom-to-image, is pretty much deprecated and hasn't received updates for a long while.

This PR replaces the library for a fork that's been rewritten and well maintained: dom-to-image-more.
The library does not fail, and instead shows a placeholder if it can't fetch the image.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Screen.Recording.2022-06-13.at.12.40.59.mov

After:

new.mov

TESTING INSTRUCTIONS

  1. Access your Workspace.
  2. Create a new Dashboard.
  3. Add a Markdown element to it.
  4. Add below code to your Markdown:
<img src="https://repository-images.githubusercontent.com/39464018/58649580-eca4-11ea-844d-c2ddca24b226">
  1. Save your changes.
  2. Click on the three ellipses > Download as image.

Ensure the dashboard image is generated.

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

codecov bot commented Jun 13, 2022

Codecov Report

Merging #20362 (11d94ce) into master (11d94ce) will not change coverage.
The diff coverage is n/a.

❗ Current head 11d94ce differs from pull request most recent head 302b402. Consider uploading reports for the commit 302b402 to get more accurate results

@@           Coverage Diff           @@
##           master   #20362   +/-   ##
=======================================
  Coverage   66.70%   66.70%           
=======================================
  Files        1739     1739           
  Lines       65135    65135           
  Branches     6897     6897           
=======================================
  Hits        43450    43450           
  Misses      19932    19932           
  Partials     1753     1753           
Flag Coverage Δ
hive 53.70% <0.00%> (ø)
javascript 51.74% <0.00%> (ø)
mysql 82.29% <0.00%> (ø)
postgres 82.36% <0.00%> (ø)
presto 53.56% <0.00%> (ø)
python 82.78% <0.00%> (ø)
sqlite 82.09% <0.00%> (ø)
unit 50.15% <0.00%> (ø)

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


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11d94ce...302b402. Read the comment docs.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

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

@rusackas
Copy link
Member

On the ephemeral environment linked above, I added a couple images to the markdown widget, and they both appear in the screenshot just fine! Not sure what you did to get the blank/broken image to appear ¯\_(ツ)_/¯

@rusackas
Copy link
Member

The dome-to-image-more.d.ts looks like a typo. Should it be dom-to-image-more.d.ts?

@rusackas
Copy link
Member

Otherwise, just needs a rebase and it looks good!

@diegomedina248 diegomedina248 force-pushed the fix/download-dashboard-image-when-image-embedded-in-md branch from 9a8620d to 302b402 Compare June 16, 2022 14:47
@diegomedina248
Copy link
Contributor Author

On the ephemeral environment linked above, I added a couple images to the markdown widget, and they both appear in the screenshot just fine! Not sure what you did to get the blank/broken image to appear ¯_(ツ)_/¯

Did you try the example from the testing instructions? The problem is not all images, is the one that we fail to query, due to cors issue for example.

@cccs-tom
Copy link
Contributor

@diegomedina248 We ran into this as well, with different types of errors (HTTP 401, 404, etc. on the image we were trying to embed). In your opinion, could this be cherry-picked to LTS / 1.5?

@diegomedina248
Copy link
Contributor Author

@diegomedina248 We ran into this as well, with different types of errors (HTTP 401, 404, etc. on the image we were trying to embed). In your opinion, could this be cherry-picked to LTS / 1.5?

@cccs-tom yeah, this should be good to go.

Copy link
Member

@stephenLYZ stephenLYZ left a comment

Choose a reason for hiding this comment

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

LGTM!

@stephenLYZ stephenLYZ merged commit c5d3678 into apache:master Jun 29, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

michael-s-molina pushed a commit that referenced this pull request Jul 6, 2022
…ge added through Markdown (#20362)

* fix: Unable to download the Dashboard as image in case there's an image added through Markdown

* licence

(cherry picked from commit c5d3678)
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
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 Preset-Patch size/M v2.0 🍒 2.0.0 🍒 2.0.1 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants