From 72dc157bbe9c6df4664716aabfaf007f6929fdf6 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Thu, 17 Sep 2020 14:01:15 +0300 Subject: [PATCH 1/6] Allow to clear selected tags on list pages (#5142) * Convert TagsList to functional component * Convert TagsList to typescript * Allow to unselect all tags * Add title to Tags block and explicit "clear filter" button * Some tweaks --- client/app/components/TagsList.jsx | 82 -------------- client/app/components/TagsList.less | 44 ++++++- client/app/components/TagsList.tsx | 107 ++++++++++++++++++ .../items-list/components/Sidebar.jsx | 6 +- client/app/pages/dashboards/DashboardList.jsx | 2 +- client/app/pages/queries-list/QueriesList.jsx | 2 +- 6 files changed, 151 insertions(+), 92 deletions(-) delete mode 100644 client/app/components/TagsList.jsx create mode 100644 client/app/components/TagsList.tsx diff --git a/client/app/components/TagsList.jsx b/client/app/components/TagsList.jsx deleted file mode 100644 index 3a09e9221f..0000000000 --- a/client/app/components/TagsList.jsx +++ /dev/null @@ -1,82 +0,0 @@ -import { map } from "lodash"; -import React from "react"; -import PropTypes from "prop-types"; -import Badge from "antd/lib/badge"; -import Menu from "antd/lib/menu"; -import getTags from "@/services/getTags"; - -import "./TagsList.less"; - -export default class TagsList extends React.Component { - static propTypes = { - tagsUrl: PropTypes.string.isRequired, - onUpdate: PropTypes.func, - }; - - static defaultProps = { - onUpdate: () => {}, - }; - - constructor(props) { - super(props); - - this.state = { - // An array of objects that with the name and count of the tagged items - allTags: [], - // A set of tag names - selectedTags: new Set(), - }; - } - - componentDidMount() { - getTags(this.props.tagsUrl).then(allTags => { - this.setState({ allTags }); - }); - } - - toggleTag(event, tag) { - const { selectedTags } = this.state; - if (event.shiftKey) { - // toggle tag - if (selectedTags.has(tag)) { - selectedTags.delete(tag); - } else { - selectedTags.add(tag); - } - } else { - // if the tag is the only selected, deselect it, otherwise select only it - if (selectedTags.has(tag) && selectedTags.size === 1) { - selectedTags.clear(); - } else { - selectedTags.clear(); - selectedTags.add(tag); - } - } - this.forceUpdate(); - - this.props.onUpdate([...this.state.selectedTags]); - } - - render() { - const { allTags, selectedTags } = this.state; - if (allTags.length > 0) { - return ( -
- - {map(allTags, tag => ( - - this.toggleTag(event, tag.name)}> - {tag.name} - - - - ))} - -
- ); - } - return null; - } -} diff --git a/client/app/components/TagsList.less b/client/app/components/TagsList.less index 3a997a2c03..27975f250e 100644 --- a/client/app/components/TagsList.less +++ b/client/app/components/TagsList.less @@ -1,15 +1,47 @@ -@import '~@/assets/less/ant'; +@import "~@/assets/less/ant"; .tags-list { + .tags-list-title { + margin: 15px 5px 5px 5px; + display: flex; + justify-content: space-between; + align-items: center; + + label { + display: block; + white-space: nowrap; + margin: 0; + } + + a { + display: block; + white-space: nowrap; + cursor: pointer; + + .anticon { + font-size: 75%; + margin-right: 2px; + } + } + } + .ant-badge-count { background-color: fade(@redash-gray, 10%); color: fade(@redash-gray, 75%); } - .ant-menu-item-selected { - .ant-badge-count { - background-color: @primary-color; - color: white; + .ant-menu.ant-menu-inline { + border: none; + + .ant-menu-item { + width: 100%; + } + + .ant-menu-item-selected { + .ant-badge-count { + background-color: @primary-color; + color: white; + } } } -} \ No newline at end of file +} diff --git a/client/app/components/TagsList.tsx b/client/app/components/TagsList.tsx new file mode 100644 index 0000000000..de9f4aabcc --- /dev/null +++ b/client/app/components/TagsList.tsx @@ -0,0 +1,107 @@ +import { map, includes, difference } from "lodash"; +import React, { useState, useCallback, useEffect } from "react"; +import Badge from "antd/lib/badge"; +import Menu from "antd/lib/menu"; +import CloseOutlinedIcon from "@ant-design/icons/CloseOutlined"; +import getTags from "@/services/getTags"; + +import "./TagsList.less"; + +type Tag = { + name: string; + count?: number; +}; + +type TagsListProps = { + tagsUrl: string; + showUnselectAll: boolean; + onUpdate?: (selectedTags: string[]) => void; +}; + +function TagsList({ tagsUrl, showUnselectAll = false, onUpdate }: TagsListProps): JSX.Element | null { + const [allTags, setAllTags] = useState([]); + const [selectedTags, setSelectedTags] = useState([]); + + useEffect(() => { + let isCancelled = false; + + getTags(tagsUrl).then(tags => { + if (!isCancelled) { + setAllTags(tags); + } + }); + + return () => { + isCancelled = true; + }; + }, [tagsUrl]); + + const toggleTag = useCallback( + (event, tag) => { + let newSelectedTags; + if (event.shiftKey) { + // toggle tag + if (includes(selectedTags, tag)) { + newSelectedTags = difference(selectedTags, [tag]); + } else { + newSelectedTags = [...selectedTags, tag]; + } + } else { + // if the tag is the only selected, deselect it, otherwise select only it + if (includes(selectedTags, tag) && selectedTags.length === 1) { + newSelectedTags = []; + } else { + newSelectedTags = [tag]; + } + } + + setSelectedTags(newSelectedTags); + if (onUpdate) { + onUpdate([...newSelectedTags]); + } + }, + [selectedTags, onUpdate] + ); + + const unselectAll = useCallback(() => { + setSelectedTags([]); + if (onUpdate) { + onUpdate([]); + } + }, [onUpdate]); + + if (allTags.length === 0) { + return null; + } + + return ( +
+
+ + {showUnselectAll && selectedTags.length > 0 && ( + + + clear selection + + )} +
+ +
+ + {map(allTags, tag => ( + + toggleTag(event, tag.name)}> + {tag.name} + + + + ))} + +
+
+ ); +} + +export default TagsList; diff --git a/client/app/components/items-list/components/Sidebar.jsx b/client/app/components/items-list/components/Sidebar.jsx index 9807e4f953..6ac10b51a1 100644 --- a/client/app/components/items-list/components/Sidebar.jsx +++ b/client/app/components/items-list/components/Sidebar.jsx @@ -132,13 +132,13 @@ ProfileImage.propTypes = { Tags */ -export function Tags({ url, onChange }) { +export function Tags({ url, onChange, showUnselectAll }) { if (url === "") { return null; } return (
- +
); } @@ -146,4 +146,6 @@ export function Tags({ url, onChange }) { Tags.propTypes = { url: PropTypes.string.isRequired, onChange: PropTypes.func.isRequired, + showUnselectAll: PropTypes.bool, + unselectAllButtonTitle: PropTypes.string, }; diff --git a/client/app/pages/dashboards/DashboardList.jsx b/client/app/pages/dashboards/DashboardList.jsx index c2c9d29e1d..dee917d981 100644 --- a/client/app/pages/dashboards/DashboardList.jsx +++ b/client/app/pages/dashboards/DashboardList.jsx @@ -95,7 +95,7 @@ class DashboardList extends React.Component { onChange={controller.updateSearch} /> - +
diff --git a/client/app/pages/queries-list/QueriesList.jsx b/client/app/pages/queries-list/QueriesList.jsx index 5bebb66468..840f02454e 100644 --- a/client/app/pages/queries-list/QueriesList.jsx +++ b/client/app/pages/queries-list/QueriesList.jsx @@ -134,7 +134,7 @@ class QueriesList extends React.Component { onChange={controller.updateSearch} /> - + {controller.isLoaded && controller.isEmpty ? ( From 83726da48a0d1768a8f726f007ce0c6fd594c2de Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Mon, 21 Sep 2020 12:54:55 +0300 Subject: [PATCH 2/6] Keep additional URL params when forking a query (#5184) --- .../pages/queries/hooks/useDuplicateQuery.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/client/app/pages/queries/hooks/useDuplicateQuery.js b/client/app/pages/queries/hooks/useDuplicateQuery.js index a825a8b5e1..8011f82250 100644 --- a/client/app/pages/queries/hooks/useDuplicateQuery.js +++ b/client/app/pages/queries/hooks/useDuplicateQuery.js @@ -1,7 +1,20 @@ -import { noop } from "lodash"; +import { noop, extend, pick } from "lodash"; import { useCallback, useState } from "react"; +import url from "url"; +import qs from "query-string"; import { Query } from "@/services/query"; +function keepCurrentUrlParams(targetUrl) { + const currentUrlParams = qs.parse(window.location.search); + targetUrl = url.parse(targetUrl); + const targetUrlParams = qs.parse(targetUrl.search); + return url.format( + extend(pick(targetUrl, ["protocol", "auth", "host", "pathname", "hash"]), { + search: qs.stringify(extend(currentUrlParams, targetUrlParams)), + }) + ); +} + export default function useDuplicateQuery(query) { const [isDuplicating, setIsDuplicating] = useState(false); @@ -16,7 +29,7 @@ export default function useDuplicateQuery(query) { setIsDuplicating(true); Query.fork({ id: query.id }) .then(newQuery => { - tab.location = newQuery.getUrl(true); + tab.location = keepCurrentUrlParams(newQuery.getUrl(true)); }) .finally(() => { setIsDuplicating(false); From 6b811c5245c0941779a12aac459fe599b45191ce Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Mon, 21 Sep 2020 23:21:14 +0300 Subject: [PATCH 3/6] Refresh CSRF tokens (#5177) * expire CSRF tokens after 6 hours * use axios' built-in cookie to header copy mechanism * add axios-auth-refresh * retry CSRF-related 400 errors by refreshing the cookie * export the auth refresh interceptor to support ejecting it if neccessary * reject the original request if it's unrelated to CSRF --- client/app/services/axios.js | 22 ++++++++++++++++------ package-lock.json | 36 ++++++++++++++++++------------------ package.json | 2 +- redash/security.py | 1 + redash/settings/__init__.py | 4 +++- 5 files changed, 39 insertions(+), 26 deletions(-) diff --git a/client/app/services/axios.js b/client/app/services/axios.js index 13de7eab7a..e50a0e9ded 100644 --- a/client/app/services/axios.js +++ b/client/app/services/axios.js @@ -1,26 +1,36 @@ import axiosLib from "axios"; import { Auth } from "@/services/auth"; import qs from "query-string"; -import Cookies from "js-cookie"; +import createAuthRefreshInterceptor from "axios-auth-refresh"; export const axios = axiosLib.create({ paramsSerializer: params => qs.stringify(params), + xsrfCookieName: "csrf_token", + xsrfHeaderName: "X-CSRF-TOKEN", }); const getData = ({ data }) => data; axios.interceptors.response.use(getData); +export const authRefreshInterceptor = createAuthRefreshInterceptor( + axios, + failedRequest => { + const message = failedRequest.response.data.message || ""; + if (message.includes("CSRF")) { + return axios.get("/ping"); + } else { + return Promise.reject(failedRequest); + } + }, + { statusCodes: [400] } +); + axios.interceptors.request.use(config => { const apiKey = Auth.getApiKey(); if (apiKey) { config.headers.Authorization = `Key ${apiKey}`; } - const csrfToken = Cookies.get("csrf_token"); - if (csrfToken) { - config.headers.common["X-CSRF-TOKEN"] = csrfToken; - } - return config; }); diff --git a/package-lock.json b/package-lock.json index 31681c4e4f..1a7f65e4fb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -17,7 +17,7 @@ "@ant-design/colors": { "version": "3.2.2", "resolved": "https://registry.npmjs.org/@ant-design/colors/-/colors-3.2.2.tgz", - "integrity": "sha1-WtQ9YZ6RHzSI66wwPWBuZqhCOQM=", + "integrity": "sha512-YKgNbG2dlzqMhA9NtI3/pbY16m3Yl/EeWBRa+lB1X1YaYxHrxNexiQYCLTWO/uDvAjLFMEDU+zR901waBtMtjQ==", "requires": { "tinycolor2": "^1.4.1" } @@ -4153,7 +4153,7 @@ "array-tree-filter": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/array-tree-filter/-/array-tree-filter-2.1.0.tgz", - "integrity": "sha1-hzrAD+yDdJ8lWsjdCDgUtPYykZA=" + "integrity": "sha512-4ROwICNlNw/Hqa9v+rk5h22KjmzB1JGTMVKP2AKJBOCgb0yL0ASf0+YvCcLNNwquOHNX48jkeZIJ3a+oOQqKcw==" }, "array-union": { "version": "1.0.2", @@ -4394,6 +4394,11 @@ } } }, + "axios-auth-refresh": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/axios-auth-refresh/-/axios-auth-refresh-3.0.0.tgz", + "integrity": "sha512-0XJnJY711f7opdT+b/au/xw1g4MYrjntXB8Oy5l48plbzOWLjUtJ+m8CtiNLgN3MAvGFJ/Q1NtQ7WKf2euKu6g==" + }, "axobject-query": { "version": "2.1.1", "resolved": "https://registry.npmjs.org/axobject-query/-/axobject-query-2.1.1.tgz", @@ -6018,12 +6023,6 @@ "monotone-convex-hull-2d": "^1.0.1" } }, - "cookie": { - "version": "0.3.1", - "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.3.1.tgz", - "integrity": "sha1-5+Ch+e9DtMi6klxcWpboBtFoc7s=", - "dev": true - }, "cookie-signature": { "version": "1.0.6", "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz", @@ -6053,7 +6052,7 @@ "copy-to-clipboard": { "version": "3.3.1", "resolved": "https://registry.npmjs.org/copy-to-clipboard/-/copy-to-clipboard-3.3.1.tgz", - "integrity": "sha1-EVqhqZmP+rYZb5MHatbaO5E2Yq4=", + "integrity": "sha512-i13qo6kIHTTpCm8/Wup+0b1mVWETvu2kIMzKoK8FpkLkFxlt0znUAHcMzox+T8sPlqtZXq3CulEjQHsYiGFJUw==", "requires": { "toggle-selection": "^1.0.6" } @@ -6927,7 +6926,7 @@ "dom-align": { "version": "1.12.0", "resolved": "https://registry.npmjs.org/dom-align/-/dom-align-1.12.0.tgz", - "integrity": "sha1-VvtxVt8LkQmYMDZNLUj4iWP1opw=" + "integrity": "sha512-YkoezQuhp3SLFGdOlr5xkqZ640iXrnHAwVYcDg8ZKRUtO7mSzSC2BA5V0VuyAwPSJA4CLIc6EDDJh4bEsD2+zA==" }, "dom-converter": { "version": "0.2.0", @@ -8434,6 +8433,12 @@ "vary": "~1.1.2" }, "dependencies": { + "cookie": { + "version": "0.3.1", + "resolved": "https://registry.npmjs.org/cookie/-/cookie-0.3.1.tgz", + "integrity": "sha1-5+Ch+e9DtMi6klxcWpboBtFoc7s=", + "dev": true + }, "debug": { "version": "2.6.9", "resolved": "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz", @@ -12142,11 +12147,6 @@ "integrity": "sha512-M7kLczedRMYX4L8Mdh4MzyAMM9O5osx+4FcOQuTvr3A9F2D9S5JXheN0ewNbrvK2UatkTRhL5ejGmGSjNMiZuw==", "dev": true }, - "js-cookie": { - "version": "2.2.1", - "resolved": "https://registry.npmjs.org/js-cookie/-/js-cookie-2.2.1.tgz", - "integrity": "sha512-HvdH2LzI/EAZcUwA8+0nKNtWHqS+ZmijLA30RwZA0bo7ToCckjK5MkGhjED9KoRcXO6BaGI3I9UIzSA1FKFPOQ==" - }, "js-tokens": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/js-tokens/-/js-tokens-4.0.0.tgz", @@ -16839,7 +16839,7 @@ "resize-observer-polyfill": { "version": "1.5.1", "resolved": "https://registry.npmjs.org/resize-observer-polyfill/-/resize-observer-polyfill-1.5.1.tgz", - "integrity": "sha1-DpAg3T0hAkRY1OvSfiPkAmmBBGQ=" + "integrity": "sha512-LwZrotdHOo12nQuZlHEmtuXdqGoOD0OhaxopaNFxWzInpEgaLWoVuAMbTzixuosCx2nEG58ngzW3vxdWoxIgdg==" }, "resolve": { "version": "1.10.0", @@ -17327,7 +17327,7 @@ "shallowequal": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/shallowequal/-/shallowequal-1.1.0.tgz", - "integrity": "sha1-GI1SHelbkIdAT9TctosT3wrk5/g=" + "integrity": "sha512-y0m1JoUZSlPAjXVtPPW70aZWfIL/dSP7AFkRnniLCrK/8MDKog3TySTBmckD+RObVxH0v4Tox67+F14PdED2oQ==" }, "sharkdown": { "version": "0.1.1", @@ -19946,7 +19946,7 @@ "warning": { "version": "4.0.3", "resolved": "https://registry.npmjs.org/warning/-/warning-4.0.3.tgz", - "integrity": "sha1-Fungd+uKhtavfWSqHgX9hbRnjKM=", + "integrity": "sha512-rpJyN222KWIvHJ/F53XSZv0Zl/accqHR8et1kpaMTD/fLCRxtV8iX8czMzY7sVZupTI3zcUTg8eycS2kNF9l6w==", "requires": { "loose-envify": "^1.0.0" } diff --git a/package.json b/package.json index bea953f4c1..73c78460f8 100644 --- a/package.json +++ b/package.json @@ -50,6 +50,7 @@ "ace-builds": "^1.4.12", "antd": "^4.4.3", "axios": "^0.19.0", + "axios-auth-refresh": "^3.0.0", "bootstrap": "^3.3.7", "classnames": "^2.2.6", "d3": "^3.5.17", @@ -58,7 +59,6 @@ "font-awesome": "^4.7.0", "history": "^4.10.1", "hoist-non-react-statics": "^3.3.0", - "js-cookie": "^2.2.1", "lodash": "^4.17.10", "markdown": "0.5.0", "material-design-iconic-font": "^2.2.0", diff --git a/redash/security.py b/redash/security.py index 3d21c8e463..d5ebaee547 100644 --- a/redash/security.py +++ b/redash/security.py @@ -27,6 +27,7 @@ def init_app(app): csrf.init_app(app) app.config["WTF_CSRF_CHECK_DEFAULT"] = False app.config["WTF_CSRF_SSL_STRICT"] = False + app.config["WTF_CSRF_TIME_LIMIT"] = settings.CSRF_TIME_LIMIT @app.after_request def inject_csrf_token(response): diff --git a/redash/settings/__init__.py b/redash/settings/__init__.py index 2aca85b3b7..dd6b41e5f0 100644 --- a/redash/settings/__init__.py +++ b/redash/settings/__init__.py @@ -505,4 +505,6 @@ def email_server_is_configured(): # This is turned off by default to avoid breaking any existing deployments but it is highly recommended to turn this toggle on to prevent CSRF attacks. ENFORCE_CSRF = parse_boolean( os.environ.get("REDASH_ENFORCE_CSRF", "false") -) \ No newline at end of file +) + +CSRF_TIME_LIMIT = int(os.environ.get("REDASH_CSRF_TIME_LIMIT", 3600 * 6)) \ No newline at end of file From aa5d4f5f4e63d116b55a230aac8ca9cf62706c70 Mon Sep 17 00:00:00 2001 From: Omer Lachish Date: Wed, 23 Sep 2020 12:54:48 +0300 Subject: [PATCH 4/6] add 'cancelled' meta directive to all cancelled jobs (#5187) --- redash/tasks/worker.py | 6 ++---- tests/handlers/test_query_results.py | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/redash/tasks/worker.py b/redash/tasks/worker.py index 19aadabfef..2fc67415a5 100644 --- a/redash/tasks/worker.py +++ b/redash/tasks/worker.py @@ -11,10 +11,8 @@ class CancellableJob(BaseJob): def cancel(self, pipeline=None): - # TODO - add tests that verify that queued jobs are removed from queue and running jobs are actively cancelled - if self.is_started: - self.meta["cancelled"] = True - self.save_meta() + self.meta["cancelled"] = True + self.save_meta() super().cancel(pipeline=pipeline) diff --git a/tests/handlers/test_query_results.py b/tests/handlers/test_query_results.py index 8e8bd24b76..c1f97df030 100644 --- a/tests/handlers/test_query_results.py +++ b/tests/handlers/test_query_results.py @@ -487,3 +487,23 @@ def test_renders_excel_file_when_rows_have_missing_columns(self): is_json=False, ) self.assertEqual(rv.status_code, 200) + + +class TestJobResource(BaseTestCase): + def test_cancels_queued_queries(self): + QUEUED = 1 + FAILED = 4 + + query = self.factory.create_query() + job_id = self.make_request( + "post", f"/api/queries/{query.id}/results", data={"parameters": {}}, + ).json["job"]["id"] + + status = self.make_request("get", f"/api/jobs/{job_id}").json["job"]["status"] + self.assertEqual(status, QUEUED) + + self.make_request("delete", f"/api/jobs/{job_id}") + + job = self.make_request("get", f"/api/jobs/{job_id}").json["job"] + self.assertEqual(job["status"], FAILED) + self.assertTrue("cancelled" in job["error"]) \ No newline at end of file From 210008c714c8bbae28fcb73fbc2e95ec26363e0b Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Wed, 23 Sep 2020 16:30:08 +0300 Subject: [PATCH 5/6] Ask user to log in when session expires (#5178) * Ask user to log in when session expires * Update implementation * Update implementation * Minor fix * Update modal * Do not intercept calls to api/session as Auth.requireSession() relies on it * Refine code; adjust popup size and position --- client/app/services/auth.js | 5 ++ client/app/services/axios.js | 36 +++++++--- client/app/services/restoreSession.jsx | 91 ++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 client/app/services/restoreSession.jsx diff --git a/client/app/services/auth.js b/client/app/services/auth.js index db5e2af18c..3901426dce 100644 --- a/client/app/services/auth.js +++ b/client/app/services/auth.js @@ -2,6 +2,7 @@ import debug from "debug"; import { includes, extend } from "lodash"; import location from "@/services/location"; import { axios } from "@/services/axios"; +import { notifySessionRestored } from "@/services/restoreSession"; export const currentUser = { canEdit(object) { @@ -46,6 +47,9 @@ export const Auth = { isAuthenticated() { return session.loaded && session.user.id; }, + getLoginUrl() { + return AuthUrls.Login; + }, setLoginUrl(loginUrl) { AuthUrls.Login = loginUrl; }, @@ -94,6 +98,7 @@ export const Auth = { .then(() => { if (Auth.isAuthenticated()) { logger("Loaded session"); + notifySessionRestored(); return session; } logger("Need to login, redirecting"); diff --git a/client/app/services/axios.js b/client/app/services/axios.js index e50a0e9ded..1d866c4210 100644 --- a/client/app/services/axios.js +++ b/client/app/services/axios.js @@ -1,7 +1,9 @@ +import { get, includes } from "lodash"; import axiosLib from "axios"; +import createAuthRefreshInterceptor from "axios-auth-refresh"; import { Auth } from "@/services/auth"; import qs from "query-string"; -import createAuthRefreshInterceptor from "axios-auth-refresh"; +import { restoreSession } from "@/services/restoreSession"; export const axios = axiosLib.create({ paramsSerializer: params => qs.stringify(params), @@ -9,23 +11,39 @@ export const axios = axiosLib.create({ xsrfHeaderName: "X-CSRF-TOKEN", }); -const getData = ({ data }) => data; - -axios.interceptors.response.use(getData); +axios.interceptors.response.use(response => response.data); -export const authRefreshInterceptor = createAuthRefreshInterceptor( +export const csrfRefreshInterceptor = createAuthRefreshInterceptor( axios, - failedRequest => { - const message = failedRequest.response.data.message || ""; - if (message.includes("CSRF")) { + error => { + const message = get(error, "response.data.message"); + if (error.isAxiosError && includes(message, "CSRF")) { return axios.get("/ping"); } else { - return Promise.reject(failedRequest); + return Promise.reject(error); } }, { statusCodes: [400] } ); +export const sessionRefreshInterceptor = createAuthRefreshInterceptor( + axios, + error => { + const status = parseInt(get(error, "response.status")); + const message = get(error, "response.data.message"); + // TODO: In axios@0.9.1 this check could be replaced with { skipAuthRefresh: true } flag. See axios-auth-refresh docs + const requestUrl = get(error, "config.url"); + if (error.isAxiosError && (status === 401 || includes(message, "Please login")) && requestUrl !== "api/session") { + return restoreSession(); + } + return Promise.reject(error); + }, + { + statusCodes: [401, 404], + pauseInstanceWhileRefreshing: false, // According to docs, `false` is default value, but in fact it's not :-) + } +); + axios.interceptors.request.use(config => { const apiKey = Auth.getApiKey(); if (apiKey) { diff --git a/client/app/services/restoreSession.jsx b/client/app/services/restoreSession.jsx new file mode 100644 index 0000000000..09f36e1132 --- /dev/null +++ b/client/app/services/restoreSession.jsx @@ -0,0 +1,91 @@ +import { map } from "lodash"; +import React from "react"; +import Modal from "antd/lib/modal"; +import { Auth } from "@/services/auth"; + +const SESSION_RESTORED_MESSAGE = "redash_session_restored"; + +export function notifySessionRestored() { + if (window.opener) { + window.opener.postMessage({ type: SESSION_RESTORED_MESSAGE }, window.location.origin); + } +} + +function getPopupPosition(width, height) { + const windowLeft = window.screenX; + const windowTop = window.screenY; + const windowWidth = window.innerWidth; + const windowHeight = window.innerHeight; + + return { + left: Math.floor((windowWidth - width) / 2 + windowLeft), + top: Math.floor((windowHeight - height) / 2 + windowTop), + width: Math.floor(width), + height: Math.floor(height), + }; +} + +function showRestoreSessionPrompt(loginUrl, onSuccess) { + let popup = null; + + Modal.warning({ + content: "Your session has expired. Please login to continue.", + okText: ( + + + Login + + ), + centered: true, + mask: true, + maskClosable: false, + keyboard: false, + onOk: closeModal => { + if (popup && !popup.closed) { + popup.focus(); + return; // popup already shown + } + + const popupOptions = { + ...getPopupPosition(640, 640), + menubar: "no", + toolbar: "no", + location: "yes", + resizable: "yes", + scrollbars: "yes", + status: "yes", + }; + + popup = window.open(loginUrl, "Restore Session", map(popupOptions, (value, key) => `${key}=${value}`).join(",")); + + const handlePostMessage = event => { + if (event.data.type === SESSION_RESTORED_MESSAGE) { + if (popup) { + popup.close(); + } + popup = null; + window.removeEventListener("message", handlePostMessage); + closeModal(); + onSuccess(); + } + }; + + window.addEventListener("message", handlePostMessage, false); + }, + }); +} + +let restoreSessionPromise = null; + +export function restoreSession() { + if (!restoreSessionPromise) { + restoreSessionPromise = new Promise(resolve => { + showRestoreSessionPrompt(Auth.getLoginUrl(), () => { + restoreSessionPromise = null; + resolve(); + }); + }); + } + + return restoreSessionPromise; +} From a473611cb0a34e471f3fa09841c675930a205648 Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Thu, 24 Sep 2020 14:39:09 +0300 Subject: [PATCH 6/6] Some Choropleth improvements/refactoring (#5186) * Directly map query results column to GeoJSON property * Use cache for geoJson requests * Don't handle bounds changes while loading geoJson data * Choropleth: fix map "jumping" on load; don't save bounds if user didn't edit them; refine code a bit * Improve cache * Optimize Japan Perfectures map (remove irrelevant GeoJson properties) * Improve getOptions for Choropleth; remove unused code * Fix test * Add US states map * Convert USA map to Albers projection * Allow to specify user-friendly field names for maps --- .../visualizationComponents.jsx | 31 +++++ .../visualizations/choropleth_spec.js | 6 +- viz-lib/src/lib/referenceCountingCache.js | 39 ++++++ .../choropleth/Editor/BoundsSettings.jsx | 50 +++++-- .../choropleth/Editor/ColorsSettings.jsx | 8 +- .../choropleth/Editor/FormatSettings.jsx | 90 ++++++------- .../choropleth/Editor/GeneralSettings.jsx | 125 +++++++++--------- .../visualizations/choropleth/Editor/utils.js | 58 ++++---- .../choropleth/Renderer/index.jsx | 42 ++---- .../choropleth/Renderer/initChoropleth.js | 55 ++++---- .../choropleth/Renderer/utils.js | 22 +-- .../visualizations/choropleth/getOptions.js | 36 ++++- .../choropleth/hooks/useLoadGeoJson.js | 39 ++++++ .../choropleth/maps/convert-projection.js | 40 ++++++ .../maps/japan.prefectures.geo.json | 55 +------- .../choropleth/maps/usa-albers.geo.json | 1 + .../choropleth/maps/usa.geo.json | 1 + 17 files changed, 407 insertions(+), 291 deletions(-) create mode 100644 viz-lib/src/lib/referenceCountingCache.js create mode 100644 viz-lib/src/visualizations/choropleth/hooks/useLoadGeoJson.js create mode 100644 viz-lib/src/visualizations/choropleth/maps/convert-projection.js create mode 100644 viz-lib/src/visualizations/choropleth/maps/usa-albers.geo.json create mode 100644 viz-lib/src/visualizations/choropleth/maps/usa.geo.json diff --git a/client/app/components/visualizations/visualizationComponents.jsx b/client/app/components/visualizations/visualizationComponents.jsx index cdaf2d5166..5eb05bed0c 100644 --- a/client/app/components/visualizations/visualizationComponents.jsx +++ b/client/app/components/visualizations/visualizationComponents.jsx @@ -6,6 +6,7 @@ import { Renderer as VisRenderer, Editor as VisEditor, updateVisualizationsSetti import { clientConfig } from "@/services/auth"; import countriesDataUrl from "@redash/viz/lib/visualizations/choropleth/maps/countries.geo.json"; +import usaDataUrl from "@redash/viz/lib/visualizations/choropleth/maps/usa-albers.geo.json"; import subdivJapanDataUrl from "@redash/viz/lib/visualizations/choropleth/maps/japan.prefectures.geo.json"; function wrapComponentWithSettings(WrappedComponent) { @@ -17,10 +18,40 @@ function wrapComponentWithSettings(WrappedComponent) { countries: { name: "Countries", url: countriesDataUrl, + fieldNames: { + name: "Short name", + name_long: "Full name", + abbrev: "Abbreviated name", + iso_a2: "ISO code (2 letters)", + iso_a3: "ISO code (3 letters)", + iso_n3: "ISO code (3 digits)", + }, + }, + usa: { + name: "USA", + url: usaDataUrl, + fieldNames: { + name: "Name", + ns_code: "National Standard ANSI Code (8-character)", + geoid: "Geographic ID", + usps_abbrev: "USPS Abbreviation", + fips_code: "FIPS Code (2-character)", + }, }, subdiv_japan: { name: "Japan/Prefectures", url: subdivJapanDataUrl, + fieldNames: { + name: "Name", + name_alt: "Name (alternative)", + name_local: "Name (local)", + iso_3166_2: "ISO-3166-2", + postal: "Postal Code", + type: "Type", + type_en: "Type (EN)", + region: "Region", + region_code: "Region Code", + }, }, }, ...pick(clientConfig, [ diff --git a/client/cypress/integration/visualizations/choropleth_spec.js b/client/cypress/integration/visualizations/choropleth_spec.js index b8d0003515..bb4446f5b0 100644 --- a/client/cypress/integration/visualizations/choropleth_spec.js +++ b/client/cypress/integration/visualizations/choropleth_spec.js @@ -48,11 +48,11 @@ describe("Choropleth", () => { cy.clickThrough(` VisualizationEditor.Tabs.General Choropleth.Editor.MapType - Choropleth.Editor.MapType.Countries + Choropleth.Editor.MapType.countries Choropleth.Editor.KeyColumn Choropleth.Editor.KeyColumn.name - Choropleth.Editor.KeyType - Choropleth.Editor.KeyType.name + Choropleth.Editor.TargetField + Choropleth.Editor.TargetField.name Choropleth.Editor.ValueColumn Choropleth.Editor.ValueColumn.value `); diff --git a/viz-lib/src/lib/referenceCountingCache.js b/viz-lib/src/lib/referenceCountingCache.js new file mode 100644 index 0000000000..35ff4721d0 --- /dev/null +++ b/viz-lib/src/lib/referenceCountingCache.js @@ -0,0 +1,39 @@ +import { each, debounce } from "lodash"; + +export default function createReferenceCountingCache({ cleanupDelay = 2000 } = {}) { + const items = {}; + + const cleanup = debounce(() => { + each(items, (item, key) => { + if (item.refCount <= 0) { + delete items[key]; + } + }); + }, cleanupDelay); + + function get(key, getter) { + if (!items[key]) { + items[key] = { + value: getter(), + refCount: 0, + }; + } + const item = items[key]; + item.refCount += 1; + return item.value; + } + + function release(key) { + if (items[key]) { + const item = items[key]; + if (item.refCount > 0) { + item.refCount -= 1; + if (item.refCount <= 0) { + cleanup(); + } + } + } + } + + return { get, release }; +} diff --git a/viz-lib/src/visualizations/choropleth/Editor/BoundsSettings.jsx b/viz-lib/src/visualizations/choropleth/Editor/BoundsSettings.jsx index 760103dc3b..e217c83672 100644 --- a/viz-lib/src/visualizations/choropleth/Editor/BoundsSettings.jsx +++ b/viz-lib/src/visualizations/choropleth/Editor/BoundsSettings.jsx @@ -1,10 +1,13 @@ -import { isFinite, cloneDeep } from "lodash"; +import { isArray, isFinite, cloneDeep } from "lodash"; import React, { useState, useEffect, useCallback } from "react"; import { useDebouncedCallback } from "use-debounce"; import * as Grid from "antd/lib/grid"; import { Section, InputNumber, ControlLabel } from "@/components/visualizations/editor"; import { EditorPropTypes } from "@/visualizations/prop-types"; +import useLoadGeoJson from "../hooks/useLoadGeoJson"; +import { getGeoJsonBounds } from "./utils"; + export default function BoundsSettings({ options, onOptionsChange }) { // Bounds may be changed in editor or on preview (by drag/zoom map). // Changes from preview does not come frequently (only when user release mouse button), @@ -16,9 +19,20 @@ export default function BoundsSettings({ options, onOptionsChange }) { const [bounds, setBounds] = useState(options.bounds); const [onOptionsChangeDebounced] = useDebouncedCallback(onOptionsChange, 200); + const [geoJson] = useLoadGeoJson(options.mapType); + + // `options.bounds` could be empty only if user didn't edit bounds yet - through preview or in this editor. + // In this case we should keep empty bounds value because it tells renderer to fit map every time. useEffect(() => { - setBounds(options.bounds); - }, [options.bounds]); + if (options.bounds) { + setBounds(options.bounds); + } else { + const defaultBounds = getGeoJsonBounds(geoJson); + if (defaultBounds) { + setBounds(defaultBounds); + } + } + }, [options.bounds, geoJson]); const updateBounds = useCallback( (i, j, v) => { @@ -33,29 +47,47 @@ export default function BoundsSettings({ options, onOptionsChange }) { [bounds, onOptionsChangeDebounced] ); + const boundsAvailable = isArray(bounds); + return (
- + - updateBounds(1, 0, value)} /> + updateBounds(1, 0, value)} + /> - updateBounds(1, 1, value)} /> + updateBounds(1, 1, value)} + />
- + - updateBounds(0, 0, value)} /> + updateBounds(0, 0, value)} + /> - updateBounds(0, 1, value)} /> + updateBounds(0, 1, value)} + /> diff --git a/viz-lib/src/visualizations/choropleth/Editor/ColorsSettings.jsx b/viz-lib/src/visualizations/choropleth/Editor/ColorsSettings.jsx index 62310e8d1d..4984252de4 100644 --- a/viz-lib/src/visualizations/choropleth/Editor/ColorsSettings.jsx +++ b/viz-lib/src/visualizations/choropleth/Editor/ColorsSettings.jsx @@ -12,7 +12,7 @@ export default function ColorsSettings({ options, onOptionsChange }) {
- Value format + Value Format } @@ -86,7 +78,7 @@ export default function GeneralSettings({ options, onOptionsChange }) { onOptionsChangeDebounced({ noValuePlaceholder: event.target.value })} @@ -100,7 +92,7 @@ export default function GeneralSettings({ options, onOptionsChange }) { data-test="Choropleth.Editor.LegendVisibility" checked={options.legend.visible} onChange={event => onOptionsChange({ legend: { visible: event.target.checked } })}> - Show legend + Show Legend
@@ -108,7 +100,7 @@ export default function GeneralSettings({ options, onOptionsChange }) { Tooltip template {templateFormatHint}} + label={Tooltip Template {templateFormatHint}} data-test="Choropleth.Editor.TooltipTemplate" disabled={!options.tooltip.enabled} defaultValue={options.tooltip.template} @@ -163,13 +155,13 @@ export default function GeneralSettings({ options, onOptionsChange }) { data-test="Choropleth.Editor.PopupEnabled" checked={options.popup.enabled} onChange={event => onOptionsChange({ popup: { enabled: event.target.checked } })}> - Show popup + Show Popup