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

Add UI notifier to indicate secret fields and to remember / reenter values #80657

Merged
merged 13 commits into from
Oct 21, 2020

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Oct 15, 2020

Resolves #78476

In this PR, I'm adding a message above encrypted fields to indicate to the user they must remember the values when creating a connector as well as explaining why the values are empty when editing the connector.

Screenshots

Create email connector Screen Shot 2020-10-15 at 2 38 50 PM
Edit email connector Screen Shot 2020-10-15 at 2 39 13 PM
Create IBM Resilient connector Screen Shot 2020-10-16 at 11 17 48 AM
Edit IBM Resilient connector Screen Shot 2020-10-15 at 2 39 52 PM
Create Jira connector Screen Shot 2020-10-16 at 11 17 27 AM
Edit Jira connector Screen Shot 2020-10-15 at 2 40 31 PM
Create PagerDuty connector Screen Shot 2020-10-15 at 2 40 50 PM
Edit PagerDuty connector Screen Shot 2020-10-15 at 2 41 10 PM
Create ServiceNow connector Screen Shot 2020-10-15 at 2 41 26 PM
Edit ServiceNow connector Screen Shot 2020-10-15 at 2 41 38 PM
Create Slack connector Screen Shot 2020-10-15 at 2 41 53 PM
Edit Slack connector Screen Shot 2020-10-15 at 2 42 06 PM
Create Webhook connector Screen Shot 2020-10-15 at 2 42 23 PM
Edit Webhook connector Screen Shot 2020-10-15 at 2 42 53 PM

Note for docs

This PR changes the connector create / edit flyouts so screenshots will need to be updated for most (minus server log, index action). The user docs could also have the same messaging about the encrypted values. Copy: #78476 (comment).

@mikecote mikecote added release_note:enhancement Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 labels Oct 15, 2020
@mikecote mikecote self-assigned this Oct 15, 2020
@mikecote
Copy link
Contributor Author

@gchaps can you please review the screenshots in the descriptions? It contains the suggestions you've proposed here: #78476 (comment). Note that I did change the wording a bit for IBM resilient (API key ID and secret are encrypted), let me know what you think.

@mdefazio can you please review the screenshots in the description from a design perspective? Just to confirm it turned out the way you were hoping it would.

@mdefazio
Copy link
Contributor

I don't think we need the green color on the text. I vote for the default EUI color text. It's the only sentence there so it's scannable enough IMO. Other than that, all looks good.

@mikecote
Copy link
Contributor Author

Thanks @mdefazio! Just to confirm, you're referring to the create forms where the message is using color="secondary" and it should just use the default color instead?

@mikecote mikecote marked this pull request as ready for review October 15, 2020 16:56
@mikecote mikecote requested a review from a team as a code owner October 15, 2020 16:56
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@mdefazio
Copy link
Contributor

mdefazio commented Oct 15, 2020 via email

@gchaps
Copy link
Contributor

gchaps commented Oct 15, 2020

Here are my comments:

Create email connector

  • Configuring email accounts > Configure email accounts
  • Because this is a link, remove the ending period

Edit email connector

  • Configuring email accounts > Configure email accounts

Create IBM Resilient connector

  • A new incident isn't updated. Can the text be "Push data to an incident in Resilient". Is Push the right word?

Edit IBM Resilient connector

  • ID and secret are encrypted. (API key isn't needed because its in the heading.)

Create Jira connector

  • Subtitle requires ending period. (Push ....)

  • Push data to an issue in Jira (see comment above)

  • Username or email address (username typically comes first)

  • API token or password (sentence caps)

Edit Jira connector

  • Username or email address

  • API token or password (sentence caps)

Create PagerDuty connector

  • Configure a PagerDuty account (remove ending period)

Edit PagerDuty connector

  • Configure a PagerDuty account (remove ending period)

Create ServiceNow connector

  • Configure Personal Developer Instance for ServiceNow is a mouthful with lots of title caps. Can it be shortened to: Configure a Personal Developer Instance (no ending period)

Edit ServiceNow connector

  • Configure a Personal Developer Instance

Create Slack connector

  • Is the word "Example" needed in the placeholder text?

  • Create a Slack Webhook URL (uppercase W for Webhook)

Edit Slack connector

  • Same as above

Create Webhook connector

  • The placeholder text is hard to read. Is there a different format to use, maybe without brackets: https://site-url or https://site-url. If so, you would also need to change it in the other connector pages

Edit Webhook connector

  • Looks good

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM pending suggested UX and wording updates!

Curious if there is a reason why some username and password fields are on a single row and some are on separate rows? Ex: Email connector vs ServiceNow connector?

@mikecote
Copy link
Contributor Author

@gchaps @mdefazio Thank you both for the feedback. I went ahead and updated the screenshots with the latest changes.

@gchaps A few things I didn't change yet, see below:

A new incident isn't updated. Can the text be "Push data to an incident in Resilient". Is Push the right word?

Push data to an issue in Jira (see comment above)

How about something like Create an incident in IBM Resilient. and Create an incident in Jira.? It seems the descriptions in the docs contains this better version and the UI forgot to be updated.

Is the word "Example" needed in the placeholder text?

The placeholder text is hard to read. Is there a different format to use, maybe without brackets: https://site-url or https://site-url. If so, you would also need to change it in the other connector pages

I will echo this feedback to the team at our next design meeting and see what we can come up with. I'll be happy to circle back and change that in a follow up PR (since that text is already in use today).

@mikecote
Copy link
Contributor Author

@ymao1 thanks for the review!

Curious if there is a reason why some username and password fields are on a single row and some are on separate rows? Ex: Email connector vs ServiceNow connector?

Good catch, no reason at all that I can think of. I'm guessing it's just an inconsistency but worth opening an issue to get it fixed.

@gchaps
Copy link
Contributor

gchaps commented Oct 15, 2020

@mikecote Create an incident in IBM Resilient. and Create an incident in Jira. sound good to me.

Good idea to talk to design about the placeholder text.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Just a comment for now, after looking at the screen shots, full review in a bit.

I like it! :-)

This is the first time I noticed the hasAuth on the email connector, and happened to remember when that was added, that webhook has the same sort of thing - the user/password are optional. I thought we opened an issue to address that, but I'm not seeing one. It would presumably need the same sort of hasAuth switch. We could extend the messaging there for now, indicating somehow they are optional, but if you did enter one before, you will need to enter it again - or whatever. Or leave it as it is in this PR, I think all the webhook calls I've seen in practice so far do require a user/pass, and we can address later.

Do you happen to know if we do have an issue on the webhook hasAuth switch? I'm not seeing one ...

@mikecote
Copy link
Contributor Author

@pmuellr

Do you happen to know if we do have an issue on the webhook hasAuth switch? I'm not seeing one ...

I don't recall seeing one and just did a search myself and turned out negative. I'm thinking of leaving it as is in this PR and opening an issue to add the hasAuth switch there as well. I always though those fields were required but turns out they're missing the (optional) label on the top. Adding the hasAuth switch will fix it!

@mikecote
Copy link
Contributor Author

@gchaps thank you, I went ahead and applied the change and updated the screenshots.

@mikecote
Copy link
Contributor Author

I'm thinking of leaving it as is in this PR and opening an issue to add the hasAuth switch there as well.

I have opened this issue to fix the Webhook connector to use the hasAuth switch: #80882.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@mikecote
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
triggersActionsUi 1.5MB 1.5MB +12.1KB

page load bundle size

id before after diff
triggersActionsUi 151.4KB 154.3KB +2.9KB

History

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

@mikecote mikecote merged commit 9aa9290 into elastic:master Oct 21, 2020
mikecote added a commit to mikecote/kibana that referenced this pull request Oct 21, 2020
…alues (elastic#80657)

* Initial work

* Add messaging to Jira connector

* Add messaging to PagerDuty connector

* Add messaging to Email connector

* Add messaging to ServiceNow connector

* Add messaging to Webhook connector

* Add unit tests

* Fix jest test

* Apply design feedback

* Apply copy feedback

* Fix failing test

* Fix connector descriptions for jira and resilient

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
mikecote added a commit that referenced this pull request Oct 21, 2020
…alues (#80657) (#81371)

* Initial work

* Add messaging to Jira connector

* Add messaging to PagerDuty connector

* Add messaging to Email connector

* Add messaging to ServiceNow connector

* Add messaging to Webhook connector

* Add unit tests

* Fix jest test

* Apply design feedback

* Apply copy feedback

* Fix failing test

* Fix connector descriptions for jira and resilient

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 22, 2020
* master: (63 commits)
  [KP] Fix Headers timeout issue (elastic#81140)
  [ML] Functional tests - stabilize typing with checks service method (elastic#81338)
  tabify - support docs (elastic#80351)
  [Security Solution][Detections] Look-back time logic fix (elastic#81383)
  [Workplace Search] Add top-level tests for Groups (elastic#81215)
  [Fleet] Fix agent action observable for long polling (elastic#81376)
  [Maps] fix feature tooltip remains open when zoom level change hides layer (elastic#81373)
  skip flaky suite (elastic#78689)
  chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies (elastic#81357)
  Ensure some data is returned (elastic#81375)
  Change dumb-init to tini (elastic#81126)
  [Reporting/Tech Debt] Convert PdfMaker class to TypeScript (elastic#81242)
  Use Storybook Controls instead of Knobs (elastic#80705)
  [junit] make sure that report paths are unique (elastic#81255)
  bump elastic/elasticsearch-js version to 7.10.0-rc1 (elastic#81288)
  run ssl tests on CI (elastic#81320)
  Fix alert defaults (elastic#81207)
  [ML] DF Analytics wizard: ensure user can set mml manually or select to use given estimate (elastic#81078)
  Add UI notifier to indicate secret fields and to remember / reenter values (elastic#80657)
  [Monitoring] Use async/await (elastic#81200)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Actions] Connector with secrets should have a UI notifier for the user, that it is a secret field.
7 participants