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: shorten url with extra request parameters #10693

Merged

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Aug 27, 2020

SUMMARY

In #10651 I added a feature that standalone iframe can use shorten url.
But later I found there is issue for the existed solution: we store full url in url tbl, which supposed to be content for form_data parameter only. For standalone chart, its full url is like: form_data={key1:value1, key2: value2}&standalone=true&height=400.

This PR is to fix this issue:

  • remove extra parameters standalone and height from shorten url request, request shorten url with only form_data parameter.
  • create standalone url from frontend, with shorten url id and attach standalone=true and height parameter. So the shorten url for standalone chart will be like http://localhost:8080/superset/explore/?r=8015&standalone=true&height=600

TEST PLAN

CI and manual test

ADDITIONAL INFORMATION

@mistercrunch @michellethomas

@graceguo-supercat graceguo-supercat requested review from villebro, willbarrett and john-bodley and removed request for willbarrett August 27, 2020 02:39
@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2020

Codecov Report

Merging #10693 into master will increase coverage by 0.05%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10693      +/-   ##
==========================================
+ Coverage   64.31%   64.36%   +0.05%     
==========================================
  Files         786      786              
  Lines       36924    36931       +7     
  Branches     3514     3514              
==========================================
+ Hits        23746    23769      +23     
+ Misses      13069    13054      -15     
+ Partials      109      108       -1     
Flag Coverage Δ
#cypress 54.88% <100.00%> (+0.28%) ⬆️
#javascript 60.82% <100.00%> (ø)
#python 59.75% <0.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
superset/views/redirects.py 60.00% <0.00%> (-10.00%) ⬇️
superset/views/utils.py 87.14% <0.00%> (-0.84%) ⬇️
...rontend/src/explore/components/EmbedCodeButton.jsx 76.92% <100.00%> (ø)
superset-frontend/src/SqlLab/reducers/sqlLab.js 41.07% <0.00%> (+1.65%) ⬆️
superset-frontend/src/SqlLab/actions/sqlLab.js 64.52% <0.00%> (+1.92%) ⬆️
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx 72.72% <0.00%> (+6.81%) ⬆️
superset-frontend/src/reduxUtils.ts 79.74% <0.00%> (+8.86%) ⬆️

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 6ff96cf...9b73f17. Read the comment docs.

@graceguo-supercat graceguo-supercat changed the title fix: shorten url with extra request parameters [wip] fix: shorten url with extra request parameters Aug 27, 2020
@pull-request-size pull-request-size bot added size/M and removed size/S labels Aug 27, 2020
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM. At some point it would be nice to start porting these to TS and do some additional simplification, especially exploreUtils has some very loaded functions.

@graceguo-supercat graceguo-supercat changed the title [wip] fix: shorten url with extra request parameters fix: shorten url with extra request parameters Aug 27, 2020
@graceguo-supercat graceguo-supercat merged commit 7fc227c into apache:master Aug 27, 2020
michellethomas pushed a commit to airbnb/superset-fork that referenced this pull request Aug 27, 2020
@graceguo-supercat graceguo-supercat deleted the gg-FixIframeShortURL branch September 11, 2020 03:33
@villebro villebro added the v0.38 label Sep 11, 2020
Ofeknielsen pushed a commit to ofekisr/incubator-superset that referenced this pull request Oct 5, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M v0.38 🚢 0.38.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants