Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add category field to edit model configuration page #310

Merged
merged 3 commits into from
Oct 14, 2020

Conversation

ohltyler
Copy link
Contributor

Issue #, if available:

Description of changes:

This PR adds the category field component on the edit model configuration page. This is an optional field that, when selected, will create and run a high cardinality / multi-entity detector.

Changes include:

  • Adds categoryField field to the detector interface
  • Adds category field component to the edit model config page (including validation & edge case of no eligible fields)
  • Refactors and adds the categoryField field to the Additional settings component on detector configuration page
  • Adds unit tests

All wording has been approved by UX and tech writer.

Screenshots:

  1. Category field (unselected):

Screen Shot 2020-10-12 at 7 53 57 PM

  1. Category field (selected):

Screen Shot 2020-10-12 at 7 58 22 PM

  1. Category field (no eligible fields in index):

Screen Shot 2020-10-12 at 7 55 31 PM

  1. Category field under 'Additional settings':

Screen Shot 2020-10-12 at 7 55 05 PM

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ohltyler ohltyler added the enhancement Enhance current feature for better performance, user experience, etc label Oct 13, 2020
Copy link
Contributor

@yizheliu-amazon yizheliu-amazon left a comment

Choose a reason for hiding this comment

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

Looks good to me. Feel free to merge it after addressing my comment.

interface CategoryFieldProps {
isHCDetector: boolean;
categoryFieldOptions: string[];
setIsHCDetector(isHCDetector: boolean): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

you may delete this method, I think original CategoryFieldProps is already good for integration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to update the isHCDetector flag in editFeatures. This was needed to properly validate the form in the case of the box being checked, but no value being set.

Copy link
Contributor Author

@ohltyler ohltyler Oct 13, 2020

Choose a reason for hiding this comment

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

Basically, since we don't perform validation immediately after enabling the checkbox now, this became an issue that had to be fixed by updating isHCDetector in editFeatures, so we can properly validate/invalidate the category field in handleSaveChanges().

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. I thought you may delete it since I already modified code to support validation only when checkbox is enabled. But I think your new implementation is better. Approved.

@yizheliu-amazon
Copy link
Contributor

When I open Edit feature page for non-HC detector in my local workspace, the no eligible fields in index callout will show up in the beginning and it will be gone very soon, does that happen to you as well?

@ohltyler
Copy link
Contributor Author

When I open Edit feature page for non-HC detector in my local workspace, the no eligible fields in index callout will show up in the beginning and it will be gone very soon, does that happen to you as well?

I see, yes it does flash that as it loads in the index details. Let me look into improving the loading state.

@yizheliu-amazon
Copy link
Contributor

When I open Edit feature page for non-HC detector in my local workspace, the no eligible fields in index callout will show up in the beginning and it will be gone very soon, does that happen to you as well?

I see, yes it does flash that as it loads in the index details. Let me look into improving the loading state.

I will see it as non-blocking issue. If it takes too long to fix it, let's create issue for it and improve it later.

@ohltyler
Copy link
Contributor Author

ohltyler commented Oct 13, 2020

When I open Edit feature page for non-HC detector in my local workspace, the no eligible fields in index callout will show up in the beginning and it will be gone very soon, does that happen to you as well?

I see, yes it does flash that as it loads in the index details. Let me look into improving the loading state.

I will see it as non-blocking issue. If it takes too long to fix it, let's create issue for it and improve it later.

really quick fix. added test case as well. thanks for finding that issue

Copy link
Contributor

@saratvemulapalli saratvemulapalli 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 good to me.

@ohltyler ohltyler merged commit 6237716 into opendistro-for-elasticsearch:master Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhance current feature for better performance, user experience, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants