add optimistic locking for dashboard editing

rd_ui/app/scripts/controllers/dashboard.js

@@ -9,7 +9,7 @@
     });
   };
 
-  var DashboardCtrl = function($scope, Events, Widget, $routeParams, $location, $http, $timeout, $q, $modal, Dashboard, User) {
+  var DashboardCtrl = function($scope, Events, Widget, $routeParams, $location, $http, $timeout, $q, $modal, Dashboard) {
     $scope.refreshEnabled = false;
     $scope.isFullscreen = false;
     $scope.refreshRate = 60;
@@ -271,7 +271,7 @@
   };
 
   angular.module('redash.controllers')
-    .controller('DashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', '$modal', 'Dashboard', 'User', DashboardCtrl])
+    .controller('DashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', '$modal', 'Dashboard', DashboardCtrl])
     .controller('PublicDashboardCtrl', ['$scope', 'Events', 'Widget', '$routeParams', '$location', '$http', '$timeout', '$q', 'Dashboard', PublicDashboardCtrl])
     .controller('WidgetCtrl', ['$scope', '$location', 'Events', 'Query', '$modal', WidgetCtrl])
 

rd_ui/app/scripts/directives/dashboard_directives.js

@@ -3,8 +3,8 @@
 
   var directives = angular.module('redash.directives');
 
-  directives.directive('editDashboardForm', ['Events', '$http', '$location', '$timeout', 'Dashboard',
+  directives.directive('editDashboardForm', ['Events', '$http', '$location', '$timeout', 'Dashboard', 'growl',
-    function(Events, $http, $location, $timeout, Dashboard) {
+    function(Events, $http, $location, $timeout, Dashboard, growl) {
       return {
         restrict: 'E',
         scope: {
@@ -81,10 +81,19 @@
               $scope.dashboard.layout = layout;
 
               layout = JSON.stringify(layout);
-              Dashboard.save({slug: $scope.dashboard.id, name: $scope.dashboard.name, layout: layout}, function(dashboard) {
+              Dashboard.save({slug: $scope.dashboard.id, name: $scope.dashboard.name,
+                  latest_version: $scope.dashboard.latest_version, layout: layout}, function(dashboard) {
                 $scope.dashboard = dashboard;
                 $scope.saveInProgress = false;
                 $(element).modal('hide');
+              }, function(error) {
+                $scope.saveInProgress = false;
+                if(error.status == 403) {
+                  growl.addErrorMessage("Unable to save dashboard: Permission denied.");
+                } else if(error.status == 409) {
+                  growl.addErrorMessage('It seems like the dashboard has been modified by another user. ' +
+                      'Please copy/backup your changes and reload this page.', {ttl: -1});
+                }
               });
               Events.record(currentUser, 'edit', 'dashboard', $scope.dashboard.id);
             } else {

redash/handlers/access.py

@@ -38,7 +38,7 @@ class AccessGrantResource(BaseResource):
                 .where(AccessPermission.access_type == access_type)
 
         if permissions.count() > 0:
-            return
+            return {'result': 'already_granted'}
 
         perm = AccessPermission()
         perm.object_type = object_type
@@ -47,6 +47,7 @@ class AccessGrantResource(BaseResource):
         perm.grantor = self.current_user
         perm.grantee = grantee
         perm.save()
+        return {'result': 'permission_added'}
 
 class AccessRevokeResource(BaseResource):
 
@@ -73,3 +74,4 @@ class AccessAttemptResource(BaseResource):
         if access:
             return {'result': 'access_granted'}
         abort(403)
+        return False

redash/handlers/dashboards.py

@@ -1,4 +1,7 @@
+import logging
+
 from flask import request, url_for
+from flask_restful import abort
 
 from funcy import distinct, take
 from itertools import chain
@@ -8,6 +11,19 @@ from redash.permissions import require_permission, require_admin_or_owner
 from redash.handlers.base import BaseResource, get_object_or_404
 
 
+def _save_change(user, dashboard_id, old_dashboard, new_dashboard, change_type):
+    change = models.Change()
+    change.object_id = dashboard_id
+    change.object_type = models.Dashboard.__name__
+    change.change_type = change_type
+    change.user = user
+    change.change = {
+        "before": old_dashboard,
+        "after": new_dashboard
+    }
+    change.save()
+    return change
+
 class RecentDashboardsResource(BaseResource):
     @require_permission('list_dashboards')
     def get(self):
@@ -25,6 +41,11 @@ class DashboardListResource(BaseResource):
     def get(self):
         dashboards = [d.to_dict() for d in models.Dashboard.all(self.current_org, self.current_user.groups, self.current_user)]
 
+        for dashboard in dashboards:
+            last_change = models.Change.get_latest(object_id=dashboard['id'], object_type=models.Dashboard.__name__)
+            if last_change:
+                dashboard['latest_version'] = last_change.id
+
         return dashboards
 
     @require_permission('create_dashboard')
@@ -35,7 +56,14 @@ class DashboardListResource(BaseResource):
                                      user=self.current_user,
                                      layout='[]')
         dashboard.save()
-        return dashboard.to_dict()
+
+        # create a new Changes record to keep track of the changes
+        new_dashboard = {'name': dashboard.name, 'layout': dashboard.layout}
+        new_change = _save_change(self.current_user, dashboard.id, None, new_dashboard, change_type=models.Change.TYPE_CREATE)
+
+        result = dashboard.to_dict()
+        result['latest_version'] = new_change.id
+        return result
 
 
 class DashboardResource(BaseResource):
@@ -49,6 +77,10 @@ class DashboardResource(BaseResource):
             response['public_url'] = url_for('redash.public_dashboard', token=api_key.api_key, org_slug=self.current_org.slug, _external=True)
             response['api_key'] = api_key.api_key
 
+        last_change = models.Change.get_latest(object_id=dashboard.id, object_type=models.Dashboard.__name__)
+        if last_change:
+            response['latest_version'] = last_change.id
+
         return response
 
     @require_permission('edit_dashboard')
@@ -56,11 +88,34 @@ class DashboardResource(BaseResource):
         dashboard_properties = request.get_json(force=True)
         # TODO: either convert all requests to use slugs or ids
         dashboard = models.Dashboard.get_by_id_and_org(dashboard_slug, self.current_org)
+
+        # check access permissions
+        if self.current_user.id != dashboard.user.id:
+            if not self.current_user.has_access(
+                    access_type=models.AccessPermission.ACCESS_TYPE_MODIFY,
+                    object_id=dashboard.id,
+                    object_type=models.Dashboard.__name__):
+                abort(403)
+
+        # Optimistic locking: figure out which user made the last
+        # change to this dashboard, and bail out if necessary
+        last_change = models.Change.get_latest(object_id=dashboard.id, object_type=models.Dashboard.__name__)
+        if last_change and 'latest_version' in dashboard_properties:
+            if last_change.id > dashboard_properties['latest_version']:
+                abort(409) # HTTP 'Conflict' status code
+
+        old_dashboard = {'name': dashboard.name, 'layout': dashboard.layout}
         dashboard.layout = dashboard_properties['layout']
         dashboard.name = dashboard_properties['name']
         dashboard.save()
 
-        return dashboard.to_dict(with_widgets=True, user=self.current_user)
+        # create a new Changes record to keep track of the changes
+        new_dashboard = {'name': dashboard.name, 'layout': dashboard.layout}
+        new_change = _save_change(self.current_user, dashboard.id, old_dashboard, new_dashboard, change_type=models.Change.TYPE_MODIFY)
+
+        result = dashboard.to_dict(with_widgets=True, user=self.current_user)
+        result['latest_version'] = new_change.id
+        return result
 
     @require_permission('edit_dashboard')
     def delete(self, dashboard_slug):

redash/handlers/queries.py

@@ -25,7 +25,7 @@ def format_sql_query(org_slug=None):
     return sqlparse.format(query, reindent=True, keyword_case='upper')
 
 
-def _save_change(user, query, old_query, new_query, change_type=models.Change.TYPE_UPDATE):
+def _save_change(user, query, old_query, new_query, change_type=models.Change.TYPE_MODIFY):
     if 'data_source' in new_query:
         new_query['data_source_id'] = new_query.pop('data_source')
     for field in ['data_source_id', 'user', 'last_modified_by', 'org']:
@@ -141,7 +141,7 @@ class QueryResource(BaseResource):
         # Optimistic locking: figure out which user made the last
         # change to this query, and bail out if necessary
         last_change = models.Change.get_latest(object_id=query.id, object_type=models.Query.__name__)
-        if last_change:
+        if last_change and 'latest_version' in query_def:
             if last_change.id > query_def['latest_version']:
                 abort(409) # HTTP 'Conflict' status code
 

redash/models.py

@@ -819,7 +819,7 @@ class Change(BaseModel):
     created_at = DateTimeTZField(default=datetime.datetime.now)
 
     TYPE_CREATE = 'create'
-    TYPE_UPDATE = 'update'
+    TYPE_MODIFY = 'modify'
 
     class Meta:
         db_table = 'changes'

tests/handlers/test_queries.py

@@ -77,6 +77,7 @@ class QueryAPITest(BaseTestCase, AuthenticationTestMixin):
         rv = self.make_request('get', '/api/queries/{}'.format(query.id), user=self.factory.create_admin())
         self.assertEquals(rv.status_code, 200)
 
+
 class QueryRefreshTest(BaseTestCase):
     def setUp(self):
         super(QueryRefreshTest, self).setUp()