Adding error message in Search Comparison Tool #267

sejli · 2023-08-28T20:59:07Z

Description

Displays an indicator in case of a query error upon searching in the Search Comparison Tool

Issues Resolved

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

codecov · 2023-08-28T21:25:17Z

Codecov Report

Merging #267 (fb1b670) into main (dff6559) will increase coverage by 3.67%.
Report is 2 commits behind head on main.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main     #267      +/-   ##
==========================================
+ Coverage   86.27%   89.95%   +3.67%     
==========================================
  Files          15       16       +1     
  Lines         204      209       +5     
  Branches       40       43       +3     
==========================================
+ Hits          176      188      +12     
+ Misses         26       20       -6     
+ Partials        2        1       -1

Flag	Coverage Δ
dashboards-search-relevance	`89.95% <94.11%> (+3.67%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...earch_components/search_configs/search_configs.tsx	`100.00% <ø> (ø)`
public/contexts/index.tsx	`83.33% <ø> (-2.39%)`	⬇️
...search_components/search_configs/search_config.tsx	`80.00% <75.00%> (+30.00%)`	⬆️
...rch_result/result_components/result_components.tsx	`88.23% <100.00%> (+1.56%)`	⬆️
...e/search_result/result_components/result_panel.tsx	`92.85% <100.00%> (+15.07%)`	⬆️
public/types/index.ts	`100.00% <100.00%> (ø)`

noCharger · 2023-08-29T07:00:51Z

Let's include screenshots of 1. the original issue and 2. the result of the fix.

sejli · 2023-08-30T16:18:54Z

@smacrakis @noCharger here's some screenshots of the before and after with a simple Query Error message. Let me know if the actual error report should be included in as well.

Before fix
After fix

noCharger · 2023-08-30T21:57:42Z

@smacrakis @noCharger here's some screenshots of the before and after with a simple Query Error message. Let me know if the actual error report should be included in as well.

Before fix

After fix

I'd want to see the style of error message be consistent with existing ones, using this issue as an example.

noCharger

The UX should be consistent with existing OSD pages. For example, for malformed syntax, we utilize https://elastic.github.io/eui/#/editors-syntax/markdown-editor#error-handling-and-feedback. And we should use toast for additional OpenSearch errors: https://elastic.github.io/eui/#/display/toast#danger

public/types/index.ts

public/components/query_compare/search_result/result_components/result_components.tsx

public/components/query_compare/search_result/result_components/result_panel.tsx

Signed-off-by: Sean Li <[email protected]>

smacrakis · 2023-09-06T14:00:59Z

Although a full error message would be nice, I'm OK with not including a full error message for now and considering that a refinement for the future.
People can debug their queries elsewhere.

Thanks for your work on this!

Signed-off-by: Sean Li <[email protected]>

sejli · 2023-09-13T16:57:40Z

Updated UI with new error messages

Query error (successfully sent request):

Query error (syntax error in code editor, request not sent):

Index not selected error (request not sent):

Open to other ideas @noCharger @KrooshalUX @kgcreative @smacrakis

kgcreative · 2023-09-13T20:27:53Z

This looks great! Nice incremental update here

noCharger · 2023-09-13T21:02:55Z

Can we make "enter a query in ..." msg also in red. I have no preference whether the "result 1" stay in black.

sejli · 2023-09-13T21:20:23Z

Can we make "enter a query in ..." msg also in red. I have no preference whether the "result 1" stay in black.

@noCharger The error message A query is required is also the same location where the code editor syntax error also appears. So, it's possible that the error in that same place would be Query syntax is invalid. Enter a valid query.. Are you suggesting that the error A query is required should be red when there is nothing in the code editory?

This error message is tied to the EuiFormRow component as part of the error prop, which is why it only appears on error. The Enter a query in OpenSearch Query DSL. Use %SearchText% to refer to the text in the search bar. message is an EuiText underneath the code editor, so it won't receive the same error state that the form row gets.

Since the EuiText is unchanged on both sides, I think it's consistent to keep the description to be black. If this is conflicting UI, I can also add a block of code that would basically check if the code editor is empty upon search, and change the EuiText to red if it is. Would love to hear some more thoughts on this.

kgcreative · 2023-09-13T21:36:42Z

I wouldn't change the default component behavior. The Enter a query using OpenSearch Query DSL. Use %Search Text to refer to the text in the search bar. message is intended as description/help text. It does not change color based on state/condition.

noCharger · 2023-09-13T21:54:52Z

The error message A query is required is also the same location where the code editor syntax error also appears. So, it's possible that the error in that same place would be Query syntax is invalid. Enter a valid query.. Are you suggesting that the error A query is required should be red when there is nothing in the code editory?

No I am barely talking about the message "Enter a query using OpenSearch Query DSL. Use %Search Text to refer to the text in the search bar."

This error message is tied to the EuiFormRow component as part of the error prop, which is why it only appears on error. The Enter a query in OpenSearch Query DSL. Use %SearchText% to refer to the text in the search bar. message is an EuiText underneath the code editor, so it won't receive the same error state that the form row gets.

It does not sound to be a reason why the message cannot be in red. @kgcreative I agree that it was designed as a description/help text, however I'd like to discuss whether we can improve the user experience while interacting with errors. For example, if the user has already input the query but it has a syntax error, does the message "Enter a query using OpenSearch Query DSL" option is used. To refer to the text in the search bar, use%Search Text." still make sense to put up on browser? On the other side, because the message "Enter a query using OpenSearch Query DSL" option is used. To refer to the text in the search bar, use%Search Text." has already been displayed yet user still not enter the query, would it make sense to display another error message regarding the same thing?

kgcreative · 2023-09-13T22:28:53Z

Forms in general have different components and sections. (See the form layouts section here: https://oui.opensearch.org/1.3/#/forms/form-layouts)

The text we are discussing, should not dissappear, because it is a hint to the user that 1) It's using Query DSL (in fact, we should link "Query DSL" to the Query DSL documentation), and it lets users know that %Search Text% is the replacement token for the search query. This should be present whether there is text in the input field or not, and it should not dissappear, since it's directly related to the constraints and syntax of the field above it.

In general, we inject any error message text between the form and helper text. I think we can consider improving this from a component/pattern perspective, but i'd rather not customize the stock components, since this will make it so updating the pattern you get those changes for free

noCharger · 2023-09-14T04:38:31Z

Forms in general have different components and sections. (See the form layouts section here: https://oui.opensearch.org/1.3/#/forms/form-layouts)

Thanks, this is a greate example!

      <OuiFormRow label="Text field" helpText="I am some friendly help text.">
        <OuiFieldText name="first" />
      </OuiFormRow>

I agree to keep the helper text since it reminds to use Query DSL and replace specific token. Can we move the content from euiText component to helperText and add a hyperlink for Query DSL? (Assume EuiFormRow support it already)

Signed-off-by: Sean Li <[email protected]>

sejli · 2023-09-14T18:41:18Z

Made the changes that @noCharger requested. Thanks for the input @kgcreative!

noCharger

This looks awesome! Let's tweak on test converage and resolve all conversation.

public/contexts/index.tsx

noCharger · 2023-09-14T23:01:35Z

public/components/query_compare/search_result/result_components/result_panel.tsx

@@ -47,7 +56,9 @@ export const ResultPanel = ({ resultNumber, queryResult }: ResultPanelProps) =>
          </EuiTitle>
        </EuiFlexItem>
      </EuiFlexGroup>
-      {queryResult?.hits?.hits?.length ? (
+      {queryError.errorResponse.statusCode !== 200 || queryError.queryString.length ? (
+        ErrorMessage


should it be <ErrorMessage />?

Also this only have the message, do we consider reveal the status code?

Including the status code would be helpful too. Do you have any suggestions on where to include it? So far, I'm thinking:

Query Error: <status code> <error message>

or

Query Error <error message> Status: <status code>

How about <status code> <error type> - <error message> , if the status code is 5xx, should it still a query error?

public/components/query_compare/search_result/result_components/result_panel.tsx

kgcreative · 2023-09-15T00:58:10Z

Looks so good!

Signed-off-by: Sean Li <[email protected]>

sejli · 2023-09-19T00:13:49Z

While writing tests to improve code coverage, I was having difficulty figuring out how to improve test coverage by simulating onChange. I went down the rabbit hole exploring Enzyme utilities and found this discussion that outlines the proper way to change component event listeners. Looking through our repo, we can definitely improve code coverage a bit by testing those onChange functions that are bundled with React components (Exhibit A and B).

Signed-off-by: Sean Li <[email protected]>

sejli · 2023-09-20T21:16:07Z

Addressed comments, added the status code to the error message as well.

I also noticed that there are many instances where setState was not used correctly. As per this SlackOverflow answer, instances where setState uses the previous state was not implemented correctly. Updated these since they were creating inconsistencies with some of my QueryError changes.

Condensed these repeated if statements into a function and renamed other functions to match accordingly as well. There's definitely more refactoring that can be done in this file, will make note in an issue.

noCharger · 2023-09-21T16:50:58Z

Addressed comments, added the status code to the error message as well.

The new error msg layout looks fine for me.

I also noticed that there are many instances where setState was not used correctly

Can you provide some specific examples? Do you mean that onClickSearch must re-render query failures before returning?

Condensed these repeated if statements into a function and renamed other functions to match accordingly as well. There's definitely more refactoring that can be done in this file, will make note in an issue.

Yes, please also link the new issue to #191 for better tracking

noCharger · 2023-09-22T13:57:09Z

public/contexts/index.tsx

 export const initialQueryErrorState: QueryError = {
  selectIndex: '',
  queryString: '',
+  errorResponse: {
+    body: '',
+    statusCode: 200,
+  },
 };


Looks duplicate to https://github.com/opensearch-project/dashboards-search-relevance/pull/267/files#diff-f2db82661845cc1032e54e64e485085dd3b86a610a60ae90e8e7d08b5735407fR68-R75

noCharger · 2023-09-22T14:18:09Z

public/components/query_compare/search_result/index.tsx

+  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,
+        },
+      }));
    }
  };


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?

typo: errorRepsonse -> errorResponse

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.

noCharger

Overall looks great. Left some comments for quick fix. It's also a good start thinking about the refactoring.

Signed-off-by: Sean Li <[email protected]>

sejli · 2023-10-04T19:00:02Z

Integration tests pass on local, but are failing for an unknown reason on GitHub actions. We have created an issue here to track.

noCharger · 2023-10-04T19:16:41Z

Tracking the CI failure opensearch-project/opensearch-dashboards-functional-test#902

* adding error message in search comparison tool Signed-off-by: Sean Li <[email protected]> * updating test snapshots Signed-off-by: Sean Li <[email protected]> * adding tests for query error Signed-off-by: Sean Li <[email protected]> * updating tests Signed-off-by: Sean Li <[email protected]> * refactoring QueryError, adding error message to result panel Signed-off-by: Sean Li <[email protected]> * moving helper text to helpText prop Signed-off-by: Sean Li <[email protected]> * adding tests and addressing comments Signed-off-by: Sean Li <[email protected]> * adding more tests, fixing state management, addressing comments Signed-off-by: Sean Li <[email protected]> * reverting changes to validateQuery, addressing comments Signed-off-by: Sean Li <[email protected]> --------- Signed-off-by: Sean Li <[email protected]> (cherry picked from commit 6fb31c8) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* adding error message in search comparison tool * updating test snapshots * adding tests for query error * updating tests * refactoring QueryError, adding error message to result panel * moving helper text to helpText prop * adding tests and addressing comments * adding more tests, fixing state management, addressing comments * reverting changes to validateQuery, addressing comments --------- (cherry picked from commit 6fb31c8) Signed-off-by: Sean Li <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

sejli requested review from macohen, mingshl, msfroh and noCharger as code owners August 28, 2023 20:59

noCharger reviewed Aug 30, 2023

View reviewed changes

evankielley mentioned this pull request Sep 1, 2023

[Good first issue] Update message shown when a single index is selected in search comparison tool #173

Closed

sejli added 4 commits September 5, 2023 19:22

adding error message in search comparison tool

612cad7

Signed-off-by: Sean Li <[email protected]>

updating test snapshots

b075fd1

Signed-off-by: Sean Li <[email protected]>

adding tests for query error

fec1fe2

Signed-off-by: Sean Li <[email protected]>

updating tests

e99ce99

Signed-off-by: Sean Li <[email protected]>

sejli force-pushed the issue-262 branch from de7c52d to e99ce99 Compare September 6, 2023 02:23

refactoring QueryError, adding error message to result panel

d5cc342

Signed-off-by: Sean Li <[email protected]>

moving helper text to helpText prop

2db8475

Signed-off-by: Sean Li <[email protected]>

noCharger reviewed Sep 14, 2023

View reviewed changes

adding tests and addressing comments

a07402e

Signed-off-by: Sean Li <[email protected]>

adding more tests, fixing state management, addressing comments

db6699e

Signed-off-by: Sean Li <[email protected]>

noCharger reviewed Sep 22, 2023

View reviewed changes

noCharger approved these changes Sep 22, 2023

View reviewed changes

reverting changes to validateQuery, addressing comments

fb1b670

Signed-off-by: Sean Li <[email protected]>

noCharger merged commit 6fb31c8 into opensearch-project:main Oct 4, 2023
10 of 12 checks passed

sejli added the backport 2.x label Oct 4, 2023

github-actions bot mentioned this pull request Oct 4, 2023

[Backport 2.x] Adding error message in Search Comparison Tool #305

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding error message in Search Comparison Tool #267

Adding error message in Search Comparison Tool #267

sejli commented Aug 28, 2023

codecov bot commented Aug 28, 2023 •

edited

Loading

noCharger commented Aug 29, 2023

sejli commented Aug 30, 2023

noCharger commented Aug 30, 2023 •

edited

Loading

noCharger left a comment

smacrakis commented Sep 6, 2023 •

edited

Loading

sejli commented Sep 13, 2023 •

edited

Loading

kgcreative commented Sep 13, 2023

noCharger commented Sep 13, 2023

sejli commented Sep 13, 2023

kgcreative commented Sep 13, 2023

noCharger commented Sep 13, 2023

kgcreative commented Sep 13, 2023

noCharger commented Sep 14, 2023

sejli commented Sep 14, 2023

noCharger left a comment

noCharger Sep 14, 2023

noCharger Sep 14, 2023

sejli Sep 15, 2023

noCharger Sep 19, 2023

kgcreative commented Sep 15, 2023

sejli commented Sep 19, 2023

sejli commented Sep 20, 2023

noCharger commented Sep 21, 2023 •

edited

Loading

noCharger Sep 22, 2023

noCharger Sep 22, 2023 •

edited

Loading

noCharger Sep 22, 2023

sejli Sep 22, 2023 •

edited

Loading

noCharger left a comment

sejli commented Oct 4, 2023 •

edited

Loading

noCharger commented Oct 4, 2023

Adding error message in Search Comparison Tool #267

Adding error message in Search Comparison Tool #267

Conversation

sejli commented Aug 28, 2023

Description

Issues Resolved

codecov bot commented Aug 28, 2023 • edited Loading

Codecov Report

noCharger commented Aug 29, 2023

sejli commented Aug 30, 2023

noCharger commented Aug 30, 2023 • edited Loading

noCharger left a comment

Choose a reason for hiding this comment

smacrakis commented Sep 6, 2023 • edited Loading

sejli commented Sep 13, 2023 • edited Loading

kgcreative commented Sep 13, 2023

noCharger commented Sep 13, 2023

sejli commented Sep 13, 2023

kgcreative commented Sep 13, 2023

noCharger commented Sep 13, 2023

kgcreative commented Sep 13, 2023

noCharger commented Sep 14, 2023

sejli commented Sep 14, 2023

noCharger left a comment

Choose a reason for hiding this comment

noCharger Sep 14, 2023

Choose a reason for hiding this comment

noCharger Sep 14, 2023

Choose a reason for hiding this comment

sejli Sep 15, 2023

Choose a reason for hiding this comment

noCharger Sep 19, 2023

Choose a reason for hiding this comment

kgcreative commented Sep 15, 2023

sejli commented Sep 19, 2023

sejli commented Sep 20, 2023

noCharger commented Sep 21, 2023 • edited Loading

noCharger Sep 22, 2023

Choose a reason for hiding this comment

noCharger Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

noCharger Sep 22, 2023

Choose a reason for hiding this comment

sejli Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

noCharger left a comment

Choose a reason for hiding this comment

sejli commented Oct 4, 2023 • edited Loading

noCharger commented Oct 4, 2023

codecov bot commented Aug 28, 2023 •

edited

Loading

noCharger commented Aug 30, 2023 •

edited

Loading

smacrakis commented Sep 6, 2023 •

edited

Loading

sejli commented Sep 13, 2023 •

edited

Loading

noCharger commented Sep 21, 2023 •

edited

Loading

noCharger Sep 22, 2023 •

edited

Loading

sejli Sep 22, 2023 •

edited

Loading

sejli commented Oct 4, 2023 •

edited

Loading