add compare query version support (re #7)

client/app/pages/queries/compare-query-dialog.css

@@ -0,0 +1,54 @@
+/* Compare Query Version container */
+/* Offers slight visual improvement (alignment) to modern UAs */
+.compare-query-version {
+  display: flex;
+  justify-content: space-between;
+  align-items: center;
+}
+
+.diff-removed {
+  background-color: rgba(208, 2, 27, 0.3);
+}
+
+.diff-added {
+  background-color: rgba(65, 117, 5, 0.3);
+}
+
+.query-diff-container span {
+  display: inline-block;
+  border-radius: 3px;
+  line-height: 20px;
+  vertical-align: middle;
+  margin: 0 5px 0 0;
+}
+
+.query-diff-container > div:not(.compare-query-version-controls) {
+  float: left;
+  width: calc(50% - 5px);
+  margin: 0 10px 0 0;
+}
+
+.compare-query-version {
+  background-color: #f5f5f5;
+  padding: 5px;
+  border: 1px solid #ccc;
+  margin-right: 15px;
+  border-radius: 3px;
+}
+
+.diff-content {
+  border: 1px solid #ccc;
+  background-color: #f5f5f5;
+  border-radius: 3px;
+  padding: 15px;
+}
+
+.query-diff-container > div:last-child {
+  margin: 0;
+}
+
+.compare-query-version-controls {
+  display: flex;
+  align-items: center;
+  margin-bottom: 25px;
+}

client/app/pages/queries/compare-query-dialog.html

@@ -0,0 +1,33 @@
+<div class="modal-header">
+    <button type="button" class="close" aria-label="Close" ng-click="$ctrl.close()">
+        <span aria-hidden="true">&times;</span>
+    </button>
+    <h4 class="modal-title">Compare Query</h4>
+</div>
+<div class="modal-body query-diff-container">
+    <div class="compare-query-version-controls">
+        <div class="compare-query-version">
+            <span>Compare current version to</span>
+            <select id="version-choice" ng-model="compareQueryVersion" ng-change="$ctrl.compareQueries()">
+                <option ng-repeat="version in $ctrl.versions.slice(0, $ctrl.versions.length-1)" value="{{version.object_version}}">Version {{version.object_version}}</option>
+            </select>
+        </div>
+        <div class="compare-query-revert-wrapper hidden"><a ng-click="$ctrl.revertQuery()" class="btn btn-default">Revert to version {{$ctrl.previousQueryVersion + 1}}</a></div>
+    </div>
+    <div>
+        <h5>Current Version {{$ctrl.currentQuery.version}}</h5>
+        <div class="diff-content">
+            <p id="current-query-diff">
+                <span ng-class="{'diff-added': part.added, 'diff-removed': part.removed}" ng-repeat="part in $ctrl.currentDiff">
+                    {{part.value}}
+                </span>
+            </p>
+        </div>
+    </div>
+    <div>
+        <h5>Previous Version {{$ctrl.previousQueryVersion + 1}}</h5>
+        <div class="diff-content">
+            <p>{{$ctrl.previousQuery}}</p>
+        </div>
+    </div>
+</div>

client/app/pages/queries/compare-query-dialog.js

@@ -0,0 +1,52 @@
+import * as jsDiff from 'diff';
+import template from './compare-query-dialog.html';
+import './compare-query-dialog.css';
+
+const CompareQueryDialog = {
+  controller: ['clientConfig', '$http', function doCompare(clientConfig, $http) {
+    this.currentQuery = this.resolve.query;
+
+    this.previousQuery = '';
+    this.currentDiff = [];
+    this.previousDiff = [];
+    this.versions = [];
+    this.previousQueryVersion = this.currentQuery.version - 2; // due to 0-indexed versions[]
+
+    this.compareQueries = (isInitialLoad) => {
+      if (!isInitialLoad) {
+        this.previousQueryVersion = document.getElementById('version-choice').value - 1; // due to 0-indexed versions[]
+      }
+
+      this.previousQuery = this.versions[this.previousQueryVersion].change.query.current;
+      this.currentDiff = jsDiff.diffChars(this.previousQuery, this.currentQuery.query);
+      document.querySelector('.compare-query-revert-wrapper').classList.remove('hidden');
+    };
+
+    this.revertQuery = () => {
+      this.resolve.query.query = this.previousQuery;
+      this.resolve.saveQuery();
+
+      // Close modal.
+      this.dismiss();
+    };
+
+    $http.get(`/api/queries/${this.currentQuery.id}/version`).then((response) => {
+      this.versions = response.data;
+      this.compareQueries(true);
+    });
+  }],
+  scope: {
+    query: '=',
+    saveQuery: '<',
+  },
+  bindings: {
+    resolve: '<',
+    close: '&',
+    dismiss: '&',
+  },
+  template,
+};
+
+export default function (ngModule) {
+  ngModule.component('compareQueryDialog', CompareQueryDialog);
+}

client/app/pages/queries/query.html

@@ -65,6 +65,9 @@ <h3>
             <li ng-if="!query.is_draft && query.id != undefined && (isQueryOwner || currentUser.hasPermission('admin'))"><a ng-click="togglePublished()">Unpublish</a></li>
             <li class="divider"></li>
             <li ng-if="query.id != undefined"><a ng-click="showApiKey()">Show API Key</a></li>
+            <li ng-if="query.id && (isQueryOwner || currentUser.hasPermission('admin'))">
+              <a ng-click="compareQueryVersion()">Query Versions</a>
+            </li>
           </ul>
         </div>
       </div>

client/app/pages/queries/view.js

@@ -304,6 +304,21 @@ function QueryViewCtrl(
     $location.hash(visualization.id);
   };
 
+  $scope.compareQueryVersion = () => {
+    if (!$scope.query.query) {
+      return;
+    }
+
+    $uibModal.open({
+      windowClass: 'modal-xl',
+      component: 'compareQueryDialog',
+      resolve: {
+        query: $scope.query,
+        saveQuery: () => $scope.saveQuery,
+      },
+    });
+  };
+
   $scope.$watch('query.name', () => {
     Title.set($scope.query.name);
   });

package.json

@@ -44,6 +44,7 @@
     "d3": "^3.5.17",
     "d3-cloud": "^1.2.4",
     "debug": "^3.1.0",
+    "diff": "^3.3.0",
     "font-awesome": "^4.7.0",
     "jquery": "^3.2.1",
     "jquery-ui": "^1.12.1",

redash/handlers/api.py

@@ -6,10 +6,10 @@
 from redash.handlers.base import org_scoped_rule
 from redash.handlers.permissions import ObjectPermissionsListResource, CheckPermissionResource
 from redash.handlers.alerts import AlertResource, AlertListResource, AlertSubscriptionListResource, AlertSubscriptionResource
-from redash.handlers.dashboards import DashboardListResource, RecentDashboardsResource, DashboardResource, DashboardShareResource, PublicDashboardResource 
+from redash.handlers.dashboards import DashboardListResource, RecentDashboardsResource, DashboardResource, DashboardShareResource, PublicDashboardResource
 from redash.handlers.data_sources import DataSourceTypeListResource, DataSourceListResource, DataSourceSchemaResource, DataSourceResource, DataSourcePauseResource, DataSourceTestResource
 from redash.handlers.events import EventsResource
-from redash.handlers.queries import QueryForkResource, QueryRefreshResource, QueryListResource, QueryRecentResource, QuerySearchResource, QueryResource, MyQueriesResource
+from redash.handlers.queries import QueryForkResource, QueryRefreshResource, QueryListResource, QueryRecentResource, QuerySearchResource, QueryResource, MyQueriesResource, QueryVersionListResource, ChangeResource
 from redash.handlers.query_results import QueryResultListResource, QueryResultResource, JobResource
 from redash.handlers.users import UserResource, UserListResource, UserInviteResource, UserResetPasswordResource
 from redash.handlers.visualizations import VisualizationListResource
@@ -74,6 +74,8 @@ def json_representation(data, code, headers=None):
 api.add_org_resource(QueryRefreshResource, '/api/queries/<query_id>/refresh', endpoint='query_refresh')
 api.add_org_resource(QueryResource, '/api/queries/<query_id>', endpoint='query')
 api.add_org_resource(QueryForkResource, '/api/queries/<query_id>/fork', endpoint='query_fork')
+api.add_org_resource(QueryVersionListResource, '/api/queries/<query_id>/version', endpoint='query_versions')
+api.add_org_resource(ChangeResource, '/api/changes/<change_id>', endpoint='changes')
 
 api.add_org_resource(ObjectPermissionsListResource, '/api/<object_type>/<object_id>/acl', endpoint='object_permissions')
 api.add_org_resource(CheckPermissionResource, '/api/<object_type>/<object_id>/acl/<access_type>', endpoint='check_permissions')

redash/handlers/dashboards.py

@@ -57,6 +57,7 @@ def post(self):
                                      user=self.current_user,
                                      is_draft=True,
                                      layout='[]')
+        dashboard.record_changes(changed_by=self.current_user)
         models.db.session.add(dashboard)
         models.db.session.commit()
         return dashboard.to_dict()

redash/handlers/queries.py

@@ -131,6 +131,7 @@ def post(self):
         query_def['org'] = self.current_org
         query_def['is_draft'] = True
         query = models.Query.create(**query_def)
+        query.record_changes(changed_by=self.current_user)
         models.db.session.add(query)
         models.db.session.commit()
 
@@ -214,6 +215,7 @@ def post(self, query_id):
 
         try:
             self.update_model(query, query_def)
+            query.record_changes(self.current_user)
             models.db.session.commit()
         except StaleDataError:
             abort(409)
@@ -287,3 +289,16 @@ def post(self, query_id):
         parameter_values = collect_parameters_from_request(request.args)
 
         return run_query(query.data_source, parameter_values, query.query_text, query.id)
+
+
+class QueryVersionListResource(BaseResource):
+    @require_permission('view_query')
+    def get(self, query_id):
+        results = models.Change.list_versions(models.Query.get_by_id(query_id))
+        return [q.to_dict() for q in results]
+
+
+class ChangeResource(BaseResource):
+    @require_permission('view_query')
+    def get(self, change_id):
+        return models.Change.query.get(change_id).to_dict()

redash/models.py

@@ -197,10 +197,6 @@ class ChangeTrackingMixin(object):
     skipped_fields = ('id', 'created_at', 'updated_at', 'version')
     _clean_values = None
 
-    def __init__(self, *a, **kw):
-        super(ChangeTrackingMixin, self).__init__(*a, **kw)
-        self.record_changes(self.user)
-
     def prep_cleanvalues(self):
         self.__dict__['_clean_values'] = {}
         for attr in inspect(self.__class__).column_attrs:
@@ -211,10 +207,10 @@ def prep_cleanvalues(self):
     def __setattr__(self, key, value):
         if self._clean_values is None:
             self.prep_cleanvalues()
-        for attr in inspect(self.__class__).column_attrs:
-            col, = attr.columns
-            previous = getattr(self, attr.key, None)
-            self._clean_values[col.name] = previous
+
+        if key in inspect(self.__class__).column_attrs:
+            previous = getattr(self, key, None)
+            self._clean_values[key] = previous
 
         super(ChangeTrackingMixin, self).__setattr__(key, value)
 
@@ -225,13 +221,19 @@ def record_changes(self, changed_by):
         for attr in inspect(self.__class__).column_attrs:
             col, = attr.columns
             if attr.key not in self.skipped_fields:
-                changes[col.name] = {'previous': self._clean_values[col.name],
-                                     'current': getattr(self, attr.key)}
+                prev = self._clean_values[col.name]
+                current = getattr(self, attr.key)
+                if prev != current:
+                    changes[col.name] = {'previous': prev, 'current': current}
 
-        db.session.add(Change(object=self,
-                              object_version=self.version,
-                              user=changed_by,
-                              change=changes))
+        if changes:
+            self.version = (self.version or 0) + 1
+            change = Change(object=self,
+                            object_version=self.version,
+                            user=changed_by,
+                            change=changes)
+            db.session.add(change)
+            return change
 
 
 class BelongsToOrgMixin(object):
@@ -847,7 +849,7 @@ def should_schedule_next(previous_iteration, now, schedule, failures):
 
 class Query(ChangeTrackingMixin, TimestampMixin, BelongsToOrgMixin, db.Model):
     id = Column(db.Integer, primary_key=True)
-    version = Column(db.Integer, default=1)
+    version = Column(db.Integer, default=0)
     org_id = Column(db.Integer, db.ForeignKey('organizations.id'))
     org = db.relationship(Organization, backref="queries")
     data_source_id = Column(db.Integer, db.ForeignKey("data_sources.id"), nullable=True)
@@ -1057,6 +1059,7 @@ def fork(self, user):
         kwargs = {a: getattr(self, a) for a in forked_list}
         forked_query = Query.create(name=u'Copy of (#{}) {}'.format(self.id, self.name),
                                     user=user, **kwargs)
+        forked_query.record_changes(changed_by=user)
 
         for v in self.visualizations:
             if v.type == 'TABLE':
@@ -1193,7 +1196,6 @@ def to_dict(self, full=True):
             'id': self.id,
             'object_id': self.object_id,
             'object_type': self.object_type,
-            'change_type': self.change_type,
             'object_version': self.object_version,
             'change': self.change,
             'created_at': self.created_at
@@ -1213,6 +1215,12 @@ def last_change(cls, obj):
             cls.object_type == obj.__class__.__tablename__).order_by(
                 cls.object_version.desc()).first()
 
+    @classmethod
+    def list_versions(cls, query):
+        return cls.query.filter(
+            cls.object_id == query.id,
+            cls.object_type == 'queries')
+
 
 class Alert(TimestampMixin, db.Model):
     UNKNOWN_STATE = 'unknown'

tests/handlers/test_queries.py

@@ -199,3 +199,30 @@ def test_format_sql_query(self):
         rv = self.make_request('post', '/api/queries/format', user=admin, data={'query': query})
 
         self.assertEqual(rv.json['query'], expected)
+
+
+class ChangeResourceTests(BaseTestCase):
+    def test_list(self):
+        query = self.factory.create_query()
+        query.name = 'version A'
+        query.record_changes(self.factory.user)
+        query.name = 'version B'
+        query.record_changes(self.factory.user)
+        rv = self.make_request('get', '/api/queries/{0}/version'.format(query.id))
+        self.assertEquals(rv.status_code, 200)
+        self.assertEquals(len(rv.json), 2)
+        self.assertEquals(rv.json[0]['change']['name']['current'], 'version A')
+        self.assertEquals(rv.json[1]['change']['name']['current'], 'version B')
+
+    def test_get(self):
+        query = self.factory.create_query()
+        query.name = 'version A'
+        ch1 = query.record_changes(self.factory.user)
+        query.name = 'version B'
+        ch2 = query.record_changes(self.factory.user)
+        rv1 = self.make_request('get', '/api/changes/' + str(ch1.id))
+        self.assertEqual(rv1.status_code, 200)
+        self.assertEqual(rv1.json['change']['name']['current'], 'version A')
+        rv2 = self.make_request('get', '/api/changes/' + str(ch2.id))
+        self.assertEqual(rv2.status_code, 200)
+        self.assertEqual(rv2.json['change']['name']['current'], 'version B')

tests/models/test_changes.py

@@ -56,23 +56,12 @@ def test_properly_log_modification(self):
         obj.record_changes(changed_by=self.factory.user)
         obj.name = 'Query 2'
         obj.description = 'description'
-        db.session.flush()
         obj.record_changes(changed_by=self.factory.user)
 
         change = Change.last_change(obj)
 
         self.assertIsNotNone(change)
-        # TODO: https://github.com/getredash/redash/issues/1550
-        # self.assertEqual(change.object_version, 2)
+        self.assertEqual(change.object_version, 2)
         self.assertEqual(change.object_version, obj.version)
         self.assertIn('name', change.change)
         self.assertIn('description', change.change)
-
-    def test_logs_create_method(self):
-        q = Query(name='Query', description='', query_text='',
-                  user=self.factory.user, data_source=self.factory.data_source,
-                  org=self.factory.org)
-        change = Change.last_change(q)
-
-        self.assertIsNotNone(change)
-        self.assertEqual(q.user, change.user)