[App Search] Migrate Create Engine view by byronhulcher · Pull Request #89816 · elastic/kibana

byronhulcher · 2021-01-30T02:06:41Z

Summary

This PR migrates the Create Engine view from ent-search, with improved test coverage and no custom STUI components.

Screenshots

Create Engine view: Empty state

Create Engine view: Supported languages

Create Engine view: Acceptable name

Create Engine view: Sanitized name

Create Engine view: Sanitized name with length of 0

Create Engine view: Duplicate engine name error state

Engine Overview view: Success message after engine creation

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
Documentation was added for features that require explanation or tutorials
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

…ll firing??)

byronhulcher · 2021-01-30T02:08:00Z

.../enterprise_search/public/applications/app_search/components/create_engine/create_engine.tsx

  const { name, rawName, language } = useValues(CreateEngineLogic);
  const { setLanguage, setRawName, submitEngine } = useActions(CreateEngineLogic);

+  // TODO these need to come from AppLogic and/or the Enterprise Search server


If you're following along commit-by-commit I remove this in 207f8b

byronhulcher · 2021-01-30T02:08:20Z

x-pack/plugins/enterprise_search/public/applications/app_search/routes.ts


 export const ENGINES_PATH = '/engines';
-export const CREATE_ENGINES_PATH = `${ENGINES_PATH}/new`;
+export const CREATE_ENGINES_PATH = `/create_engine`;


@zumwalt is good with this change #89156 (comment)

byronhulcher · 2021-01-30T02:09:16Z

...e_search/public/applications/app_search/components/engine_overview/engine_overview_empty.tsx

          </EuiButton>
        </EuiPageHeaderSection>
      </EuiPageHeader>
+      <FlashMessages />


I need this for displaying the setQueuedSuccessMessage from CreateEngineLogic.actions.onCreateEngineSuccess

byronhulcher · 2021-01-30T02:09:25Z

...search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx

          </h1>
        </EuiTitle>
      </EuiPageHeader>
+      <FlashMessages />


I don't need this here... but it seems silly not to include this the normal view if I'm including it in the empty state view

byronhulcher · 2021-01-30T02:13:30Z

@constancecchen how do you handle decisions about release notes?

cee-chen · 2021-02-09T16:56:45Z

...terprise_search/public/applications/app_search/components/engines/components/empty_state.tsx

                defaultMessage="Create an engine"
              />
-            </EuiButton>
+            </EuiButtonTo>


Awesome catch on this button! 🎉

cee-chen · 2021-02-09T16:59:36Z

...ise_search/public/applications/app_search/components/engines/components/empty_state.test.tsx

-    expect(mockTelemetryActions.sendAppSearchTelemetry).toHaveBeenCalled();
+    it('sends a user to engine creation', () => {
+      expect(button.prop('to')).toEqual('/engine_creation');
+    });


It's not clear to me why this had to be split up into separate it blocks / beforeEach's etc. Just IMO, neither the component nor the test descriptions are distinct enough to warrant this extra amount of cruft. I would probably have opted to added a single line of expect(button.prop('to')).toEqual('/engine_creation'); to the original test and called it a day.

cee-chen · 2021-02-09T17:03:45Z

...erprise_search/public/applications/app_search/components/engine_creation/engine_creation.tsx

+      </EuiPageHeader>
+      <EuiPageBody>
+        <FlashMessages />
+        <EuiPanel>


[Super optional/not a change request, just me thinking out loud] I think if we use EuiPageBody here that shows up w/ the panel appearance as well - that being said I don't 100% know what the semantic distinction is over PageBody vs Panel, or how much that's changing in Amsterdam 🤔 Mostly just posting to see if anyone else does haha

Thanks for clarifying, I'll keep this in mind as I'm wrapping up Meta Engine Creation

cee-chen · 2021-02-09T17:10:13Z

...se_search/public/applications/app_search/components/engine_creation/engine_creation.test.tsx

+  it('EngineCreationForm calls submitEngine on form submit', () => {
+    const wrapper = shallow(<EngineCreation />);
+    const simulatedEvent = {
+      preventDefault: jest.fn(),


[optional] If we're defining preventDefault in a referenceable var, we might as well check that it was called at the end of the test, e.g.

expect(simulatedEvent.preventDefault).toHaveBeenCalledTimes(1); expect(MOCK_ACTIONS.submitEngine).toHaveBeenCalledTimes(1);

While I'm here also, .toHaveBeenCalledTimes(1); (vs. just .toHaveBeenCalled()) is a little more specific than we're doing in other files. I don't feel super strongly either way we go, but it would be nice to be consistent across files vs writing things slightly differently across the codebase.

I like the specificity personally

cee-chen · 2021-02-09T17:14:02Z

...se_search/public/applications/app_search/components/engine_creation/engine_creation.test.tsx

+      const wrapper = shallow(<EngineCreation />);
+      const formRow = wrapper.find('[data-test-subj="EngineCreationNameFormRow"]').dive();
+
+      expect(formRow.contains('Your engine will be named')).toBeTruthy();


This assertion feels like it's missing the actual name? Is there any way we can assert on that?

Suggested change

expect(formRow.contains('Your engine will be named')).toBeTruthy();

expect(formRow.contains('Your engine will be named un-sanitized-name')).toBeTruthy();

not sure if the DOM works out that way, but that would make the test make a lot more sense when reading it. If it takes more than 5 minutes to do or it's a pain than no worries.

cee-chen · 2021-02-09T18:01:50Z

Sorry just to clarify, all of my new comments are optional except for #89816 (comment) - would ask that file change be reverted since the branch needs to be rebased / --fixed / in any case and CI needs to re-run.

cee-chen

Approving as my blocking/requested comments have been addressed. Would love to hear your thoughts on the optional ones added recently if you have time!

kibanamachine · 2021-02-10T18:59:53Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1191	1195	+4

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.9MB	+10.2KB

History

💛 Build #105318 was flaky 2ffcb6e
💔 Build #105309 failed 4b6c281
💛 Build #104792 was flaky 822d342
💔 Build #104769 failed 31501a7
💔 Build #103578 failed e6eb59c

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

kibanamachine · 2021-02-15T13:47:08Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 89816 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-02-16T13:57:07Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 89816 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-02-17T14:07:07Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 89816 or prevent reminders by adding the backport:skip label.

* New CreateEngine view component * Add CreateEngine to index router * Add Layout-level components for CreateEngine * Static create engine view * Add new POST route for engines API endpoint * Logic for Create Engine view WIP tests failing * Fix enterpriseSearchRequestHandler path * Use setQueuedSuccessMessage after engine has been created * Use exact path for CREATE_ENGINES_PATH (but EngineRouter logic is still firing??) * Add TODO note * Put CreateEngine inside the common App Search Layout * Fix CreateEngineLogic jest tests * Move create engine view to /create_engine from /engines/new * Add Create an Engine button to Engines Overview * Missing FlashMessages on EngineOverview * Fix test for CreateEngine route * Fix strong'd text in santized name note * Use local constant for Supported Languages * Disable submit button when name is empty * Bad conflict fix * Lint nits * Improve CreateEngineLogic tests * Improve EngineOverview tests * Disable EnginesOverview header responsiveness * Moving CreateEngine route * create_engine/CreateEngine -> engine_creation/EngineCreation * Use static values for tests * Fixing constants, better casing, better ID names, i18ning dropdown labels * Removing unused imports * Fix EngineCreation tests * Fix Engines EmptyState tests * Fix EnginesOverview tests * Lint fixes * Reset mocks after tests * Update MockRouter properties * Revert newline change * Lint fix

kibanamachine · 2021-02-18T14:17:08Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-02-19T14:27:07Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-02-22T14:37:07Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

byronhulcher added 18 commits January 29, 2021 20:48

New CreateEngine view component

1b3083d

Add CreateEngine to index router

9d92b03

Add Layout-level components for CreateEngine

cad6494

Static create engine view

4cc79b2

Add new POST route for engines API endpoint

3886740

Logic for Create Engine view WIP tests failing

7dae33b

Fix enterpriseSearchRequestHandler path

714fca7

Use setQueuedSuccessMessage after engine has been created

efc7de3

Use exact path for CREATE_ENGINES_PATH (but EngineRouter logic is sti…

49e7693

…ll firing??)

Add TODO note

6f7f512

Put CreateEngine inside the common App Search Layout

35b107f

Fix CreateEngineLogic jest tests

120c749

Move create engine view to /create_engine from /engines/new

be7a3df

Add Create an Engine button to Engines Overview

8737c69

Missing FlashMessages on EngineOverview

3820860

Fix test for CreateEngine route

e9382fc

Fix strong'd text in santized name note

66dc0b6

Use local constant for Supported Languages

207f8b6

byronhulcher commented Jan 30, 2021

View reviewed changes

byronhulcher marked this pull request as ready for review January 30, 2021 02:09

byronhulcher requested a review from a team January 30, 2021 02:09

byronhulcher added v7.12.0 Feature:Plugins labels Jan 30, 2021

byronhulcher requested review from JasonStoltz and cee-chen January 30, 2021 02:15

byronhulcher self-assigned this Jan 30, 2021

byronhulcher added 2 commits January 29, 2021 21:20

Disable submit button when name is empty

2ac0a29

Bad conflict fix

60bb55f

byronhulcher force-pushed the create-engine-view branch from 5f8beff to 60bb55f Compare January 30, 2021 02:24

cee-chen reviewed Feb 9, 2021

View reviewed changes

byronhulcher added 4 commits February 9, 2021 13:28

Merge remote-tracking branch 'origin/master' into create-engine-view

34bf6cf

Revert newline change

4b6c281

Lint fix

3313354

Merge remote-tracking branch 'origin/master' into create-engine-view

2ffcb6e

cee-chen approved these changes Feb 9, 2021

View reviewed changes

byronhulcher requested a review from JasonStoltz February 9, 2021 20:58

Merge remote-tracking branch 'origin/master' into create-engine-view

0bbbfa9

JasonStoltz approved these changes Feb 11, 2021

View reviewed changes

byronhulcher merged commit 57e619b into elastic:master Feb 11, 2021

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 15, 2021

byronhulcher mentioned this pull request Feb 17, 2021

[7.x] [App Search] Migrate Create Engine view (#89816) #91655

Merged

cee-chen added v7.13.0 and removed v7.12.0 labels Feb 22, 2021

cee-chen mentioned this pull request Feb 22, 2021

[App Search] Remove external/popout icon from Create Engine CTA #92272

Merged

JasonStoltz pushed a commit that referenced this pull request Feb 23, 2021

[App Search] Migrate Create Engine view (#89816) (#91655)

6ac5fab

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 23, 2021

	expect(formRow.contains('Your engine will be named')).toBeTruthy();
	expect(formRow.contains('Your engine will be named un-sanitized-name')).toBeTruthy();

Conversation

byronhulcher commented Jan 30, 2021

Summary

Screenshots

Create Engine view: Empty state

Create Engine view: Supported languages

Create Engine view: Acceptable name

Create Engine view: Sanitized name

Create Engine view: Sanitized name with length of 0

Create Engine view: Duplicate engine name error state

Engine Overview view: Success message after engine creation

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!

Choose a reason for hiding this comment

Uh oh!

byronhulcher commented Jan 30, 2021

Uh oh!

cee-chen Feb 9, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Feb 9, 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!

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Feb 9, 2021

Uh oh!

cee-chen left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 10, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Feb 15, 2021

Uh oh!

kibanamachine commented Feb 16, 2021

Uh oh!

kibanamachine commented Feb 17, 2021

Uh oh!

kibanamachine commented Feb 18, 2021

Uh oh!

kibanamachine commented Feb 19, 2021

Uh oh!

kibanamachine commented Feb 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cee-chen Feb 9, 2021 •

edited

Loading

cee-chen Feb 9, 2021 •

edited

Loading