Refactoring of @christophervalles work on query delete feature:

- Change delete into archive.
- Safely remove widgets.
- Make sure archived queries don't get scheduled, or show up in search.
- If direct link to query used, show notification.
- Tests.
- Some more.

migrations/0001_allow_delete_query.py

@@ -1,5 +1,5 @@
 from playhouse.migrate import Migrator
-from redash import db
+from redash.models import db
 from redash import models
 
 if __name__ == '__main__':

rd_ui/app/index.html

@@ -17,6 +17,7 @@
     <link rel="stylesheet" href="/bower_components/select2/select2.css">
     <link rel="stylesheet" href="/bower_components/angular-ui-select/dist/select.css">
     <link rel="stylesheet" href="/bower_components/pace/themes/pace-theme-minimal.css">
+    <link rel="stylesheet" href="/bower_components/font-awesome/css/font-awesome.css">
     <link rel="stylesheet" href="/styles/redash.css">
     <!-- endbuild -->
 </head>

rd_ui/app/scripts/controllers/query_view.js

@@ -69,7 +69,7 @@
       Events.record(currentUser, 'cancel_execute', 'query', $scope.query.id);
     };
     
-    $scope.deleteQuery = function(options, data) {
+    $scope.archiveQuery = function(options, data) {
       if (data) {
         data.id = $scope.query.id;
       } else {
@@ -79,14 +79,16 @@
       $scope.isDirty = false;
       
       options = _.extend({}, {
-        successMessage: 'Query deleted',
-        errorMessage: 'Query could not be deleted'
+        successMessage: 'Query archived',
+        errorMessage: 'Query could not be archived'
       }, options);
       
       return Query.delete({id: data.id}, function() {
+        $scope.query.is_archived = true;
+        $scope.query.ttl = -1;
         growl.addSuccessMessage(options.successMessage);
-          $('#delete-confirmation-modal').modal('hide');
-          $location.path('/queries');
+          // This feels dirty.
+          $('#archive-confirmation-modal').modal('hide');
         }, function(httpResponse) {
           growl.addErrorMessage(options.errorMessage);
         }).$promise;

rd_ui/app/views/query.html

@@ -1,6 +1,7 @@
 
 <div class="container">
 
+    <p class="alert alert-warning" ng-if="query.is_archived">This query is archived and can't be used in dashboards, and won't appear in search results.</p>
     <alert-unsaved-changes ng-if="canEdit" is-dirty="isDirty"></alert-unsaved-changes>
 
     <div class="row">
@@ -133,25 +134,25 @@
                     <span class="glyphicon glyphicon-cloud-download"></span>
                     <span class="rd-hidden-xs">Download Dataset</span>
                 </a>
-            </p>
-            <p ng-show="query.id != undefined && (isQueryOwner || currentUser.hasPermission('delete_queries'))">
-                <a class="btn btn-danger btn-sm" ng-disabled="queryExecuting" data-toggle="modal" data-target="#delete-confirmation-modal">
-                    <span class="glyphicon glyphicon-remove"></span>
-                    <span class="rd-hidden-xs">Delete query</span>
+
+                <a class="btn btn-warning btn-sm" ng-disabled="queryExecuting" data-toggle="modal" data-target="#archive-confirmation-modal"
+                   ng-show="!query.is_archived && query.id != undefined && (isQueryOwner || currentUser.hasPermission('admin'))">
+                    <i class="fa fa-archive" title="Archive Query"></i>
                 </a>
                 
-                <div class="modal fade" id="delete-confirmation-modal" tabindex="-1" role="dialog" aria-labelledby="deleteConfirmationModal" aria-hidden="true">
+                <div class="modal fade" id="archive-confirmation-modal" tabindex="-1" role="dialog" aria-labelledby="archiveConfirmationModal" aria-hidden="true">
                   <div class="modal-dialog">
                     <div class="modal-content">
                       <div class="modal-header">
-                        <h4 class="modal-title">Query deletion</h4>
+                        <h4 class="modal-title">Query Archive</h4>
                       </div>
                       <div class="modal-body">
-                        Are you sure you want to delete the query?
+                        Are you sure you want to archive this query? <br/>
+                        All dashboard widgets created with its visualizations will be deleted.
                       </div>
                       <div class="modal-footer">
                         <button type="button" class="btn btn-default" data-dismiss="modal">No</button>
-                        <button type="button" class="btn btn-primary" ng-click="deleteQuery()">Yes, delete!</button>
+                        <button type="button" class="btn btn-primary" ng-click="archiveQuery()">Yes, archive.</button>
                       </div>
                     </div>
                   </div>

rd_ui/bower.json

@@ -26,7 +26,8 @@
     "marked": "~0.3.2",
     "bucky": "~0.2.6",
     "pace": "~0.5.1",
-    "angular-ui-select": "0.8.2"
+    "angular-ui-select": "0.8.2",
+    "font-awesome": "~4.2.0"
   },
   "devDependencies": {
     "angular-mocks": "1.2.18",

redash/controllers.py

@@ -271,13 +271,6 @@ class WidgetAPI(BaseResource):
     @require_permission('edit_dashboard')
     def delete(self, widget_id):
         widget = models.Widget.get(models.Widget.id == widget_id)
-        # TODO: reposition existing ones
-        layout = json.loads(widget.dashboard.layout)
-        layout = map(lambda row: filter(lambda w: w != widget_id, row), layout)
-        layout = filter(lambda row: len(row) > 0, layout)
-        widget.dashboard.layout = json.dumps(layout)
-        widget.dashboard.save()
-
         widget.delete_instance()
 
 api.add_resource(WidgetListAPI, '/api/widgets', endpoint='widgets')
@@ -316,9 +309,7 @@ class QueryListAPI(BaseResource):
 
     @require_permission('view_query')
     def get(self):
-        # TODO: encapsulate the is_archived logic in the model
-        return [q.to_dict(with_stats=True) for q in
-                models.Query.all_queries().where(models.Query.is_archived==False)]
+        return [q.to_dict(with_stats=True) for q in models.Query.all_queries()]
 
 
 class QueryAPI(BaseResource):
@@ -345,35 +336,22 @@ class QueryAPI(BaseResource):
     @require_permission('view_query')
     def get(self, query_id):
         q = models.Query.get(models.Query.id == query_id)
-        if q and q.is_archived == False:
+        if q:
             return q.to_dict(with_visualizations=True)
         else:
             abort(404, message="Query not found.")
-    
+
+    # TODO: move to resource of its own? (POST /queries/{id}/archive)
     def delete(self, query_id):
         q = models.Query.get(models.Query.id == query_id)
+
         if q:
-            if q.user.id == self.current_user.id:
-                q.is_archived = True
-                q.save()
-                
-                # Delete widgets using this query
-                vis_ids = [v.id for v in q.visualizations]
-                models.Widget.delete().where(models.Widget.visualization << vis_ids).execute()
+            if q.user.id == self.current_user.id or self.current_user.has_permission('admin'):
+                q.archive()
             else:
                 self.delete_others_query(query_id)
         else:
             abort(404, message="Query not found.")
-    
-    @require_permission('delete_queries')
-    def delete_others_query(self, query_id):
-        q = models.Query.get(models.Query.id == query_id)
-        q.is_archived = True
-        q.save()
-        
-        # Delete widgets using this query
-        vis_ids = [v.id for v in q.visualizations]
-        models.Widget.delete().where(models.Widget.visualization << vis_ids).execute()
 
 api.add_resource(QuerySearchAPI, '/api/queries/search', endpoint='queries_search')
 api.add_resource(QueryRecentAPI, '/api/queries/recent', endpoint='recent_queries')
@@ -410,7 +388,7 @@ class VisualizationAPI(BaseResource):
 
         return vis.to_dict(with_query=False)
 
-    @require_permission('delete_query')
+    @require_permission('edit_query')
     def delete(self, visualization_id):
         vis = models.Visualization.get(models.Visualization.id == visualization_id)
         vis.delete_instance()

redash/models.py

@@ -60,13 +60,26 @@ class BaseModel(peewee.Model):
         return cls.get(cls.id == model_id)
 
 
-class AnonymousUser(AnonymousUserMixin):
+class PermissionsCheckMixin(object):
+    def has_permission(self, permission):
+        return self.has_permissions((permission,))
+
+    def has_permissions(self, permissions):
+        has_permissions = reduce(lambda a, b: a and b,
+                                 map(lambda permission: permission in self.permissions,
+                                     permissions),
+                                 True)
+
+        return has_permissions
+
+
+class AnonymousUser(AnonymousUserMixin, PermissionsCheckMixin):
     @property
     def permissions(self):
         return []
 
 
-class ApiUser(UserMixin):
+class ApiUser(UserMixin, PermissionsCheckMixin):
     def __init__(self, api_key):
         self.id = api_key
 
@@ -77,7 +90,7 @@ class ApiUser(UserMixin):
 
 class Group(BaseModel):
     DEFAULT_PERMISSIONS = ['create_dashboard', 'create_query', 'edit_dashboard', 'edit_query',
-                           'view_query', 'view_source', 'execute_query', 'delete_query']
+                           'view_query', 'view_source', 'execute_query']
 
     id = peewee.PrimaryKeyField()
     name = peewee.CharField(max_length=100)
@@ -101,7 +114,7 @@ class Group(BaseModel):
         return unicode(self.id)
 
 
-class User(BaseModel, UserMixin):
+class User(BaseModel, UserMixin, PermissionsCheckMixin):
     DEFAULT_GROUPS = ['default']
 
     id = peewee.PrimaryKeyField()
@@ -325,11 +338,22 @@ class Query(BaseModel):
 
         return d
 
+    def archive(self):
+        self.is_archived = True
+        self.ttl = -1
+
+        for vis in self.visualizations:
+            for w in vis.widgets:
+                w.delete_instance()
+
+        self.save()
+
     @classmethod
     def all_queries(cls):
         q = Query.select(Query, User, QueryResult.retrieved_at, QueryResult.runtime)\
             .join(QueryResult, join_type=peewee.JOIN_LEFT_OUTER)\
             .switch(Query).join(User)\
+            .where(Query.is_archived==False)\
             .group_by(Query.id, User.id, QueryResult.id, QueryResult.retrieved_at, QueryResult.runtime)\
             .order_by(cls.created_at.desc())
 
@@ -343,6 +367,7 @@ class Query(BaseModel):
             peewee.Func('first_value', cls.id).over(partition_by=[cls.query_hash, cls.data_source])) \
             .join(QueryResult) \
             .where(cls.ttl > 0,
+                   cls.is_archived==False,
                    (QueryResult.retrieved_at +
                     (cls.ttl * peewee.SQL("interval '1 second'"))) <
                    peewee.SQL("(now() at time zone 'utc')"))
@@ -361,6 +386,8 @@ class Query(BaseModel):
         if term.isdigit():
             where |= cls.id == term
 
+        where &= cls.is_archived == False
+
         return cls.select().where(where).order_by(cls.created_at.desc())
 
     @classmethod
@@ -370,7 +397,8 @@ class Query(BaseModel):
             where(Event.action << ('edit', 'execute', 'edit_name', 'edit_description', 'view_source')).\
             where(Event.user == user_id).\
             where(~(Event.object_id >> None)).\
-            where(Event.object_type == 'query').\
+            where(Event.object_type == 'query'). \
+            where(cls.is_archived == False).\
             group_by(Event.object_id, Query.id).\
             order_by(peewee.SQL("count(0) desc"))
 
@@ -551,6 +579,13 @@ class Widget(BaseModel):
     def __unicode__(self):
         return u"%s" % self.id
 
+    def delete_instance(self, *args, **kwargs):
+        layout = json.loads(self.dashboard.layout)
+        layout = map(lambda row: filter(lambda w: w != self.id, row), layout)
+        layout = filter(lambda row: len(row) > 0, layout)
+        self.dashboard.layout = json.dumps(layout)
+        self.dashboard.save()
+        super(Widget, self).delete_instance(*args, **kwargs)
 
 class Event(BaseModel):
     user = peewee.ForeignKeyField(User, related_name="events")
@@ -597,13 +632,8 @@ def create_db(create_tables, drop_tables):
         if drop_tables and model.table_exists():
             # TODO: submit PR to peewee to allow passing cascade option to drop_table.
             db.database.execute_sql('DROP TABLE %s CASCADE' % model._meta.db_table)
-            #model.drop_table()
 
         if create_tables and not model.table_exists():
             model.create_table()
-    
-    Group.insert(name='admin', permissions=['admin'], tables=['*']).execute()
-    Group.insert(name='api', permissions=['view_query'], tables=['*']).execute()
-    Group.insert(name='default', permissions=Group.DEFAULT_PERMISSIONS, tables=['*']).execute()
 
     db.close_db(None)

redash/permissions.py

@@ -10,10 +10,7 @@ class require_permissions(object):
     def __call__(self, fn):
         @functools.wraps(fn)
         def decorated(*args, **kwargs):
-            has_permissions = reduce(lambda a, b: a and b,
-                                      map(lambda permission: permission in current_user.permissions,
-                                          self.permissions),
-                                      True)
+            has_permissions = current_user.has_permissions(self.permissions)
 
             if has_permissions:
                 return fn(*args, **kwargs)

tests/factories.py

@@ -60,6 +60,7 @@ query_factory = ModelFactory(redash.models.Query,
                              query='SELECT 1',
                              ttl=-1,
                              user=user_factory.create,
+                             is_archived=False,
                              data_source=data_source_factory.create)
 
 query_result_factory = ModelFactory(redash.models.QueryResult,

tests/test_models.py

@@ -2,7 +2,7 @@ import datetime
 import json
 from tests import BaseTestCase
 from redash import models
-from factories import dashboard_factory, query_factory, data_source_factory, query_result_factory, user_factory
+from factories import dashboard_factory, query_factory, data_source_factory, query_result_factory, user_factory, widget_factory
 from redash.utils import gen_query_hash
 
 
@@ -65,6 +65,53 @@ class QueryTest(BaseTestCase):
         self.assertNotIn(q2, queries)
 
 
+class QueryArchiveTest(BaseTestCase):
+    def setUp(self):
+        super(QueryArchiveTest, self).setUp()
+
+    def test_archive_query_sets_flag(self):
+        query = query_factory.create(ttl=1)
+        query.archive()
+        query = models.Query.get_by_id(query.id)
+
+        self.assertEquals(query.is_archived, True)
+
+    def test_archived_query_doesnt_return_in_all(self):
+        query = query_factory.create(ttl=1)
+        yesterday = datetime.datetime.now() - datetime.timedelta(days=1)
+        query_result = models.QueryResult.store_result(query.data_source.id, query.query_hash, query.query, "1",
+                                                       123, yesterday)
+
+        query.latest_query_data = query_result
+        query.save()
+
+        self.assertIn(query, models.Query.all_queries())
+        self.assertIn(query, models.Query.outdated_queries())
+
+        query.archive()
+
+        self.assertNotIn(query, models.Query.all_queries())
+        self.assertNotIn(query, models.Query.outdated_queries())
+
+    def test_removes_associated_widgets_from_dashboards(self):
+        widget = widget_factory.create()
+        query = widget.visualization.query
+
+        query.archive()
+
+        self.assertRaises(models.Widget.DoesNotExist, models.Widget.get_by_id, widget.id)
+
+    def test_removes_scheduling(self):
+        query = query_factory.create(ttl=1)
+
+        query.archive()
+
+        query = models.Query.get_by_id(query.id)
+
+        self.assertEqual(-1, query.ttl)
+
+
+
 class QueryResultTest(BaseTestCase):
     def setUp(self):
         super(QueryResultTest, self).setUp()
@@ -237,4 +284,25 @@ class TestEvents(BaseTestCase):
 
         event = models.Event.record(raw_event)
 
-        self.assertDictEqual(json.loads(event.additional_properties), additional_properties)
+        self.assertDictEqual(json.loads(event.additional_properties), additional_properties)
+
+
+class TestWidgetDeleteInstance(BaseTestCase):
+    def test_delete_removes_from_layout(self):
+        widget = widget_factory.create()
+        widget2 = widget_factory.create(dashboard=widget.dashboard)
+        widget.dashboard.layout = json.dumps([[widget.id, widget2.id]])
+        widget.dashboard.save()
+        widget.delete_instance()
+
+        self.assertEquals(json.dumps([[widget2.id]]), widget.dashboard.layout)
+
+    def test_delete_removes_empty_rows(self):
+        widget = widget_factory.create()
+        widget2 = widget_factory.create(dashboard=widget.dashboard)
+        widget.dashboard.layout = json.dumps([[widget.id, widget2.id]])
+        widget.dashboard.save()
+        widget.delete_instance()
+        widget2.delete_instance()
+
+        self.assertEquals("[]", widget.dashboard.layout)