From 2ccb9d664fd5a41c1e0727b331e7e1acf5c96c38 Mon Sep 17 00:00:00 2001 From: Davor Spasovski Date: Mon, 6 Feb 2017 13:39:46 -0500 Subject: [PATCH] add compare query version support (re #7) --- .../pages/queries/compare-query-dialog.css | 54 +++++++++++++++++++ .../pages/queries/compare-query-dialog.html | 33 ++++++++++++ .../app/pages/queries/compare-query-dialog.js | 52 ++++++++++++++++++ client/app/pages/queries/query.html | 3 ++ client/app/pages/queries/view.js | 15 ++++++ package.json | 1 + redash/handlers/api.py | 6 ++- redash/handlers/dashboards.py | 1 + redash/handlers/queries.py | 15 ++++++ redash/models.py | 40 ++++++++------ tests/handlers/test_queries.py | 27 ++++++++++ tests/models/test_changes.py | 13 +---- tests/test_models.py | 3 +- 13 files changed, 232 insertions(+), 31 deletions(-) create mode 100644 client/app/pages/queries/compare-query-dialog.css create mode 100644 client/app/pages/queries/compare-query-dialog.html create mode 100644 client/app/pages/queries/compare-query-dialog.js 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..bdb5c4d97e --- /dev/null +++ b/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); +} diff --git a/client/app/pages/queries/query.html b/client/app/pages/queries/query.html index 9102e1d7a9..305bace1b2 100644 --- a/client/app/pages/queries/query.html +++ b/client/app/pages/queries/query.html @@ -65,6 +65,9 @@

  • Unpublish
  • Show API Key
    • +
  • + Query Versions +
    • diff --git a/client/app/pages/queries/view.js b/client/app/pages/queries/view.js index a73a3df620..009e5404ff 100644 --- a/client/app/pages/queries/view.js +++ b/client/app/pages/queries/view.js @@ -314,6 +314,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); }); diff --git a/package.json b/package.json index 0181bf82ec..4153069350 100644 --- a/package.json +++ b/package.json @@ -46,6 +46,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", "jquery": "^3.2.1", diff --git a/redash/handlers/api.py b/redash/handlers/api.py index 9d5006bc27..fa8b02a384 100644 --- a/redash/handlers/api.py +++ b/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//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 21564e05ee..79f352bdff 100644 --- a/redash/handlers/dashboards.py +++ b/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() diff --git a/redash/handlers/queries.py b/redash/handlers/queries.py index f22ceaaf21..265db47e11 100644 --- a/redash/handlers/queries.py +++ b/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() diff --git a/redash/models.py b/redash/models.py index 7b2c19446a..548adc9edc 100644 --- a/redash/models.py +++ b/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): @@ -849,7 +851,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) @@ -1059,6 +1061,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': @@ -1195,7 +1198,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 @@ -1215,6 +1217,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' diff --git a/tests/handlers/test_queries.py b/tests/handlers/test_queries.py index 52b4eac626..4027267c21 100644 --- a/tests/handlers/test_queries.py +++ b/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') 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) diff --git a/tests/test_models.py b/tests/test_models.py index abaddf1c52..6c490dd52d 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -180,7 +180,8 @@ def test_failure_extends_schedule(self): Execution failures recorded for a query result in exponential backoff for scheduling future execution. """ - query = self.factory.create_query(schedule="60", schedule_failures=4) + query = self.factory.create_query(schedule="60") + query.schedule_failures = 4 retrieved_at = utcnow() - datetime.timedelta(minutes=16) query_result = self.factory.create_query_result( retrieved_at=retrieved_at, query_text=query.query_text,