-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Component templates] Form wizard #69732
[Component templates] Form wizard #69732
Conversation
…nent_templates_wizard
…nent_templates_wizard
…nent_templates_wizard
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
@sebelga I removed the version field toggle based on Lee's feedback that |
…nent_templates_wizard
…nent_templates_wizard
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @alisonelizabeth ! Tested locally and works as expected. I left a few comments, none are blockers, but have a look at the encodeURI
for edge cases.
@@ -135,7 +150,7 @@ export const ComponentTable: FunctionComponent<Props> = ({ | |||
{...reactRouterNavigate( | |||
history, | |||
{ | |||
pathname: `/component_templates/${name}`, | |||
pathname: `/component_templates/${encodeURIComponent(name)}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not enough if you create a component with this name: &%$!"test
. You also need to encode the uri, so it becomes
pathname: encodeURI(`/component_templates/${encodeURIComponent(name)}`),
I wonder if it wouldn't be a good idea to have a routing.ts
in your "lib" folder that returns those encoded paths in case they are needed in multiple place. Like we have inside the index_management/application/services folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. Should be fixed now across the code!
|
||
export const ComponentTemplateClone: FunctionComponent<RouteComponentProps<Params>> = (props) => { | ||
const { sourceComponentTemplateName } = props.match.params; | ||
const decodedSourceComponentTemplateName = attemptToDecodeURI(sourceComponentTemplateName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know we strive to be explicit, but sometime the context of the filename we work on give us that context. And here, IMO, decodedName
would have been just as clear. 😊
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [error, isLoading]); | ||
|
||
if (isLoading && isInitialRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to have more than 1 request? Do we need the isInitialRequest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This is not necessary.
return; | ||
} | ||
|
||
history.push(`/component_templates/${encodeURIComponent(name)}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here would be another place where we'd need the double encode (encodeURI
).
const [isSaving, setIsSaving] = useState<boolean>(false); | ||
const [saveError, setSaveError] = useState<any>(null); | ||
|
||
const decodedName = attemptToDecodeURI(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great var name! 😄
}} | ||
/> | ||
) : ( | ||
// <FormRow/> requires children or a field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably something I'd need to fix at some point! 😊
deserializer: stringifyJson, | ||
validations: [ | ||
{ | ||
validator: (validationArg) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can use the isJsonField
validator. I added an optional options
object to allow empty JSON. This is how it is used in index template
{
validator: isJsonField(
i18n.translate('xpack.idxMgmt.templateForm.stepLogistics.metaFieldEditorJsonError', {
defaultMessage: 'The _meta field JSON is not valid.',
}),
{ allowEmptyString: true }
),
},
} | ||
|
||
export const StepReview: React.FunctionComponent<Props> = React.memo(({ componentTemplate }) => { | ||
const { name, version } = componentTemplate!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the exclamation mark necessary if the prop is marked as required?
const { name, version } = componentTemplate!; | ||
|
||
const serializedComponentTemplate = serializeComponentTemplate( | ||
stripEmptyFields(componentTemplate!, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here?
<EuiFlexItem> | ||
<EuiDescriptionList textStyle="reverse"> | ||
{/* Version */} | ||
{version && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I saw you change this line with typeof version !== "undefined" &&
, but now I see it like this again.. Ah no, I think you did change it for the details panel, it might be good to do it here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! I missed fixing it here. Should be good now.
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* master: (53 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...
* master: (46 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...
* actions/feature: (46 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...
This PR adds the form wizard to the component templates UI.
Since this PR completes the main functionality planned for the component templates UI in
7.9
, I've added a release note. Related work was completed via #68031 and #68732.Meta issue: #64771.
How to test
Verify create, edit and clone component template actions all work as expected. The UX should be very similar to the existing index templates flow. The only differences are the "Logistics" and "Review" steps.
Screenshots
Note: I did not include screenshots of the settings/mappings/aliases steps as these are the same that exist today for the index templates flow.
Tests
Release note
A new tab called Component Templates is available in Index Management. It provides a way to manage Elasticsearch's component templates. Users can create, edit, clone, and delete a component template.