Re-add dashboard short links #5398

jaylindquist · 2018-07-16T14:22:56Z

This PR re-adds the changes from #4760 to include a short link button for dashboards that was accidentally removed in #4528.

Additionally:

The button is now available to all users, not just those with save permissions
The shortened link will include all active dashboard filters so users can share filtered dashboards with each other

* Make the short link available to all users * Include filters in the short link for dashboards

…link

codecov-io · 2018-07-17T12:05:44Z

Codecov Report

Merging #5398 into master will decrease coverage by 0.09%.
The diff coverage is 89.47%.

@@            Coverage Diff            @@
##           master    #5398     +/-   ##
=========================================
- Coverage   63.28%   63.18%   -0.1%     
=========================================
  Files         349      351      +2     
  Lines       22121    22204     +83     
  Branches     2457     2466      +9     
=========================================
+ Hits        13999    14030     +31     
- Misses       8108     8159     +51     
- Partials       14       15      +1

Impacted Files	Coverage Δ
...erset/assets/src/dashboard/util/getDashboardUrl.js	`100% <100%> (ø)`
...erset/assets/src/components/URLShortLinkButton.jsx	`92.3% <100%> (+0.3%)`	⬆️
...uperset/assets/src/dashboard/components/Header.jsx	`63.95% <81.25%> (+4.65%)`	⬆️
...src/dashboard/components/HeaderActionsDropdown.jsx	`81.63% <88.88%> (-0.19%)`	⬇️
...perset/assets/src/components/URLShortLinkModal.jsx	`92.85% <92.85%> (ø)`
superset/assets/src/modules/visUtils.js	`58.33% <0%> (-16.67%)`	⬇️
superset/connectors/sqla/views.py	`64.34% <0%> (-6.13%)`	⬇️
superset/assets/src/explore/store.js	`21.95% <0%> (-1.13%)`	⬇️
...rc/explore/components/controls/AnnotationLayer.jsx	`22.66% <0%> (-0.7%)`	⬇️
...uperset/assets/src/SqlLab/components/ResultSet.jsx	`84.21% <0%> (-0.64%)`	⬇️
... and 21 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 3b6cafc...0c7c33a. Read the comment docs.

mistercrunch · 2018-07-19T01:18:05Z

🏆 !
Can you share a screenshot of what it looks like?

…link

jaylindquist · 2018-07-19T03:22:15Z

It's the same as the change by @hughhhh in #4760

When a user does not have the ability to save the dashboard:

williaster · 2018-07-20T16:13:42Z

I'm on PTO and will take a look at this next week when I'm back. Re the UI We'd like all links in the drop-down to simplify the top level UI. We've also gotten rid of use of icons as they're often cryptic. Can you match the aesthetics of the Current drop-down options?

williaster

This looks good overall, but I'd request the following tweaks:

move the link button into the dropdown
remove the icon and instead use text inside the dropdown "Share dashboard" (or something like this)
when a user doesn't have permissions to save, "Edit" is disabled but you can still click the dropdown to see the "Share dashboard" link

You may also need to rebase this if @graceguo-supercat 's PR to remove the v1<>v2 switch gets in first. (#5418).

…link

jaylindquist · 2018-07-31T16:45:33Z

Sorry for the late response on this PR:

The short link is now a menu item with a modal instead of a button with a pop-over to match other options (e.g. refresh interval)
Read-only users now have a menu with a disabled "Edit dashboard" button with the following options:
- Force refresh
- Auto refresh
- Share dashboard
The "Email dashboard link" menu item is now replaced with "Share dashboard"

Read-only view:

Edit mode for users who can save dashboards:

New modal:

williaster

This looks great, thanks for making the changes! I had a couple nit comments but I think we could also just merge it.

williaster · 2018-08-03T07:27:11Z

superset/assets/spec/javascripts/dashboard/components/HeaderActionsDropdown_spec.jsx

+    return wrapper;
+  }
+
+  it('read-only', () => {


nit -- I would make each of these (read only, write, write + edit) nested describe blocks and then have on it() per assertion because it's easier for debugging. I'm really psyched that you wrote tests for this tho so not a blocker.

williaster · 2018-08-03T07:28:15Z

superset/assets/spec/javascripts/dashboard/util/getDashboardUrl_spec.js

+
+import getDashboardUrl from '../../../../src/dashboard/util/getDashboardUrl';
+
+describe('getChartIdsFromLayout', () => {


williaster · 2018-08-03T07:29:59Z

superset/assets/src/components/URLShortLinkModal.jsx

+        ref={this.setModalRef}
+        isMenuItem={this.props.isMenuItem}
+        triggerNode={this.props.triggerNode}
+        beforeOpen={this.getCopyUrl.bind(this)}


I would bind this in the constructor, otherwise it's a new function per render. not a huge deal since this isn't rendered a lot, but we'll have a lint rule soon to enforce.

williaster · 2018-08-03T17:26:41Z

thanks @jaylindquist !

* Re-add dashboard short links * Make the short link available to all users * Include filters in the short link for dashboards * Remove duplicate key causing linter error * Change URL Short link button into a menu item with Modal * Split out tests, bind URL short link in constructor

Re-add dashboard short links

eff0fe6

* Make the short link available to all users * Include filters in the short link for dashboards

mistercrunch requested a review from williaster July 16, 2018 15:29

jaylindquist added 3 commits July 16, 2018 11:01

Merge remote-tracking branch 'origin/master' into jay/dashboard-short…

22041d0

…link

Remove duplicate key causing linter error

473778f

Merge remote-tracking branch 'origin/master' into jay/dashboard-short…

3c1a57c

…link

Merge remote-tracking branch 'origin/master' into jay/dashboard-short…

4013def

…link

williaster requested changes Jul 23, 2018

View reviewed changes

jaylindquist added 2 commits July 30, 2018 07:41

Merge remote-tracking branch 'origin/master' into jay/dashboard-short…

5e3ac2a

…link

Change URL Short link button into a menu item with Modal

c3bce1b

williaster approved these changes Aug 3, 2018

View reviewed changes

Split out tests, bind URL short link in constructor

0c7c33a

mistercrunch merged commit aa9b30c into apache:master Aug 3, 2018

jaylindquist deleted the jay/dashboard-shortlink branch August 6, 2018 12:46

jaylindquist mentioned this pull request Aug 17, 2018

Hide 'Edit Dashboard' Button on Dashboard View #5660

Closed

3 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-add dashboard short links #5398

Re-add dashboard short links #5398

jaylindquist commented Jul 16, 2018

codecov-io commented Jul 17, 2018 •

edited

Loading

mistercrunch commented Jul 19, 2018

jaylindquist commented Jul 19, 2018 •

edited

Loading

williaster commented Jul 20, 2018 via email •

edited

Loading

williaster left a comment

jaylindquist commented Jul 31, 2018

williaster left a comment

williaster Aug 3, 2018

williaster Aug 3, 2018

williaster Aug 3, 2018

williaster commented Aug 3, 2018


		import getDashboardUrl from '../../../../src/dashboard/util/getDashboardUrl';

		describe('getChartIdsFromLayout', () => {

Re-add dashboard short links #5398

Re-add dashboard short links #5398

Conversation

jaylindquist commented Jul 16, 2018

codecov-io commented Jul 17, 2018 • edited Loading

Codecov Report

mistercrunch commented Jul 19, 2018

jaylindquist commented Jul 19, 2018 • edited Loading

williaster commented Jul 20, 2018 via email • edited Loading

williaster left a comment

Choose a reason for hiding this comment

jaylindquist commented Jul 31, 2018

williaster left a comment

Choose a reason for hiding this comment

williaster Aug 3, 2018

Choose a reason for hiding this comment

williaster Aug 3, 2018

Choose a reason for hiding this comment

williaster Aug 3, 2018

Choose a reason for hiding this comment

williaster commented Aug 3, 2018

codecov-io commented Jul 17, 2018 •

edited

Loading

jaylindquist commented Jul 19, 2018 •

edited

Loading

williaster commented Jul 20, 2018 via email •

edited

Loading