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

feat(query-builder): Add basic support for selecting multiple values #71457

Merged
merged 3 commits into from
May 28, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
2 changes: 1 addition & 1 deletion static/app/components/searchQueryBuilder/filter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ function FilterValue({token, state, item}: SearchQueryTokenProps) {
<ValueEditing ref={ref} {...mergeProps(focusWithinProps, filterButtonProps)}>
<SearchQueryBuilderValueCombobox
token={token}
onChange={() => {
onCommit={() => {
setIsEditing(false);
if (state.collection.getKeyAfter(item.key)) {
state.selectionManager.setFocusedKey(
Expand Down
76 changes: 57 additions & 19 deletions static/app/components/searchQueryBuilder/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import type {TagCollection} from 'sentry/types/group';
import {FieldKey, FieldKind} from 'sentry/utils/fields';

const MOCK_SUPPORTED_KEYS: TagCollection = {
[FieldKey.AGE]: {key: FieldKey.AGE, name: 'Age', kind: FieldKind.FIELD},
[FieldKey.AGE]: {
key: FieldKey.AGE,
name: 'Age',
kind: FieldKind.FIELD,
predefined: true,
},
[FieldKey.ASSIGNED]: {
key: FieldKey.ASSIGNED,
name: 'Assigned To',
Expand Down Expand Up @@ -95,55 +100,88 @@ describe('SearchQueryBuilder', function () {
).toBeInTheDocument();
});

it('can modify the value by clicking into it', async function () {
render(
<SearchQueryBuilder {...defaultProps} initialQuery="browser.name:firefox" />
);
it('can modify the value by clicking into it (single-select)', async function () {
// `age` is a duration filter which only accepts single values
render(<SearchQueryBuilder {...defaultProps} initialQuery="age:-1d" />);

// Should display as "firefox" to start
// Should display as "-1d" to start
expect(
within(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
).getByText('firefox')
screen.getByRole('button', {name: 'Edit value for filter: age'})
).getByText('-1d')
).toBeInTheDocument();

await userEvent.click(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
screen.getByRole('button', {name: 'Edit value for filter: age'})
);
// Should have placeholder text of previous value
expect(screen.getByRole('combobox', {name: 'Edit filter value'})).toHaveAttribute(
'placeholder',
'firefox'
'-1d'
);
await userEvent.click(screen.getByRole('combobox', {name: 'Edit filter value'}));

// Clicking the "Chrome option should update the value"
await userEvent.click(screen.getByRole('option', {name: 'Chrome'}));
expect(screen.getByRole('row', {name: 'browser.name:Chrome'})).toBeInTheDocument();
// Clicking the "+14d" option should update the value
await userEvent.click(screen.getByRole('option', {name: '+14d'}));
expect(screen.getByRole('row', {name: 'age:+14d'})).toBeInTheDocument();
expect(
within(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
).getByText('Chrome')
screen.getByRole('button', {name: 'Edit value for filter: age'})
).getByText('+14d')
).toBeInTheDocument();
});

it('escapes values with spaces and reserved characters', async function () {
it('can modify the value by clicking into it (multi-select)', async function () {
// `browser.name` is a string filter which accepts multiple values
render(
<SearchQueryBuilder {...defaultProps} initialQuery="browser.name:firefox" />
);

// Should display as "firefox" to start
expect(
within(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
).getByText('firefox')
).toBeInTheDocument();

await userEvent.click(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
);
await userEvent.keyboard('some" value{enter}');
// Previous value should be rendered before the input
expect(
within(screen.getByRole('row', {name: 'browser.name:firefox'})).getByText(
'firefox,'
)
).toBeInTheDocument();
await userEvent.click(screen.getByRole('combobox', {name: 'Edit filter value'}));

// Clicking the "Chrome option should add it to the list and commit changes
await userEvent.click(screen.getByRole('option', {name: 'Chrome'}));
expect(
screen.getByRole('row', {name: 'browser.name:[firefox,Chrome]'})
).toBeInTheDocument();
expect(
within(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
).getByText('[firefox,Chrome]')
).toBeInTheDocument();
});

it('escapes values with spaces and reserved characters', async function () {
render(<SearchQueryBuilder {...defaultProps} initialQuery="" />);
await userEvent.click(screen.getByRole('combobox', {name: 'Add a search term'}));
await userEvent.type(
screen.getByRole('combobox', {name: 'Add a search term'}),
'assigned:some" value{enter}'
);
// Value should be surrounded by quotes and escaped
expect(
screen.getByRole('row', {name: 'browser.name:"some\\" value"'})
screen.getByRole('row', {name: 'assigned:"some\\" value"'})
).toBeInTheDocument();
// Display text should be display the original value
expect(
within(
screen.getByRole('button', {name: 'Edit value for filter: browser.name'})
screen.getByRole('button', {name: 'Edit value for filter: assigned'})
).getByText('some" value')
).toBeInTheDocument();
});
Expand Down
85 changes: 83 additions & 2 deletions static/app/components/searchQueryBuilder/useQueryBuilderState.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {type Reducer, useCallback, useReducer} from 'react';
import {
type ParseResultToken,
TermOperator,
type Token,
Token,
type TokenResult,
} from 'sentry/components/searchSyntax/parser';
import {stringifyToken} from 'sentry/components/searchSyntax/utils';
Expand Down Expand Up @@ -35,11 +35,24 @@ type UpdateTokenValueAction = {
value: string;
};

type MultiSelectFilterValueAction = {
token: TokenResult<Token.FILTER>;
type: 'TOGGLE_FILTER_VALUE';
value: string;
};

type DeleteLastMultiSelectFilterValueAction = {
token: TokenResult<Token.FILTER>;
type: 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE';
};

export type QueryBuilderActions =
| DeleteTokenAction
| UpdateFreeTextAction
| UpdateFilterOpAction
| UpdateTokenValueAction;
| UpdateTokenValueAction
| MultiSelectFilterValueAction
| DeleteLastMultiSelectFilterValueAction;

function removeQueryToken(query: string, token: TokenResult<Token>): string {
return (
Expand Down Expand Up @@ -101,6 +114,70 @@ function updateFreeText(
};
}

function updateFilterMultipleValues(
state: QueryBuilderState,
token: TokenResult<Token.FILTER>,
values: string[]
) {
const uniqNonEmptyValues = Array.from(
new Set(values.filter(value => value.length > 0))
);
if (uniqNonEmptyValues.length === 0) {
return {...state, query: replaceQueryToken(state.query, token.value, '')};
}

const newValue =
uniqNonEmptyValues.length > 1
? `[${uniqNonEmptyValues.join(',')}]`
: uniqNonEmptyValues[0];

return {...state, query: replaceQueryToken(state.query, token.value, newValue)};
}

function mutliSelectTokenValue(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function mutliSelectTokenValue(
function multiSelectTokenValue(

last one I promise
(One usage of this function below too)

Copy link
Member Author

Choose a reason for hiding this comment

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

How do I keep messing this up hah

state: QueryBuilderState,
action: MultiSelectFilterValueAction
) {
const tokenValue = action.token.value;

switch (tokenValue.type) {
case Token.VALUE_TEXT_LIST:
case Token.VALUE_NUMBER_LIST:
const values = tokenValue.items.map(item => item.value?.text ?? '');
const containsValue = values.includes(action.value);
const newValues = containsValue
? values.filter(value => value !== action.value)
: [...values, action.value];

return updateFilterMultipleValues(state, action.token, newValues);
default:
if (tokenValue.text === action.value) {
return updateFilterMultipleValues(state, action.token, ['']);
}
return updateFilterMultipleValues(state, action.token, [
tokenValue.text,
action.value,
]);
}
}

function deleteLastMultiSelectTokenValue(
state: QueryBuilderState,
action: DeleteLastMultiSelectFilterValueAction
) {
const tokenValue = action.token.value;

switch (tokenValue.type) {
case Token.VALUE_TEXT_LIST:
case Token.VALUE_NUMBER_LIST:
const newValues = tokenValue.items.slice(0, -1).map(item => item.value?.text ?? '');

return updateFilterMultipleValues(state, action.token, newValues);
default:
return updateFilterMultipleValues(state, action.token, ['']);
}
}

export function useQueryBuilderState({initialQuery}: {initialQuery: string}) {
const initialState: QueryBuilderState = {query: initialQuery};

Expand All @@ -121,6 +198,10 @@ export function useQueryBuilderState({initialQuery}: {initialQuery: string}) {
return {
query: replaceQueryToken(state.query, action.token, action.value),
};
case 'TOGGLE_FILTER_VALUE':
return mutliSelectTokenValue(state, action);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return mutliSelectTokenValue(state, action);
return multiSelectTokenValue(state, action);

case 'DELETE_LAST_MULTI_SELECT_FILTER_VALUE':
return deleteLastMultiSelectTokenValue(state, action);
default:
return state;
}
Expand Down
Loading
Loading