From 35e41385dc57150bc18fa3ad3661ebffd72ea21e Mon Sep 17 00:00:00 2001 From: Levko Kravets Date: Thu, 23 Jan 2020 17:03:37 +0200 Subject: [PATCH] Fixes several bugs on dashboard page (see description) (#4571) * Move each hook to own file; move hooks and components to own folders * Update URL and timer only when refresh rate changes * Skip dashboard refresh if previous refresh is still running * Fix test --- client/app/pages/dashboards/DashboardList.jsx | 2 +- client/app/pages/dashboards/DashboardPage.jsx | 2 +- .../pages/dashboards/PublicDashboardPage.jsx | 2 +- .../DashboardListEmptyState.jsx | 0 .../{ => components}/ShareDashboardDialog.jsx | 0 .../dashboards/{ => hooks}/useDashboard.js | 167 ++---------------- .../dashboards/hooks/useEditModeHandler.js | 102 +++++++++++ .../dashboards/hooks/useFullscreenHandler.js | 14 ++ .../dashboards/hooks/useRefreshRateHandler.js | 40 +++++ .../components/QueryExecutionStatus.jsx | 1 + .../integration/query/parameter_spec.js | 2 +- 11 files changed, 172 insertions(+), 160 deletions(-) rename client/app/pages/dashboards/{ => components}/DashboardListEmptyState.jsx (100%) rename client/app/pages/dashboards/{ => components}/ShareDashboardDialog.jsx (100%) rename client/app/pages/dashboards/{ => hooks}/useDashboard.js (62%) create mode 100644 client/app/pages/dashboards/hooks/useEditModeHandler.js create mode 100644 client/app/pages/dashboards/hooks/useFullscreenHandler.js create mode 100644 client/app/pages/dashboards/hooks/useRefreshRateHandler.js diff --git a/client/app/pages/dashboards/DashboardList.jsx b/client/app/pages/dashboards/DashboardList.jsx index 5c84d7f59..a8937009e 100644 --- a/client/app/pages/dashboards/DashboardList.jsx +++ b/client/app/pages/dashboards/DashboardList.jsx @@ -17,7 +17,7 @@ import Layout from "@/components/layouts/ContentWithSidebar"; import { Dashboard } from "@/services/dashboard"; -import DashboardListEmptyState from "./DashboardListEmptyState"; +import DashboardListEmptyState from "./components/DashboardListEmptyState"; import "./dashboard-list.css"; diff --git a/client/app/pages/dashboards/DashboardPage.jsx b/client/app/pages/dashboards/DashboardPage.jsx index e6f9640d4..72d0fbcd5 100644 --- a/client/app/pages/dashboards/DashboardPage.jsx +++ b/client/app/pages/dashboards/DashboardPage.jsx @@ -23,7 +23,7 @@ import getTags from "@/services/getTags"; import { clientConfig } from "@/services/auth"; import { policy } from "@/services/policy"; import { durationHumanize } from "@/lib/utils"; -import useDashboard, { DashboardStatusEnum } from "./useDashboard"; +import useDashboard, { DashboardStatusEnum } from "./hooks/useDashboard"; import "./DashboardPage.less"; diff --git a/client/app/pages/dashboards/PublicDashboardPage.jsx b/client/app/pages/dashboards/PublicDashboardPage.jsx index d1dc41b76..fa37c138d 100644 --- a/client/app/pages/dashboards/PublicDashboardPage.jsx +++ b/client/app/pages/dashboards/PublicDashboardPage.jsx @@ -10,7 +10,7 @@ import Filters from "@/components/Filters"; import { ErrorBoundaryContext } from "@/components/ErrorBoundary"; import { Dashboard } from "@/services/dashboard"; import logoUrl from "@/assets/images/redash_icon_small.png"; -import useDashboard from "./useDashboard"; +import useDashboard from "./hooks/useDashboard"; import "./PublicDashboardPage.less"; diff --git a/client/app/pages/dashboards/DashboardListEmptyState.jsx b/client/app/pages/dashboards/components/DashboardListEmptyState.jsx similarity index 100% rename from client/app/pages/dashboards/DashboardListEmptyState.jsx rename to client/app/pages/dashboards/components/DashboardListEmptyState.jsx diff --git a/client/app/pages/dashboards/ShareDashboardDialog.jsx b/client/app/pages/dashboards/components/ShareDashboardDialog.jsx similarity index 100% rename from client/app/pages/dashboards/ShareDashboardDialog.jsx rename to client/app/pages/dashboards/components/ShareDashboardDialog.jsx diff --git a/client/app/pages/dashboards/useDashboard.js b/client/app/pages/dashboards/hooks/useDashboard.js similarity index 62% rename from client/app/pages/dashboards/useDashboard.js rename to client/app/pages/dashboards/hooks/useDashboard.js index d4aa9909e..e6b46d906 100644 --- a/client/app/pages/dashboards/useDashboard.js +++ b/client/app/pages/dashboards/hooks/useDashboard.js @@ -1,40 +1,20 @@ import { useState, useEffect, useMemo, useCallback } from "react"; -import { - isEmpty, - isNaN, - includes, - compact, - map, - has, - pick, - keys, - extend, - every, - find, - debounce, - isMatch, - pickBy, - max, - min, - get, -} from "lodash"; +import { isEmpty, includes, compact, map, has, pick, keys, extend, every, get } from "lodash"; import notification from "@/services/notification"; import location from "@/services/location"; import { Dashboard, collectDashboardFilters } from "@/services/dashboard"; import { currentUser } from "@/services/auth"; import recordEvent from "@/services/recordEvent"; -import { policy } from "@/services/policy"; import AddWidgetDialog from "@/components/dashboards/AddWidgetDialog"; import TextboxDialog from "@/components/dashboards/TextboxDialog"; import PermissionsEditorDialog from "@/components/PermissionsEditorDialog"; import { editableMappingsToParameterMappings, synchronizeWidgetTitles } from "@/components/ParameterMappingInput"; -import ShareDashboardDialog from "./ShareDashboardDialog"; +import ShareDashboardDialog from "../components/ShareDashboardDialog"; +import useFullscreenHandler from "./useFullscreenHandler"; +import useRefreshRateHandler from "./useRefreshRateHandler"; +import useEditModeHandler from "./useEditModeHandler"; -export const DashboardStatusEnum = { - SAVED: "saved", - SAVING: "saving", - SAVING_FAILED: "saving_failed", -}; +export { DashboardStatusEnum } from "./useEditModeHandler"; function getAffectedWidgets(widgets, updatedParameters = []) { return !isEmpty(updatedParameters) @@ -51,133 +31,6 @@ function getAffectedWidgets(widgets, updatedParameters = []) { : widgets; } -function getChangedPositions(widgets, nextPositions = {}) { - return pickBy(nextPositions, (nextPos, widgetId) => { - const widget = find(widgets, { id: Number(widgetId) }); - const prevPos = widget.options.position; - return !isMatch(prevPos, nextPos); - }); -} - -function getLimitedRefreshRate(refreshRate) { - const allowedIntervals = policy.getDashboardRefreshIntervals(); - return max([30, min(allowedIntervals), refreshRate]); -} - -function getRefreshRateFromUrl() { - const refreshRate = parseFloat(location.search.refresh); - return isNaN(refreshRate) ? null : getLimitedRefreshRate(refreshRate); -} - -function useFullscreenHandler() { - const [fullscreen, setFullscreen] = useState(has(location.search, "fullscreen")); - useEffect(() => { - document.body.classList.toggle("headless", fullscreen); - location.setSearch({ fullscreen: fullscreen ? true : null }, true); - }, [fullscreen]); - - const toggleFullscreen = () => setFullscreen(!fullscreen); - return [fullscreen, toggleFullscreen]; -} - -function useRefreshRateHandler(refreshDashboard) { - const [refreshRate, setRefreshRate] = useState(getRefreshRateFromUrl()); - - useEffect(() => { - location.setSearch({ refresh: refreshRate || null }, true); - if (refreshRate) { - const refreshTimer = setInterval(refreshDashboard, refreshRate * 1000); - return () => clearInterval(refreshTimer); - } - }, [refreshDashboard, refreshRate]); - - return [refreshRate, rate => setRefreshRate(getLimitedRefreshRate(rate)), () => setRefreshRate(null)]; -} - -function useEditModeHandler(canEditDashboard, widgets) { - const [editingLayout, setEditingLayout] = useState(canEditDashboard && has(location.search, "edit")); - const [dashboardStatus, setDashboardStatus] = useState(DashboardStatusEnum.SAVED); - const [recentPositions, setRecentPositions] = useState([]); - const [doneBtnClickedWhileSaving, setDoneBtnClickedWhileSaving] = useState(false); - - useEffect(() => { - location.setSearch({ edit: editingLayout ? true : null }, true); - }, [editingLayout]); - - useEffect(() => { - if (doneBtnClickedWhileSaving && dashboardStatus === DashboardStatusEnum.SAVED) { - setDoneBtnClickedWhileSaving(false); - setEditingLayout(false); - } - }, [doneBtnClickedWhileSaving, dashboardStatus]); - - const saveDashboardLayout = useCallback( - positions => { - if (!canEditDashboard) { - setDashboardStatus(DashboardStatusEnum.SAVED); - return; - } - - const changedPositions = getChangedPositions(widgets, positions); - - setDashboardStatus(DashboardStatusEnum.SAVING); - setRecentPositions(positions); - const saveChangedWidgets = map(changedPositions, (position, id) => { - // find widget - const widget = find(widgets, { id: Number(id) }); - - // skip already deleted widget - if (!widget) { - return Promise.resolve(); - } - - return widget.save("options", { position }); - }); - - return Promise.all(saveChangedWidgets) - .then(() => setDashboardStatus(DashboardStatusEnum.SAVED)) - .catch(() => { - setDashboardStatus(DashboardStatusEnum.SAVING_FAILED); - notification.error("Error saving changes."); - }); - }, - [canEditDashboard, widgets] - ); - - const saveDashboardLayoutDebounced = useCallback( - (...args) => { - setDashboardStatus(DashboardStatusEnum.SAVING); - return debounce(() => saveDashboardLayout(...args), 2000)(); - }, - [saveDashboardLayout] - ); - - const retrySaveDashboardLayout = useCallback(() => saveDashboardLayout(recentPositions), [ - recentPositions, - saveDashboardLayout, - ]); - - const setEditing = useCallback( - editing => { - if (!editing && dashboardStatus !== DashboardStatusEnum.SAVED) { - setDoneBtnClickedWhileSaving(true); - return; - } - setEditingLayout(canEditDashboard && editing); - }, - [dashboardStatus, canEditDashboard] - ); - - return { - editingLayout: canEditDashboard && editingLayout, - setEditingLayout: setEditing, - saveDashboardLayout: editingLayout ? saveDashboardLayoutDebounced : saveDashboardLayout, - retrySaveDashboardLayout, - doneBtnClickedWhileSaving, - dashboardStatus, - }; -} - function useDashboard(dashboardData) { const [dashboard, setDashboard] = useState(dashboardData); const [filters, setFilters] = useState([]); @@ -272,10 +125,12 @@ function useDashboard(dashboardData) { const refreshDashboard = useCallback( updatedParameters => { - setRefreshing(true); - loadDashboard(true, updatedParameters).finally(() => setRefreshing(false)); + if (!refreshing) { + setRefreshing(true); + loadDashboard(true, updatedParameters).finally(() => setRefreshing(false)); + } }, - [loadDashboard] + [refreshing, loadDashboard] ); const archiveDashboard = useCallback(() => { diff --git a/client/app/pages/dashboards/hooks/useEditModeHandler.js b/client/app/pages/dashboards/hooks/useEditModeHandler.js new file mode 100644 index 000000000..8d9dcce6b --- /dev/null +++ b/client/app/pages/dashboards/hooks/useEditModeHandler.js @@ -0,0 +1,102 @@ +import { debounce, find, has, isMatch, map, pickBy } from "lodash"; +import { useCallback, useEffect, useState } from "react"; +import location from "@/services/location"; +import notification from "@/services/notification"; + +export const DashboardStatusEnum = { + SAVED: "saved", + SAVING: "saving", + SAVING_FAILED: "saving_failed", +}; + +function getChangedPositions(widgets, nextPositions = {}) { + return pickBy(nextPositions, (nextPos, widgetId) => { + const widget = find(widgets, { id: Number(widgetId) }); + const prevPos = widget.options.position; + return !isMatch(prevPos, nextPos); + }); +} + +export default function useEditModeHandler(canEditDashboard, widgets) { + const [editingLayout, setEditingLayout] = useState(canEditDashboard && has(location.search, "edit")); + const [dashboardStatus, setDashboardStatus] = useState(DashboardStatusEnum.SAVED); + const [recentPositions, setRecentPositions] = useState([]); + const [doneBtnClickedWhileSaving, setDoneBtnClickedWhileSaving] = useState(false); + + useEffect(() => { + location.setSearch({ edit: editingLayout ? true : null }, true); + }, [editingLayout]); + + useEffect(() => { + if (doneBtnClickedWhileSaving && dashboardStatus === DashboardStatusEnum.SAVED) { + setDoneBtnClickedWhileSaving(false); + setEditingLayout(false); + } + }, [doneBtnClickedWhileSaving, dashboardStatus]); + + const saveDashboardLayout = useCallback( + positions => { + if (!canEditDashboard) { + setDashboardStatus(DashboardStatusEnum.SAVED); + return; + } + + const changedPositions = getChangedPositions(widgets, positions); + + setDashboardStatus(DashboardStatusEnum.SAVING); + setRecentPositions(positions); + const saveChangedWidgets = map(changedPositions, (position, id) => { + // find widget + const widget = find(widgets, { id: Number(id) }); + + // skip already deleted widget + if (!widget) { + return Promise.resolve(); + } + + return widget.save("options", { position }); + }); + + return Promise.all(saveChangedWidgets) + .then(() => setDashboardStatus(DashboardStatusEnum.SAVED)) + .catch(() => { + setDashboardStatus(DashboardStatusEnum.SAVING_FAILED); + notification.error("Error saving changes."); + }); + }, + [canEditDashboard, widgets] + ); + + const saveDashboardLayoutDebounced = useCallback( + (...args) => { + setDashboardStatus(DashboardStatusEnum.SAVING); + return debounce(() => saveDashboardLayout(...args), 2000)(); + }, + [saveDashboardLayout] + ); + + const retrySaveDashboardLayout = useCallback(() => saveDashboardLayout(recentPositions), [ + recentPositions, + saveDashboardLayout, + ]); + + const setEditing = useCallback( + editing => { + if (!editing && dashboardStatus !== DashboardStatusEnum.SAVED) { + setDoneBtnClickedWhileSaving(true); + return; + } + setEditingLayout(canEditDashboard && editing); + }, + [dashboardStatus, canEditDashboard] + ); + + return { + editingLayout: canEditDashboard && editingLayout, + setEditingLayout: setEditing, + saveDashboardLayout: editingLayout ? saveDashboardLayoutDebounced : saveDashboardLayout, + retrySaveDashboardLayout, + doneBtnClickedWhileSaving, + dashboardStatus, + }; +} diff --git a/client/app/pages/dashboards/hooks/useFullscreenHandler.js b/client/app/pages/dashboards/hooks/useFullscreenHandler.js new file mode 100644 index 000000000..38af0ca6e --- /dev/null +++ b/client/app/pages/dashboards/hooks/useFullscreenHandler.js @@ -0,0 +1,14 @@ +import { has } from "lodash"; +import { useEffect, useState } from "react"; +import location from "@/services/location"; + +export default function useFullscreenHandler() { + const [fullscreen, setFullscreen] = useState(has(location.search, "fullscreen")); + useEffect(() => { + document.body.classList.toggle("headless", fullscreen); + location.setSearch({ fullscreen: fullscreen ? true : null }, true); + }, [fullscreen]); + + const toggleFullscreen = () => setFullscreen(!fullscreen); + return [fullscreen, toggleFullscreen]; +} diff --git a/client/app/pages/dashboards/hooks/useRefreshRateHandler.js b/client/app/pages/dashboards/hooks/useRefreshRateHandler.js new file mode 100644 index 000000000..5838019b1 --- /dev/null +++ b/client/app/pages/dashboards/hooks/useRefreshRateHandler.js @@ -0,0 +1,40 @@ +import { isNaN, max, min } from "lodash"; +import { useEffect, useState, useRef, useMemo } from "react"; +import location from "@/services/location"; +import { policy } from "@/services/policy"; + +function getLimitedRefreshRate(refreshRate) { + const allowedIntervals = policy.getDashboardRefreshIntervals(); + return max([30, min(allowedIntervals), refreshRate]); +} + +function getRefreshRateFromUrl() { + const refreshRate = parseFloat(location.search.refresh); + return isNaN(refreshRate) ? null : getLimitedRefreshRate(refreshRate); +} + +export default function useRefreshRateHandler(refreshDashboard) { + const [refreshRate, setRefreshRate] = useState(getRefreshRateFromUrl()); + + // `refreshDashboard` may change quite frequently (on every update of `dashboard` instance), but we + // have to keep the same timer running, because timer will restart when re-creating, and instead of + // running refresh every N seconds - it will run refresh every N seconds after last dashboard update + // (which is not right obviously) + const refreshDashboardRef = useRef(); + refreshDashboardRef.current = refreshDashboard; + + // URL and timer should be updated only when `refreshRate` changes + useEffect(() => { + location.setSearch({ refresh: refreshRate || null }, true); + if (refreshRate) { + const refreshTimer = setInterval(() => { + refreshDashboardRef.current(); + }, refreshRate * 1000); + return () => clearInterval(refreshTimer); + } + }, [refreshRate]); + + return useMemo(() => [refreshRate, rate => setRefreshRate(getLimitedRefreshRate(rate)), () => setRefreshRate(null)], [ + refreshRate, + ]); +} diff --git a/client/app/pages/queries/components/QueryExecutionStatus.jsx b/client/app/pages/queries/components/QueryExecutionStatus.jsx index 97aac89cf..774915cb7 100644 --- a/client/app/pages/queries/components/QueryExecutionStatus.jsx +++ b/client/app/pages/queries/components/QueryExecutionStatus.jsx @@ -37,6 +37,7 @@ export default function QueryExecutionStatus({ status, updatedAt, error, isCance return ( diff --git a/client/cypress/integration/query/parameter_spec.js b/client/cypress/integration/query/parameter_spec.js index 8df45944b..c1d6a227a 100644 --- a/client/cypress/integration/query/parameter_spec.js +++ b/client/cypress/integration/query/parameter_spec.js @@ -117,7 +117,7 @@ describe("Parameter", () => { cy.getByTestId("ParameterApplyButton").click(); // ensure that query is being executed - cy.get('[data-test="ExecuteButton"]:disabled').should("exist"); + cy.getByTestId("QueryExecutionStatus").should("exist"); cy.getByTestId("TableVisualization").should("contain", "value2"); });