Add "Published" feature to dashboards #4725
Conversation
|
This is neat. A few thoughts:
|
|
Thanks for such useful and quick feedback on this, A couple thoughts and things to do yet
Things to do:
Thanks again for your feedback! |
Codecov Report
@@ Coverage Diff @@
## master #4725 +/- ##
=========================================
- Coverage 63.51% 63.5% -0.02%
=========================================
Files 360 361 +1
Lines 22890 22955 +65
Branches 2549 2551 +2
=========================================
+ Hits 14539 14577 +38
- Misses 8336 8363 +27
Partials 15 15
Continue to review full report at Codecov.
|
|
Hey guys, Any update on this merge? Would love to be able to roll this feature out. Thanks. |
|
I completely agree "Releasing the feature: Perhaps it'd be fine to just have a script that sets all dashboards to published since this wouldn't affect the system at all and then people would just be able to modify it from then on? " |
|
Hey @daniel5001 , This feature is still in the works and hasn't been forgotten, I'm currently working on ironing out a few bugs I've found as a result of adding an icon to toggle the status in the UI (and just trying to make it pleasant since it would show up on every dashboard) but other than that it's becoming pretty close to being finished! |
|
a quick visual tour of the things that this PR changes: Initial list of dashboards
After John publishes his secret dashboard
Toggling the published status of a dashboard you own
Trying to toggle the status of a dashboard you do not own
|
Tresdon
changed the title
|
@mistercrunch This pull request should be finished now, let me know what you think. One thing I didn't mention was that this doesn't get rid of the |
|
LGTM, I checked functionality not code |
|
Looks awesome @Tresdon can't thank you enough for putting this together. It's a real lifesaver when utilizing this tool in a large enterprise. |
|
@mistercrunch anything left to be done here? Looking forward to this feature. Thanks! |
|
|
|
@mistercrunch I've updated this feature to work with the dashboard improvements, anything else you'd like to see here? |
|
@Tresdon can you fix the dashboard tests issue? |
|
Some thoughts on the product experience: My understanding is that unpublished dashboards are viewable, but less discoverable. Specifically, the only differences between published vs unpublished dashboard are (a) When viewing the dashboard, the eye icon has a strikethrough or not and (b) whether it shows up in the list of dashboards (on the /welcome page, not the crud UI). My feedback is based on this understanding.
|
|
Hey there @sylvia-tomiyama, Thanks for taking a look at this and giving the user's experience some thought. I agree that some of the terminology surrounding the feature needs to be defined more clearly since there's some ambiguity around the term "published". You hit the nail on the head when you said that My understanding is that unpublished dashboards are viewable, but less discoverable – which is further bolstered by your distinction of prod vs in-progress. I think the comment about migration (which mentioned that dashboards would all of the sudden be gone) was referring to the visibility of them within these list views but they would not actually be gone or become truly unviewable. With that in mind I've considered some of the excellent points you've made and here's what I think: Welcome List & CRUD View List for DashboardsYou mentioned as a part of your understanding that the visibility of the dashboard will change on the Right now I say that only admins should be able to see all dashboards because I believe this view is used more frequently for finding dashboards than Displaying and Altering the Published StatusI think you make a great point about the eye icon being a bit misleading and a bit too subtle. I think a better solution might be something like a [draft] badge next to the title of the dashboard (possibly with a hover-over tooltip) which indicates a few things:
This indicator seems like it would make the intention of the feature much more understandable. As for altering the status, I think you're totally right that it makes more sense to put this in the actions dropdown especially if we forgo the icon which was both the indicator and the toggle. The Way ForwardHere's a couple of action items which I think are appropriate after this discussion for me to work on:
Let me know what you think and thanks again for your consideration :) |
- The eye next to the title has been replaced with a [draft] badge - Published status is now toggled in the Header Action Dropdown - CRUD list shows published status
Codecov Report
@@ Coverage Diff @@
## master #4725 +/- ##
==========================================
- Coverage 63.72% 63.71% -0.01%
==========================================
Files 386 387 +1
Lines 23532 23601 +69
Branches 2621 2627 +6
==========================================
+ Hits 14996 15038 +42
- Misses 8523 8550 +27
Partials 13 13
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #4725 +/- ##
==========================================
+ Coverage 65.73% 65.79% +0.06%
==========================================
Files 459 460 +1
Lines 21986 22046 +60
Branches 2415 2419 +4
==========================================
+ Hits 14452 14506 +54
- Misses 7413 7418 +5
- Partials 121 122 +1
Continue to review full report at Codecov.
|
Add some tests to make sure the published status is rendered and Make it so that users cannot see dashboards that are published if they don't have access to any of the slices within
Some tests are failing in travis CI and not locally so I would like to see why
|
I would like to take over getting this story merged since @Tresdon got pulled into other things. |
|
If you have write access to @Tresdon 's fork you could take over it. Otherwise you'll have to open a new PR off of your fork... |
|
Thanks for the assistance @1AB9502! |
|
I found some time to get this green again – I've also set up @1AB9502 with access to my fork so we can tag team the issue in the future. One thing I'm wondering about is the process for releasing this feature – since it requires a db upgrade it could be that we want to set the |
There was a problem hiding this comment.
@Tresdon just one thing, let's set the database migration to set all existing dashboards to published = True
We could also set a feature flag in front of the feature if people feel strongly about NOT enabling this feature in their environment, but I doubt this would be the case.
|
Hey sorry about the new conflicts, but they should be easy to solve, we just started using |
|
Zero worries, I think the formatting as well as the migration initialization are both figured out – but waiting to hear if travis has anything to say about linting. I quite like the feature flag system for the project and am willing to implement it – but I also don't foresee many folks finding that it's a feature they can't live with. |
|
@mistercrunch Any idea how to re-run travis without changing files? The error it got seems to be a |
…into publish_dashboards
|
@mistercrunch any thoughts on the changes or does it all look good to you? |
mistercrunch
added
🏷️ bot
This feature allows users to “publish” dashboards which essentially adds a notion of dashboards being public or private. The goal of this is to reduce the clutter that comes from a whole organization creating dashboards, many of which are not relevant to other users (or only a subset, whom they can share with selectively). The changes affect the following contexts:
On creating a dashboard:
- Add a boolean published column to the dashboard model and allow the owners of a given dashboard to toggle this field in the UI.
On listing dashboards:
- Don’t show all dashboards but instead only list dashboards with the following properties
1. Dashboards the current user owns
2. Dashboards that have been published (by any user)
3. Dashboards which the user has favorited
Regarding point (3) above – The idea here was that no dashboard is strictly “private” in that it cannot be viewed if it is not published. This means that if a user creates an unpublished dashboard, they should still be able to send the link to others to share it with them. However, since the dashboard is unpublished the visitor wouldn’t see it in the list so they would need to keep the link around which isn’t a very friendly user experience. So to get around this we allow the user to favorite the dashboard and then it will show up in their list of dashboards.
issues which might be relevant to this: #1799 #2792 #4002
Tasks: