[Enterprise Search] Fix bug in Add Schema modal #104024

scottybollinger · 2021-06-30T22:05:31Z

Summary

This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the body property was missed in the error object.

closes https://github.com/elastic/workplace-search-team/issues/1784

This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object.

…s no message

byronhulcher · 2021-07-01T13:41:07Z

x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts

@@ -31,12 +31,13 @@ interface Options {
  isQueued?: boolean;
 }

+// TODO I know our error messages from the BE are not i18n-ized but should this be?


@constancecchen

Currently none of our error messages from the BE are localized, but if this one is in public/ it has access to i18n, so we should likely i18n it just in case.

Also want to add it wouldn't be my preference to export this const. I'd just check for the static 'An unexpected error occurred' string in the schema logic file. It's short, easy to read, and gives developers human-readable context over a variable that means nothing if they don't open another file to check it

Oh wait, sorry, just saw we're using it in the actual logic file, not just the test. Hm, sec

I'll i18n this

yakhinvadim

This looks great, thanks, Byron and Scotty! 🙌

P.S: really liked that we also verify scrollTo(0,0) behavior!

cee-chen · 2021-07-01T16:23:43Z

...public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts

        if (isAdding) {
-          actions.onSchemaSetFormErrors(e?.message);
+          // We expect body.message to be a string[] for actions.onSchemaSetFormErrors
+          const message: string[] = e?.body?.message || [defaultErrorMessage];
+          actions.onSchemaSetFormErrors(message);
        } else {
          flashAPIErrors(e);
        }


Why not just add the e?.body?.message check into the conditional and let invalid error formats fall back to flashAPIErrors, instead having to manually fall back to defaultErrorMessage?

Suggested change

if (isAdding) {

actions.onSchemaSetFormErrors(e?.message);

// We expect body.message to be a string[] for actions.onSchemaSetFormErrors

const message: string[] = e?.body?.message || [defaultErrorMessage];

actions.onSchemaSetFormErrors(message);

} else {

flashAPIErrors(e);

}

const errorMessage: string[] = e?.body?.message;

if (isAdding && errorMessage) {

actions.onSchemaSetFormErrors(errorMessage);

} else {

flashAPIErrors(e);

}

I think this would be a slight functional difference, in that when the message is missing, it would appear as a Flash Message error. actions.onSchemaSetFormErrors sets addFormFieldErrors which is used inside a SchemaAddFieldModal. I think we'd still want the default error message to appear in the SchemaAddFieldModal.

I don't have a strong opinion either way. I guess I'm leaning slightly towards showing the error in the modal, closer to the user action that lead to the error. But I see the point in showing it as a flash message: if the error is unexpected, it's probably not caused by user action.

I will merge the PR as-is because the code fixes the original bug and is already tested.

scottybollinger · 2021-07-01T21:26:09Z

@elasticmachine merge upstream

kibanamachine · 2021-07-02T01:40:00Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	2.1MB	2.1MB	+2.5KB

History

💔 Build #136172 failed 3a6f3d0
💚 Build #135972 succeeded 50ed764
💔 Build #135837 failed cbbf500

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* [Enterprise Search] Fix bug in Add Schema modal This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object. * Use default message when error response for actions.setServerField has no message * i18n-ize default error message * Remove TODO Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-07-02T14:28:40Z

💚 Backport successful

Status	Branch	Result
✅	7.14

This backport PR will be merged automatically after passing CI.

* [Enterprise Search] Fix bug in Add Schema modal This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object. * Use default message when error response for actions.setServerField has no message * i18n-ize default error message * Remove TODO Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co> Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

In elastic#104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array.

…ing state (#104360) * Fix an issue from previous PR In #104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name

…ing state (elastic#104360) * Fix an issue from previous PR In elastic#104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name

…ing state (#104360) (#104636) * Fix an issue from previous PR In #104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>

* [Enterprise Search] Fix bug in Add Schema modal This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object. * Use default message when error response for actions.setServerField has no message * i18n-ize default error message * Remove TODO Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…ing state (elastic#104360) * Fix an issue from previous PR In elastic#104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name

) * [Enterprise Search] Fix bug in Add Schema modal (#104024) * [Enterprise Search] Fix bug in Add Schema modal This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object. * Use default message when error response for actions.setServerField has no message * i18n-ize default error message * Remove TODO Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> * [Workplace Search] Fix bug where error was behind modal stuck in loading state (#104360) * Fix an issue from previous PR In #104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co> Co-authored-by: Byron Hulcher <byronhulcher@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[Enterprise Search] Fix bug in Add Schema modal

cbbf500

This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object.

scottybollinger added release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 30, 2021

scottybollinger requested a review from a team June 30, 2021 22:05

Use default message when error response for actions.setServerField ha…

50ed764

…s no message

scottybollinger requested a review from a team July 1, 2021 13:20

byronhulcher reviewed Jul 1, 2021

View reviewed changes

yakhinvadim approved these changes Jul 1, 2021

View reviewed changes

cee-chen reviewed Jul 1, 2021

View reviewed changes

Merge branch 'master' into scottybollinger/schema-bug

3a6f3d0

scottybollinger enabled auto-merge (squash) July 1, 2021 21:26

scottybollinger disabled auto-merge July 1, 2021 21:26

byronhulcher added 2 commits July 1, 2021 19:27

i18n-ize default error message

906c4b9

Remove TODO

299c963

yakhinvadim merged commit 0681120 into elastic:master Jul 2, 2021

kibanamachine mentioned this pull request Jul 2, 2021

[7.14] [Enterprise Search] Fix bug in Add Schema modal (#104024) #104254

Merged

scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Jul 5, 2021

Fix an issue from previous PR

3295dc4

In elastic#104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array.

spalger mentioned this pull request Jul 7, 2021

[7.x] [Enterprise Search] Fix bug in Add Schema modal (#104024) #104702

Merged

spalger added the v7.15.0 label Jul 7, 2021

scottybollinger deleted the scottybollinger/schema-bug branch March 15, 2022 15:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Fix bug in Add Schema modal #104024

[Enterprise Search] Fix bug in Add Schema modal #104024

scottybollinger commented Jun 30, 2021

byronhulcher Jul 1, 2021

cee-chen Jul 1, 2021

cee-chen Jul 1, 2021

cee-chen Jul 1, 2021

byronhulcher Jul 1, 2021

yakhinvadim left a comment

cee-chen Jul 1, 2021 •

edited

Loading

byronhulcher Jul 1, 2021

yakhinvadim Jul 2, 2021

scottybollinger commented Jul 1, 2021

kibanamachine commented Jul 2, 2021

kibanamachine commented Jul 2, 2021

[Enterprise Search] Fix bug in Add Schema modal #104024

[Enterprise Search] Fix bug in Add Schema modal #104024

Conversation

scottybollinger commented Jun 30, 2021

Summary

byronhulcher Jul 1, 2021

Choose a reason for hiding this comment

cee-chen Jul 1, 2021

Choose a reason for hiding this comment

cee-chen Jul 1, 2021

Choose a reason for hiding this comment

cee-chen Jul 1, 2021

Choose a reason for hiding this comment

byronhulcher Jul 1, 2021

Choose a reason for hiding this comment

yakhinvadim left a comment

Choose a reason for hiding this comment

cee-chen Jul 1, 2021 • edited Loading

Choose a reason for hiding this comment

byronhulcher Jul 1, 2021

Choose a reason for hiding this comment

yakhinvadim Jul 2, 2021

Choose a reason for hiding this comment

scottybollinger commented Jul 1, 2021

kibanamachine commented Jul 2, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Jul 2, 2021

💚 Backport successful

cee-chen Jul 1, 2021 •

edited

Loading