Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GS providers improvements #75174

Merged
merged 3 commits into from
Aug 18, 2020

Conversation

pgayvallet
Copy link
Contributor

Summary

Fix #75050

  • exclude apps without visible navlinks from application results
  • perform prefix search for SO provider

Checklist

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes REASSIGN from Team:Core UI Deprecated label for old Core UI team Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0 labels Aug 17, 2020
@pgayvallet pgayvallet marked this pull request as ready for review August 17, 2020 15:28
@pgayvallet pgayvallet requested a review from a team as a code owner August 17, 2020 15:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core-ui (Team:Core UI)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

Comment on lines +127 to 133
const navLinkStatus =
app.navLinkStatus === AppNavLinkStatus.default
? app.status === AppStatus.inaccessible
? AppNavLinkStatus.hidden
: AppNavLinkStatus.visible
: app.navLinkStatus!;
if (isLegacyApp(app)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAppInfo now computes the navlink status as it was more or less done in

hidden: useAppStatus
? app.status === AppStatus.inaccessible
: app.navLinkStatus === AppNavLinkStatus.hidden,
disabled: useAppStatus ? false : app.navLinkStatus === AppNavLinkStatus.disabled,

As AppServiceStart.applications$ is a public API that is supposed to return the computed state of the apps, I think it make sense, instead of letting the consumers manually apply this logic.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
core 1.2MB +158.0B 1.2MB
globalSearchProviders 9.9KB +2.0B 9.9KB
total +160.0B

History

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

@@ -25,7 +25,7 @@ export const createSavedObjectsResultProvider = (): GlobalSearchResultProvider =
const responsePromise = client.find({
page: 1,
perPage: maxResults,
search: term,
search: term ? `${term}*` : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any issue with using *${term}* so we can match anywhere in the field? Maybe this reduces the quality of the search results.

Ideally, we'd be boosting results where the beginning of the field matches the search term, probably using multiple should clauses, one with the phrase_prefix match type. Definitely out of scope here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any issue with using ${term} so we can match anywhere in the field? Maybe this reduces the quality of the search results.

Yea, I'm not really sure. Without proper boost, searching anywhere in the field can have some unexpected behavior in term of results. I feel like until we are able to construct a more precise search dsl, prefix-only may be better.

@ryankeairns WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m happy with the prefix-only change for now and collecting more feedback post-merge/release.

@pgayvallet pgayvallet merged commit c844187 into elastic:master Aug 18, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Aug 18, 2020
* exclude apps with non visible navlinks from results

* change SO provider to prefix search

* fix service tests
@ryankeairns
Copy link
Contributor

Thanks for the quick work on this!

pgayvallet added a commit that referenced this pull request Aug 18, 2020
* exclude apps with non visible navlinks from results

* change SO provider to prefix search

* fix service tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 18, 2020
* master:
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  [Visualize] First version of by-value visualize editor (elastic#72256)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 19, 2020
…emove-header

* saved-objects/version-on-create: (59 commits)
  remove version when loading sample data
  omit version from SO import/export
  Skip failing test in CI (elastic#75266)
  [Task Manager] time out work when it overruns in poller (elastic#74980)
  [Drilldowns] misc improvements & fixes (elastic#75276)
  Small README note on bumping memory for builds (elastic#75247)
  [Security Solution][Detections] Adds exception modal tests (elastic#74596)
  Revert "Revert "added missing core docs""
  Revert "Revert "added version to saved object bulk creation""
  [Dashboard] Sample data link does not work (elastic#75262)
  [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905)
  [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166)
  convert processor labels to sentence case (elastic#75278)
  [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160)
  Clarify no documents error message when filtering by is_training (elastic#75227)
  [Lens] Fix crash when two layers xychart  switches to pie (elastic#75267)
  [Observability Homepage] Fix console error because of side effect (elastic#75258)
  [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146)
  [ML] Functional tests - re-activate DFA test suites (elastic#75257)
  GS providers improvements (elastic#75174)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REASSIGN from Team:Core UI Deprecated label for old Core UI team release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[GS] Excluding specific search results
5 participants