refactor: dashboard->explore url generation

[suddjian]

SUMMARY

Just a small refactor that I didn't get a chance to push before #17123 was merged. This separates the dashboard-specific function from the function used by Explore. Extending rather than modifying to minimize the footprint of this change.

TESTING INSTRUCTIONS

Go to a dashboard and Explore Chart on one of the charts.

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]

Codecov Report

Merging #17145 (6cdc8ff) into master (ae4ced8) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #17145      +/-   ##
==========================================
+ Coverage   76.68%   76.90%   +0.22%     
==========================================
  Files        1038     1038              
  Lines       55516    55544      +28     
  Branches     7564     7564              
==========================================
+ Hits        42570    42717     +147     
+ Misses      12696    12577     -119     
  Partials      250      250              

Flag	Coverage Δ
hive	81.44% <ø> (?)
javascript	70.93% <100.00%> (+0.02%)	⬆️
mysql	81.90% <ø> (ø)
postgres	81.91% <ø> (ø)
presto	81.77% <ø> (?)
python	82.41% <ø> (+0.41%)	⬆️
sqlite	81.58% <ø> (ø)

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

Impacted Files	Coverage Δ
.../src/dashboard/components/gridComponents/Chart.jsx	80.37% <100.00%> (ø)
...uperset-frontend/src/explore/exploreUtils/index.js	67.07% <100.00%> (+0.40%)	⬆️
...src/dashboard/components/DashboardBuilder/state.ts	71.42% <0.00%> (-1.75%)	⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx	80.64% <0.00%> (-0.47%)	⬇️
...board/components/nativeFilters/FilterBar/index.tsx	88.09% <0.00%> (-0.10%)	⬇️
...Filters/FilterBar/FilterControls/FilterControl.tsx	100.00% <0.00%> (ø)
.../FilterBar/CascadeFilters/CascadePopover/index.tsx	63.23% <0.00%> (ø)
...frontend/src/components/TimezoneSelector/index.tsx	97.77% <0.00%> (+0.05%)	⬆️
superset/utils/core.py	90.09% <0.00%> (+0.11%)	⬆️
...src/filters/components/Range/RangeFilterPlugin.tsx	88.57% <0.00%> (+0.33%)	⬆️
... and 13 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 ae4ced8...6cdc8ff. Read the comment docs.

[graceguo-supercat]

Hi @suddjian thanks for the work. But I do not think from dashboard -> explore chart should use GET request with full form params in the url. Superset used to handle this issue very well, but was broken since #15668. I do not think this solution, reducing a couple of extra parameters from dashboard, can fix the root cause: browser has limits for the url length, and many companies with Nginx also limits the total length of GET request url.

May i know why not use POST request to resolve this problem, as Superset did before?

[suddjian]

The goal with moving from POST to an <a> link was to improve UX by allowing proper browser controls (such as "open in a new tab" vs in-tab navigation). It's usually good UX practice to navigate using links.

I agree that this won't completely solve the long url issue, but it will help in certain cases. I'd really like to understand better what the differences are between the POST solution and this one, to be able to solve your issues. The getExploreLongUrl function is also used by Explore to set the url, so presumably any issues with long urls would also apply there.

I think a workable plan to get rid of the long url issue permanently could be to set up some kind of "exploration context" object in the db that has all this information and can be referenced by id.

[graceguo-supercat]

• this is a quick summary that compares GET vs POST:

https://www.w3schools.com/tags/ref_httpmethods.asp
https://www.guru99.com/difference-get-post-http.html
they all mentioned about the length limit of the URL

• <a> is html tag, it can trigger POST request, just add your own onClick event handler, and check if it is simple click or context menu click. in the click handler you can decide open in the same window or open another window.

• Superset use POST to handle click event in many other places, and they didn't cause any issue. (surprised?) check this menu:

(you can image that chart's query could be very long very complicated right? by GET method there is no way to handle this request)

[rusackas]

• in the click handler you can decide open in the same window or open another window

I think this is the center of the issue. With the click handler, the developer must decide to open in the same window or in a new tab/window. With an old-school href link, the user has the choice of opening in the same window or opening in a new tab. Giving the user the choice was the reason for going that route.

An onClick handler might be able to capture modifier keys to emulate this behavior, but it would not have the same level of support (no right click menu, accessibility concerns).

The longer term idea we're stewing on would be a combination... opening the context menu could do a POST to push all the context of the dashboard (or other superset object, potentially) into a database row, and return a URL with a slug. That short URL could be used in a conventional link to take you to Explore (or other superset area) which would look up all of the state/context needed to load as desired.

Approving this PR since it improves the existing feature. Going back to a simple onClick/POST will just rekindle the usability discussions we've had before. I think if we build a system like I'm (only briefly) attempting to describe above, it will solve the long URL problem AND improve accessibility/usability for this instance, and perhaps several other areas of Superset.