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

[App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling #107603

Merged

Conversation

byronhulcher
Copy link
Contributor

@byronhulcher byronhulcher commented Aug 3, 2021

Summary

Migrates the CrawlerStatusIndicator and CrawlerStatusBanner components from our standalone UX. To support this, I've also migrates our Crawl Request polling.

Checklist

Delete any items that are not applicable to this PR.

@byronhulcher byronhulcher force-pushed the migrate-crawl-status-indicator branch 2 times, most recently from 3882342 to f8ab0fc Compare August 3, 2021 21:29
@byronhulcher byronhulcher force-pushed the migrate-crawl-status-indicator branch from aaadbc7 to b4668d5 Compare August 4, 2021 15:37
@byronhulcher byronhulcher changed the title [App Search] Migrate Crawl status indicator [App Search] Migrate Crawl Status Indicator, Crawl Status Banner, and Crawl Request polling Aug 4, 2021
@byronhulcher byronhulcher changed the title [App Search] Migrate Crawl Status Indicator, Crawl Status Banner, and Crawl Request polling [App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling Aug 4, 2021
].includes(crawlRequests[0]?.status)
) {
actions.createNewTimeoutForCrawlRequests(POLLING_DURATION);
} else if (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest is telling me this line is not covered........ I can't figure out why since lines 206-208 are covered!!!

Comment on lines +71 to +72
closePopover();
stopCrawl();
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 can't figure out how to write a test that will hit this codepath. See my comment at https://github.com/elastic/kibana/pull/107603/files#diff-d1ef29a662c4e757d0513752525db5973ddeb30c2bd74b8b51e1af3125eac914R35

@byronhulcher byronhulcher marked this pull request as ready for review August 4, 2021 15:43
@byronhulcher byronhulcher requested a review from a team as a code owner August 4, 2021 15:43
@byronhulcher byronhulcher added auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.15.0 labels Aug 4, 2021
Copy link
Contributor

@orhantoy orhantoy left a comment

Choose a reason for hiding this comment

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

💪 Nice work!

Comment on lines +41 to +44
setMockValues({
...MOCK_VALUES,
mostRecentCrawlRequestStatus: null,
});
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 seeing this

setMockValues({
  ...MOCK_VALUES,
  key: value
});

repeated a lot. Would it make sense to define a helper function like

function setMockValuesWithDefaults(values) { /* not loving the function name but you get the idea :) */
  setMockValues({
    ...MOCK_VALUES,
    ...values
  });
}

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, wrapping a single function call in another single function call doesn't feel any DRYer to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not repeating ...MOCK_VALUES every time is kind of DRY'er but I'm happy either way 😄

Comment on lines 27 to 53
it('can be opened to stop crawls', () => {
const wrapper = shallow(<StopCrawlPopoverContextMenu stopCrawl={stopCrawl} />);

wrapper.dive().find(EuiButton).simulate('click');
rerender(wrapper);

expect(wrapper.prop('isOpen')).toEqual(true);

// TODO I can't figure out how to find the EuiContextMenuItem inside this component's
// EuiContextMenuPanel. It renders inside of an EuiResizeObserver and I figure out how
// to get in it

// const menuItem = wrapper
// .find(EuiContextMenuPanel)
// .find(EuiResizeObserver)
// .find(EuiContextMenuItem);

// expect(menuItem).toHaveLength(1);

// menuItem.simulate('click');

// expect(stopCrawl).toHaveBeenCalled();

// rerender(wrapper);

// expect(wrapper.dive().find(EuiContextMenuPanel)).toHaveLength(0);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to work:

it('can be opened to stop crawls', () => {
  const wrapper = mount(<StopCrawlPopoverContextMenu stopCrawl={stopCrawl} />);

  wrapper.find(EuiButton).simulate('click');

  expect(wrapper.find(EuiPopover).prop('isOpen')).toEqual(true);

  const menuItem = wrapper
    .find(EuiContextMenuPanel)
    .find(EuiResizeObserver)
    .find(EuiContextMenuItem);

  expect(menuItem).toHaveLength(1);

  menuItem.simulate('click');

  expect(stopCrawl).toHaveBeenCalled();
});

But not sure if we are OK with not using shallow in that test.

Diff
diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/crawler/components/crawler_status_indicator/stop_crawl_popover_context_menu.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/crawler/components/crawler_status_indicator/stop_crawl_popover_context_menu.test.tsx
index 654aa9226cd..a2a30f03047 100644
--- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/crawler/components/crawler_status_indicator/stop_crawl_popover_context_menu.test.tsx
+++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/crawler/components/crawler_status_indicator/stop_crawl_popover_context_menu.test.tsx
@@ -6,11 +6,15 @@
  */
 import React from 'react';
 
-import { shallow } from 'enzyme';
+import { shallow, mount } from 'enzyme';
 
-import { EuiButton, EuiPopover } from '@elastic/eui';
-
-import { rerender } from '../../../../../test_helpers';
+import {
+  EuiButton,
+  EuiContextMenuItem,
+  EuiContextMenuPanel,
+  EuiPopover,
+  EuiResizeObserver,
+} from '@elastic/eui';
 
 import { StopCrawlPopoverContextMenu } from './stop_crawl_popover_context_menu';
 
@@ -25,30 +29,21 @@ describe('StopCrawlsPopoverContextMenu', () => {
   });
 
   it('can be opened to stop crawls', () => {
-    const wrapper = shallow(<StopCrawlPopoverContextMenu stopCrawl={stopCrawl} />);
-
-    wrapper.dive().find(EuiButton).simulate('click');
-    rerender(wrapper);
-
-    expect(wrapper.prop('isOpen')).toEqual(true);
-
-    // TODO I can't figure out how to find the EuiContextMenuItem inside this component's
-    // EuiContextMenuPanel.  It renders inside of an EuiResizeObserver and I figure out how
-    // to get in it
+    const wrapper = mount(<StopCrawlPopoverContextMenu stopCrawl={stopCrawl} />);
 
-    // const menuItem = wrapper
-    //   .find(EuiContextMenuPanel)
-    //   .find(EuiResizeObserver)
-    //   .find(EuiContextMenuItem);
+    wrapper.find(EuiButton).simulate('click');
 
-    // expect(menuItem).toHaveLength(1);
+    expect(wrapper.find(EuiPopover).prop('isOpen')).toEqual(true);
 
-    // menuItem.simulate('click');
+    const menuItem = wrapper
+      .find(EuiContextMenuPanel)
+      .find(EuiResizeObserver)
+      .find(EuiContextMenuItem);
 
-    // expect(stopCrawl).toHaveBeenCalled();
+    expect(menuItem).toHaveLength(1);
 
-    // rerender(wrapper);
+    menuItem.simulate('click');
 
-    // expect(wrapper.dive().find(EuiContextMenuPanel)).toHaveLength(0);
+    expect(stopCrawl).toHaveBeenCalled();
   });
 });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh let me try with mountWithIntl which is a customized mount which handles our i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 thanks @orhantoy

Copy link
Contributor

@orhantoy orhantoy left a comment

Choose a reason for hiding this comment

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

🎸

@byronhulcher byronhulcher enabled auto-merge (squash) August 5, 2021 17:13
@byronhulcher byronhulcher enabled auto-merge (squash) August 5, 2021 17:17
@byronhulcher byronhulcher enabled auto-merge (squash) August 5, 2021 17:17
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1462 1465 +3

Async chunks

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

id before after diff
enterpriseSearch 2.1MB 2.1MB +7.4KB

History

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

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 5, 2021
… and Crawl Request polling (#107603) (#107810)

Co-authored-by: Byron Hulcher <byronhulcher@gmail.com>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (392 commits)
  update linting doc (elastic#105748)
  [APM] Various improvements from elastic#104851 (elastic#107726)
  Update dependency @elastic/charts to v33.2.0 (master) (elastic#107842)
  Fix default route link on kibana homepage (elastic#107809)
  [APM] Invalidate trackPageview on route change (elastic#107741)
  Service map backend links (elastic#107317)
  [index patterns] index pattern create modal (elastic#101853)
  [RAC] integrating rbac search strategy with alert table (elastic#107242)
  [Security Solution] Siem signals -> alerts as data field and index aliases (elastic#106049)
  [Metrics UI] Add checkbox to optionally drop partial buckets (elastic#107676)
  [Metrics UI] Fix metric threshold preview regression (elastic#107674)
  Disable Product check in @elastic/elasticsearch-js (elastic#107642)
  [App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling (elastic#107603)
  [Security Solution, Lists] Replace legacy imports from 'elasticsearch' package (elastic#107226)
  [maps] asset tracking tutorial (elastic#104552)
  [scripts/build_ts_refs] when using `--clean` initialize caches (elastic#107777)
  Upgrade EUI to v36.1.0 (elastic#107231)
  [RAC] [TGrid] Implements cell actions in the TGrid (elastic#107771)
  Realign cypress/ccs_integration with cypress/integration (elastic#107743)
  Allow optional OSS to X-Pack dependencies (elastic#107432)
  ...

# Conflicts:
#	x-pack/examples/reporting_example/public/application.tsx
#	x-pack/examples/reporting_example/public/components/app.tsx
#	x-pack/plugins/canvas/public/services/legacy/stubs/reporting.ts
#	x-pack/plugins/reporting/common/types.ts
#	x-pack/plugins/reporting/public/lib/reporting_api_client/context.tsx
#	x-pack/plugins/reporting/public/management/mount_management_section.tsx
#	x-pack/plugins/reporting/public/management/report_listing.test.tsx
#	x-pack/plugins/reporting/public/plugin.ts
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
#	x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants