Skip to content

Commit bd1a27a

Browse files
authored
fix(dashboards): temporary filters broken when changing values (#104482)
1. Add ability to disable removeFilter option, this is currently done when the dashboard is prebuilt, and a filter exists in it's prebuilt config. 2. remove the `temporaryFilters` query param and add `isTemporary` property to `GlobalFilters` which is easier to manage then having two separate query params 3. Add tests cases to dashboard link field renderer and filter bar
1 parent a25c2d0 commit bd1a27a

File tree

10 files changed

+281
-91
lines changed

10 files changed

+281
-91
lines changed

static/app/utils/discover/fieldRenderers.spec.tsx

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
1+
import {OrganizationFixture} from 'sentry-fixture/organization';
12
import {ThemeFixture} from 'sentry-fixture/theme';
23
import {UserFixture} from 'sentry-fixture/user';
4+
import {WidgetFixture} from 'sentry-fixture/widget';
35

46
import {initializeOrg} from 'sentry-test/initializeOrg';
57
import {act, render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
@@ -8,14 +10,17 @@ import ProjectsStore from 'sentry/stores/projectsStore';
810
import EventView from 'sentry/utils/discover/eventView';
911
import {getFieldRenderer} from 'sentry/utils/discover/fieldRenderers';
1012
import {SPAN_OP_RELATIVE_BREAKDOWN_FIELD} from 'sentry/utils/discover/fields';
13+
import {WidgetType, type DashboardFilters} from 'sentry/views/dashboards/types';
1114

1215
const theme = ThemeFixture();
1316

1417
describe('getFieldRenderer', () => {
1518
let location: any, context: any, project: any, organization: any, data: any, user: any;
1619

1720
beforeEach(() => {
18-
context = initializeOrg();
21+
context = initializeOrg({
22+
organization: OrganizationFixture({features: ['dashboards-drilldown-flow']}),
23+
});
1924
organization = context.organization;
2025
project = context.project;
2126
act(() => ProjectsStore.loadInitialData([project]));
@@ -113,6 +118,44 @@ describe('getFieldRenderer', () => {
113118
expect(screen.getByText(data.numeric)).toBeInTheDocument();
114119
});
115120

121+
it('can render dashboard links', () => {
122+
const widget = WidgetFixture({
123+
widgetType: WidgetType.SPANS,
124+
queries: [
125+
{
126+
linkedDashboards: [{dashboardId: '123', field: 'transaction'}],
127+
aggregates: [],
128+
columns: [],
129+
conditions: '',
130+
name: '',
131+
orderby: '',
132+
},
133+
],
134+
});
135+
const dashboardFilters: DashboardFilters = {};
136+
137+
const renderer = getFieldRenderer(
138+
'transaction',
139+
{transaction: 'string'},
140+
undefined,
141+
widget,
142+
dashboardFilters
143+
);
144+
145+
render(
146+
renderer(data, {
147+
location,
148+
organization,
149+
theme,
150+
}) as React.ReactElement<any, any>
151+
);
152+
153+
expect(screen.getByRole('link')).toHaveAttribute(
154+
'href',
155+
'/organizations/org-slug/dashboard/123/?globalFilter=%7B%22dataset%22%3A%22spans%22%2C%22tag%22%3A%7B%22key%22%3A%22transaction%22%2C%22name%22%3A%22transaction%22%2C%22kind%22%3A%22tag%22%7D%2C%22value%22%3A%22transaction%3A%5Bapi.do_things%5D%22%2C%22isTemporary%22%3Atrue%7D'
156+
);
157+
});
158+
116159
describe('rate', () => {
117160
it('can render null rate', () => {
118161
const renderer = getFieldRenderer(

static/app/utils/discover/fieldRenderers.tsx

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1430,7 +1430,11 @@ function getDashboardUrl(
14301430
if (dashboardLink && dashboardLink.dashboardId !== '-1') {
14311431
const newTemporaryFilters: GlobalFilter[] = [
14321432
...(dashboardFilters[DashboardFilterKeys.GLOBAL_FILTER] ?? []),
1433-
].filter(filter => Boolean(filter.value));
1433+
].filter(
1434+
filter =>
1435+
Boolean(filter.value) &&
1436+
!(filter.tag.key === field && filter.dataset === widget.widgetType)
1437+
);
14341438

14351439
// Format the value as a proper filter condition string
14361440
const mutableSearch = new MutableSearch('');
@@ -1440,6 +1444,7 @@ function getDashboardUrl(
14401444
dataset: widget.widgetType,
14411445
tag: {key: field, name: field, kind: FieldKind.TAG},
14421446
value: formattedValue,
1447+
isTemporary: true,
14431448
});
14441449

14451450
// Preserve project, environment, and time range query params
@@ -1455,7 +1460,7 @@ function getDashboardUrl(
14551460
const url = `/organizations/${organization.slug}/dashboard/${dashboardLink.dashboardId}/?${qs.stringify(
14561461
{
14571462
...filterParams,
1458-
[DashboardFilterKeys.TEMPORARY_FILTERS]: newTemporaryFilters.map(filter =>
1463+
[DashboardFilterKeys.GLOBAL_FILTER]: newTemporaryFilters.map(filter =>
14591464
JSON.stringify(filter)
14601465
),
14611466
}

static/app/views/dashboards/detail.tsx

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -960,7 +960,6 @@ class DashboardDetail extends Component<Props, State> {
960960
filters={{}} // Default Dashboards don't have filters set
961961
location={location}
962962
hasUnsavedChanges={false}
963-
hasTemporaryFilters={false}
964963
isEditingDashboard={false}
965964
isPreview={false}
966965
onDashboardFilterChange={this.handleChangeFilter}
@@ -1043,10 +1042,6 @@ class DashboardDetail extends Component<Props, State> {
10431042
dashboardState !== DashboardState.CREATE &&
10441043
hasUnsavedFilterChanges(dashboard, location);
10451044

1046-
const hasTemporaryFilters = defined(
1047-
location.query?.[DashboardFilterKeys.TEMPORARY_FILTERS]
1048-
);
1049-
10501045
const eventView = generatePerformanceEventView(location, projects, {}, organization);
10511046

10521047
const isDashboardUsingTransaction = dashboard.widgets.some(
@@ -1179,15 +1174,14 @@ class DashboardDetail extends Component<Props, State> {
11791174
dashboardCreator={dashboard.createdBy}
11801175
location={location}
11811176
hasUnsavedChanges={!this.isEmbedded && hasUnsavedFilters}
1182-
hasTemporaryFilters={hasTemporaryFilters}
11831177
isEditingDashboard={
11841178
dashboardState !== DashboardState.CREATE &&
11851179
this.isEditingDashboard
11861180
}
11871181
isPreview={this.isPreview}
11881182
onDashboardFilterChange={this.handleChangeFilter}
11891183
shouldBusySaveButton={this.state.isSavingDashboardFilters}
1190-
isPrebuiltDashboard={defined(dashboard.prebuiltId)}
1184+
prebuiltDashboardId={dashboard.prebuiltId}
11911185
onCancel={() => {
11921186
resetPageFilters(dashboard, location);
11931187
trackAnalytics('dashboards2.filter.cancel', {
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
// create a basic test for filters bar
2+
3+
import {LocationFixture} from 'sentry-fixture/locationFixture';
4+
import {OrganizationFixture} from 'sentry-fixture/organization';
5+
import {ReleaseFixture} from 'sentry-fixture/release';
6+
import {TagsFixture} from 'sentry-fixture/tags';
7+
8+
import {render, screen, waitForElementToBeRemoved} from 'sentry-test/reactTestingLibrary';
9+
10+
import type {Organization} from 'sentry/types/organization';
11+
import {FieldKind} from 'sentry/utils/fields';
12+
import FiltersBar, {type FiltersBarProps} from 'sentry/views/dashboards/filtersBar';
13+
import {
14+
DashboardFilterKeys,
15+
WidgetType,
16+
type GlobalFilter,
17+
} from 'sentry/views/dashboards/types';
18+
import {PrebuiltDashboardId} from 'sentry/views/dashboards/utils/prebuiltConfigs';
19+
20+
describe('FiltersBar', () => {
21+
let organization: Organization;
22+
23+
beforeEach(() => {
24+
mockNetworkRequests();
25+
26+
organization = OrganizationFixture({
27+
features: ['dashboards-basic', 'dashboards-edit', 'dashboards-global-filters'],
28+
});
29+
});
30+
31+
afterEach(() => {
32+
MockApiClient.clearMockResponses();
33+
jest.clearAllMocks();
34+
});
35+
36+
const renderFilterBar = (overrides: Partial<FiltersBarProps> = {}) => {
37+
const props: FiltersBarProps = {
38+
filters: {},
39+
hasUnsavedChanges: false,
40+
isEditingDashboard: false,
41+
isPreview: false,
42+
location: LocationFixture(),
43+
onDashboardFilterChange: () => {},
44+
...overrides,
45+
};
46+
47+
return render(<FiltersBar {...props} />, {organization});
48+
};
49+
50+
it('should render basic global filter', async () => {
51+
const newLocation = LocationFixture({
52+
query: {
53+
[DashboardFilterKeys.GLOBAL_FILTER]: JSON.stringify({
54+
dataset: WidgetType.SPANS,
55+
tag: {key: 'browser.name', name: 'Browser Name', kind: FieldKind.FIELD},
56+
value: `browser.name:[Chrome]`,
57+
} satisfies GlobalFilter),
58+
},
59+
});
60+
renderFilterBar({location: newLocation});
61+
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));
62+
expect(
63+
screen.getByRole('button', {name: /browser\.name.*Chrome/i})
64+
).toBeInTheDocument();
65+
});
66+
67+
it('should render save button with unsaved changes', async () => {
68+
const newLocation = LocationFixture({
69+
query: {
70+
[DashboardFilterKeys.GLOBAL_FILTER]: JSON.stringify({
71+
dataset: WidgetType.SPANS,
72+
tag: {key: 'browser.name', name: 'Browser Name', kind: FieldKind.FIELD},
73+
value: `browser.name:[Chrome]`,
74+
} satisfies GlobalFilter),
75+
},
76+
});
77+
renderFilterBar({location: newLocation, hasUnsavedChanges: true});
78+
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));
79+
expect(screen.getByRole('button', {name: 'Save'})).toBeInTheDocument();
80+
expect(screen.getByRole('button', {name: 'Cancel'})).toBeInTheDocument();
81+
});
82+
83+
it('should not render save button with temporary filter', async () => {
84+
const newLocation = LocationFixture({
85+
query: {
86+
[DashboardFilterKeys.GLOBAL_FILTER]: JSON.stringify({
87+
dataset: WidgetType.SPANS,
88+
tag: {key: 'browser.name', name: 'Browser Name', kind: FieldKind.FIELD},
89+
value: `browser.name:[Chrome]`,
90+
isTemporary: true,
91+
} satisfies GlobalFilter),
92+
},
93+
});
94+
95+
renderFilterBar({location: newLocation});
96+
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));
97+
expect(
98+
screen.getByRole('button', {name: /browser\.name.*Chrome/i})
99+
).toBeInTheDocument();
100+
expect(screen.queryByRole('button', {name: 'Save'})).not.toBeInTheDocument();
101+
expect(screen.queryByRole('button', {name: 'Cancel'})).not.toBeInTheDocument();
102+
});
103+
104+
it('should not render save button on prebuilt dashboard', async () => {
105+
const newLocation = LocationFixture({
106+
query: {
107+
[DashboardFilterKeys.GLOBAL_FILTER]: JSON.stringify({
108+
dataset: WidgetType.SPANS,
109+
tag: {key: 'browser.name', name: 'Browser Name', kind: FieldKind.FIELD},
110+
value: `browser.name:[Chrome]`,
111+
} satisfies GlobalFilter),
112+
},
113+
});
114+
renderFilterBar({
115+
location: newLocation,
116+
prebuiltDashboardId: PrebuiltDashboardId.FRONTEND_SESSION_HEALTH,
117+
});
118+
await waitForElementToBeRemoved(() => screen.queryByTestId('loading-indicator'));
119+
expect(
120+
screen.getByRole('button', {name: /browser\.name.*Chrome/i})
121+
).toBeInTheDocument();
122+
expect(screen.queryByRole('button', {name: 'Save'})).not.toBeInTheDocument();
123+
expect(screen.queryByRole('button', {name: 'Cancel'})).not.toBeInTheDocument();
124+
});
125+
});
126+
127+
const mockNetworkRequests = () => {
128+
MockApiClient.addMockResponse({
129+
url: '/organizations/org-slug/releases/',
130+
body: [ReleaseFixture()],
131+
});
132+
MockApiClient.addMockResponse({
133+
url: '/organizations/org-slug/tags/',
134+
body: TagsFixture(),
135+
});
136+
MockApiClient.addMockResponse({
137+
url: `/organizations/org-slug/measurements-meta/`,
138+
body: {
139+
'measurements.custom.measurement': {
140+
functions: ['p99'],
141+
},
142+
'measurements.another.custom.measurement': {
143+
functions: ['p99'],
144+
},
145+
},
146+
});
147+
148+
const mockSearchResponse = [
149+
{
150+
key: 'browser.name',
151+
value: 'Chrome',
152+
name: 'Chrome',
153+
first_seen: null,
154+
last_seen: null,
155+
times_seen: null,
156+
},
157+
{
158+
key: 'browser.name',
159+
value: 'Firefox',
160+
name: 'Firefox',
161+
first_seen: null,
162+
last_seen: null,
163+
times_seen: null,
164+
},
165+
];
166+
167+
MockApiClient.addMockResponse({
168+
url: `/organizations/org-slug/trace-items/attributes/browser.name/values/`,
169+
body: mockSearchResponse,
170+
match: [MockApiClient.matchQuery({attributeType: 'string'})],
171+
});
172+
};

0 commit comments

Comments
 (0)