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

[ResponseOps][Connectors] Move create connector button in Management #190125

Merged

Conversation

guskovaue
Copy link
Contributor

@guskovaue guskovaue commented Aug 8, 2024

Issue: #188189

Here "create connector" button should be moved to the header (top right) for consistency with Rules and Cases page.

How to check:

So it should looks like this on Connectors tab:
Screenshot 2024-08-12 at 17 15 25

And like this (without Create connector button) on tab Logs:
Screenshot 2024-08-12 at 17 15 37

Create connector flyout should open when Create connector button will be pushed.
Screenshot 2024-08-12 at 17 15 48

Also good to check if edit flyout will open when edit a connector.

Checklist

@guskovaue guskovaue self-assigned this Aug 8, 2024
@guskovaue guskovaue added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:skip Skip the PR/issue when compiling release notes v8.16.0 labels Aug 8, 2024
@guskovaue guskovaue marked this pull request as ready for review August 12, 2024 15:38
@guskovaue guskovaue requested a review from a team as a code owner August 12, 2024 15:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@guskovaue
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

I tested and it's working as expected.

Left some small comments 👍

Comment on lines 115 to 117
expect(createFirstActionButton).toBeEnabled();
userEvent.click(createFirstActionButton);
expect(setAddFlyoutVisibility).toBeCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
expect(createFirstActionButton).toBeEnabled();
userEvent.click(createFirstActionButton);
expect(setAddFlyoutVisibility).toBeCalled();
userEvent.click(createFirstActionButton);
await waitFor(() => {
expect(setAddFlyoutVisibility).toBeCalled();
});

Two minor things that I would do differently based on the flaky tests we had in the past:

  1. no need to check if the button is enabled when we are clicking it anyway in the next line.
  2. better to wrap the expect(function).toBeCalled() in a waitFor block for a longer timeout.

const queryClient = new QueryClient();

describe('ActionsConnectorsHome', () => {
beforeEach(() => {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
beforeEach(() => {});

location: createLocation('/logs'),
match: {
isExact: true,
path: `/logs`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
path: `/logs`,
path: '/logs',

Comment on lines 177 to 179
await waitFor(() => {
expect(screen.queryByRole('button', { name: 'Create connector' })).not.toBeInTheDocument();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await waitFor(() => {
expect(screen.queryByRole('button', { name: 'Create connector' })).not.toBeInTheDocument();
});
expect(screen.queryByRole('button', { name: 'Create connector' })).not.toBeInTheDocument();

I don't think you need the waitFor in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adcoelho I had doubts here. I render the component. So should wait until it will be render. Everywhere I used find in the same situation. But here I cannot because if queryBy, so I used waitFor to follow the same logic.
So you do not think I need for render to happened? How does it work?

Copy link
Contributor

Choose a reason for hiding this comment

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

So should wait until it will be render. Everywhere I used find in the same situation. But here I cannot because if queryBy, so I used waitFor to follow the same logic.
So you do not think I need for render to happened? How does it work?

I am not sure I understand what you are trying to say 😅 but all waitFor does is run the callback you pass it until it either does not throw or a timeout happens.

In your test, the condition expect(screen.queryByRole('button', ...)).not.toBeInTheDocument(); will never throw because the button is not there so it is pointless to have the waitFor.

As a side note, I don't think "waiting for render" as you said applies. The documentation as an example where getBy is used and no waitFor is there to "wait for render".

test('renders a message', () => {
  const {asFragment, getByText} = render(<Greeting />)
  expect(getByText('Hello, world!')).toBeInTheDocument()

It is useful if you have lots of async stuff going on that you need to wait for before something happens but it doesn't mean nothing rendered.

@guskovaue guskovaue force-pushed the MX-88189-move-create-connector-button branch from 7ef21c0 to f90ba9a Compare August 14, 2024 11:14
@guskovaue guskovaue force-pushed the MX-88189-move-create-connector-button branch 2 times, most recently from 6c42bd0 to ce762bd Compare August 14, 2024 14:17
@adcoelho
Copy link
Contributor

Why the last commit adding getBy instead of findBy? that part looked fine.

@guskovaue guskovaue force-pushed the MX-88189-move-create-connector-button branch 2 times, most recently from 9193e85 to a077119 Compare August 14, 2024 15:57
@guskovaue guskovaue force-pushed the MX-88189-move-create-connector-button branch from a077119 to f6deeb3 Compare August 14, 2024 17:21
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #9 / console app misc console behavior keyboard shortcuts open documentation should open documentation when Ctrl+/ is pressed

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
triggersActionsUi 1.6MB 1.6MB -16.4KB

Page load bundle

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

id before after diff
triggersActionsUi 120.9KB 120.9KB +6.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
triggersActionsUi 125 127 +2

Total ESLint disabled count

id before after diff
triggersActionsUi 131 133 +2

History

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

cc @guskovaue

@guskovaue guskovaue merged commit 0a29b68 into elastic:main Aug 15, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 15, 2024
@guskovaue guskovaue linked an issue Aug 15, 2024 that may be closed by this pull request
2 tasks
@cnasikas cnasikas changed the title [MX] Move create connector button in Management [ResponseOps][Connectors] Move create connector button in Management Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][Connectors] Create connector button position
5 participants