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

[Automatic Import] Fix the enter bug #199894

Merged
merged 25 commits into from
Dec 11, 2024

Conversation

ilyannn
Copy link
Contributor

@ilyannn ilyannn commented Nov 13, 2024

Release Notes

Fixes the bug where pressing Enter reloaded the Automatic Import.

Summary

Details

When the user presses the Enter inside our input field, the expected action is to send the form, in this case completing the step. However, previously the form submission would instead lead to reloading the whole Automatic Import page.

In this PR we capture the form submission event and bubble it up as completeStep to the main component. We also move the implementation from the Footer up to this main component CreateIntegrationAssistant. This helps collect all the details about the step order in one place and refactor this logic.

As a result, pressing Enter in any field now either

  • Is processed by the field itself (in case of multi-line fields);
  • Leads to the same action as pressing the "Next" button (desired result); or
  • Does nothing (e.g. in the inputs in the "Define data stream and upload logs" group – the reason for this is unclear).

We add CEL-specific telemetry identifiers so that telemetry for step 5 is not always reported as Deploy Step.

We also rename a bunch of stuff that was named ...StepReady into ...StepReadyToComplete as the previous name was ambiguous. To demonstrate this ambiguity we've enlisted the help of GPT 4o:

SCR-20241125-tiaa

Testing

We provide a Cypress test for Enter behavior: pressing it on the "integration title" input should let the flow proceed to the next step. This test fails on main.

We also provide unit tests for all steps of navigation functionality in x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx:

  CreateIntegration
    when step is 1
      ✓ shoud report telemetry for assistant open (93 ms)
      ✓ should render connector step (36 ms)
      ✓ should call isConnectorStepReadyToComplete (32 ms)
      ✓ should show "Next" on the next button (29 ms)
      when connector step is not done
        ✓ should disable the next button (28 ms)
        ✓ should still enable the back button (28 ms)
        ✓ should still enable the cancel button (29 ms)
      when connector step is done
        ✓ should enable the next button (32 ms)
        ✓ should enable the back button (29 ms)
        ✓ should enable the cancel button (27 ms)
        when next button is clicked
          ✓ should report telemetry for connector step completion (44 ms)
      when back button is clicked
        ✓ should not report telemetry (28 ms)
        ✓ should navigate to the landing page (26 ms)
    when step is 2
      ✓ should render integration (27 ms)
      ✓ should call isIntegrationStepReadyToComplete (25 ms)
      ✓ should show "Next" on the next button (26 ms)
      when integration step is not done
        ✓ should disable the next button (26 ms)
        ✓ should still enable the back button (26 ms)
        ✓ should still enable the cancel button (26 ms)
      when integration step is done
        ✓ should enable the next button (28 ms)
        ✓ should enable the back button (27 ms)
        ✓ should enable the cancel button (30 ms)
        when next button is clicked
          ✓ should report telemetry for integration step completion (38 ms)
      when back button is clicked
        ✓ should not report telemetry (37 ms)
        ✓ should show connector step (39 ms)
        ✓ should enable the next button (38 ms)
    when step is 3
      ✓ should render data stream (28 ms)
      ✓ should call isDataStreamStepReadyToComplete (25 ms)
      ✓ should show "Analyze logs" on the next button (25 ms)
      when data stream step is not done
        ✓ should disable the next button (25 ms)
        ✓ should still enable the back button (24 ms)
        ✓ should still enable the cancel button (25 ms)
      when data stream step is done
        ✓ should enable the next button (24 ms)
        ✓ should enable the back button (24 ms)
        ✓ should enable the cancel button (24 ms)
        when next button is clicked
          ✓ should report telemetry for data stream step completion (39 ms)
          ✓ should show loader on the next button (60 ms)
          ✓ should disable the next button (66 ms)
      when back button is clicked
        ✓ should not report telemetry (38 ms)
        ✓ should show integration step (38 ms)
        ✓ should enable the next button (37 ms)
    when step is 4
      ✓ should render review (26 ms)
      ✓ should call isReviewStepReadyToComplete (24 ms)
      ✓ should show the "Add to Elastic" on the next button (25 ms)
      when review step is not done
        ✓ should disable the next button (24 ms)
        ✓ should still enable the back button (25 ms)
        ✓ should still enable the cancel button (24 ms)
      when review step is done
        ✓ should enable the next button (25 ms)
        ✓ should enable the back button (24 ms)
        ✓ should enable the cancel button (24 ms)
        when next button is clicked
          ✓ should report telemetry for review step completion (35 ms)
          ✓ should show deploy step (61 ms)
          ✓ should enable the next button (61 ms)
    when step is 5
      ✓ should render deploy (22 ms)
      ✓ should hide the back button (21 ms)
      ✓ should hide the next button (22 ms)
      ✓ should enable the cancel button (21 ms)
      ✓ should show "Close" on the cancel button (22 ms)
  CreateIntegration with generateCel enabled
    when step is 5 and has celInput
      ✓ should render cel input (24 ms)
      ✓ should call isCelInputStepReadyToComplete (23 ms)
      ✓ should show "Generate CEL input configuration" on the next button (25 ms)
      ✓ should enable the back button (44 ms)
      when cel input step is not done
        ✓ should disable the next button (39 ms)
      when cel input step is done
        ✓ should enable the next button (24 ms)
        when next button is clicked
          ✓ should report telemetry for cel input step completion (35 ms)
          ✓ should show loader on the next button (59 ms)
          ✓ should disable the next button (59 ms)
      when back button is clicked
        ✓ should not report telemetry (36 ms)
        ✓ should show review step (34 ms)
        ✓ should enable the next button (34 ms)
    when step is 5 and does not have celInput
      ✓ should render deploy (20 ms)
      ✓ should hide the back button (21 ms)
      ✓ should hide the next button (21 ms)
      ✓ should enable the cancel button (21 ms)
      ✓ should show "Close" on the cancel button (21 ms)
    when step is 6
      ✓ should render review (23 ms)
      ✓ should call isReviewCelStepReadyToComplete (24 ms)
      ✓ should show the "Add to Elastic" on the next button (24 ms)
      when cel review step is not done
        ✓ should disable the next button (24 ms)
        ✓ should still enable the back button (24 ms)
        ✓ should still enable the cancel button (22 ms)
      when cel review step is done
        ✓ should enable the next button (23 ms)
        ✓ should enable the back button (24 ms)
        ✓ should enable the cancel button (24 ms)
        when next button is clicked
          ✓ should report telemetry for review step completion (34 ms)
          ✓ should show deploy step (58 ms)
          ✓ should enable the next button (62 ms)
    when step is 7
      ✓ should render deploy (21 ms)
      ✓ should hide the back button (22 ms)
      ✓ should hide the next button (22 ms)
      ✓ should enable the cancel button (20 ms)
      ✓ should show "Close" on the cancel button (20 ms)

@ilyannn ilyannn added Feature:AutomaticImport Team:Security-Scalability Team label for Security Integrations Scalability Team bug Fixes for quality problems that affect the customer experience refactoring backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix labels Nov 13, 2024
@ilyannn ilyannn changed the title Fix the enter bug [Automatic Import] Fix the enter bug Nov 13, 2024
@ilyannn ilyannn marked this pull request as ready for review November 13, 2024 13:26
@ilyannn ilyannn requested a review from a team as a code owner November 13, 2024 13:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-scalability (Team:Security-Scalability)

@ilyannn
Copy link
Contributor Author

ilyannn commented Nov 13, 2024

@elasticmachine merge upstream

Copy link
Contributor

@bhapas bhapas left a comment

Choose a reason for hiding this comment

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

Can you also add the corresponding cypress tests to test these UI changes

@ilyannn ilyannn marked this pull request as draft November 19, 2024 14:30
@ilyannn ilyannn self-assigned this Nov 19, 2024
@ilyannn ilyannn added the accessibility: keyboard navigation Accessibility keyboard navigation and focus label Nov 19, 2024
@ilyannn
Copy link
Contributor Author

ilyannn commented Nov 26, 2024

Can you also add the corresponding cypress tests to test these UI changes

It looks like this will be the first time we add Cypress tests to our plugin. Shouldn't we best do it in a feature PR and not in a bugfix?

@ilyannn ilyannn requested a review from kgeller November 26, 2024 12:15
@ilyannn ilyannn marked this pull request as ready for review December 6, 2024 15:59
@ilyannn ilyannn requested a review from a team as a code owner December 6, 2024 15:59
@ilyannn ilyannn removed the request for review from semd December 8, 2024 22:23
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Dec 8, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@ilyannn ilyannn enabled auto-merge (squash) December 8, 2024 22:25
Copy link
Contributor

@kgeller kgeller left a comment

Choose a reason for hiding this comment

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

LGTM

@bhapas bhapas disabled auto-merge December 10, 2024 06:37
Copy link
Contributor

@bhapas bhapas left a comment

Choose a reason for hiding this comment

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

LGTM. Do we want to merge this before the sustainable kibana changes

@ilyannn ilyannn enabled auto-merge (squash) December 11, 2024 10:36
@ilyannn ilyannn removed the request for review from a team December 11, 2024 13:28
@ilyannn ilyannn merged commit d8bb72e into elastic:main Dec 11, 2024
8 checks passed
@elasticmachine
Copy link
Contributor

💚 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
integrationAssistant 879.8KB 880.5KB +702.0B

History

cc @ilyannn

@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12279126884

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 11, 2024
## Release Notes

Fixes the bug where pressing Enter reloaded the Automatic Import.

## Summary

- Fixes elastic#198238
- Adds/fixes telemetry for CEL events.
- Refactors navigation functionality.
- Adds extensive unit tests and a Cypress test for it.

## Details

When the user presses the Enter inside our input field, the expected
action is to send the form, in this case completing the step. However,
previously the form submission would instead lead to reloading the whole
Automatic Import page.

In this PR we capture the form submission event and bubble it up as
`completeStep` to the main component. We also move the implementation
from the `Footer` up to this main component
`CreateIntegrationAssistant`. This helps collect all the details about
the step order in one place and refactor this logic.

As a result, pressing `Enter` in any field now either
 - Is processed by the field itself (in case of multi-line fields);
- Leads to the same action as pressing the "Next" button (desired
result); or
- Does nothing (e.g. in the inputs in the "Define data stream and upload
logs" group – the reason for this is unclear).

We add CEL-specific telemetry identifiers so that telemetry for step 5
is not always reported as `Deploy Step`.

We also rename a bunch of stuff that was named `...StepReady` into
`...StepReadyToComplete` as the previous name was ambiguous. To
demonstrate this ambiguity we've enlisted the help of GPT 4o:

<img width="832" alt="SCR-20241125-tiaa"
src="https://github.com/user-attachments/assets/ad6bcf7c-7cb2-41c2-ac6b-38924ce990d3">

## Testing

We provide a Cypress test for Enter behavior: pressing it on the
"integration title" input should let the flow proceed to the next step.
This test fails on `main`.

We also provide unit tests for all steps of navigation functionality in
`x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx`:

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit d8bb72e)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Dec 11, 2024
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Automatic Import] Fix the enter bug
(#199894)](#199894)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ilya
Nikokoshev","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-12-11T15:04:45Z","message":"[Automatic
Import] Fix the enter bug (#199894)\n\n## Release Notes\r\n\r\nFixes the
bug where pressing Enter reloaded the Automatic Import.\r\n\r\n##
Summary\r\n\r\n- Fixes #198238\r\n- Adds/fixes telemetry for CEL
events.\r\n- Refactors navigation functionality.\r\n- Adds extensive
unit tests and a Cypress test for it.\r\n\r\n## Details\r\n\r\nWhen the
user presses the Enter inside our input field, the expected\r\naction is
to send the form, in this case completing the step.
However,\r\npreviously the form submission would instead lead to
reloading the whole\r\nAutomatic Import page.\r\n\r\nIn this PR we
capture the form submission event and bubble it up as\r\n`completeStep`
to the main component. We also move the implementation\r\nfrom the
`Footer` up to this main component\r\n`CreateIntegrationAssistant`. This
helps collect all the details about\r\nthe step order in one place and
refactor this logic.\r\n\r\nAs a result, pressing `Enter` in any field
now either\r\n - Is processed by the field itself (in case of multi-line
fields);\r\n- Leads to the same action as pressing the \"Next\" button
(desired\r\nresult); or\r\n- Does nothing (e.g. in the inputs in the
\"Define data stream and upload\r\nlogs\" group – the reason for this is
unclear).\r\n\r\nWe add CEL-specific telemetry identifiers so that
telemetry for step 5\r\nis not always reported as `Deploy
Step`.\r\n\r\nWe also rename a bunch of stuff that was named
`...StepReady` into\r\n`...StepReadyToComplete` as the previous name was
ambiguous. To\r\ndemonstrate this ambiguity we've enlisted the help of
GPT 4o:\r\n\r\n<img width=\"832\"
alt=\"SCR-20241125-tiaa\"\r\nsrc=\"https://github.com/user-attachments/assets/ad6bcf7c-7cb2-41c2-ac6b-38924ce990d3\">\r\n\r\n\r\n##
Testing\r\n\r\nWe provide a Cypress test for Enter behavior: pressing it
on the\r\n\"integration title\" input should let the flow proceed to the
next step.\r\nThis test fails on `main`.\r\n\r\nWe also provide unit
tests for all steps of navigation functionality
in\r\n`x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx`:\r\n\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d8bb72ebfdb1514f1fc03857d4c4e02f70fb7403","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","refactoring","Team:Fleet","v9.0.0","accessibility:
keyboard
navigation","backport:prev-minor","Team:Security-Scalability","Feature:AutomaticImport"],"title":"[Automatic
Import] Fix the enter
bug","number":199894,"url":"https://github.com/elastic/kibana/pull/199894","mergeCommit":{"message":"[Automatic
Import] Fix the enter bug (#199894)\n\n## Release Notes\r\n\r\nFixes the
bug where pressing Enter reloaded the Automatic Import.\r\n\r\n##
Summary\r\n\r\n- Fixes #198238\r\n- Adds/fixes telemetry for CEL
events.\r\n- Refactors navigation functionality.\r\n- Adds extensive
unit tests and a Cypress test for it.\r\n\r\n## Details\r\n\r\nWhen the
user presses the Enter inside our input field, the expected\r\naction is
to send the form, in this case completing the step.
However,\r\npreviously the form submission would instead lead to
reloading the whole\r\nAutomatic Import page.\r\n\r\nIn this PR we
capture the form submission event and bubble it up as\r\n`completeStep`
to the main component. We also move the implementation\r\nfrom the
`Footer` up to this main component\r\n`CreateIntegrationAssistant`. This
helps collect all the details about\r\nthe step order in one place and
refactor this logic.\r\n\r\nAs a result, pressing `Enter` in any field
now either\r\n - Is processed by the field itself (in case of multi-line
fields);\r\n- Leads to the same action as pressing the \"Next\" button
(desired\r\nresult); or\r\n- Does nothing (e.g. in the inputs in the
\"Define data stream and upload\r\nlogs\" group – the reason for this is
unclear).\r\n\r\nWe add CEL-specific telemetry identifiers so that
telemetry for step 5\r\nis not always reported as `Deploy
Step`.\r\n\r\nWe also rename a bunch of stuff that was named
`...StepReady` into\r\n`...StepReadyToComplete` as the previous name was
ambiguous. To\r\ndemonstrate this ambiguity we've enlisted the help of
GPT 4o:\r\n\r\n<img width=\"832\"
alt=\"SCR-20241125-tiaa\"\r\nsrc=\"https://github.com/user-attachments/assets/ad6bcf7c-7cb2-41c2-ac6b-38924ce990d3\">\r\n\r\n\r\n##
Testing\r\n\r\nWe provide a Cypress test for Enter behavior: pressing it
on the\r\n\"integration title\" input should let the flow proceed to the
next step.\r\nThis test fails on `main`.\r\n\r\nWe also provide unit
tests for all steps of navigation functionality
in\r\n`x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx`:\r\n\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d8bb72ebfdb1514f1fc03857d4c4e02f70fb7403"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/199894","number":199894,"mergeCommit":{"message":"[Automatic
Import] Fix the enter bug (#199894)\n\n## Release Notes\r\n\r\nFixes the
bug where pressing Enter reloaded the Automatic Import.\r\n\r\n##
Summary\r\n\r\n- Fixes #198238\r\n- Adds/fixes telemetry for CEL
events.\r\n- Refactors navigation functionality.\r\n- Adds extensive
unit tests and a Cypress test for it.\r\n\r\n## Details\r\n\r\nWhen the
user presses the Enter inside our input field, the expected\r\naction is
to send the form, in this case completing the step.
However,\r\npreviously the form submission would instead lead to
reloading the whole\r\nAutomatic Import page.\r\n\r\nIn this PR we
capture the form submission event and bubble it up as\r\n`completeStep`
to the main component. We also move the implementation\r\nfrom the
`Footer` up to this main component\r\n`CreateIntegrationAssistant`. This
helps collect all the details about\r\nthe step order in one place and
refactor this logic.\r\n\r\nAs a result, pressing `Enter` in any field
now either\r\n - Is processed by the field itself (in case of multi-line
fields);\r\n- Leads to the same action as pressing the \"Next\" button
(desired\r\nresult); or\r\n- Does nothing (e.g. in the inputs in the
\"Define data stream and upload\r\nlogs\" group – the reason for this is
unclear).\r\n\r\nWe add CEL-specific telemetry identifiers so that
telemetry for step 5\r\nis not always reported as `Deploy
Step`.\r\n\r\nWe also rename a bunch of stuff that was named
`...StepReady` into\r\n`...StepReadyToComplete` as the previous name was
ambiguous. To\r\ndemonstrate this ambiguity we've enlisted the help of
GPT 4o:\r\n\r\n<img width=\"832\"
alt=\"SCR-20241125-tiaa\"\r\nsrc=\"https://github.com/user-attachments/assets/ad6bcf7c-7cb2-41c2-ac6b-38924ce990d3\">\r\n\r\n\r\n##
Testing\r\n\r\nWe provide a Cypress test for Enter behavior: pressing it
on the\r\n\"integration title\" input should let the flow proceed to the
next step.\r\nThis test fails on `main`.\r\n\r\nWe also provide unit
tests for all steps of navigation functionality
in\r\n`x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx`:\r\n\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"d8bb72ebfdb1514f1fc03857d4c4e02f70fb7403"}}]}]
BACKPORT-->

Co-authored-by: Ilya Nikokoshev <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
## Release Notes

Fixes the bug where pressing Enter reloaded the Automatic Import.

## Summary

- Fixes elastic#198238
- Adds/fixes telemetry for CEL events.
- Refactors navigation functionality.
- Adds extensive unit tests and a Cypress test for it.

## Details

When the user presses the Enter inside our input field, the expected
action is to send the form, in this case completing the step. However,
previously the form submission would instead lead to reloading the whole
Automatic Import page.

In this PR we capture the form submission event and bubble it up as
`completeStep` to the main component. We also move the implementation
from the `Footer` up to this main component
`CreateIntegrationAssistant`. This helps collect all the details about
the step order in one place and refactor this logic.

As a result, pressing `Enter` in any field now either
 - Is processed by the field itself (in case of multi-line fields);
- Leads to the same action as pressing the "Next" button (desired
result); or
- Does nothing (e.g. in the inputs in the "Define data stream and upload
logs" group – the reason for this is unclear).

We add CEL-specific telemetry identifiers so that telemetry for step 5
is not always reported as `Deploy Step`.

We also rename a bunch of stuff that was named `...StepReady` into
`...StepReadyToComplete` as the previous name was ambiguous. To
demonstrate this ambiguity we've enlisted the help of GPT 4o:

<img width="832" alt="SCR-20241125-tiaa"
src="https://github.com/user-attachments/assets/ad6bcf7c-7cb2-41c2-ac6b-38924ce990d3">


## Testing

We provide a Cypress test for Enter behavior: pressing it on the
"integration title" input should let the flow proceed to the next step.
This test fails on `main`.

We also provide unit tests for all steps of navigation functionality in
`x-pack/plugins/integration_assistant/public/components/create_integration/create_integration_assistant/create_integration_assistant.test.tsx`:


Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility: keyboard navigation Accessibility keyboard navigation and focus backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience Feature:AutomaticImport refactoring release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team Team:Security-Scalability Team label for Security Integrations Scalability Team v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Automatic Import] Bug: pressing Enter resets the process
6 participants