Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pagination refactoring/cleanup #3205

Merged
merged 19 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Unreleased

### Changes

- Removed the hardcoding of page size on frontend ([#3205](https://github.com/grafana/oncall/pull/#3205))
- Prevent additional polling on Incidents if the previous request didn't complete
([#3205](https://github.com/grafana/oncall/pull/#3205))

## v1.3.47 (2023-10-25)

### Fixed
Expand Down
29 changes: 17 additions & 12 deletions grafana-plugin/src/containers/RemoteFilters/RemoteFilters.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { Component } from 'react';

import { SelectableValue, TimeRange } from '@grafana/data';
import { KeyValue, SelectableValue, TimeRange } from '@grafana/data';
import {
InlineSwitch,
MultiSelect,
Expand Down Expand Up @@ -31,16 +31,15 @@ import LocationHelper from 'utils/LocationHelper';
import { PAGE } from 'utils/consts';

import { parseFilters } from './RemoteFilters.helpers';
import { FilterOption, RemoteFiltersType } from './RemoteFilters.types';
import { FilterOption } from './RemoteFilters.types';

import styles from './RemoteFilters.module.css';

const cx = cn.bind(styles);

interface RemoteFiltersProps extends WithStoreProps {
value: RemoteFiltersType;
onChange: (filters: { [key: string]: any }, isOnMount: boolean, invalidateFn: () => boolean) => void;
query: { [key: string]: any };
query: KeyValue;
page: PAGE;
defaultFilters?: FiltersValues;
extraFilters?: (state, setState, onFiltersValueChange) => React.ReactNode;
Expand Down Expand Up @@ -82,11 +81,18 @@ class RemoteFilters extends Component<RemoteFiltersProps, RemoteFiltersState> {
}

async componentDidMount() {
const { query, page, store, defaultFilters } = this.props;

const { filtersStore } = store;
const {
query,
page,
store: { filtersStore },
defaultFilters,
} = this.props;

const filterOptions = await filtersStore.updateOptionsForPage(page);
const currentTablePageNum = parseInt(filtersStore.currentTablePageNum[page] || query.p || 1, 10);

// set the current page from filters/query or default it to 1
filtersStore.currentTablePageNum[page] = currentTablePageNum;
teodosii marked this conversation as resolved.
Show resolved Hide resolved

let { filters, values } = parseFilters({ ...query, ...filtersStore.globalValues }, filterOptions, query);

Expand Down Expand Up @@ -422,10 +428,7 @@ class RemoteFilters extends Component<RemoteFiltersProps, RemoteFiltersState> {
}

const currentRequestId = this.getNewRequestId();

this.setState({
lastRequestId: currentRequestId,
});
this.setState({ lastRequestId: currentRequestId });

LocationHelper.update({ ...values }, 'partial');
onChange(values, isOnMount, this.invalidateFn.bind(this, currentRequestId));
Expand All @@ -443,4 +446,6 @@ class RemoteFilters extends Component<RemoteFiltersProps, RemoteFiltersState> {
debouncedOnChange = debounce(this.onChange, 500);
}

export default withMobXProviderContext(RemoteFilters);
export default withMobXProviderContext(RemoteFilters) as unknown as React.ComponentClass<
Omit<RemoteFiltersProps, 'store'>
>;
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class AlertReceiveChannelStore extends BaseStore {
searchResult: Array<AlertReceiveChannel['id']>;

@observable.shallow
paginatedSearchResult: { count?: number; results?: Array<AlertReceiveChannel['id']> } = {};
paginatedSearchResult: { count?: number; results?: Array<AlertReceiveChannel['id']>; page_size?: number } = {};

@observable.shallow
items: { [id: string]: AlertReceiveChannel } = {};
Expand Down Expand Up @@ -81,6 +81,7 @@ export class AlertReceiveChannelStore extends BaseStore {
}

return {
page_size: this.paginatedSearchResult.page_size,
count: this.paginatedSearchResult.count,
results:
this.paginatedSearchResult.results &&
Expand Down Expand Up @@ -133,7 +134,7 @@ export class AlertReceiveChannelStore extends BaseStore {

async updatePaginatedItems(query: any = '', page = 1, updateCounters = false, invalidateFn = undefined) {
const filters = typeof query === 'string' ? { search: query } : query;
const { count, results } = await makeRequest(this.path, { params: { ...filters, page } });
const { count, results, page_size } = await makeRequest(this.path, { params: { ...filters, page } });

if (invalidateFn?.()) {
return undefined;
Expand All @@ -155,6 +156,7 @@ export class AlertReceiveChannelStore extends BaseStore {
this.paginatedSearchResult = {
count,
results: results.map((item: AlertReceiveChannel) => item.id),
page_size,
};

if (updateCounters) {
Expand Down
63 changes: 28 additions & 35 deletions grafana-plugin/src/models/alertgroup/alertgroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,12 @@ export class AlertGroupStore extends BaseStore {
incidentsItemsPerPage?: number;

@observable
alertsSearchResult: any = {};
alertsSearchResult: {
prev?: string;
next?: string;
results?: any;
page_size?: number;
} = {};
brojd marked this conversation as resolved.
Show resolved Hide resolved

@observable
alerts = new Map<string, Alert>();
Expand Down Expand Up @@ -89,29 +94,6 @@ export class AlertGroupStore extends BaseStore {
}).catch(showApiError);
}

@action // FIXME for `attach to` feature ONLY
async updateItems(query = '') {
const { results } = await makeRequest(`${this.path}`, {
params: { search: query, resolved: false, is_root: true },
});

this.items = {
...this.items,
...results.reduce(
(acc: { [key: string]: Alert }, item: Alert) => ({
...acc,
[item.pk]: item,
}),
{}
),
};

this.searchResult = {
...this.searchResult,
[query]: results.map((item: Alert) => item.pk),
};
}

async updateItem(id: Alert['pk']) {
const item = await this.getById(id);

Expand Down Expand Up @@ -220,12 +202,13 @@ export class AlertGroupStore extends BaseStore {
// TODO check if methods are dublicating existing ones
@action
async updateIncidents() {
this.getNewIncidentsStats();
this.getAcknowledgedIncidentsStats();
this.getResolvedIncidentsStats();
this.getSilencedIncidentsStats();

this.updateAlertGroups();
await Promise.all([
this.getNewIncidentsStats(),
this.getAcknowledgedIncidentsStats(),
this.getResolvedIncidentsStats(),
this.getSilencedIncidentsStats(),
this.updateAlertGroups(),
]);

this.liveUpdatesPaused = false;
}
Expand All @@ -238,7 +221,7 @@ export class AlertGroupStore extends BaseStore {

this.incidentFilters = params;

this.updateIncidents();
await this.updateIncidents();
}

@action
Expand Down Expand Up @@ -271,11 +254,12 @@ export class AlertGroupStore extends BaseStore {
results,
next: nextRaw,
previous: previousRaw,
page_size,
} = await makeRequest(`${this.path}`, {
params: {
...this.incidentFilters,
cursor: this.incidentsCursor,
perpage: this.incidentsItemsPerPage,
cursor: this.incidentsCursor,
is_root: true,
},
}).catch(refreshPageError);
Expand All @@ -294,21 +278,30 @@ export class AlertGroupStore extends BaseStore {
// @ts-ignore
this.alerts = new Map<number, Alert>([...this.alerts, ...newAlerts]);

this.incidentsItemsPerPage = page_size;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to store this in alertgroupstore as a separate field? can we get it as getSearchResult().page_size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you're right, let me try it 🤔

Copy link
Member Author

@teodosii teodosii Oct 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just doesn't seem to work quite as expected if we only replace it with getSearchResult() because there's no way to trigger the pagination update nested in the state of the component 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird but ok


this.alertsSearchResult['default'] = {
prev: prevCursor,
next: nextCursor,
results: results.map((alert: Alert) => alert.pk),
page_size,
};

this.alertGroupsLoading = false;
}

getAlertSearchResult(query: string) {
if (!this.alertsSearchResult[query]) {
return undefined;
const result = this.alertsSearchResult[query];
if (!result) {
return {};
}

return this.alertsSearchResult[query].results.map((pk: Alert['pk']) => this.alerts.get(pk));
return {
prev: result.prev,
next: result.next,
page_size: result.page_size,
results: result.results.map((pk: Alert['pk']) => this.alerts.get(pk)),
};
}

@action
Expand Down
3 changes: 3 additions & 0 deletions grafana-plugin/src/models/filters/filters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ export class FiltersStore extends BaseStore {
@observable.shallow
public values: { [page: string]: FiltersValues } = {};

@observable.shallow
public currentTablePageNum: { [page: string]: number } = {};

private _globalValues: FiltersValues = {};

@observable
Expand Down
1 change: 1 addition & 0 deletions grafana-plugin/src/models/schedule/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ export class ScheduleStore extends BaseStore {
if (!this.searchResult) {
return undefined;
}

return {
page_size: this.searchResult.page_size,
count: this.searchResult.count,
Expand Down
6 changes: 4 additions & 2 deletions grafana-plugin/src/models/user/user.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { User } from './user.types';

export class UserStore extends BaseStore {
@observable.shallow
searchResult: { count?: number; results?: Array<User['pk']> } = {};
searchResult: { count?: number; results?: Array<User['pk']>; page_size?: number } = {};

@observable.shallow
items: { [pk: string]: User } = {};
Expand Down Expand Up @@ -122,7 +122,7 @@ export class UserStore extends BaseStore {
return;
}

const { count, results } = response;
const { count, results, page_size } = response;

this.items = {
...this.items,
Expand All @@ -140,6 +140,7 @@ export class UserStore extends BaseStore {

this.searchResult = {
count,
page_size,
results: results.map((item: User) => item.pk),
};

Expand All @@ -148,6 +149,7 @@ export class UserStore extends BaseStore {

getSearchResult() {
return {
page_size: this.searchResult.page_size,
count: this.searchResult.count,
results: this.searchResult.results && this.searchResult.results.map((userPk: User['pk']) => this.items?.[userPk]),
};
Expand Down
Loading
Loading