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

[App Search] Implement various Relevance Tuning states and form actions #92644

Merged
merged 20 commits into from
Mar 1, 2021

Conversation

JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Feb 24, 2021

Summary

The commits in this PR are atomic, you should be able to follow along commit by commit.

  1. Added Save and Reset buttons

    To test:

    • The Reset button should clear all current boosts and weights
    • The Save button should save your current relevance changes and show a success message
    • These buttons should not appear if you have not yet uploaded documents to an engine
  2. Added the UnsavedChangesPrompt to prevent users from leaving the form if they have unsaved changes

    To test:

    • Try navigating away from the page when there are changes
  3. Re-arranged the header to use the new description, body, and action fields

  4. Added an Invalid boosts error message

    To test:

    • Change a field to "number" (in old UI "Schema" screen) and create a functional boost
    • Switch the schema back to "text" (in old UI "Schema" scree
    • In the new UI, you should see the following message
    • In the new UI, delete this boost and click save, the message should go away

invalid_boosts

  1. Added an unconfirmed fields message

    To test:

    • Upload a document with a new field that is not yet in the schema
    • Navigate to the Relevance Tuning scree and you should see this message:

unconfirmed

  1. Added a schema conflicts message

Note that I moved the position of this callout to be located with the other callouts, and removed the "dismiss" button, which seemed like overkill.

To test:
- Create a meta engine with two source engines that share a field with the same name, but each engine has the field in their schema as a different type
- Navigate to this screen and you should see both a message at the top AND the fields listed at the bottom of the form

schema-conflicts

  1. Add an empty state

    To test:

    • Create a new engine with no documents
    • Navigate to this page and you should see the following message:

empty

  1. Un-title cases text in a few places, which was feedback from my previous PR

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@JasonStoltz JasonStoltz changed the title [App Search] Relevance Tuning WIP [App Search] Implement Various Relevance Tuning and form actions Feb 24, 2021
@JasonStoltz JasonStoltz changed the title [App Search] Implement Various Relevance Tuning and form actions [App Search] Implement various Relevance Tuning states and form actions Feb 24, 2021
@JasonStoltz JasonStoltz marked this pull request as ready for review February 25, 2021 13:55
@JasonStoltz JasonStoltz requested a review from a team February 25, 2021 13:55
@JasonStoltz JasonStoltz added v7.13.0 release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed labels Feb 25, 2021
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

some test mock comments

@cee-chen

This comment has been minimized.

@cee-chen

This comment has been minimized.

JasonStoltz and others added 5 commits February 26, 2021 11:51
…h/components/relevance_tuning/relevance_tuning.test.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
…h/components/relevance_tuning/relevance_tuning_form/relevance_tuning_form.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
…h/components/relevance_tuning/relevance_tuning.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
…h/components/relevance_tuning/relevance_tuning.test.tsx

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@JasonStoltz
Copy link
Member Author

@constancecchen Removed that old comment: 9c20faf

@JasonStoltz
Copy link
Member Author

@constancecchen Cherry-picked 55e9ebc

@JasonStoltz
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1281 1305 +24

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.9MB 2.0MB +87.8KB
triggersActionsUi 1.6MB 1.5MB -23.9KB
total +63.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 104.0KB 104.1KB +82.0B
Unknown metric groups

async chunk count

id before after diff
triggersActionsUi 41 42 +1

History

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

Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Changes look fantastic! Really love the new Layout and Callout components 😍

Comment on lines +94 to +95
<RelevanceTuningLayout engineBreadcrumb={engineBreadcrumb}>
<UnsavedChangesPrompt hasUnsavedChanges={unsavedChanges} />
Copy link
Member

Choose a reason for hiding this comment

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

[optional, not a blocker] I think you could probably bake UnsavedChangesPrompt into the RelevanceTuningLayout, unless you think that's too hidden or confusing. Up to you!

Comment on lines +75 to +81
if (dataLoading) {
return <Loading />;
}

if (!engineHasSchemaFields) {
return <EmptyCallout />;
}
Copy link
Member

Choose a reason for hiding this comment

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

Also optional, but I think Scotty stated a preference for single line returns. I'm fine with whatever though lol, not a big deal to me, we can yolo it and come back to returns later

expect(mockEngineActions.initializeEngine).toHaveBeenCalled();
});

it('will re-fetch the current engine after settings are updated if there were unconfirmed search fieldds', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

damnit, only just noticed this, sorry! optional though, feel free to fix in a future PR instead if you'd rather save time on CI cycles

Suggested change
it('will re-fetch the current engine after settings are updated if there were unconfirmed search fieldds', async () => {
it('will re-fetch the current engine after settings are updated if there were unconfirmed search fields', async () => {

@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #93135

Successful backport PRs will be merged automatically after passing CI.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 2, 2021
* master: (42 commits)
  [Lens] Introduces new chart switcher (elastic#91844)
  [Lens] fix selection when dragging (elastic#93034)
  Converts usage collection README to .mdx (elastic#92982)
  Fix expanding document when using saved search data grid (elastic#92999)
  [SECURITY SOLUTIONS] Bug case connector (elastic#93104)
  [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025)
  [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942)
  [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139)
  [DOCS] Fixes links for machine learning alerts (elastic#92744)
  [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748)
  [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010)
  [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095)
  [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667)
  Bump ems landing page to 7.12 (elastic#93065)
  [App Search] Implement various Relevance Tuning states and form actions (elastic#92644)
  [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760)
  [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092)
  Hide instances latency distribution chart (elastic#92869)
  [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087)
  [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (elastic#92932)
  ...
v1v added a commit to shahzad31/kibana that referenced this pull request Mar 2, 2021
… playwright-ftr-e2e

* 'playwright-ftr-e2e' of github.com:shahzad31/kibana: (38 commits)
  [chore] Enable core's eslint rule: `@ts-expect-error` (elastic#93086)
  [Lens] Introduces new chart switcher (elastic#91844)
  [Lens] fix selection when dragging (elastic#93034)
  Converts usage collection README to .mdx (elastic#92982)
  Fix expanding document when using saved search data grid (elastic#92999)
  [SECURITY SOLUTIONS] Bug case connector (elastic#93104)
  [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (elastic#92025)
  [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (elastic#92942)
  [APM] Fix hidden search bar in error pages while loading (elastic#84476) (elastic#93139)
  [DOCS] Fixes links for machine learning alerts (elastic#92744)
  [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (elastic#92748)
  [SecuritySolution][Case] Disable cases on detections in read-only mode (elastic#93010)
  [Security Solution][Case][Bug] Prevent closing collection when pushing (elastic#93095)
  [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (elastic#92667)
  Bump ems landing page to 7.12 (elastic#93065)
  [App Search] Implement various Relevance Tuning states and form actions (elastic#92644)
  [actions] for simplistic email servers, set rejectUnauthorized to false (elastic#91760)
  [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (elastic#93092)
  Hide instances latency distribution chart (elastic#92869)
  [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (elastic#93087)
  ...
kibanamachine added a commit that referenced this pull request Mar 2, 2021
…ns (#92644) (#93135)

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
jloleysens added a commit that referenced this pull request Mar 3, 2021
… ilm/rollup-v2-action

* 'ilm/rollup-v2-action' of github.com:elastic/kibana: (30 commits)
  Fix expanding document when using saved search data grid (#92999)
  [SECURITY SOLUTIONS] Bug case connector (#93104)
  [Security Solution] [Timeline] Bugfix to include unmapped fields in the timeline event details JSON (#92025)
  [Alerting][Docs] Changed alerting documentation to point to a single source of explaining the configurations. (#92942)
  [APM] Fix hidden search bar in error pages while loading (#84476) (#93139)
  [DOCS] Fixes links for machine learning alerts (#92744)
  [Security Solution][Detections] -Fixes rule edit flow bug with max_signals (#92748)
  [SecuritySolution][Case] Disable cases on detections in read-only mode (#93010)
  [Security Solution][Case][Bug] Prevent closing collection when pushing (#93095)
  [Security Solution][Detections][7.12] Critical Threshold Rule Fixes (#92667)
  Bump ems landing page to 7.12 (#93065)
  [App Search] Implement various Relevance Tuning states and form actions (#92644)
  [actions] for simplistic email servers, set rejectUnauthorized to false (#91760)
  [Security Solution][Case] Migrate category & subcategory fields of ServiceNow ITSM connector (#93092)
  Hide instances latency distribution chart (#92869)
  [Maps] fix MapboxDraw import from pointing to dist just pointing to folder (#93087)
  [Maps] fix results trimmed tooltip message doubles feature count for line and polygon features (#92932)
  [Security Solution][Detecttions] Indicator enrichment tweaks (#92989)
  [Maps] fix fit to data on heatmap not working (#92697)
  [Security Solution][Endpoint][Admin] Fixes policy sticky footer save test (#92919)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants