From 70fccdfc11209696c8e99b19233f3f00ee6fe65f 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 | 66 +++++++++++++++++++ client/app/pages/queries/query.html | 3 + client/app/pages/queries/view.js | 15 +++++ package.json | 1 + redash/handlers/api.py | 8 +++ redash/handlers/dashboards.py | 1 + redash/handlers/queries.py | 17 +++++ redash/models/__init__.py | 3 +- redash/models/changes.py | 35 +++++----- tests/handlers/test_queries.py | 27 ++++++++ tests/models/test_changes.py | 18 +---- tests/tasks/test_refresh_queries.py | 2 +- 14 files changed, 244 insertions(+), 39 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..2315b91841 --- /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 1d93990266..fabfdac20f 100644 --- a/client/app/pages/queries/query.html +++ b/client/app/pages/queries/query.html @@ -74,6 +74,9 @@

  • Show API Key
    • +
  • + Query Versions +
    • diff --git a/client/app/pages/queries/view.js b/client/app/pages/queries/view.js index 1925973fda..a488e47bd6 100644 --- a/client/app/pages/queries/view.js +++ b/client/app/pages/queries/view.js @@ -415,6 +415,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 88b0df3ec1..d3084d2701 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", "hoist-non-react-statics": "^3.3.0", "jquery": "^3.2.1", diff --git a/redash/handlers/api.py b/redash/handlers/api.py index ef479ef3c3..8778219cd5 100644 --- a/redash/handlers/api.py +++ b/redash/handlers/api.py @@ -46,6 +46,7 @@ ObjectPermissionsListResource, ) from redash.handlers.queries import ( + ChangeResource, MyQueriesResource, QueryArchiveResource, QueryFavoriteListResource, @@ -57,6 +58,7 @@ QuerySearchResource, QueryTagsResource, QueryRegenerateApiKeyResource, + QueryVersionListResource, ) from redash.handlers.query_results import ( JobResource, @@ -221,6 +223,12 @@ def json_representation(data, code, headers=None): "/api/queries//regenerate_api_key", endpoint="query_regenerate_api_key", ) +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, diff --git a/redash/handlers/dashboards.py b/redash/handlers/dashboards.py index d5b7488f94..d27494f7da 100644 --- a/redash/handlers/dashboards.py +++ b/redash/handlers/dashboards.py @@ -105,6 +105,7 @@ def post(self): 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 129f7e3e6d..b367977b98 100644 --- a/redash/handlers/queries.py +++ b/redash/handlers/queries.py @@ -256,6 +256,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() @@ -376,6 +377,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) @@ -549,3 +551,18 @@ 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 e6d909d7cf..eda39f42cc 100644 --- a/redash/models/__init__.py +++ b/redash/models/__init__.py @@ -441,7 +441,7 @@ def should_schedule_next( ) 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) @@ -767,6 +767,7 @@ def fork(self, user): forked_query = Query( name="Copy of (#{}) {}".format(self.id, self.name), user=user, **kwargs ) + forked_query.record_changes(changed_by=user) for v in sorted(self.visualizations, key=lambda v: v.id): forked_v = v.copy() diff --git a/redash/models/changes.py b/redash/models/changes.py index a670703514..25bab013ef 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, @@ -50,25 +49,19 @@ 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: - col, = attr.columns + (col,) = attr.columns # 'query' is col name but not attr name self._clean_values[col.name] = None 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): @@ -76,18 +69,20 @@ def record_changes(self, changed_by): db.session.flush() changes = {} for attr in inspect(self.__class__).column_attrs: - col, = attr.columns + (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( + 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 0091573085..4c2e4d0581 100644 --- a/tests/handlers/test_queries.py +++ b/tests/handlers/test_queries.py @@ -510,3 +510,30 @@ 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/{}/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 e9847b0a0a..07525746d7 100644 --- a/tests/models/test_changes.py +++ b/tests/models/test_changes.py @@ -60,28 +60,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/tasks/test_refresh_queries.py b/tests/tasks/test_refresh_queries.py index 452c1b7702..d43e081510 100644 --- a/tests/tasks/test_refresh_queries.py +++ b/tests/tasks/test_refresh_queries.py @@ -124,7 +124,7 @@ def test_doesnt_enqueue_parameterized_queries_with_invalid_parameters(self): add_job_mock.assert_not_called() def test_doesnt_enqueue_parameterized_queries_with_dropdown_queries_that_are_detached_from_data_source( - self + self, ): """ Scheduled queries with a dropdown parameter which points to a query that is detached from its data source are skipped.