diff --git a/client/app/pages/queries/compare-query-dialog.css b/client/app/pages/queries/compare-query-dialog.css new file mode 100644 index 0000000000..ce2d01370e --- /dev/null +++ b/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; +} diff --git a/client/app/pages/queries/compare-query-dialog.html b/client/app/pages/queries/compare-query-dialog.html new file mode 100644 index 0000000000..5214046055 --- /dev/null +++ b/client/app/pages/queries/compare-query-dialog.html @@ -0,0 +1,33 @@ +
+ +

Compare Query

+
+
+
+
+ Compare current version to + +
+ +
+
+
Current Version {{$ctrl.currentQuery.version}}
+
+

+ + {{part.value}} + +

+
+
+
+
Previous Version {{$ctrl.previousQueryVersion + 1}}
+
+

{{$ctrl.previousQuery}}

+
+
+
diff --git a/client/app/pages/queries/compare-query-dialog.js b/client/app/pages/queries/compare-query-dialog.js new file mode 100644 index 0000000000..e95e9cb8e1 --- /dev/null +++ b/client/app/pages/queries/compare-query-dialog.js @@ -0,0 +1,66 @@ +import * as jsDiff from 'diff'; +import template from './compare-query-dialog.html'; +import './compare-query-dialog.css'; + +const CompareQueryDialog = { + controller(clientConfig, $http) { + 'ngInject'; + + 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; + + const compare = (a, b) => { + if (a.object_version < b.object_version) { + return -1; + } else if (a.object_version > b.object_version) { + return 1; + } + return 0; + }; + + this.versions.sort(compare); + this.compareQueries(true); + }); + }, + scope: { + query: '=', + saveQuery: '<', + }, + bindings: { + resolve: '<', + close: '&', + dismiss: '&', + }, + template, +}; + +export default function init(ngModule) { + ngModule.component('compareQueryDialog', CompareQueryDialog); +} + +init.init = true; diff --git a/client/app/pages/queries/query.html b/client/app/pages/queries/query.html index a950b2b534..1744f9be79 100644 --- a/client/app/pages/queries/query.html +++ b/client/app/pages/queries/query.html @@ -69,6 +69,9 @@

  • Show API Key
    • +
  • + Query Versions +
    • diff --git a/client/app/pages/queries/view.js b/client/app/pages/queries/view.js index ae52974ee1..8d6c647408 100644 --- a/client/app/pages/queries/view.js +++ b/client/app/pages/queries/view.js @@ -393,6 +393,21 @@ function QueryViewCtrl( }); }; + $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); }); diff --git a/package.json b/package.json index a4dc2719d9..908ced0c6f 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "d3": "^3.5.17", "d3-cloud": "^1.2.4", "debug": "^3.1.0", + "diff": "^3.3.0", "font-awesome": "^4.7.0", "gridstack": "^0.3.0", "hoist-non-react-statics": "^3.3.0", diff --git a/redash/handlers/api.py b/redash/handlers/api.py index 012c121f63..75fdf7681f 100644 --- a/redash/handlers/api.py +++ b/redash/handlers/api.py @@ -35,6 +35,7 @@ QueryForkResource, QueryListResource, QueryRecentResource, QueryRefreshResource, QueryResource, QuerySearchResource, + QueryVersionListResource, ChangeResource, QueryTagsResource) from redash.handlers.query_results import (JobResource, QueryResultDropdownResource, @@ -115,6 +116,8 @@ def json_representation(data, code, headers=None): api.add_org_resource(QueryRefreshResource, '/api/queries//refresh', endpoint='query_refresh') api.add_org_resource(QueryResource, '/api/queries/', endpoint='query') api.add_org_resource(QueryForkResource, '/api/queries//fork', endpoint='query_fork') +api.add_org_resource(QueryVersionListResource, '/api/queries//version', endpoint='query_versions') +api.add_org_resource(ChangeResource, '/api/changes/', endpoint='changes') api.add_org_resource(ObjectPermissionsListResource, '/api///acl', endpoint='object_permissions') api.add_org_resource(CheckPermissionResource, '/api///acl/', endpoint='check_permissions') diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index 954e9da4f7..869a0f5982 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -105,6 +105,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 serialize_dashboard(dashboard) diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index 278ca54567..7ff91eba45 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -234,6 +234,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() @@ -342,6 +343,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) @@ -488,3 +490,19 @@ def get(self): }) return response + + +class QueryVersionListResource(BaseResource): + @require_permission('view_query') + def get(self, query_id): + results = models.Change.query.filter( + models.Change.object_id == query_id, + models.Change.object_type == 'queries', + ) + 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() diff --git a/redash/models/__init__.py b/redash/models/__init__.py index a4160ea006..97599841f1 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -397,7 +397,7 @@ def should_schedule_next(previous_iteration, now, interval, time=None, day_of_we 'is_archived', 'is_draft', 'schedule', '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) @@ -646,6 +646,7 @@ def fork(self, user): # Query.create will add default TABLE visualization, so use constructor to create bare copy of query forked_query = Query(name=u'Copy of (#{}) {}'.format(self.id, self.name), user=user, **kwargs) + forked_query.record_changes(changed_by=user) for v in self.visualizations: forked_v = v.copy() diff --git a/redash/models/changes.py b/redash/models/changes.py index 1529c4b323..82ac0f1ce6 100644 --- a/redash/models/changes.py +++ b/redash/models/changes.py @@ -22,7 +22,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 @@ -49,10 +48,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: @@ -63,11 +58,9 @@ 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) def record_changes(self, changed_by): @@ -77,10 +70,16 @@ 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)} - - db.session.add(Change(object=self, - object_version=self.version, - user=changed_by, - change=changes)) + prev = self._clean_values[col.name] + current = getattr(self, attr.key) + if prev != current: + changes[col.name] = {'previous': prev, 'current': current} + + 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 diff --git a/tests/handlers/test_queries.py b/tests/handlers/test_queries.py index 3a898b1fa3..1c2485db2f 100644 --- a/tests/handlers/test_queries.py +++ b/tests/handlers/test_queries.py @@ -406,3 +406,29 @@ def test_format_sql_query(self): 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') diff --git a/tests/models/test_changes.py b/tests/models/test_changes.py index 124e17a30d..3d7c7496e8 100644 --- a/tests/models/test_changes.py +++ 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)