-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Change: make draft status for queries and dashboards toggleable #1353
Conversation
@@ -91,6 +91,9 @@ <h4 class="modal-title">Query Archive</h4> | |||
<div class="col-lg-12 p-5 bg-orange c-white" ng-if="query.is_archived"> | |||
This query is archived and can't be used in dashboards, and won't appear in search results. | |||
</div> | |||
<div class="col-lg-12 p-5 bg-orange c-white" ng-if="query.is_draft"> | |||
This query is a draft. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be nicer to have an icon next to the query or dashboard name rather than the message. Maybe a pencil (fa fa-pencil
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Not sure I understand the html/css enough to do that yet :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can allow edit from maintainers and I will add it.
else: | ||
q = q.where(Query.name != 'New Query') | ||
|
||
q = q.where(Query.is_draft == False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now all the "New Query" queries will appear every where before they didn't... Not sure how I feel about this. We could change all the existing ones to drafts, but then it will be different behavior than the one going forward :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it'd be better to ensure that newly created queries are marked as drafts. I've updated this below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking that new queries would be drafts, and that maybe we could have a migration step that goes through the existing queries and marks as draft any queries where Query.name == 'New Query', so upon upgrade the set of draft queries wouldn't change.
The one thing we haven't addressed yet is the ability for folks to find draft queries through the search interface, since as is I think they won't show up at all unless they're explicitly promoted from draft to public. Did you have ideas re: how you want that to look, @arikfr?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to adding this migration.
As for the other question - basically we want to maintain the balance between "open by default" to have some room for people to work in "private". I can think of a few options:
- Have all queries (draft or not) discoverable via search, but drafts won't appear in "Recent" queries or the main queries list.
- Maintain the current behavior: new query starts as a draft, but once the user gives it a name we promote it to published status. The user can choose to unpublish it.
- Option Visualizations workflow & object #2 + change the query rename UI to have two buttons: "Save & Publish" (default) and "Save". This to reduce friction from the flow where you want to name a draft but not publish it.
Each options has its pros and cons. The main benefit of #2 is that it maintains current behavior & easy to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rafrombrc Any opinions on what our users will prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option 1 is my preference, although option 3 could also work. Option 2 wouldn't be great for our needs. Our issue is that we have a lot of users, and most of the queries that each user generates are only of interest to that particular user. Having them show up in the query lists creates a lot of noise, making it very hard to find the signal that is the queries that are meant to be for public consumption. Also our users have found the "change the name and it's not a draft" behaviour to be confusing... most folks report not really understanding what drafts are and how to use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you decide you want to implement #3, then I prefer we reduce the scope of this pull request to adding the new field and maintain current behavior (i.e. implement #2). And then in a future PR, implement the new search/recents logic. I think the search change might take time, and I want to avoid having a long living pull request hanging around.
And of course, we can implement #3 in this pull request and decide later on if we want to "upgrade" to #1.
6e4f4fd
to
33fd24a
Compare
7ca1200
to
8c13958
Compare
I've implemented option #1 described above: all queries should be accessible in search results, and draft queries/dashboards are excluded from the main and recent listings. I've added a couple tests as well for this. |
|
||
result = dashboard.to_dict() | ||
return result | ||
dashboard = models.Dashboard(name=dashboard_properties['name'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be models.Dashboard.create
. I'm fixing this in the webpack
branch, but FYI just in case you plan to use this branch before.
Also the migration sets all dashboards to be drafts.
Change: make draft status for queries and dashboards toggleable
This puts an
is_draft
flag on queries and dashboards and a menu item for toggling it. This replaces the existing draft behavior for queries.