-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
from playhouse.migrate import PostgresqlMigrator, migrate | ||
|
||
from redash.models import db | ||
from redash import models | ||
|
||
if __name__ == '__main__': | ||
db.connect_db() | ||
migrator = PostgresqlMigrator(db.database) | ||
|
||
with db.database.transaction(): | ||
migrate( | ||
migrator.add_column('queries', 'is_draft', models.Query.is_draft) | ||
) | ||
migrate( | ||
migrator.add_column('dashboards', 'is_draft', models.Query.is_draft) | ||
) | ||
db.database.execute_sql("UPDATE queries SET is_draft = (name = 'New Query')") | ||
db.close_db(None) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,13 +31,12 @@ def get(self): | |
@require_permission('create_dashboard') | ||
def post(self): | ||
dashboard_properties = request.get_json(force=True) | ||
dashboard = models.Dashboard.create(name=dashboard_properties['name'], | ||
org=self.current_org, | ||
user=self.current_user, | ||
layout='[]') | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. This should be Also the migration sets all dashboards to be drafts. |
||
org=self.current_org, | ||
user=self.current_user, | ||
is_draft=True, | ||
layout='[]') | ||
return dashboard.to_dict() | ||
|
||
|
||
class DashboardResource(BaseResource): | ||
|
@@ -63,7 +62,8 @@ def post(self, dashboard_slug): | |
|
||
require_object_modify_permission(dashboard, self.current_user) | ||
|
||
updates = project(dashboard_properties, ('name', 'layout', 'version')) | ||
updates = project(dashboard_properties, ('name', 'layout', 'version', | ||
'is_draft')) | ||
updates['changed_by'] = self.current_user | ||
|
||
try: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -702,6 +702,7 @@ class Query(ChangeTrackingMixin, ModelTimestampsMixin, BaseVersionedModel, Belon | |
user = peewee.ForeignKeyField(User) | ||
last_modified_by = peewee.ForeignKeyField(User, null=True, related_name="modified_queries") | ||
is_archived = peewee.BooleanField(default=False, index=True) | ||
is_draft = peewee.BooleanField(default=True, index=True) | ||
schedule = peewee.CharField(max_length=10, null=True) | ||
options = JSONField(default={}) | ||
|
||
|
@@ -719,6 +720,7 @@ def to_dict(self, with_stats=False, with_visualizations=False, with_user=True, w | |
'schedule': self.schedule, | ||
'api_key': self.api_key, | ||
'is_archived': self.is_archived, | ||
'is_draft': self.is_draft, | ||
'updated_at': self.updated_at, | ||
'created_at': self.created_at, | ||
'data_source_id': self.data_source_id, | ||
|
@@ -771,10 +773,9 @@ def all_queries(cls, groups, drafts=False): | |
.order_by(cls.created_at.desc()) | ||
|
||
if drafts: | ||
q = q.where(Query.name == 'New Query') | ||
q = q.where(Query.is_draft == True) | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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:
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 commentThe 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 commentThe 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 commentThe 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. |
||
return q | ||
|
||
@classmethod | ||
|
@@ -818,17 +819,22 @@ def search(cls, term, groups): | |
|
||
@classmethod | ||
def recent(cls, groups, user_id=None, limit=20): | ||
query = cls.select(Query, User).where(Event.created_at > peewee.SQL("current_date - 7")).\ | ||
join(Event, on=(Query.id == Event.object_id.cast('integer'))). \ | ||
join(DataSourceGroup, on=(Query.data_source==DataSourceGroup.data_source)). \ | ||
switch(Query).join(User).\ | ||
where(Event.action << ('edit', 'execute', 'edit_name', 'edit_description', 'view_source')).\ | ||
where(~(Event.object_id >> None)).\ | ||
where(Event.object_type == 'query'). \ | ||
where(DataSourceGroup.group << groups).\ | ||
where(cls.is_archived == False).\ | ||
group_by(Event.object_id, Query.id, User.id).\ | ||
order_by(peewee.SQL("count(0) desc")) | ||
query = ( | ||
cls.select(Query, User) | ||
.where(Event.created_at > peewee.SQL("current_date - 7")) | ||
.join(Event, on=(Query.id == Event.object_id.cast('integer'))) | ||
.join(DataSourceGroup, on=(Query.data_source==DataSourceGroup.data_source)) | ||
.switch(Query).join(User) | ||
.where(Event.action << ('edit', 'execute', 'edit_name', | ||
'edit_description', 'toggle_published', | ||
'view_source')) | ||
.where(~(Event.object_id >> None)) | ||
.where(Event.object_type == 'query') | ||
.where(DataSourceGroup.group << groups) | ||
.where(cls.is_archived == False) | ||
.where(cls.is_draft == False) | ||
.group_by(Event.object_id, Query.id, User.id) | ||
.order_by(peewee.SQL("count(0) desc"))) | ||
|
||
if user_id: | ||
query = query.where(Event.user == user_id) | ||
|
@@ -1077,6 +1083,7 @@ class Dashboard(ChangeTrackingMixin, ModelTimestampsMixin, BaseVersionedModel, B | |
layout = peewee.TextField() | ||
dashboard_filters_enabled = peewee.BooleanField(default=False) | ||
is_archived = peewee.BooleanField(default=False, index=True) | ||
is_draft = peewee.BooleanField(default=False, index=True) | ||
|
||
class Meta: | ||
db_table = 'dashboards' | ||
|
@@ -1129,24 +1136,29 @@ def to_dict(self, with_widgets=False, user=None): | |
'dashboard_filters_enabled': self.dashboard_filters_enabled, | ||
'widgets': widgets_layout, | ||
'is_archived': self.is_archived, | ||
'is_draft': self.is_draft, | ||
'updated_at': self.updated_at, | ||
'created_at': self.created_at, | ||
'version': self.version | ||
} | ||
|
||
@classmethod | ||
def all(cls, org, groups, user_id): | ||
query = cls.select().\ | ||
join(Widget, peewee.JOIN_LEFT_OUTER, on=(Dashboard.id == Widget.dashboard)). \ | ||
join(Visualization, peewee.JOIN_LEFT_OUTER, on=(Widget.visualization == Visualization.id)). \ | ||
join(Query, peewee.JOIN_LEFT_OUTER, on=(Visualization.query == Query.id)). \ | ||
join(DataSourceGroup, peewee.JOIN_LEFT_OUTER, on=(Query.data_source == DataSourceGroup.data_source)). \ | ||
where(Dashboard.is_archived == False). \ | ||
where((DataSourceGroup.group << groups) | | ||
(Dashboard.user == user_id) | | ||
(~(Widget.dashboard >> None) & (Widget.visualization >> None))). \ | ||
where(Dashboard.org == org). \ | ||
group_by(Dashboard.id) | ||
query = (cls.select() | ||
.join(Widget, peewee.JOIN_LEFT_OUTER, | ||
on=(Dashboard.id == Widget.dashboard)) | ||
.join(Visualization, peewee.JOIN_LEFT_OUTER, | ||
on=(Widget.visualization == Visualization.id)) | ||
.join(Query, peewee.JOIN_LEFT_OUTER, | ||
on=(Visualization.query == Query.id)) | ||
.join(DataSourceGroup, peewee.JOIN_LEFT_OUTER, | ||
on=(Query.data_source == DataSourceGroup.data_source)) | ||
.where(Dashboard.is_archived == False) | ||
.where((DataSourceGroup.group << groups & (Dashboard.is_draft != True)) | | ||
(Dashboard.user == user_id) | | ||
(~(Widget.dashboard >> None) & (Widget.visualization >> None))) | ||
.where(Dashboard.org == org) | ||
.group_by(Dashboard.id)) | ||
|
||
return query | ||
|
||
|
@@ -1162,6 +1174,7 @@ def recent(cls, org, groups, user_id, for_user=False, limit=20): | |
where(~(Event.object_id >> None)). \ | ||
where(Event.object_type == 'dashboard'). \ | ||
where(Dashboard.is_archived == False). \ | ||
where(Dashboard.is_draft == False). \ | ||
where(Dashboard.org == org). \ | ||
where((DataSourceGroup.group << groups) | | ||
(Dashboard.user == user_id) | | ||
|
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.