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][Stack Connectors] Opsgenie connector UI #142411

Merged
merged 29 commits into from
Oct 18, 2022

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Sep 30, 2022

Issue: #142776

This PR implements the Opsgenie connector's UI for phase 1 of the issue.

For creating the alert it includes the fields: message, description, and alias. For closing the alert it includes the fields: note and alias.

Notes

  • This PR does not include the Opsgenie logo. We're still waiting to get that.

Testing

Refer to this PR's testing section for details on how to create an Opsgenie environment: #142164

Examples

Connector card icon

image

Create Opsgenie Connector

image

Test Connector

Creating alert

image

Closing alert

image

Create alert when rules fires

image

Close alert when rule recovers

image

Release Notes

This feature allows users to create and close alerts within Opsgenie.

@jonathan-buttner jonathan-buttner added backport:skip This commit does not require backporting Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes v8.6.0 labels Sep 30, 2022
@jonathan-buttner jonathan-buttner marked this pull request as ready for review October 13, 2022 19:45
@jonathan-buttner jonathan-buttner requested review from a team as code owners October 13, 2022 19:45
@elasticmachine
Copy link
Contributor

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

import React from 'react';
import { LogoProps } from '../../types';

const Logo = (props: LogoProps) => <EuiIcon type="casesApp" size="xl" />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just using the cases logo until we get the Opsgenie one.

@@ -46,7 +46,6 @@ export {
SlackConnectorTypeId,
TeamsConnectorTypeId,
WebhookConnectorTypeId,
OpsgenieConnectorTypeId,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to the common folder so that the ui can access it too.

@@ -62,55 +62,14 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
await nameInput.click();
}

async function defineIndexThresholdAlert(alertName: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this into the rules.common service.

@@ -279,22 +289,6 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
});
});

async function createConnector(connectorName: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these to the utils.ts file. Maybe we should move them to the service api like cases 🤷‍♂️


const currentSubAction = useRef<string>(subAction ?? OpsgenieSubActions.CreateAlert);

const actionOptions = [
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can be moved outside the component as it is not depend on the props or the state.

Comment on lines 19 to 21
const createBtnIsVisible = await createBtn.isDisplayed();
if (createBtnIsVisible) {
await createBtn.click();
Copy link
Member

Choose a reason for hiding this comment

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

I think is better to fail if the button does not exist. You can use testSubjets.existsOrFail.

const testSubjects = getService('testSubjects');

return {
async clickCreateFirstConnectorButton() {
Copy link
Member

Choose a reason for hiding this comment

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

nit: the naming is a bit confusing. Maybe we can call it createConnectorFromEmptyPrompt or similar.

Comment on lines 29 to 32
await find.clickByCssSelector(
'[data-test-subj="create-connector-flyout-save-btn"]:not(disabled)'
);
},
Copy link
Member

@cnasikas cnasikas Oct 18, 2022

Choose a reason for hiding this comment

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

Maybe we can do:

Suggested change
await find.clickByCssSelector(
'[data-test-subj="create-connector-flyout-save-btn"]:not(disabled)'
);
},
const button = await testSubjects.find("create-connector-flyout-save-btn")
const isDisabled = await button.isEnabled() // or button.getAttribute('disabled')
expect(isDisabled).toBe(false)
},

return connector;
};

export const getConnector = async (
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use get the connector by ID: https://www.elastic.co/guide/en/kibana/current/get-connector-api.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to use this function is when we don't have the connector id. A few of the tests create the connector via the UI instead of an API call.

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'll update the name to getConnectorByName

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Thanks!

})
.expect(200);

await common.navigateToApp('triggersActionsConnectors');
Copy link
Member

Choose a reason for hiding this comment

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

I think API utils functions should not do side effects like navigating to UI components.

getService: FtrProviderContext['getService'];
}) => {
const common = getPageObject('common');
const supertest = getService('supertest');
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better if we pass the supertest directly as an argument.

import { FtrProviderContext } from '../../../ftr_provider_context';
import { getTestActionData } from '../../../lib/get_test_data';

export const createConnectorAndObjectRemover = async ({
Copy link
Member

Choose a reason for hiding this comment

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

nit: As it is not generic and creates a slack connector maybe is better to rename it to createSlackConnectorAndObjectRemover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Code LTGM! I tested and everything is working as expected. Thank you so much for adding the test services 🚀

@jonathan-buttner jonathan-buttner enabled auto-merge (squash) October 18, 2022 18:50
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackConnectors 133 141 +8

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
stackConnectors 2 4 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
stackConnectors 327.5KB 337.8KB +10.3KB

Page load bundle

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

id before after diff
stackConnectors 20.8KB 22.5KB +1.6KB
Unknown metric groups

API count

id before after diff
stackConnectors 2 4 +2

async chunk count

id before after diff
stackConnectors 50 54 +4

ESLint disabled line counts

id before after diff
stackConnectors 67 72 +5

Total ESLint disabled count

id before after diff
stackConnectors 71 76 +5

History

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

@jonathan-buttner jonathan-buttner merged commit e71a522 into elastic:main Oct 18, 2022
kc13greiner pushed a commit to kc13greiner/kibana that referenced this pull request Oct 18, 2022
* Starting opsgenie backend

* Adding more integration tests

* Updating readme

* Starting ui

* Adding hash and alias

* Fixing tests

* Switch to platinum for now

* Adding server side translations

* Fixing merge issues

* Fixing file location error

* Working ui

* Default alias is working

* Almost working validation fails sometimes

* Adding end to end tests

* Adding more tests

* Adding note and description fields

* Removing todo

* Fixing test errors

* Addressing feedback

* Trying to fix test flakiness
@jonathan-buttner jonathan-buttner deleted the opsgenie-ui branch October 18, 2022 20:45
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 Feature:Actions release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants