[App Search] Add a Sample Engine CTA panel to the engines table when empty by byronhulcher · Pull Request #94647 · elastic/kibana

byronhulcher · 2021-03-15T22:06:34Z

Summary

Migrates over the Sample Engine CTA from the old repository. This encourages users to get up and running quickly in App Search by using our national parks demo dataset.

Screenshots

QA

Spin up Kibana and Enterprise Search
Log in to Enterprise Search, then App Search, and go to the Engines Overview page
Delete any and all existing engines you have, you need to be on the empty state for the Engines Overview page

Observe the new CTA as in the screenshot above

Click the button

Observe the button goes into a loading/spinner state
Observe you are brought to Engine Detail page for the national-parks-demo engine
Observe a success message at the top of the page

Checklist

Delete any items that are not applicable to this PR.

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

JasonStoltz · 2021-03-23T15:31:57Z

...terprise_search/public/applications/app_search/components/sample_engine_creation_cta/i18n.ts

+
+import { i18n } from '@kbn/i18n';
+
+export const SAMPLE_ENGINE_CREATION_CTA_TITLE = i18n.translate(


Looks like this is a pattern used elsewhere in Kibana, so I'm good with it.

Ping for awareness of this pattern, @constancecchen.

This pattern being "a seperate i18n.ts file", right?

JasonStoltz

A couple of suggestions to consider

JasonStoltz · 2021-03-23T15:40:38Z

...cations/app_search/components/sample_engine_creation_cta/sample_engine_creation_cta.test.tsx

+    });
+
+    it('calls createSampleEngine on click', () => {
+      const wrapper = mountWithIntl(<SampleEngineCreationCta />);


You only need to use mountWithIntl if you are using FormattedMessage. I believe you could shallow in this file.

JasonStoltz · 2021-03-23T15:43:22Z

...cations/app_search/components/sample_engine_creation_cta/sample_engine_creation_cta.test.tsx

+      expect(ctaButton.props().onClick).toEqual(MOCK_ACTIONS.createSampleEngine);
+    });
+
+    it('by default enabled', () => {


[optional] I don't always get this right either, but it's nice to make tests read like sentences.
"CTA button is enabled by default" as opposed to "CTA button by default enabled"

JasonStoltz · 2021-03-23T15:45:27Z

...applications/app_search/components/sample_engine_creation_cta/sample_engine_creation_cta.tsx

+  const { createSampleEngine } = useActions(SampleEngineCreationCtaLogic);
+
+  return (
+    <EuiPanel data-test-subj="SampleEngineCreationCta">


I don't think this data-test-subj, or the one further down in this file are used anywhere. I suggest removing them.

JasonStoltz · 2021-03-23T15:50:00Z

...ns/app_search/components/sample_engine_creation_cta/sample_engine_creation_cta_logic.test.ts

+      });
+
+      afterAll(() => {
+        jest.clearAllMocks();


I suggest moving this to a beforeEach so that your tests are isolated from each other. As it is now you could be asserting expect(flashAPIErrors).toHaveBeenCalledTimes(1) in one test and have it pass, even if it was called in a previous test.

JasonStoltz · 2021-03-23T16:07:37Z

...cations/app_search/components/sample_engine_creation_cta/sample_engine_creation_cta_logic.ts

+  actions: {
+    createSampleEngine: true,
+    onSampleEngineCreationSuccess: true,
+    setIsLoading: (isLoading) => ({ isLoading }),


Actions like this are generally an anti-pattern in Kea.

It is so pervasive in our code that I generally ignore it in reviews. However, since this logic is small (and new?) I want to point it out. When we design logic files we should keep this mind.

The preferable way to do this is:

actions: { createSampleEngine: true, onSampleEngineCreationSuccess: true, onSampleEngineCreationFailure: true, }, reducers: { isLoading: [ false, { createSampleEngine: () => true, onSampleEngineCreationSuccess: () => false, onSampleEngineCreationFailure: () => false, }, ], }, .... try { await http.post('/api/app_search/onboarding_complete', { body, }); actions.onSampleEngineCreationSuccess(); } catch (e) { actions.onSampleEngineCreationFailure(); flashAPIErrors(e); }

I actually like this a lot more!

Sweet. And just FYI, after re-reading this I don't know if I was totally clear about what the anti-pattern is. From that link:

If you read the reducers section above, you'll remember that it's an anti-pattern to only have setThis and setThat actions that only update this or that.

A helpful mental model to understand actions is the one of events in computer programming.

This is something I've mentioned on a couple of PRs previously, but would be new to you:

#83353 (comment)

#86822 (review)

Some lengthy discussion in that second link! I lay out what I perceive as the benefits there.

JasonStoltz · 2021-03-23T16:19:58Z

x-pack/plugins/enterprise_search/server/lib/enterprise_search_request_handler.test.ts

      });
    });
+
+    it('works if resposne contains no json data', async () => {


Suggested change

it('works if resposne contains no json data', async () => {

it('works if response contains no json data', async () => {

JasonStoltz

Nice work!

byronhulcher · 2021-03-29T17:32:32Z

@elasticmachine merge upstream

kibanamachine · 2021-03-29T19:37:38Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1290	1293	+3

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.0MB	2.0MB	+4.6KB

History

💔 Build #115824 failed 522a82a
💔 Build #114999 failed c698f9b
💚 Build #114923 succeeded 7aac42c
💚 Build #114468 succeeded 7924d22
💔 Build #114460 failed e9e44ca5982280477dd34d27a00979e7be89a8f2

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

cc @byronhulcher

…empty (elastic#94647) * Added new onboarding complete route for App Search * Allow responses without JSON bodies in Enterprise Search * New SampleEngineCreationCtaLogic * New SampleEngineCreationCta component * Add SampleEngineCreationCTA to engines EmptyState * Improve SampleEngineCreationCta * Fix spelling error in Enterprise Search request handler test * Improve SampleEngineCreationCtaLogic * Fix types * Fix tests after origin/master merge * Turns out I 'fixed' my tests by removing this test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-03-30T10:51:48Z

💚 Backport successful

✅ 7.x / #95766

This backport PR will be merged automatically after passing CI.

…empty (#94647) (#95766) * Added new onboarding complete route for App Search * Allow responses without JSON bodies in Enterprise Search * New SampleEngineCreationCtaLogic * New SampleEngineCreationCta component * Add SampleEngineCreationCTA to engines EmptyState * Improve SampleEngineCreationCta * Fix spelling error in Enterprise Search request handler test * Improve SampleEngineCreationCtaLogic * Fix types * Fix tests after origin/master merge * Turns out I 'fixed' my tests by removing this test Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>

byronhulcher added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 15, 2021

byronhulcher self-assigned this Mar 15, 2021

byronhulcher force-pushed the demo-engine-cta branch 5 times, most recently from a758024 to e4dc73f Compare March 22, 2021 23:13

byronhulcher added 3 commits March 22, 2021 19:33

Added new onboarding complete route for App Search

500ac52

Allow responses without JSON bodies in Enterprise Search

1cd26f1

New SampleEngineCreationCtaLogic

0e5d075

byronhulcher force-pushed the demo-engine-cta branch from e4dc73f to e9e44ca Compare March 22, 2021 23:34

byronhulcher added 2 commits March 22, 2021 20:17

New SampleEngineCreationCta component

d0877ef

Add SampleEngineCreationCTA to engines EmptyState

7924d22

byronhulcher force-pushed the demo-engine-cta branch from e9e44ca to 7924d22 Compare March 23, 2021 00:17

byronhulcher marked this pull request as ready for review March 23, 2021 02:48

byronhulcher requested review from a team, JasonStoltz and cee-chen and removed request for cee-chen March 23, 2021 02:48

JasonStoltz reviewed Mar 23, 2021

View reviewed changes

JasonStoltz requested changes Mar 23, 2021

View reviewed changes

byronhulcher added 4 commits March 24, 2021 11:02

Improve SampleEngineCreationCta

01a6c8d

Fix spelling error in Enterprise Search request handler test

0b052db

Improve SampleEngineCreationCtaLogic

7aac42c

Merge remote-tracking branch 'origin/master' into demo-engine-cta

c698f9b

JasonStoltz approved these changes Mar 24, 2021

View reviewed changes

Merge remote-tracking branch 'origin/master' into demo-engine-cta

6ce7cf0

byronhulcher added 3 commits March 29, 2021 09:24

Fix types

13c9801

Fix tests after origin/master merge

a117565

Turns out I 'fixed' my tests by removing this test

522a82a

Merge branch 'master' into demo-engine-cta

3f17722

byronhulcher merged commit e91d0d4 into elastic:master Mar 30, 2021

kibanamachine mentioned this pull request Mar 30, 2021

[7.x] [App Search] Add a Sample Engine CTA panel to the engines table when empty (#94647) #95766

Merged


		import { i18n } from '@kbn/i18n';

		export const SAMPLE_ENGINE_CREATION_CTA_TITLE = i18n.translate(

	it('works if resposne contains no json data', async () => {
	it('works if response contains no json data', async () => {

Conversation

byronhulcher commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots

QA

Checklist

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!

JasonStoltz left a comment

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!

JasonStoltz Mar 24, 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!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

byronhulcher commented Mar 29, 2021

Uh oh!

kibanamachine commented Mar 29, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Mar 30, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

byronhulcher commented Mar 15, 2021 •

edited

Loading

JasonStoltz Mar 24, 2021 •

edited

Loading