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

[Cases] Update UI to use the new connector's API #149276

Merged
merged 38 commits into from
Jan 31, 2023

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Jan 20, 2023

Summary

This PR updates the UI to use the new connector's API introduced by this PR #147295. In this PR I refactor a bit some of the logic we have for the connectors. I left comments to explain the reasoning.

Testing

Please test the following:

  • The fields of each connector are preserved correctly when changing connectors
  • The following callout is shown when you do not have selected any connector

Screenshot 2023-01-23 at 2 21 33 PM

  • The edit connector button (pencil) is not shown if you do not have access to use connectors. Also, the following text should be shown.

Screenshot 2023-01-23 at 2 20 30 PM

  • You see the following callout if a) select a connector to a case and b) then delete the connector

Screenshot 2023-01-23 at 2 11 49 PM

  • The text of the connector's push button is shown correctly. It should be Push <connector_nane> for the first push and Updated Push <connector_nane> for any other push.

Screenshot 2023-01-25 at 12 09 29 PM

Screenshot 2023-01-25 at 12 09 17 PM

  • The connector's push button is disabled correctly.
  • When the connector's push button is disabled a tooltip is shown

Screenshot 2023-01-25 at 12 11 27 PM

  • The push user action label is displayed correctly. It should be pushed as new incident... for the first push and updated incident... for any other push.

Screenshot 2023-01-25 at 9 54 23 AM

Screenshot 2023-01-25 at 11 56 56 AM

  • The push arrow indicators are shown correctly. The top arrow should be displayed only if it is the latest push. The bottom arrow should be displayed only if it is the latest push and new changes need to be pushed. Changes are considered the update of text or description, the addition or edit of a comment, and the addition or removal of a case.

Screenshot 2023-01-11 at 1 38 37 PM

Screenshot 2023-01-11 at 1 37 31 PM

  • The "View <incident_name>" shows the latest push incident of each connector when changing connectors.

Screenshot 2023-01-23 at 2 25 30 PM

Depends on: #149451, #149535

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.7.0 labels Jan 20, 2023
@cnasikas cnasikas self-assigned this Jan 20, 2023
@cnasikas cnasikas force-pushed the get_connectors_info_from_server branch from e4efb8f to 71e543d Compare January 21, 2023 16:46
@@ -127,16 +132,12 @@ export const CaseViewActivity = ({
[assignees, onUpdateField]
);

const { isLoading: isLoadingConnectors, data: connectors = [] } = useGetConnectors();

const [connectorName, isValidConnector] = useMemo(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic inside the EditConnector component


const onSubmitConnector = useCallback(
(connectorId, connectorFields, onError, onSuccess) => {
const connector = getConnectorById(connectorId, connectors);
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic inside the EditConnector component

onSubmit={onSubmitConnector}
userActions={userActionsData.caseUserActions}
key={caseData.connector.id}
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to reset the state of the EditConnector component every time the current connector changes. We use the key do to that. This helps to remove all unnecessary useEffects that update the state when the caseData changes.

@@ -20,12 +19,6 @@ interface ConnectorCardProps {
isLoading: boolean;
}

const StyledText = styled.span`
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed all styles. I converted the component to use EuiFlex* instead of EuiCard. The result is the same as before.

}

const MyFlexGroup = styled(EuiFlexGroup)`
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the styling of the text inside x-pack/plugins/cases/public/components/connectors/card.tsx. The result is the same as before.

{
...initialState,
fields: caseFields,
currentConnector: getConnectorById(caseData.connector.id, allAvailableConnectors),
Copy link
Member Author

@cnasikas cnasikas Jan 23, 2023

Choose a reason for hiding this comment

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

We init the state with data from the caseData. The use of key in https://github.com/elastic/kibana/pull/149276/files#r1083999611 resets the initial state of the reducer every time the caseData.connector.id changes.

connectors={connectors}
hasDataToPush={userActionsData.hasDataToPush}
isLoading={isLoadingConnectors || (isLoading && loadingKey === 'connector')}
isValidConnector={isLoadingConnectors ? true : isValidConnector}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic inside the EditConnector component

<EditConnector
caseData={caseData}
caseServices={userActionsData.caseServices}
connectorName={connectorName}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic inside the EditConnector component

caseServices={userActionsData.caseServices}
connectorName={connectorName}
connectors={connectors}
hasDataToPush={userActionsData.hasDataToPush}
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the logic inside the usePushToService hook which is used by the EditConnector component

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Tested locally. Looks good 👍
Added couple of minor comments.

{!isLoading && !editConnector && pushCallouts && actionsReadCapabilities && (
<EuiFlexItem data-test-subj="push-callouts">{pushCallouts}</EuiFlexItem>
)}
<DisappearingFlexItem $isHidden={!editConnector}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently if user has only read permission / viewer role, and there are no connectors added to case then user sees only header External Incident Management System.
Screenshot 2023-01-26 at 11 35 26

Is it expected behaviour or it should be hidden?

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Great work! Left some suggestions, still need to test it. I'll do that shortly.

return [connector?.name ?? '', !!connector];
}, [connectors, caseData.connector]);
const { isLoading: isLoadingAllAvailableConnectors, data: allAvailableConnectors } =
useGetConnectors();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe rename this hook to something like useGetConnectorsList or useGetConfiguredConnectors. At first glance I couldn't remember why we needed this one and the new useGetCaseConnectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonathan-buttner What about getAllConnectors or getAllActionConnectors? useGetConfiguredConnectors is also fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the API a little more, this one is retrieving all case allowed action connectors right? Maybe getSupportedActionConnectors? The term all doesn't really tell us what's happening beyond it's getting all of something but I'd say getAllActionConnectors is better than getAllConnectors.

Copy link
Member Author

@cnasikas cnasikas Jan 27, 2023

Choose a reason for hiding this comment

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

useGetSupportedActionConnectors sounds good to me.

@@ -88,7 +87,7 @@ export const usePushToService = ({
* By priority of importance:
* 1. Show license error.
* 2. Show configuration error.
* 3. Show connector configuration error if the connector is set to none or no connectors have been created.
* 3. Show connector configuration error if the connector is set to none or no allAvailableConnectors have been created.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what we mean by allAvailableConnectors should this just be connectors?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to distinguish between the case connectors returned from the new API and the action connectors returned by configure/_find. What about supportedActionConnectors? Any other ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, what part of the boolean expression indicates that there were no connectors returned by configure/_find?

connector.id === 'none' && !isLoadingLicense && !hasLicenseError

Basically are we actually checking for that? If not, I'd say we just remove that part of the comment. If I'm misunderstanding it and we are checking then I'd vote for ...or no action connectors have been created that are supported by cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right. Effectively we only check if the connector is set to none. If hasLicenseError=true then the first check (license error) will return. What about "Show connector missing information if the connector is set to none" ?

@jonathan-buttner
Copy link
Contributor

Did some testing! Parity with the previous functionality looks good 👍 I tested the things you suggested in the PR description. I did notice a couple weird rerendering things, I'm not sure if they existed already though. Let me know what you think.

Last user activity jumps When switching between connectors the last user activity appears, then disappears, then reappears. Not sure if that is how it was before though.
rerender.after.switch.connectors.mov
Renders two push sections for a connector

When pushing new activity to a connector that had already had some portion pushed, the activity view shows the old push arrows, the new push arrows for a short time and then removes them to only be the up down arrow.

Renders this for a very short period

two already requires push sections

Eventually renders this

image

renders.two.push.sections.mov

@cnasikas
Copy link
Member Author

cnasikas commented Jan 27, 2023

Tested locally. Looks good 👍 Added couple of minor comments.

Thanks @js-jankisalvi! Yes, this is the current behavior. I opened an issue to improve it #149692.

@jonathan-buttner
Copy link
Contributor

I can't reproduce the jumping or double arrows bugs so removing the connector id must have fixed it 👍

Copy link
Contributor

@jonathan-buttner jonathan-buttner left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, looks good 👍

@cnasikas
Copy link
Member Author

cnasikas commented Jan 30, 2023

I can't reproduce the jumping or double arrows bugs so removing the connector id must have fixed it 👍

Yes, @adcoelho awesome suggestion fixed the bug without me even realizing it 🙂

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.

Tested and lgtm! Should have approved it before, sorry 🙌

Copy link
Contributor

@js-jankisalvi js-jankisalvi 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 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cases 591 593 +2

Async chunks

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

id before after diff
cases 393.5KB 391.7KB -1.8KB

Page load bundle

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

id before after diff
cases 129.6KB 129.6KB -3.0B
Unknown metric groups

ESLint disabled in files

id before after diff
cases 16 18 +2

ESLint disabled line counts

id before after diff
cases 58 55 -3

Total ESLint disabled count

id before after diff
cases 74 73 -1

History

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

cc @cnasikas

@cnasikas cnasikas merged commit d715cf6 into elastic:main Jan 31, 2023
@cnasikas cnasikas deleted the get_connectors_info_from_server branch January 31, 2023 16:12
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 31, 2023
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## Summary

This PR updates the UI to use the new connector's API introduced by this
PR elastic#147295. In this PR I refactor
a bit some of the logic we have for the connectors. I left comments to
explain the reasoning.

## Testing

Please test the following:

- The fields of each connector are preserved correctly when changing
connectors
- The following callout is shown when you do not have selected any
connector

<img width="784" alt="Screenshot 2023-01-23 at 2 21 33 PM"
src="https://user-images.githubusercontent.com/7871006/214534706-5d447472-a40b-4fb4-aa4f-733bdcc76d9e.png">

- The edit connector button (pencil) is not shown if you do not have
access to use connectors. Also, the following text should be shown.

<img width="784" alt="Screenshot 2023-01-23 at 2 20 30 PM"
src="https://user-images.githubusercontent.com/7871006/214535027-4b6b17a6-9d2c-4634-b78a-e966d70ae966.png">

- You see the following callout if a) select a connector to a case and
b) then delete the connector

<img width="778" alt="Screenshot 2023-01-23 at 2 11 49 PM"
src="https://user-images.githubusercontent.com/7871006/214535408-ad72ed87-d9ea-4449-8340-4b17d63ed7b4.png">

- The text of the connector's push button is shown correctly. It should
be `Push <connector_nane>` for the first push and `Updated Push
<connector_nane>` for any other push.

<img width="214" alt="Screenshot 2023-01-25 at 12 09 29 PM"
src="https://user-images.githubusercontent.com/7871006/214536143-72e8f5f7-bb9d-4e4f-b2e7-66186ac76cdb.png">

<img width="292" alt="Screenshot 2023-01-25 at 12 09 17 PM"
src="https://user-images.githubusercontent.com/7871006/214536169-765a682f-f4d9-4389-ad25-ec8184f99040.png">

- The connector's push button is disabled correctly.
- When the connector's push button is disabled a tooltip is shown

<img width="291" alt="Screenshot 2023-01-25 at 12 11 27 PM"
src="https://user-images.githubusercontent.com/7871006/214536498-4058f1bd-2548-4a87-8e7c-589e1717e0a2.png">

- The push user action label is displayed correctly. It should be
`pushed as new incident...` for the first push and `updated incident...`
for any other push.

<img width="630" alt="Screenshot 2023-01-25 at 9 54 23 AM"
src="https://user-images.githubusercontent.com/7871006/214533317-ed29f938-298c-4d5f-94f9-9f11821f9365.png">
<img width="614" alt="Screenshot 2023-01-25 at 11 56 56 AM"
src="https://user-images.githubusercontent.com/7871006/214533604-5be88d51-a021-4095-ab70-1938d82c6bdc.png">

- The push arrow indicators are shown correctly. The top arrow should be
displayed only if it is the latest push. The bottom arrow should be
displayed only if it is the latest push and new changes need to be
pushed. Changes are considered the update of text or description, the
addition or edit of a comment, and the addition or removal of a case.

<img width="371" alt="Screenshot 2023-01-11 at 1 38 37 PM"
src="https://user-images.githubusercontent.com/7871006/214534308-29da895a-2dff-4ae7-ace5-31ff7574feac.png">
<img width="322" alt="Screenshot 2023-01-11 at 1 37 31 PM"
src="https://user-images.githubusercontent.com/7871006/214534312-9e28a398-ae64-4cc2-9f07-0ed72546e337.png">

- The "View <incident_name>" shows the latest push incident of each
connector when changing connectors.

<img width="229" alt="Screenshot 2023-01-23 at 2 25 30 PM"
src="https://user-images.githubusercontent.com/7871006/214039111-0d4692c3-e7c3-4b7b-825b-bd86a0f03a96.png">

Depends on: elastic#149451,
elastic#149535


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Jonathan Buttner <[email protected]>
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:Cases Cases feature 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.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants