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

feat: save active tabs in dashboard permalink #19983

Merged
merged 2 commits into from
Jun 29, 2022

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented May 7, 2022

SUMMARY

Save which tabs are selected in permalink state and restore the tab selection so we can restore the same tab selection when users open a dashboard using permalink.

Also renamed properties in the dashboard permalink state to make them easier to understand.

This is in preparation for #19354 where we allow users to set tab selection when generating email reports for dashboards. The next step is to generate dashboard screenshots from permalink as well and attach the permalink to the email reports (we will generate a new permalink in the backend based on reports metadata). The next next step is to allow saving filter states in dashboard reports, too.

Depends on #19966

This PR includes a db migration where we renamed a couple of properties in permalink states. The migration should be fairly low risk since we are only updating permalinks for dashboards. At Airbnb, after running Superset with the new permalink feature for 3 weeks, there are only about 1400 links generated.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

  1. Go to a tabbed dashboard with multiple tabbed sections
  2. Switch more than one tab sections and copy a permalink for some tab
  3. When opening the permalink, the previous tab selection should be retained

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

@ktmud ktmud requested a review from a team as a code owner May 7, 2022 00:40
@codecov
Copy link

codecov bot commented May 7, 2022

Codecov Report

Merging #19983 (f4cae95) into master (f29cde2) will decrease coverage by 0.00%.
The diff coverage is 56.31%.

❗ Current head f4cae95 differs from pull request most recent head a3076c1. Consider uploading reports for the commit a3076c1 to get more accurate results

@@            Coverage Diff             @@
##           master   #19983      +/-   ##
==========================================
- Coverage   66.85%   66.85%   -0.01%     
==========================================
  Files        1749     1750       +1     
  Lines       65411    65424      +13     
  Branches     6906     6906              
==========================================
+ Hits        43730    43737       +7     
- Misses      19931    19937       +6     
  Partials     1750     1750              
Flag Coverage Δ
hive 53.77% <41.93%> (-0.01%) ⬇️
mysql 82.39% <54.83%> (-0.02%) ⬇️
postgres 82.46% <54.83%> (-0.02%) ⬇️
presto 53.63% <41.93%> (-0.01%) ⬇️
python 82.89% <54.83%> (-0.02%) ⬇️
sqlite 82.25% <54.83%> (-0.02%) ⬇️
unit 50.66% <54.83%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...d/packages/superset-ui-chart-controls/src/types.ts 100.00% <ø> (ø)
.../superset-ui-core/src/query/types/QueryFormData.ts 100.00% <ø> (ø)
.../legacy-plugin-chart-world-map/src/controlPanel.ts 25.00% <ø> (ø)
...s/legacy-preset-chart-nvd3/src/Bar/controlPanel.ts 14.28% <ø> (ø)
...gacy-preset-chart-nvd3/src/DistBar/controlPanel.ts 8.33% <0.00%> (ø)
.../legacy-preset-chart-nvd3/src/Line/controlPanel.ts 50.00% <ø> (ø)
...harts/src/BigNumber/BigNumberTotal/controlPanel.ts 100.00% <ø> (ø)
.../BigNumber/BigNumberWithTrendline/controlPanel.tsx 20.00% <ø> (ø)
...s/plugin-chart-echarts/src/BoxPlot/controlPanel.ts 25.00% <0.00%> (ø)
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx 57.14% <ø> (ø)
... and 79 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 f29cde2...a3076c1. Read the comment docs.

const canEdit = canUserEditDashboard(dashboardData, user);
const canEdit = canUserEditDashboard(dashboard, user);

console.log(activeTabs);
Copy link
Member

Choose a reason for hiding this comment

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

left over console log

Copy link
Member Author

@ktmud ktmud May 10, 2022

Choose a reason for hiding this comment

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

Thanks for catching this! I'll keep iterating on this PR until #19966 is merged.

@ktmud ktmud force-pushed the dashboard-active-tabs-in-url branch from 7b670a3 to e50a8eb Compare June 27, 2022 22:00
@ktmud ktmud force-pushed the dashboard-active-tabs-in-url branch from 8019609 to d31f0a8 Compare June 27, 2022 22:07
@ktmud
Copy link
Member Author

ktmud commented Jun 27, 2022

@eschutho This should be ready for re-review.

@villebro @michael-s-molina in case you have an interest in this as well.

onClick={e => {
e.stopPropagation();
}}
>
Copy link
Member Author

Choose a reason for hiding this comment

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

Stop clicking on the copy icon from switching tabs, which is kind of surprising/annoying----if clicking on the link icon doesn't switch tabs, then clicking on things in the popover triggered by the link icon shouldn't switch tabs either.

Before

tab-copy-click-before

After

tab-copy-click-after

@ktmud ktmud force-pushed the dashboard-active-tabs-in-url branch from d31f0a8 to 61092e5 Compare June 28, 2022 00:07
@ktmud ktmud force-pushed the dashboard-active-tabs-in-url branch from 61092e5 to dbe28d1 Compare June 28, 2022 00:18
Copy link
Member

@etr2460 etr2460 left a comment

Choose a reason for hiding this comment

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

a few comments, but nothing blocking i think

@@ -66,7 +75,14 @@ export default function URLShortLinkButton({
trigger="click"
placement={placement}
content={
<div id="shorturl-popover" data-test="shorturl-popover">
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
Copy link
Member

Choose a reason for hiding this comment

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

hmm, shouldn't the stop propagation go on the copy to clipboard link instead? It's weird that we need to handle a click on a div

Copy link
Member Author

Choose a reason for hiding this comment

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

The click event also triggers popover open. Since this is related to the popover, I thought it'd make more sense to handle the event on the direct children of the popover trigger. If we stop propagation on the icon, the popover may not even open.

Comment on lines -139 to -144
.catch(response =>
// @ts-ignore
getClientErrorObject(response).then(({ error, statusText }) =>
Promise.reject(error || statusText),
),
);
Copy link
Member

Choose a reason for hiding this comment

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

we'll never get an error here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Errors should be handled downstream---when it's about to be rendered. These adhoc handling of API errors makes it more difficult to handle errors consistently.

@@ -21,14 +21,16 @@
from superset import security_manager
from superset.models.helpers import AuditMixinNullable, ImportExportMixin

VALUE_MAX_SIZE = 2**24 - 1
Copy link
Member

Choose a reason for hiding this comment

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

heh, nice bycatch

from superset.migrations.shared.utils import paginated_update

Base = declarative_base()
VALUE_MAX_SIZE = 2**24 - 1
Copy link
Member

Choose a reason for hiding this comment

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

can we import this from the model file instead of defining it twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't import things from modules for migrations as modules may be move around and these constant values may change but migrations need to be deterministic.

@@ -2001,13 +2001,13 @@ def dashboard_permalink( # pylint: disable=no-self-use
return redirect("/dashboard/list/")
if not value:
return json_error_response(_("permalink state not found"), status=404)
dashboard_id = value["dashboardId"]
dashboard_id, state = value["dashboardId"], value.get("state", {})
Copy link
Member

Choose a reason for hiding this comment

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

is this standard styling? i would just use 2 lines tbh

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, just my style choice. Since they are both derived from the same variable. It's more concise to be written in one line. This is similar to object expansion in JS.

@ktmud ktmud merged commit cadd259 into apache:master Jun 29, 2022
@ktmud ktmud deleted the dashboard-active-tabs-in-url branch July 11, 2022 22:36
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
john-bodley pushed a commit to airbnb/superset-fork that referenced this pull request Sep 8, 2022
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 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 size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants