[App Search] Updated Search UI to new URL #101320

JasonStoltz · 2021-06-03T19:08:38Z

Summary

This PR does 2 things:

Changes Search UI to POST to the new authentication-not-required search experience page in ent-search, including a valid public search key.
Changes Search UI to show an error message if no valid public search key is available.

When testing:

Be sure to run this against the latest master version of ent-search, in an incognito window, so you are signed out of everything. Ensure that you are not prompted to log into ent-search when clicking on "Generate search experience".
Verify that there is no way to copy/paste a url for the generated search experience in a way that would expose a user's search key unintentionally.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

JasonStoltz · 2021-06-03T19:09:07Z

@elasticmachine merge upstream

JasonStoltz · 2021-06-03T19:11:28Z

...k/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts

@@ -87,6 +90,14 @@ export const EngineLogic = kea<MakeLogicType<EngineValues, EngineActions>>({
      () => [selectors.engine],
      (engine) => engine?.unconfirmedFields?.length > 0,
    ],
+    searchKey: [


We need to POST the search key to the new search experience URL from the Search UI form. Since all keys that are valid for a particular engine are already available on engine.apiTokens from state, I added a selector to easily access the first available search token.

Since tokens that do not have access to the current engine are not returned in this object, I do not have to check their access level, as we already know it has access to the current engine.

I really like this added logic/selector. Feels elegant and makes sense.

JasonStoltz · 2021-06-03T19:14:08Z

...ins/enterprise_search/public/applications/app_search/components/search_ui/search_ui_logic.ts

-      const { engineName } = EngineLogic.values;
+      const { searchKey, engineName } = EngineLogic.values;
+
+      if (!searchKey) {


We make a call to fetch all the data that the Search UI form needs below. As part of this API call, the API will actualy return an "errors" field that will return the message "It looks like you don't have any Public Search Keys with access to the 'engine1' engine. Please visit the Credentials page to set one up." In ent-search, we simply printed the error from that field to the screen.

In this UI, I opted to explicitly check for that condition and add that error message to an i18n string, so that it can be translated.

I'm not sure if I agree with this approach. It's seems strange to add this to listener that largely does an http call. I think I would have kept this as a static part of the SearchUiForm with useValues(EngineLogic)

True. I think I chose to put it in this logic file because I wanted to use our flash messaging system, which is stateful and requires a call to something like setErrorMessage to use.

I could have done that in like a useEffect in SearchUiForm but that seems clunkier. I like it here because we can short circuit the API call and all flash messages are generated from the logic file.

It could also be a stateless check where we just do like !searchKey && <EuiCallout..., but then that conflicts with the flash messaging system we use. Like it gets hairy having them both exist on the page together.

Ah, I think I see Byron's point - it does seem odd to skip loading fieldData entirely just because a searchKey is missing. I don't think this is overly clunky to put in a useEffect personally - would you be cool with making that change?

const { searchKey, engineName } = useValues(EngineLogic); useEffect(() => { if (!searchKey) setErrorMessage(NO_SEARCH_KEY_ERROR(engineName)); }, [searchKey]);

3 or so lines doesn't feel so bad to me, and I do think it makes more sense (conceptually) in the view than this specific listener.

Definitely ++ to keeping this using our flash messages + i18n though!

JasonStoltz · 2021-06-03T19:15:25Z

.../plugins/enterprise_search/public/applications/app_search/components/search_ui/utils.test.ts

@@ -33,7 +33,7 @@ describe('generatePreviewUrl', () => {
        empty2: [''], // Empty fields should be stripped
      })
    ).toEqual(
-      'http://localhost:3002/as/engines/national-parks-demo/reference_application/preview?facets[]=baz&facets[]=qux&sortFields[]=quux&sortFields[]=quuz&titleField=foo&urlField=bar'
+      'http://localhost:3002/as/engines/national-parks-demo/search_experience/preview?facets[]=baz&facets[]=qux&sortFields[]=quux&sortFields[]=quuz&titleField=foo&urlField=bar'


This is a new route that was set up specifically for this form, which accepts a search token in order to render the search experience preview, rather than authenticating like the former route.

JasonStoltz · 2021-06-03T19:16:20Z

...ise_search/public/applications/app_search/components/search_ui/components/search_ui_form.tsx

@@ -119,8 +127,8 @@ export const SearchUIForm: React.FC = () => {
        />
      </EuiFormRow>
      <EuiButton


Rather than being a link as before, this is not a form with a submit button so that we can POST to the new route.

byronhulcher

Code looks good, I had two question-that-is-really-more-of-a-comments. I'm going to QA now

byronhulcher · 2021-06-04T18:31:10Z

...k/plugins/enterprise_search/public/applications/app_search/components/engine/engine_logic.ts

+    searchKey: [
+      () => [selectors.engine],
+      (engine: Partial<EngineDetails>) => {
+        const isSearchKey = (token: ApiToken) => token.type === ApiTokenTypes.Search;


You could pull this function out instead of declaring it in here every time (idk if its worth writing a test for it)

Going completely the opposite direction, I would have just left it as an inline fn in .find() But it doesn't bother me either way personally

Haha, quite the array of opinions! I just pulled this out for readability. It was long in the tooth inline in the find.

byronhulcher · 2021-06-04T18:36:00Z

...ins/enterprise_search/public/applications/app_search/components/search_ui/search_ui_logic.ts

-      const { engineName } = EngineLogic.values;
+      const { searchKey, engineName } = EngineLogic.values;
+
+      if (!searchKey) {


I'm not sure if I agree with this approach. It's seems strange to add this to listener that largely does an http call. I think I would have kept this as a static part of the SearchUiForm with useValues(EngineLogic)

JasonStoltz · 2021-06-07T12:59:30Z

@elasticmachine merge upstream

JasonStoltz · 2021-06-08T12:59:21Z

@elasticmachine merge upstream

kibanamachine · 2021-06-08T15:00:49Z

💚 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	+3.6KB

History

💚 Build #129476 succeeded c224c8c
💛 Build #129015 was flaky 3135796

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

JasonStoltz · 2021-06-08T15:41:19Z

@byronhulcher Are you looking for any changes in particular before approving?

...earch/public/applications/app_search/components/search_ui/components/search_ui_form.test.tsx

cee-chen · 2021-06-08T16:02:51Z

...earch/public/applications/app_search/components/search_ui/components/search_ui_form.test.tsx

-      titleField: 'bar',
-      facets: ['baz'],
-      sortFields: ['qux'],
+  it('should disable everything while data is loading', () => {


Thanks for adding this piece of UX + test - love it! 🎉

cee-chen · 2021-06-08T16:05:48Z

This LGTM - I think I'm +1 to Byron on #101320 (comment), and moving the error display to a view useEffect is my only real change request. If you feel strongly otherwise though lmk

JasonStoltz · 2021-06-08T16:33:33Z

Ok. I'll make the change, though TBH, I'm really medium on it and don't really feel like expending the cycles.

JasonStoltz · 2021-06-08T16:33:47Z

Just keep an eye on it so we can get this approved and merged please.

cee-chen · 2021-06-08T16:36:37Z

@JasonStoltz if you're feeling medium on it but wouldn't mind it happening, I'd be cool pushing up the change in your stead to save you some effort. Totally up to you though!

JasonStoltz · 2021-06-08T16:39:46Z

Actually, I do feel strongly about this. I want the fields to be disabled if there is no search key, there's no way you can generate a UI without one. So it's pointless to fetch the fields. I am also relying on the dataLoading field to disable all of the fields. So if there's any kind of error here... whether it's an error on the server, or the error is is that there is no search key, it all fits into the same flow right now. It also has the advantage of being written and tested. I don't think there is a compelling enough reason for any of us to spend cycles changing this.

cee-chen · 2021-06-08T16:41:57Z

Ahhh that's a solid point about disabling fields that I missed! I think that's a good enough argument to not block this PR and I'm fine with merging this in as-is 👍

JasonStoltz · 2021-06-08T16:42:32Z

🙇‍♂️ 🙏

kibanamachine · 2021-06-08T16:45:31Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>

* master: (54 commits) Implement "select all" rules feature (elastic#100554) [ML] Remove script fields from the Anomaly detection alerting rule executor (elastic#101607) [Security solutions][Endpoint] Update event filtering texts (elastic#101563) [Enterprise Search] Mocks/tests tech debt - avoid hungry mocking (elastic#101107) [FTR] Updates esArchive paths [FTR] Updates esArchive paths [Security Solution][Detection Engine] Adds runtime field tests (elastic#101664) Added APM PHP agent to the list of agent names (elastic#101062) [CI] Restore old version_info behavior when .git directory is present (elastic#101642) [Fleet] Add fleet server telemetry (elastic#101400) [APM] Syncs agent config settings to APM Fleet policies (elastic#100744) [esArchiver] drop support for --dir, use repo-relative paths instead (elastic#101345) Revert "[xpack/test] restore incremental: false in ts project" [Security Solution] Remove Host Isolation feature flag (elastic#101655) [xpack/test] restore incremental: false in ts project [DOCS] Adds link to video landing page (elastic#101413) [ML] Move Index Data Visualizer into separate plugin (Part 1) (elastic#100922) Improve security plugin return types (elastic#101492) [ts] migrate `x-pack/test` to composite ts project (elastic#101441) [App Search] Updated Search UI to new URL (elastic#101320) ...

JasonStoltz added 2 commits June 3, 2021 13:00

Use new search experience url

cf96bf5

Show no key error

6069260

JasonStoltz requested a review from a team June 3, 2021 19:08

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

Merge branch 'master' into new-search-experience-url

3135796

JasonStoltz commented Jun 3, 2021

View reviewed changes

byronhulcher reviewed Jun 4, 2021

View reviewed changes

Merge branch 'master' into new-search-experience-url

c224c8c

JasonStoltz requested a review from byronhulcher June 8, 2021 12:59

Merge branch 'master' into new-search-experience-url

8c0f0c7

cee-chen reviewed Jun 8, 2021

View reviewed changes

...earch/public/applications/app_search/components/search_ui/components/search_ui_form.test.tsx Show resolved Hide resolved

cee-chen reviewed Jun 8, 2021

View reviewed changes

cee-chen approved these changes Jun 8, 2021

View reviewed changes

JasonStoltz merged commit a9e64ab into elastic:master Jun 8, 2021

JasonStoltz deleted the new-search-experience-url branch June 8, 2021 16:42

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 8, 2021

[App Search] Updated Search UI to new URL (elastic#101320)

05c5c1f

kibanamachine mentioned this pull request Jun 8, 2021

[7.x] [App Search] Updated Search UI to new URL (#101320) #101644

Merged

kibanamachine added a commit that referenced this pull request Jun 8, 2021

[App Search] Updated Search UI to new URL (#101320) (#101644)

93ca47b

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>

JasonStoltz mentioned this pull request Jun 30, 2021

Removed Search UI access for Editor role #103951

Closed

2 tasks

JasonStoltz added v7.15.0 and removed v7.15.0 labels Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Updated Search UI to new URL #101320

[App Search] Updated Search UI to new URL #101320

JasonStoltz commented Jun 3, 2021

JasonStoltz commented Jun 3, 2021

JasonStoltz Jun 3, 2021

cee-chen Jun 8, 2021

JasonStoltz Jun 3, 2021

byronhulcher Jun 4, 2021

JasonStoltz Jun 7, 2021

cee-chen Jun 8, 2021 •

edited

Loading

JasonStoltz Jun 3, 2021

JasonStoltz Jun 3, 2021

byronhulcher left a comment

byronhulcher Jun 4, 2021

cee-chen Jun 8, 2021

JasonStoltz Jun 8, 2021

byronhulcher Jun 4, 2021

JasonStoltz commented Jun 7, 2021

JasonStoltz commented Jun 8, 2021

kibanamachine commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

cee-chen Jun 8, 2021

cee-chen commented Jun 8, 2021 •

edited

Loading

JasonStoltz commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

cee-chen commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

cee-chen commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

kibanamachine commented Jun 8, 2021

[App Search] Updated Search UI to new URL #101320

[App Search] Updated Search UI to new URL #101320

Conversation

JasonStoltz commented Jun 3, 2021

Summary

Checklist

For maintainers

JasonStoltz commented Jun 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byronhulcher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz commented Jun 7, 2021

JasonStoltz commented Jun 8, 2021

kibanamachine commented Jun 8, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

JasonStoltz commented Jun 8, 2021

Choose a reason for hiding this comment

cee-chen commented Jun 8, 2021 • edited Loading

JasonStoltz commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

cee-chen commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

cee-chen commented Jun 8, 2021

JasonStoltz commented Jun 8, 2021

kibanamachine commented Jun 8, 2021

💚 Backport successful

cee-chen Jun 8, 2021 •

edited

Loading

cee-chen commented Jun 8, 2021 •

edited

Loading