Skip to content

[Enterprise Search] Added a Credentials page to App Search #79749

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
Oct 8, 2020
Merged

[Enterprise Search] Added a Credentials page to App Search #79749

merged 30 commits into from
Oct 8, 2020

Conversation

@JasonStoltz
Copy link
Member

@JasonStoltz JasonStoltz commented Oct 6, 2020 •
edited by cee-chen
Loading

Summary

(This PR temporarily cherry picks 3a0c9b2 until the corresponding PR is merged into master)

(Also, paging will not work until this is merged: #79747, but the logic is correct in this PR.)

This PR implements the view portion of Credentials, without:

  • An empty state
  • Hidden Text
  • Loading State
  • Flyout form

53ace58 - This simply adds the route and a stub view

screencapture-localhost-5601-app-enterprise-search-app-search-credentials-2020-10-02-15_08_45

6c352b8 - This adds the Credentials Page, but without the list of credentials.

screencapture-localhost-5601-app-enterprise-search-app-search-credentials-2020-10-06-11_35_18

9434cc8 - This commit adds the list of Credentials to the Credentials page, printing the credential without hiding it with stars

screencapture-localhost-5601-app-enterprise-search-app-search-credentials-2020-10-06-11_36_09

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@JasonStoltz JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 6, 2020
@JasonStoltz JasonStoltz requested a review from a team October 6, 2020 19:34
cee-chen
cee-chen reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

First pass on credentials.tsx - super excited, I see a lot of opportunity to clean up our DOM and markup here!

cee-chen
cee-chen reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Sorry, I keep hitting single comments instead of chunking my reviews haha. credentials.test.tsx looks great - have a cleanup suggestion that saves a few lines

cee-chen
cee-chen reviewed Oct 6, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Here's my pass on the util helpers - great stuff!

cee-chen
cee-chen reviewed Oct 7, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

OK, phew, here's the credentials_list.tsx pass! I had a lot of changes so apologies for that, let me know if you'd rather look at a full diff instead of a bunch of disparate comments.

I'm going to hold off on reviewing credentials.list.test.tsx for now since it's getting to the end of my work day and it probably makes sense to wait to review once all changes have been made. Hope that's OK!

JasonStoltz and others added 12 commits October 7, 2020 09:55
@JasonStoltz
Merge branch 'master' into JasonStoltz/credentials-view
09e7d93
@JasonStoltz
Implement feedback items:
c010b8a 
Unnecessary wrapper - we can lose it without it affecting the UI
elastic#79749 (comment)
Similar comment as above, this flex wrapper isn't needed
elastic#79749 (comment)
There's actually a semantic EUI element for this!
elastic#79749 (comment)
See above comment
elastic#79749 (comment)
See above comment
elastic#79749 (comment)
There's a CSS utility class for this!
elastic#79749 (comment)
Let's lose the generic div wrapper if we don't need it
elastic#79749 (comment)
See above - we also don't need the span wrapper
elastic#79749 (comment)
@JasonStoltz
Remove unused imports
e659382
@JasonStoltz
PR Feedback
2c9a99a 
We actually don't need to specify the I*Actions/I*Values
elastic#79749 (comment)
See above - awesome right?
elastic#79749 (comment)
@cee-chen @JasonStoltz
Add reusable shallow unmount helper
91c4ff8
@cee-chen @JasonStoltz
Simplify Kea test mocks
714da22
@JasonStoltz
Switch to ApiTokenTypes
4917cbb 
Not a change request, just me musing out loud
elastic#79749 (comment)
@JasonStoltz
Apply suggestions from code review
280b2f7 
Co-authored-by: Constance <[email protected]>
@JasonStoltz
Use ApiTokenTypes for IApiToken
30c52a7
@JasonStoltz
PR Feedback
343290e 
Having the 'real' text personally helps me a lot with visualizing
elastic#79749 (comment)
@JasonStoltz
We can lose these CSS classes...
9e6b0c4 
elastic#79749 (comment)
@JasonStoltz
I think we can rewrite this return flow
564737a 
elastic#79749 (comment)
@JasonStoltz JasonStoltz requested a review from cee-chen October 7, 2020 18:46
@JasonStoltz
Copy link
Member Author

JasonStoltz commented Oct 7, 2020

@constancecchen Thanks for the review. I think it's ready for a second look.

cee-chen
cee-chen reviewed Oct 8, 2020
View reviewed changes
cee-chen
cee-chen reviewed Oct 8, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

This is looking great! I think this is my final batch of comments!

...ic/applications/app_search/components/credentials/credentials_list/credentials_list.test.tsx Outdated
Comment on lines 182 to 189
const subject = (token: object): [any, any] => {
const copyMock = jest.fn();
const column = columns[2];
const wrapper = shallow(<div>{column.render(token)}</div>);
const children = wrapper.find(EuiCopy).props().children;
const copyEl = shallow(children(copyMock));
return [copyMock, copyEl];
};
Copy link
Contributor

@cee-chen cee-chen Oct 8, 2020

Choose a reason for hiding this comment

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

Ooo, this is super cool. Very nice!

Copy link
Member Author

@JasonStoltz JasonStoltz Oct 8, 2020

Choose a reason for hiding this comment

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

What is?

Copy link
Contributor

@cee-chen cee-chen Oct 8, 2020

Choose a reason for hiding this comment

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

The subject() helper - I like it a lot, we might want to DRY it out in the future if we have any more components that use EuiCopy (although I think credentials might be the only one, so nevermind haha).

@JasonStoltz JasonStoltz requested a review from cee-chen October 8, 2020 14:14
cee-chen
cee-chen approved these changes Oct 8, 2020
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Woohoo! I did a final quick QA pass (mobile/cross-browser/screen-reader/dark-mode testing) and this all looks great. Awesome work Jason!

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 8, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
enterpriseSearch 403 414 +11

async chunks size

id before after diff
enterpriseSearch 638.5KB 661.8KB +23.3KB

History

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

@JasonStoltz JasonStoltz merged commit 44b48a2 into elastic:master Oct 8, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 12, 2020
@gmmorris
Merge branch 'master' into task-manager/health
6dddf5c 
* master: (23 commits)
  Table visualization renderer (elastic#79455)
  Migrate Jest JUnit reporter to TS (elastic#79919)
  store sorted bundleRefExportIds (elastic#80011)
  update chromedriver dependency to 86.0.0 (elastic#79972)
  [Security Solution][Case] Fix bug when changing connectors (elastic#80002)
  [kbn/std] add observable helpers to aid with rxjs 7 upgrade (elastic#79752)
  [Security Solution][Resolver] Pill numbers in compact notation (elastic#80038)
  [Logs UI] Sync logs timerange with wider Kibana (elastic#79444)
  [DOCS] Adds quick start (elastic#78822)
  [Ingest Manager]Fix ingest manager UI renaming (elastic#80036)
  [Monitoring] Fixed internal monitoring check (elastic#79241)
  [Security Solution][Exception Modal] Removes list operators in exception modal for EQL rules (elastic#79871)
  Update development documentation about REST API best practices (elastic#80009)
  [Monitoring] Improve indices loading against larger metricbeat-* indices (elastic#79190)
  [CI] Move kibana build dir outside of repo for functional tests (elastic#80018)
  [kbn/optimizer] bump low or add missing limits when updating automatically (elastic#80013)
  [Enterprise Search] Added a Credentials page to App Search (elastic#79749)
  [DOCS] Canvas refresh for 7.10 (elastic#80017)
  [TSVB] Add ignore global filters to series options (elastic#79337)
  Remove this check (elastic#79202)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 12, 2020
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 12, 2020

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

4 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 13, 2020

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

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 14, 2020

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

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 15, 2020

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

@kibanamachine
Copy link
Contributor

kibanamachine commented Oct 16, 2020

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

@cee-chen
Copy link
Contributor

cee-chen commented Oct 16, 2020

@JasonStoltz would you mind running yarn backport on this when you have a chance? Thanks!

@JasonStoltz JasonStoltz mentioned this pull request Oct 19, 2020
Merged
JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Oct 19, 2020
@JasonStoltz
[Enterprise Search] Added a Credentials page to App Search (elastic#7…
4464dd6 
…9749)
JasonStoltz added a commit that referenced this pull request Oct 19, 2020
@JasonStoltz
[Enterprise Search] Added a Credentials page to App Search (#79749) (#…
f05c72a 
…80968)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 19, 2020
@JasonStoltz JasonStoltz deleted the JasonStoltz/credentials-view branch April 6, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cee-chen cee-chen cee-chen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.11.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JasonStoltz @kibanamachine @cee-chen