From 23bd51721334356bed2b14c6663c312dcd342ca6 Mon Sep 17 00:00:00 2001 From: Dominik Broj Date: Mon, 19 Feb 2024 10:23:04 +0100 Subject: [PATCH] fix issues related to alert groups table (#3905) # What this PR does - get rid of duplicated GET requests during polling - ignore results of older requests - fix Refresh btn that doesn't disappear - show correct data in alert groups table - show placeholder instead of table when filters are updated ## Which issue(s) this PR fixes https://github.com/grafana/oncall/issues/3894 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] Documentation added (or `pr:no public docs` PR label added if not required) - [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not required) --- CHANGELOG.md | 1 + .../src/components/GTable/GTable.tsx | 2 - .../ColumnsSelector/ColumnsSelector.tsx | 6 +- .../ColumnsSelectorWrapper/ColumnsModal.tsx | 4 +- .../ColumnsSelectorWrapper.tsx | 5 +- .../containers/Rotations/SchedulePersonal.tsx | 8 +-- .../src/models/alertgroup/alertgroup.ts | 46 +++++++------ .../src/models/loader/action-keys.ts | 5 ++ .../src/models/loader/loader.helpers.ts | 7 ++ grafana-plugin/src/models/loader/loader.ts | 14 ++-- .../src/pages/incidents/Incidents.module.scss | 5 ++ .../src/pages/incidents/Incidents.tsx | 67 +++++++++++++------ .../src/pages/insights/Insights.hooks.ts | 4 +- .../src/pages/insights/Insights.module.scss | 4 ++ .../src/pages/insights/Insights.tsx | 8 ++- .../src/pages/integrations/Integrations.tsx | 1 - .../src/pages/schedules/Schedules.tsx | 1 - grafana-plugin/src/utils/decorators.ts | 8 ++- grafana-plugin/src/utils/hooks.tsx | 7 +- 19 files changed, 129 insertions(+), 74 deletions(-) create mode 100644 grafana-plugin/src/models/loader/loader.helpers.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 528365f7..788bca3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix edit default team by admin @mderynck ([#3885](https://github.com/grafana/oncall/pull/3885)) - Unblock slack install by skipping check chatops gateway link in OSS deployment @mderynck ([#3893](https://github.com/grafana/oncall/pull/3893)) +- Fix multiple issues of alert groups table ([#3894](https://github.com/grafana/oncall/issues/3894)) - Improvements for dragging the add rotation form in Schedules ([#3904](https://github.com/grafana/oncall/pull/3904)) ### Changed diff --git a/grafana-plugin/src/components/GTable/GTable.tsx b/grafana-plugin/src/components/GTable/GTable.tsx index 2a110ca8..212b26e8 100644 --- a/grafana-plugin/src/components/GTable/GTable.tsx +++ b/grafana-plugin/src/components/GTable/GTable.tsx @@ -11,7 +11,6 @@ import styles from './GTable.module.css'; const cx = cn.bind(styles); export interface Props extends TableProps { - loading?: boolean; pagination?: { page: number; total: number; @@ -38,7 +37,6 @@ export const GTable = (props: data, className, pagination, - loading, rowSelection, rowKey, expandable, diff --git a/grafana-plugin/src/containers/ColumnsSelector/ColumnsSelector.tsx b/grafana-plugin/src/containers/ColumnsSelector/ColumnsSelector.tsx index bd2fb35d..3758f197 100644 --- a/grafana-plugin/src/containers/ColumnsSelector/ColumnsSelector.tsx +++ b/grafana-plugin/src/containers/ColumnsSelector/ColumnsSelector.tsx @@ -29,6 +29,7 @@ import { AlertGroupColumn, AlertGroupColumnType } from 'models/alertgroup/alertg import { ActionKey } from 'models/loader/action-keys'; import { useStore } from 'state/useStore'; import { UserActions } from 'utils/authorization/authorization'; +import { useIsLoading } from 'utils/hooks'; import { openErrorNotification } from 'utils/utils'; import { getColumnsSelectorStyles } from './ColumnsSelector.styles'; @@ -115,7 +116,8 @@ interface ColumnsSelectorProps { export const ColumnsSelector: React.FC = observer( ({ onColumnAddModalOpen, onConfirmRemovalModalOpen }) => { - const { alertGroupStore, loaderStore } = useStore(); + const { alertGroupStore } = useStore(); + const isResetLoading = useIsLoading(ActionKey.RESET_COLUMNS_FROM_ALERT_GROUP); const styles = useStyles2(getColumnsSelectorStyles); @@ -133,8 +135,6 @@ export const ColumnsSelector: React.FC = observer( }) ); - const isResetLoading = loaderStore.isLoading(ActionKey.RESET_COLUMNS_FROM_ALERT_GROUP); - return (
diff --git a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx index 4e16d849..3d12a2b0 100644 --- a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx +++ b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsModal.tsx @@ -26,7 +26,7 @@ import { components } from 'network/oncall-api/autogenerated-api.types'; import { useStore } from 'state/useStore'; import { UserActions } from 'utils/authorization/authorization'; import { WrapWithGlobalNotification } from 'utils/decorators'; -import { useDebouncedCallback } from 'utils/hooks'; +import { useDebouncedCallback, useIsLoading } from 'utils/hooks'; import { pluralize } from 'utils/utils'; import { getColumnsSelectorWrapperStyles } from './ColumnsSelectorWrapper.styles'; @@ -56,7 +56,7 @@ export const ColumnsModal: React.FC = observer( const [searchResults, setSearchResults] = useState([]); const debouncedOnInputChange = useDebouncedCallback(onInputChange, DEBOUNCE_MS); - const isLoading = store.loaderStore.isLoading(ActionKey.ADD_NEW_COLUMN_TO_ALERT_GROUP); + const isLoading = useIsLoading(ActionKey.ADD_NEW_COLUMN_TO_ALERT_GROUP); const availableKeysForSearching = useMemo(() => { const cols = store.alertGroupStore.columns; return labelKeys.filter( diff --git a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsSelectorWrapper.tsx b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsSelectorWrapper.tsx index 747bae8f..80eda2a4 100644 --- a/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsSelectorWrapper.tsx +++ b/grafana-plugin/src/containers/ColumnsSelectorWrapper/ColumnsSelectorWrapper.tsx @@ -13,6 +13,7 @@ import { ApiSchemas } from 'network/oncall-api/api.types'; import { useStore } from 'state/useStore'; import { UserActions } from 'utils/authorization/authorization'; import { WrapAutoLoadingState, WrapWithGlobalNotification } from 'utils/decorators'; +import { useIsLoading } from 'utils/hooks'; import { ColumnsModal } from './ColumnsModal'; @@ -23,8 +24,8 @@ export const ColumnsSelectorWrapper: React.FC = obs const [columnToBeRemoved, setColumnToBeRemoved] = useState(undefined); const [isColumnAddModalOpen, setIsColumnAddModalOpen] = useState(false); const [isFloatingDisplayOpen, setIsFloatingDisplayOpen] = useState(false); - const [labelKeys, setLabelKeys] = useState>([]); + const isRemoveLoading = useIsLoading(ActionKey.REMOVE_COLUMN_FROM_ALERT_GROUP); const inputRef = useRef(null); const wrappingFloatingContainerRef = useRef(null); @@ -49,8 +50,6 @@ export const ColumnsSelectorWrapper: React.FC = obs }; }, []); - const isRemoveLoading = store.loaderStore.isLoading(ActionKey.REMOVE_COLUMN_FROM_ALERT_GROUP); - return ( <> = observer(({ userPk, onSlotClick, history }) => { const store = useStore(); - const { timezoneStore, scheduleStore, userStore, loaderStore } = store; + const { timezoneStore, scheduleStore, userStore } = store; + const updatePersonalEventsLoading = useIsLoading(ActionKey.UPDATE_PERSONAL_EVENTS); useEffect(() => { updatePersonalEvents(); @@ -74,9 +76,7 @@ const _SchedulePersonal: FC = observer(({ userPk, onSlotC const storeUser = userStore.items[userPk]; - const emptyRotationsText = loaderStore.isLoading(ActionKey.UPDATE_PERSONAL_EVENTS) - ? 'Loading ...' - : 'There are no schedules relevant to user'; + const emptyRotationsText = updatePersonalEventsLoading ? 'Loading ...' : 'There are no schedules relevant to user'; return (
diff --git a/grafana-plugin/src/models/alertgroup/alertgroup.ts b/grafana-plugin/src/models/alertgroup/alertgroup.ts index 29cca3f7..28e54e99 100644 --- a/grafana-plugin/src/models/alertgroup/alertgroup.ts +++ b/grafana-plugin/src/models/alertgroup/alertgroup.ts @@ -28,9 +28,6 @@ export class AlertGroupStore extends BaseStore { @observable.shallow searchResult: { [key: string]: Array } = {}; - @observable - alertGroupsLoading = false; - @observable incidentFilters: any; @@ -70,6 +67,9 @@ export class AlertGroupStore extends BaseStore { @observable liveUpdatesPaused = false; + @observable + latestFetchAlertGroupsTimestamp: number; + @observable columns: AlertGroupColumn[] = []; @@ -231,17 +231,14 @@ export class AlertGroupStore extends BaseStore { }); } - // methods were moved from rootBaseStore. - // TODO check if methods are dublicating existing ones - async updateIncidents() { + async fetchIncidentsAndStats(isPollingJob = false) { await Promise.all([ this.getNewIncidentsStats(), this.getAcknowledgedIncidentsStats(), this.getResolvedIncidentsStats(), this.getSilencedIncidentsStats(), - this.updateAlertGroups(), + this.fetchAlertGroups(isPollingJob), ]); - this.setLiveUpdatesPaused(false); } @@ -251,21 +248,20 @@ export class AlertGroupStore extends BaseStore { } @action - async updateIncidentFilters(params: any, keepCursor = false) { + @AutoLoadingState(ActionKey.UPDATE_FILTERS_AND_FETCH_INCIDENTS) + async updateIncidentFiltersAndRefetchIncidentsAndStats(params: any, keepCursor = false) { if (!keepCursor) { this.setIncidentsCursor(undefined); } - this.incidentFilters = params; - - await this.updateIncidents(); + await this.fetchIncidentsAndStats(); } @action async updateIncidentsCursor(cursor: string) { this.setIncidentsCursor(cursor); - this.updateAlertGroups(); + this.fetchAlertGroups(); } @action @@ -279,13 +275,17 @@ export class AlertGroupStore extends BaseStore { async setIncidentsItemsPerPage() { this.setIncidentsCursor(undefined); - this.updateAlertGroups(); + this.fetchAlertGroups(); } @action.bound - async updateAlertGroups() { - this.alertGroupsLoading = true; - + async fetchAlertGroups(isPollingJob = false) { + this.rootStore.loaderStore.setLoadingAction( + isPollingJob ? ActionKey.FETCH_INCIDENTS_POLLING : ActionKey.FETCH_INCIDENTS, + true + ); + const timestamp = new Date().getTime(); + this.latestFetchAlertGroupsTimestamp = timestamp; const { results, next: nextRaw, @@ -306,12 +306,16 @@ export class AlertGroupStore extends BaseStore { const newAlerts = new Map( results.map((alert: Alert) => { const oldAlert = this.alerts.get(alert.pk) || {}; - const mergedAlertData = { ...oldAlert, ...alert }; + const mergedAlertData = { ...oldAlert, ...alert, undoAction: alert.undoAction }; return [alert.pk, mergedAlertData]; }) ); runInAction(() => { + // If previous fetch took longer than the next one, we ignore result of the previous fetch + if (timestamp !== this.latestFetchAlertGroupsTimestamp) { + return; + } // @ts-ignore this.alerts = new Map([...this.alerts, ...newAlerts]); @@ -321,8 +325,10 @@ export class AlertGroupStore extends BaseStore { results: results.map((alert: Alert) => alert.pk), page_size, }; - - this.alertGroupsLoading = false; + this.rootStore.loaderStore.setLoadingAction( + [ActionKey.FETCH_INCIDENTS, ActionKey.FETCH_INCIDENTS_POLLING], + false + ); }); } diff --git a/grafana-plugin/src/models/loader/action-keys.ts b/grafana-plugin/src/models/loader/action-keys.ts index 4d7fdf50..10505e2a 100644 --- a/grafana-plugin/src/models/loader/action-keys.ts +++ b/grafana-plugin/src/models/loader/action-keys.ts @@ -3,4 +3,9 @@ export enum ActionKey { REMOVE_COLUMN_FROM_ALERT_GROUP = 'REMOVE_COLUMN_FROM_ALERT_GROUP', RESET_COLUMNS_FROM_ALERT_GROUP = 'RESET_COLUMNS_FROM_ALERT_GROUP', UPDATE_PERSONAL_EVENTS = 'UPDATE_PERSONAL_EVENTS', + UPDATE_ALERT_GROUP = 'UPDATE_ALERT_GROUP', + FETCH_INCIDENTS = 'FETCH_INCIDENTS', + FETCH_INCIDENTS_POLLING = 'FETCH_INCIDENTS_POLLING', + FETCH_INCIDENTS_AND_STATS = 'FETCH_INCIDENTS_AND_STATS', + UPDATE_FILTERS_AND_FETCH_INCIDENTS = 'UPDATE_FILTERS_AND_FETCH_INCIDENTS', } diff --git a/grafana-plugin/src/models/loader/loader.helpers.ts b/grafana-plugin/src/models/loader/loader.helpers.ts new file mode 100644 index 00000000..02c7ee70 --- /dev/null +++ b/grafana-plugin/src/models/loader/loader.helpers.ts @@ -0,0 +1,7 @@ +import { LoaderStore } from './loader'; + +export class LoaderHelper { + static isLoading(store: typeof LoaderStore, actionKey: string | string[]) { + return typeof actionKey === 'string' ? store.items[actionKey] : actionKey.some((key) => store.items[key]); + } +} diff --git a/grafana-plugin/src/models/loader/loader.ts b/grafana-plugin/src/models/loader/loader.ts index 49fff8b1..a24822d9 100644 --- a/grafana-plugin/src/models/loader/loader.ts +++ b/grafana-plugin/src/models/loader/loader.ts @@ -13,13 +13,15 @@ class LoaderStoreClass { } @action.bound - setLoadingAction(actionKey: string, isLoading: boolean) { - this.items[actionKey] = isLoading; + setLoadingAction(actionKey: string | string[], isLoading: boolean) { + if (Array.isArray(actionKey)) { + actionKey.forEach((key) => { + this.items[key] = isLoading; + }); + } else { + this.items[actionKey] = isLoading; + } } - - isLoading = (actionKey: string): boolean => { - return !!this.items[actionKey]; - }; } export const LoaderStore = new LoaderStoreClass(); diff --git a/grafana-plugin/src/pages/incidents/Incidents.module.scss b/grafana-plugin/src/pages/incidents/Incidents.module.scss index b73e8560..ab7d3d44 100644 --- a/grafana-plugin/src/pages/incidents/Incidents.module.scss +++ b/grafana-plugin/src/pages/incidents/Incidents.module.scss @@ -81,6 +81,11 @@ max-width: 25%; } +.loadingPlaceholder { + margin-bottom: 0; + text-align: center; +} + @media (max-width: 1200px) { .col { flex: 0 0 50%; diff --git a/grafana-plugin/src/pages/incidents/Incidents.tsx b/grafana-plugin/src/pages/incidents/Incidents.tsx index 732970bb..893dcaa5 100644 --- a/grafana-plugin/src/pages/incidents/Incidents.tsx +++ b/grafana-plugin/src/pages/incidents/Incidents.tsx @@ -2,7 +2,15 @@ import React, { SyntheticEvent } from 'react'; import { SelectableValue } from '@grafana/data'; import { LabelTag } from '@grafana/labels'; -import { Button, HorizontalGroup, Icon, RadioButtonGroup, Tooltip, VerticalGroup } from '@grafana/ui'; +import { + Button, + HorizontalGroup, + Icon, + LoadingPlaceholder, + RadioButtonGroup, + Tooltip, + VerticalGroup, +} from '@grafana/ui'; import cn from 'classnames/bind'; import { capitalize } from 'lodash-es'; import { observer } from 'mobx-react'; @@ -36,6 +44,8 @@ import { AlertGroupColumnType, } from 'models/alertgroup/alertgroup.types'; import { LabelKeyValue } from 'models/label/label.types'; +import { ActionKey } from 'models/loader/action-keys'; +import { LoaderHelper } from 'models/loader/loader.helpers'; import { renderRelatedUsers } from 'pages/incident/Incident.helpers'; import { AppFeature } from 'state/features'; import { PageProps, WithStoreProps } from 'state/types'; @@ -66,6 +76,7 @@ interface IncidentsPageState { showAddAlertGroupForm: boolean; isSelectorColumnMenuOpen: boolean; isHorizontalScrolling: boolean; + isFirstIncidentsFetchDone: boolean; } const POLLING_NUM_SECONDS = 15; @@ -122,6 +133,7 @@ class _IncidentsPage extends React.Component { + fetchIncidentData = async (filters: IncidentsFiltersType) => { const { store } = this.props; - await store.alertGroupStore.updateIncidentFilters(filters, isOnMount); // this line fetches the incidents + await store.alertGroupStore.updateIncidentFiltersAndRefetchIncidentsAndStats( + filters, + !this.state.isFirstIncidentsFetchDone + ); LocationHelper.update({ ...store.alertGroupStore.incidentFilters }, 'partial'); + this.setState({ isFirstIncidentsFetchDone: true }); }; onChangeCursor = (cursor: string, direction: 'prev' | 'next') => { @@ -418,6 +434,7 @@ class _IncidentsPage extends React.Component 0; + const isLoading = LoaderHelper.isLoading(store.loaderStore, ActionKey.FETCH_INCIDENTS); const hasInvalidatedAlert = Boolean( (results && results.some((alert: AlertType) => alert.undoAction)) || Object.keys(affectedRows).length ); @@ -475,20 +492,19 @@ class _IncidentsPage extends React.Component
- + Results out of date - + + + + { - store.alertGroupStore.updateIncidents(); + store.alertGroupStore.fetchIncidentsAndStats(); }); }; @@ -949,13 +965,20 @@ class _IncidentsPage extends React.Component { this.pollingIntervalId = setTimeout( async () => { const isBrowserWindowInactive = document.hidden; - if (!isBrowserWindowInactive) { - await this.fetchIncidentData(filters, isOnMount); + if ( + !isBrowserWindowInactive && + !LoaderHelper.isLoading(this.props.store.loaderStore, [ + ActionKey.FETCH_INCIDENTS, + ActionKey.FETCH_INCIDENTS_POLLING, + ]) && + !this.props.store.alertGroupStore.liveUpdatesPaused + ) { + await this.props.store.alertGroupStore.fetchIncidentsAndStats(true); } if (this.pollingIntervalId === null) { diff --git a/grafana-plugin/src/pages/insights/Insights.hooks.ts b/grafana-plugin/src/pages/insights/Insights.hooks.ts index 1d9b5ee0..13a320ce 100644 --- a/grafana-plugin/src/pages/insights/Insights.hooks.ts +++ b/grafana-plugin/src/pages/insights/Insights.hooks.ts @@ -7,7 +7,7 @@ const FIVE_SECS = 5_000; export const useAlertCreationChecker = () => { const { - alertGroupStore: { updateAlertGroups, alerts }, + alertGroupStore: { fetchAlertGroups, alerts }, } = useStore(); const [isFirstAlertCheckDone, setIsFirstAlertCheckDone] = useState(false); @@ -20,7 +20,7 @@ export const useAlertCreationChecker = () => { useEffect(() => { const fetch = async () => { if (!isAnyAlertCreatedMoreThan20SecsAgo) { - await updateAlertGroups(); + await fetchAlertGroups(); } setIsFirstAlertCheckDone(true); }; diff --git a/grafana-plugin/src/pages/insights/Insights.module.scss b/grafana-plugin/src/pages/insights/Insights.module.scss index eec1969a..5287a43b 100644 --- a/grafana-plugin/src/pages/insights/Insights.module.scss +++ b/grafana-plugin/src/pages/insights/Insights.module.scss @@ -9,3 +9,7 @@ .spaceTop { margin-top: 16px; } + +.alertBox { + margin-top: 32px; +} diff --git a/grafana-plugin/src/pages/insights/Insights.tsx b/grafana-plugin/src/pages/insights/Insights.tsx index c2de19c4..69944ae8 100644 --- a/grafana-plugin/src/pages/insights/Insights.tsx +++ b/grafana-plugin/src/pages/insights/Insights.tsx @@ -131,9 +131,11 @@ const NoDatasourceWarning = () => { ); return alertVisible ? ( - setAlertVisible(false)} severity="warning" title="" className={styles.alertBox}> - Insights data has missing Prometheus configuration. Open OnCall {docsLink} to see how to setup it. - +
+ setAlertVisible(false)} severity="warning" title=""> + Insights data has missing Prometheus configuration. Open OnCall {docsLink} to see how to setup it. + +
) : null; }; diff --git a/grafana-plugin/src/pages/integrations/Integrations.tsx b/grafana-plugin/src/pages/integrations/Integrations.tsx index 9b09eed6..06eb3805 100644 --- a/grafana-plugin/src/pages/integrations/Integrations.tsx +++ b/grafana-plugin/src/pages/integrations/Integrations.tsx @@ -270,7 +270,6 @@ class _IntegrationsPage extends React.Component(callback: (...args: A) => } export const useIsLoading = (actionKey: ActionKey) => { - const { - loaderStore: { isLoading }, - } = useStore(); - return isLoading(actionKey); + const { loaderStore } = useStore(); + return LoaderHelper.isLoading(loaderStore, actionKey); };