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

[Actions] Adding hasAuth to Webhook Configuration to avoid confusing UX #81390

Merged
merged 7 commits into from
Oct 27, 2020

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Oct 21, 2020

Resolves #80882

Summary

Added hasAuth to webhook connector create/edit form controlled by a UI switch to indicate whether webhook connector needs username and password.

Webhook connector create with username and password
Screen Shot 2020-10-22 at 8 40 04 AM

Webhook connector create with no auth
Screen Shot 2020-10-22 at 8 40 12 AM

Webhook connector edit with auth
Screen Shot 2020-10-22 at 8 41 12 AM

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

@ymao1 ymao1 changed the title Alerting/webhook secrets [Actions] Adding hasAuth to Webhook Configuration to avoid confusing UX Oct 22, 2020
@ymao1 ymao1 self-assigned this Oct 22, 2020
@ymao1 ymao1 added Feature:Actions release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.11.0 v8.0.0 labels Oct 22, 2020
@ymao1 ymao1 marked this pull request as ready for review October 22, 2020 12:47
@ymao1 ymao1 requested a review from a team as a code owner October 22, 2020 12:47
@elasticmachine
Copy link
Contributor

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

@gmmorris gmmorris self-requested a review October 22, 2020 15:14
@ymao1
Copy link
Contributor Author

ymao1 commented Oct 26, 2020

@elasticmachine merge upstream

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, UX is behaving as expected and test coverage is good 👍

One request though: can we add a test that ensures the migrations work as expected?
We have unit tests but no e2e tests (they can probably go under https://github.com/elastic/kibana/blob/master/x-pack/test/alerting_api_integration/spaces_only/tests/actions/migrations.ts)

@mikecote mikecote self-requested a review October 26, 2020 14:43
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

@mikecote
Copy link
Contributor

@ymao1 just wanted to drop a note about a bug caught in the email hasAuth work: #81673.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

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

page load bundle size

id before after diff
triggersActionsUi 154.3KB 154.8KB +596.0B

History

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

@ymao1 ymao1 merged commit fd7f6b5 into elastic:master Oct 27, 2020
ymao1 added a commit to ymao1/kibana that referenced this pull request Oct 27, 2020
…g UX (elastic#81390)

* Adding hasAuth to server and client

* Adding migration and fixing tests

* Fixing test

* Adding spacing

* Adding functional test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ymao1 added a commit that referenced this pull request Oct 27, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master:
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  [Lens] Fix URL query loss on redirect (elastic#81475)
  Log reason for 404 in field existence check (elastic#81315)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
* master: (87 commits)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81778)
  [i18n] add get_kibana_translation_paths tests (elastic#81624)
  [UX] Fix search term reset from url (elastic#81654)
  [Lens] Improved range formatter (elastic#80132)
  [Resolver] `SideEffectContext` changes, remove `@testing-library` uses (elastic#81077)
  [Time to Visualize] Update Library Text with Call to Action (elastic#81667)
  [docs] Resolving failed Kibana upgrade migrations (elastic#80999)
  [ftr/menuToggle] provide helper for enhanced menu toggle handling (elastic#81709)
  [Task Manager] adds basic observability into Task Manager's runtime operations (elastic#77868)
  Doc changes for stack management and grouped feature privileges (elastic#80486)
  Added functional test for alerts list filters by status, alert type and action type. Did a code refactoring and splitting for alerts tests. (elastic#81422)
  [Security Solution][Endpoint][Admin] Malware Protections Notify User Version (elastic#81415)
  Revert "[Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)"
  [Maps] Use default format when proxying EMS-files (elastic#79760)
  [Discover] Deangularize context.html (elastic#81442)
  Use the displayName property in default editor (elastic#73311)
  adds bug label to Bug report for Security Solution template (elastic#81643)
  [ML] Transforms: Remove index field limitation for custom query. (elastic#81467)
  [Actions] Adding `hasAuth` to Webhook Configuration to avoid confusing UX (elastic#81390)
  [Task Manager] Mark task as failed if maxAttempts has been met. (elastic#80681)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 29, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 81390 or prevent reminders by adding the backport:skip label.

@ymao1 ymao1 added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 29, 2020
@mikecote mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2020
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: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.

Confusing UX when editing Webhook connector with secrets
5 participants