[App Search] Final Document Creation API, Logic, & Summary/Error views by cee-chen · Pull Request #86822 · elastic/kibana

cee-chen · 2020-12-22T18:49:42Z

Summary

This is the (hopefully!) final PR for the Document Creation component/flyout. It's significantly different from the standalone UI code in that it has been heavily cleaned up, modernized to use Kea logic/state, converted from custom/STUI components to EUI, and improved UI & UX wise (flyout over modal, more specific JSON errors, unifying DocumentCreationButtons, etc.).

In this final PR, users should be able to:

Upload JSON via text or file and have new documents show up immediately in the Documents view
See accurate document summaries and/or errors after uploading JSON

Screencaps

Paste text JSON upload

File upload

Invalid documents

QA

Valid documents testing

Confirm that uploading the default national parks JSON snippet for Paste JSON works without errors
Upload a valid JSON file (e.g. video-games.json in our standalone repo) and confirm that it uploads without errors
Confirm the # of documents is correct and the listed schema fields are all present and correct

Large documents testing

Go to https://github.com/elastic/ent-search/pull/435 and download the 50MB+ JSON file that Byron linked
Open your devtools and track XHR calls pre-emptively
Upload the file and confirm that a yellow warning banner shows during upload:
Confirm that multiple XHR calls (batched by 100 documents) are being successfully made

Error testing

Paste Text: Invalid JSON
- Type the text invalid into the textarea and submit.
- Confirm you see the error: JSON.parse: unexpected character at line 1 column 1 of the JSON data
- Type just the word true, false, null, or 1 into the textarea and submit.
- Confirm that you see the error: Document contents must be a valid JSON array or object.
- Type/paste {"ok": {"__proto__": "not ok"}} into the textarea and submit.
- Confirm that you see the error:
Upload file: Invalid JSON
- Create a file and just populate it with invalid. Name it test.json (or anything .json)
- Upload the above file and confirm you see the following JSON parse error:

Invalid document testing

Bad schema
- Create a sample engine if don't already have one
- In the sample engine, go to Documents > Index Document, and click Paste JSON
- In the example JSON, scroll down to any visitor key and change the value from a number to text, e.g. "visitors": "test",
- Click continue and confirm that you see the following invalid document w/ the listed error:
Invalid keys
- In any engine > Documents > Index Document > Paste JSON, replace the textarea with: {"test.key": "value"}
- Click continue and confirm you see the following invalid document w/ the listed error:

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

- errors can/should be shown in the EuiFlyoutBody banner (better UX since the JSON/file is right there for reference) vs its own page - No need to distinguish between ShowErrorSummary and ShowSuccessSummary + placeholder Summary view for now

- split up into subcomponents for easier reading/testing

cee-chen · 2020-12-22T18:56:12Z

@JasonStoltz Since so much code was refactored from the standalone UI, it would be awesome if you could go through and confirm/check off off the QA steps in the PR description to confirm that all behavior is still functioning as before. Please feel free to go beyond them also try and break this component as much as possible.

Thanks again, I know this is a super huge PR and tried to split the commits into logical / separate pieces as much as possible (but there's still just a ton of code/tests).

cee-chen · 2020-12-22T21:42:26Z

.../public/applications/app_search/components/document_creation/document_creation_logic.test.ts

+  const mount = (defaults?: object) => {
+    if (!defaults) {
+      resetContext({});
+    } else {
+      resetContext({
+        defaults: {
+          enterprise_search: {
+            app_search: {
+              document_creation_logic: {
+                ...defaults,
+              },
+            },
+          },
+        },
+      });
+    }


I'm thinking about writing a reusable test helper for this logic at some point in the future 🤔 some day!!

I'd be cool if it could extract the path from the logic automatically.

const mount = new Mounter(EngineLogic) ... mount() mount({ engineName: "foo" })

Follow-up: #87247

cee-chen · 2020-12-22T21:45:48Z

.../public/applications/app_search/components/document_creation/document_creation_logic.test.ts

+        // NOTE: I can't seem to reproduce this in a production setting.
+        it('handles errors returned from the API', async () => {


I'm not sure if I should keep this comment or not, I haven't been able to actually QA/repro this functionality (getting a summary.errors response) in production, but our documents endpoint does seem to indicate it's there (if I'm reading the Ruby code correctly, which it's very possible I'm not).

That being said there's other scenarios that realistically wouldn't happen that we're still guarding for (such as a non-file or corrupted file being submitted) but not actively QAing for, so if that's one of those scenarios maybe I should just not leave the comment because it's not super helpful.

I don't have an opinion here.

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts

JasonStoltz · 2020-12-23T14:48:18Z

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts

+
+      try {
+        const responses = await Promise.all(promises);
+        const summary: DocumentCreationSummary = {


This is not a change request. I wonder if a summary would have been better as a selector. Store the raw response, and use a summary selector to parse up the response into a summary. Just something to break up this monster logic file a bit. The parseSummary could even be its own function that could be unit tested independently.

Instead of a monster summary you could even have more fine grained selectors that parse up the response as needed.

Super interesting, I really like that idea! I think my only hangup there is storing response in state, just because that feels a little weird since we're not really using it directly. I definitely agree the merging logic/parseSummary does feel like it could be a separate utility for sure.

Yeah, something to think about. State doesn't necessarily need to be used directly.

JasonStoltz · 2020-12-23T14:51:09Z

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts

+        if (summary.errors.length > 0) {
+          actions.setErrors(summary.errors);
+        } else {
+          actions.setSummary(summary);


A pattern to try out in the future would be to avoid individual set methods like this and instead have "success" and "failure" actions for listeners.

https://kea.js.org/docs/guide/concepts#listeners

So instead of calling setErrors on failure, and setSummary + setCreationStep on success ... call uploadDocumentsFailure or uploadDocumentsSuccess respectively, and handle setting errors, summary, and creationStep in reducers that listen for those actions.

I'd love to have a team session (maybe a tech talk or even just a rubber ducky) about this sometime actually - I struggle a little bit about writing things "the Kea way" because it feels super unintuitive to me. Like, I get that using reducers saves us extra actions.something calls, but to me having the action fns listed is really explicit and helpful from a developer POV as opposed to me having to ctrl+F all over the file to figure out what all vars/values an action happens to affect (vs. having them listed clearly in a row in one place).

Definitely would be super helpful for me to discuss this as a team!

Yeah, its advice I've heard more than once, not only in the Kea context but also for Redux. As far as I understand it is common wisdom. It would be interesting to hear arguments against though, for sure.

Ya I'd def love to see a code diff of what this logic file for example would look like. The failure/success actions make sense & are fairly straightforward, what's tripping me up I think is what to replace setWarnings with since it feels like that goes along with setErrors and setSummary.

I think I'll leave this as-is for now since I'm not sure what the 'ideal' code should look like and I'd rather walk it through with someone who's got a clearer picture in their head than me, but I'm very very interested in doing that w/ someone next year!

I think what's also extra weird for me for this specific logic file / use case is that the summary can also include "errors" (aka invalid documents) which is a failure state, so it's not 100% a success state... and it's really hard to wrap my mind around that / describe that accurately via method names other than just 'showSummary'/'setSummary'.

I think I'm going to go ahead and merge this PR in since I have another test helper PR that depends on this work, but I'll see if I can't open up a new PR with your proposed changes as a follow-up and continue the discussion there.

Oh yeah sorry, this is totally non-blocking feedback. I don't think you need to make that change now, or ever. I think there's low value in going back and rewriting things in that style. I think there can be value for new code that we write, however, which is why I opened up the conversation.

I'm using that style now in a new logic file that I'm writing, so your value has been realized!! 😁

One thing that I'd also like to chat about (maybe in the PR that I'll be opening shortly) is naming for actions that follow this style. I'll admit a lot of this is habit on my part, but historically for functions I try to name them with verbSomething (e.g., setX, fetchY, getZ) instead of a name that sounds like a noun - this makes it slightly easier for me to distinguish functions from variables at a glance. That breaks a bit for this style of writing logic files, so I tried out writing story actions with an onX prefix - e.g., onUploadDocumentsSuccess, onFetchY, and so forth.

Let me know what you think about that naming convention? I'm a little torn because onX var names makes me think of jQuery and callback hells for some reason but I also think it makes sense in this context.

Haha. I think a past tense verb would be appropriate for an action name.

uploadDocumentsSucceeded

So you're saying "what happened" in code as you write it.

Hmm! My only thought is that could definitely be mistaken for a boolean at a glance - we've used past tense for boolean states in the standalone UI. Normally I prefer a isX/hasY/didZ prefix/naming to indicate bools, but we're currently not consistent on that.

It also could be I'm being too prescriptive about my var/fn/bool names based on my past experiences - I've found it super helpful previously, but it's potentially not anymore since we have Typescript and Typescript should in theory be checking and documenting types for us.

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts

...ugins/enterprise_search/public/applications/app_search/components/document_creation/utils.ts

...rch/public/applications/app_search/components/document_creation/document_creation_flyout.tsx

JasonStoltz · 2020-12-23T19:54:04Z

.../public/applications/app_search/components/document_creation/document_creation_logic.test.ts

+
+        it('handles client-side errors', async () => {
+          const { http } = HttpLogic.values;
+          const promise = (http.post as jest.Mock).mockReturnValueOnce(new Error());


I don't under stand this. How does promise get assigned a promise value here?. You're return value that you are returning is not a promise 🤔

I'm not actually sure.... but for some reason the test fails / doesn't work without it :dead_inside: I think it's because of the await Promise.all(promises) on line 187.

JasonStoltz

I left a couple of small comments but I am generally OK with this code. I tested it out, it all works well. There's great unit test coverage. I have no concerns.

cee-chen · 2021-01-04T16:11:59Z

@elasticmachine merge upstream

kibanamachine · 2021-01-04T18:24:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6d9f9b7

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1034	1054	+20

Async chunks

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

id	before	after	diff
`enterpriseSearch`	1.8MB	1.8MB	+64.9KB

Distributable file count

id	before	after	diff
`default`	47266	48027	+761

History

💚 Build #96216 succeeded 0fe6f1c
💔 Build #96204 failed 60b6b81b1f1b63e0df5646a272d12160ac4248c2
💚 Build #96089 succeeded 0949eea
💚 Build #96046 succeeded 746eccc

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

elastic#86822) * [Setup] Server API route * [Cleanup] Remove unnecessary DocumentCreationSteps - errors can/should be shown in the EuiFlyoutBody banner (better UX since the JSON/file is right there for reference) vs its own page - No need to distinguish between ShowErrorSummary and ShowSuccessSummary + placeholder Summary view for now * Add DocumentCreationLogic file upload logic * Update creation form components to show error/warning feedback * Add final post-upload summary view - split up into subcomponents for easier reading/testing * [lint] oops, double licenses * [PR feedback] map -> forEach * [PR feedback] Reset form state on flyout close Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

#86822) (#87211) * [Setup] Server API route * [Cleanup] Remove unnecessary DocumentCreationSteps - errors can/should be shown in the EuiFlyoutBody banner (better UX since the JSON/file is right there for reference) vs its own page - No need to distinguish between ShowErrorSummary and ShowSuccessSummary + placeholder Summary view for now * Add DocumentCreationLogic file upload logic * Update creation form components to show error/warning feedback * Add final post-upload summary view - split up into subcomponents for easier reading/testing * [lint] oops, double licenses * [PR feedback] map -> forEach * [PR feedback] Reset form state on flyout close Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cee-chen added 5 commits December 22, 2020 08:43

[Setup] Server API route

62b8881

Add DocumentCreationLogic file upload logic

eeccbf2

Update creation form components to show error/warning feedback

15fd00a

Add final post-upload summary view

746eccc

- split up into subcomponents for easier reading/testing

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Dec 22, 2020

cee-chen requested review from a team and JasonStoltz December 22, 2020 18:49

cee-chen changed the title ~~[App Search] Final Document creation API, Logic, & Summary/Error views~~ [App Search] Final Document Creation API, Logic, & Summary/Error views Dec 22, 2020

cee-chen commented Dec 22, 2020

View reviewed changes

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts Show resolved Hide resolved

[lint] oops, double licenses

0949eea

JasonStoltz reviewed Dec 23, 2020

View reviewed changes

...earch/public/applications/app_search/components/document_creation/document_creation_logic.ts Show resolved Hide resolved

JasonStoltz reviewed Dec 23, 2020

View reviewed changes

...ugins/enterprise_search/public/applications/app_search/components/document_creation/utils.ts Show resolved Hide resolved

JasonStoltz reviewed Dec 23, 2020

View reviewed changes

...rch/public/applications/app_search/components/document_creation/document_creation_flyout.tsx Show resolved Hide resolved

cee-chen added 2 commits December 23, 2020 11:11

Merge remote-tracking branch 'upstream/master' into document-creation-3

5ec23c4

[PR feedback] map -> forEach

f306732

JasonStoltz reviewed Dec 23, 2020

View reviewed changes

JasonStoltz approved these changes Dec 23, 2020

View reviewed changes

[PR feedback] Reset form state on flyout close

0fe6f1c

cee-chen force-pushed the document-creation-3 branch from 60b6b81 to 0fe6f1c Compare December 23, 2020 21:46

Merge branch 'master' into document-creation-3

6d9f9b7

cee-chen merged commit 5241603 into elastic:master Jan 4, 2021

cee-chen deleted the document-creation-3 branch January 4, 2021 19:11

cee-chen mentioned this pull request Jan 4, 2021

[7.x] [App Search] Final Document Creation API, Logic, & Summary/Error views (#86822) #87211

Merged

This was referenced Jan 4, 2021

[Enterprise Search] Kea mount test helper #87247

Merged

[App Search] EnginesLogic + minor UX fix #87561

Merged

JasonStoltz mentioned this pull request Mar 24, 2021

[App Search] Add a Sample Engine CTA panel to the engines table when empty #94647

Merged

10 tasks

		// NOTE: I can't seem to reproduce this in a production setting.
		it('handles errors returned from the API', async () => {

Conversation

cee-chen commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screencaps

Paste text JSON upload

File upload

Invalid documents

QA

Valid documents testing

Large documents testing

Error testing

Invalid document testing

Checklist

Uh oh!

cee-chen commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JasonStoltz Dec 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Jan 4, 2021

Uh oh!

kibanamachine commented Jan 4, 2021

💚 Build Succeeded

Metrics [docs]

cee-chen commented Dec 22, 2020 •

edited

Loading

cee-chen commented Dec 22, 2020 •

edited

Loading

JasonStoltz Dec 23, 2020 •

edited

Loading

cee-chen Dec 23, 2020 •

edited

Loading

cee-chen Jan 6, 2021 •

edited

Loading

JasonStoltz Dec 23, 2020 •

edited

Loading