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.

Sign up for GitHub

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

Jump to bottom

Adding error message in Search Comparison Tool #267

Merged
merged 9 commits into from
Oct 4, 2023
Merged

Adding error message in Search Comparison Tool #267

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
612cad7
adding error message in search comparison tool
sejli Aug 28, 2023
b075fd1
updating test snapshots
sejli Aug 28, 2023
fec1fe2
adding tests for query error
sejli Aug 28, 2023
e99ce99
updating tests
sejli Aug 28, 2023
d5cc342
refactoring QueryError, adding error message to result panel
sejli Sep 13, 2023
2db8475
moving helper text to helpText prop
sejli Sep 14, 2023
a07402e
adding tests and addressing comments
sejli Sep 19, 2023
db6699e
adding more tests, fixing state management, addressing comments
sejli Sep 20, 2023
fb1b670
reverting changes to validateQuery, addressing comments
sejli Sep 23, 2023
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
147 changes: 100 additions & 47 deletions public/components/query_compare/search_result/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,20 @@
import React, { useState } from 'react';
import { EuiPageContentBody } from '@elastic/eui';

import { CoreStart, ChromeBreadcrumb } from '../../../../../../src/core/public';
import { CoreStart } from '../../../../../../src/core/public';
import { SearchConfigsPanel } from './search_components/search_configs/search_configs';
import { SearchInputBar } from './search_components/search_bar';
import { ServiceEndpoints } from '../../../../common';
import { Header } from '../../common/header';
import { SearchResults, QueryError, QueryStringError, SelectIndexError } from '../../../types/index';
import {
SearchResults,
QueryError,
QueryStringError,
SelectIndexError,
initialQueryErrorState,
} from '../../../types/index';
import { ResultComponents } from './result_components/result_components';
import { useSearchRelevanceContext, initialQueryErrorState } from '../../../contexts';
import { useSearchRelevanceContext } from '../../../contexts';

const DEFAULT_QUERY = '{}';

Expand All @@ -26,77 +32,112 @@ export const SearchResult = ({ http }: SearchResultProps) => {
const [queryString2, setQueryString2] = useState(DEFAULT_QUERY);
const [queryResult1, setQueryResult1] = useState<SearchResults>({} as any);
const [queryResult2, setQueryResult2] = useState<SearchResults>({} as any);
const [queryError1, setQueryError1] = useState<QueryError>(initialQueryErrorState);
const [queryError2, setQueryError2] = useState<QueryError>(initialQueryErrorState);
const [searchBarValue, setSearchBarValue] = useState('');

const {
updateComparedResult1,
updateComparedResult2,
selectedIndex1,
selectedIndex2,
setQueryError1,
setQueryError2,
} = useSearchRelevanceContext();

const onClickSearch = () => {
const queryErrors = [{ ...initialQueryErrorState }, { ...initialQueryErrorState }];
const jsonQueries = [{}, {}];

validateQuery(selectedIndex1, queryString1, queryErrors[0]);
jsonQueries[0] = rewriteQuery(searchBarValue, queryString1, queryErrors[0]);
validateQuery(selectedIndex1, queryString1, setQueryError1);
jsonQueries[0] = rewriteQuery(searchBarValue, queryString1, setQueryError1);

validateQuery(selectedIndex2, queryString2, queryErrors[1]);
jsonQueries[1] = rewriteQuery(searchBarValue, queryString2, queryErrors[1]);
validateQuery(selectedIndex2, queryString2, setQueryError2);
jsonQueries[1] = rewriteQuery(searchBarValue, queryString2, setQueryError2);

handleQuery(jsonQueries, queryErrors);
handleSearch(jsonQueries);
};

const validateQuery = (selectedIndex: string, queryString: string, queryError: QueryError) => {
const validateQuery = (
selectedIndex: string,
queryString: string,
setQueryError: React.Dispatch<React.SetStateAction<QueryError>>
) => {
// Check if select an index
if (!selectedIndex.length) {
queryError.selectIndex = SelectIndexError.unselected;
setQueryError((error: QueryError) => ({
...error,
selectIndex: SelectIndexError.unselected,
}));
}

// Check if query string is empty
if (!queryString.length) {
queryError.queryString = QueryStringError.empty;
setQueryError((error: QueryError) => ({
...error,
queryString: QueryStringError.empty,
errorRepsonse: {
body: '',
statusCode: 400,
},
}));
}
};
Copy link
Collaborator

@noCharger noCharger Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Using functional forms of setState makes sense here to handle potential state consistency issue. For validateQuery and rewriteQuery, if it's being refactor to pass state setter instead of the queryError object, the states are updated instantly. Is it a concern since they are helper functions of onClickSearch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

typo: errorRepsonse -> errorResponse

Copy link
Member Author

@sejli sejli Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. Had a discussion offline, and we concluded that the validateQuery helper function does more than what it is supposed to do. Ideally, a validation function should only run validation logic, i.e. it should only return true or false and nothing else; the setting should be done in another function. Previously, a variable is passed into validateQuery and then is set if it meets conditions. That variable is used later to set an error state. I believe that the validateQuery should be used to return true or false in the function that sets states to avoid unnecessary stateful functions, and the error state should be set in a stateful function to maintain consistency. The code before my change seems to do this, will change it back to that.


const rewriteQuery = (searchBarValue: string, queryString: string, queryError: QueryError) => {
const rewriteQuery = (
value: string,
queryString: string,
setQueryError: React.Dispatch<React.SetStateAction<QueryError>>
) => {
if (queryString.trim().length > 0) {
try {
return JSON.parse(queryString.replace(/%SearchText%/g, searchBarValue));
return JSON.parse(queryString.replace(/%SearchText%/g, value));
} catch {
queryError.queryString = QueryStringError.invalid;
setQueryError((error: QueryError) => ({
...error,
queryString: QueryStringError.invalid,
errorRepsonse: {
body: '',
statusCode: 400,
},
}));
}
}
};

const handleQuery = (jsonQueries: any, queryErrors: QueryError[]) => {
let requestBody = {};

// Handle query1
if (queryErrors[0].queryString.length || queryErrors[0].selectIndex.length) {
setQueryError1(queryErrors[0]);
setQueryResult1({} as any);
updateComparedResult1({} as any);
} else if (!queryErrors[0].queryString.length && !queryErrors[0].selectIndex.length) {
requestBody = {
query1: { index: selectedIndex1, ...jsonQueries[0] },
};
const handleQuery = (
queryError: QueryError,
selectedIndex: string,
jsonQuery: any,
updateComparedResult: (result: SearchResults) => void,
setQueryResult: React.Dispatch<React.SetStateAction<SearchResults>>,
setQueryError: React.Dispatch<React.SetStateAction<QueryError>>
) => {
if (queryError.queryString.length || queryError.selectIndex.length) {
setQueryResult({} as any);
updateComparedResult({} as any);
} else if (!queryError.queryString.length && !queryError.selectIndex) {
setQueryError(initialQueryErrorState);
return { index: selectedIndex, ...jsonQuery };
}
};

// Handle query2
if (queryErrors[1].queryString.length || queryErrors[1].selectIndex.length) {
setQueryError2(queryErrors[1]);
setQueryResult2({} as any);
updateComparedResult2({} as any);
} else if (!queryErrors[1].queryString.length && !queryErrors[1].selectIndex.length) {
requestBody = {
...requestBody,
query2: { index: selectedIndex2, ...jsonQueries[1] },
};
}
const handleSearch = (jsonQueries: any) => {
const requestBody = {
query1: handleQuery(
queryError1,
selectedIndex1,
jsonQueries[0],
updateComparedResult1,
setQueryResult1,
setQueryError1
),
query2: handleQuery(
queryError2,
selectedIndex2,
jsonQueries[1],
updateComparedResult2,
setQueryResult2,
setQueryError2
),
};

if (Object.keys(requestBody).length !== 0) {
http
Expand All @@ -115,20 +156,23 @@ export const SearchResult = ({ http }: SearchResultProps) => {
}

if (res.errorMessage1) {
setQueryError1({
...queryErrors[0],
setQueryError1((error: QueryError) => ({
...error,
queryString: res.errorMessage1,
});
errorResponse: res.errorMessage1,
}));
}

if (res.errorMessage2) {
setQueryError2({
...queryErrors[1],
setQueryError2((error: QueryError) => ({
...error,
queryString: res.errorMessage2,
});
errorResponse: res.errorMessage2,
}));
}
})
.catch((error: Error) => {
// eslint-disable-next-line no-console
console.error(error);
});
}
Expand All @@ -149,8 +193,17 @@ export const SearchResult = ({ http }: SearchResultProps) => {
queryString2={queryString2}
setQueryString1={setQueryString1}
setQueryString2={setQueryString2}
queryError1={queryError1}
queryError2={queryError2}
setQueryError1={setQueryError1}
setQueryError2={setQueryError2}
/>
<ResultComponents
queryResult1={queryResult1}
queryResult2={queryResult2}
queryError1={queryError1}
queryError2={queryError2}
/>
<ResultComponents queryResult1={queryResult1} queryResult2={queryResult2} />
</EuiPageContentBody>
</>
);
Expand Down
60 changes: 60 additions & 0 deletions ...are/search_result/result_components/__test__/__snapshots__/result_component.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,26 @@
exports[`Result component Renders result component 1`] = `
<SearchRelevanceContextProvider>
<ResultComponents
queryError1={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryError2={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryResult1={
Object {
"_shards": Object {
Expand Down Expand Up @@ -385,6 +405,26 @@ exports[`Result component Renders result component 1`] = `
}
>
<ResultPanels
queryError1={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryError2={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryResult1={
Object {
"_shards": Object {
Expand Down Expand Up @@ -782,6 +822,16 @@ exports[`Result component Renders result component 1`] = `
className="euiPanel euiPanel--borderRadiusMedium euiPanel--plain euiPanel--noShadow euiPanel--noBorder euiPanel--flexGrowZero euiSplitPanel euiSplitPanel--row"
>
<ResultPanel
queryError={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryResult={
Object {
"_shards": Object {
Expand Down Expand Up @@ -3749,6 +3799,16 @@ exports[`Result component Renders result component 1`] = `
</_EuiSplitPanelInner>
</ResultPanel>
<ResultPanel
queryError={
Object {
"errorResponse": Object {
"body": "",
"statusCode": 200,
},
"queryString": "",
"selectIndex": "",
}
}
queryResult={
Object {
"_shards": Object {
Expand Down
Loading
Loading