Small Refactor: Do not fetch account id if not on service map page (#169

)
grafana · Jan 23, 2023 · 499c093 · 499c093
1 parent 5addb24
commit 499c093
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 29 deletions.
diff --git a/src/components/QueryEditor/AccountIdDropdown.tsx b/src/components/QueryEditor/AccountIdDropdown.tsx
@@ -0,0 +1,41 @@
+import { XrayDataSource } from 'DataSource';
+import { useAccountIds } from './useAccountIds';
+import { XrayQuery } from '../../types';
+import { TimeRange } from '@grafana/data';
+import { config } from '@grafana/runtime';
+import React from 'react';
+import { InlineFormLabel, MultiSelect } from '@grafana/ui';
+
+type Props = {
+  datasource: XrayDataSource;
+  query: XrayQuery;
+  range?: TimeRange;
+  onChange: (items: string[]) => void;
+};
+
+export const AccountIdDropdown = (props: Props) => {
+  const accountIds = useAccountIds(props.datasource, props.query, props.range);
+  const hasStoredAccountIdFilter = !!(props.query.accountIds && props.query.accountIds.length);
+  const showAccountIdDropdown = config.featureToggles.cloudWatchCrossAccountQuerying || hasStoredAccountIdFilter;
+
+  if (!showAccountIdDropdown) {
+    return null;
+  }
+
+  return (
+    <div className="gf-form">
+      <InlineFormLabel className="query-keyword" width="auto">
+        AccountId
+      </InlineFormLabel>
+      <MultiSelect
+        options={(accountIds || []).map((accountId: string) => ({
+          value: accountId,
+          label: accountId,
+        }))}
+        value={props.query.accountIds}
+        onChange={(items) => props.onChange(items.map((item) => item.value || ''))}
+        placeholder={'All'}
+      />
+    </div>
+  );
+};
diff --git a/src/components/QueryEditor/QueryEditor.test.tsx b/src/components/QueryEditor/QueryEditor.test.tsx
@@ -1,5 +1,5 @@
 import React from 'react';
-import { render, screen, fireEvent, act, waitFor } from '@testing-library/react';
+import { render, screen, fireEvent, act, waitFor, waitForElementToBeRemoved } from '@testing-library/react';
 import { QueryEditor } from './QueryEditor';
 import { Group, Region, XrayJsonData, XrayQuery, XrayQueryType } from '../../types';
 import { XrayDataSource } from '../../DataSource';
@@ -158,7 +158,7 @@ describe('QueryEditor', () => {
 
     fireEvent.change(field, { target: { value: '1-5f160a8b-83190adad07f429219c0e259' } });
 
-    expect(onChange.mock.calls[3][0]).toEqual({
+    expect(onChange.mock.calls[1][0]).toEqual({
       refId: 'A',
       query: '1-5f160a8b-83190adad07f429219c0e259',
       queryType: XrayQueryType.getTrace,
@@ -221,11 +221,16 @@ describe('QueryEditor', () => {
   });
 
   it('shows the accountIds in a dropdown on service map selection', async () => {
+    const mockGetAccountIdsForServiceMap = jest.fn(() => Promise.resolve(['account1', 'account2']));
     await act(async () => {
       render(
         <QueryEditor
           {...{
             ...defaultProps,
+            datasource: {
+              ...defaultProps.datasource,
+              getAccountIdsForServiceMap: mockGetAccountIdsForServiceMap,
+            },
             query: {
               refId: 'A',
               queryType: 'getServiceMap',
@@ -237,6 +242,33 @@ describe('QueryEditor', () => {
       );
       expect(screen.getByText('', { selector: '.fa-spinner' })).toBeDefined();
       await waitFor(() => expect(screen.getByText('account1')).toBeDefined());
+      expect(mockGetAccountIdsForServiceMap).toHaveBeenCalled();
+    });
+  });
+
+  it('does not fetch account ids if service map is not selected', async () => {
+    const mockGetAccountIdsForServiceMap = jest.fn(() => Promise.resolve(['account1', 'account2']));
+    await act(async () => {
+      render(
+        <QueryEditor
+          {...{
+            ...defaultProps,
+            datasource: {
+              ...defaultProps.datasource,
+              getAccountIdsForServiceMap: mockGetAccountIdsForServiceMap,
+            },
+            query: {
+              refId: 'A',
+              queryType: XrayQueryType.getTrace,
+              accountIds: [],
+            } as any,
+          }}
+          onChange={() => {}}
+        />
+      );
+      expect(screen.getByText('', { selector: '.fa-spinner' })).toBeDefined();
+      await waitForElementToBeRemoved(() => screen.getByText('', { selector: '.fa-spinner' }));
+      expect(mockGetAccountIdsForServiceMap).not.toHaveBeenCalled();
     });
   });
 });

diff --git a/src/components/QueryEditor/QueryEditorForm.tsx b/src/components/QueryEditor/QueryEditorForm.tsx
@@ -17,8 +17,8 @@ import {
 import { XrayDataSource } from '../../DataSource';
 import { QuerySection } from './QuerySection';
 import { XrayLinks } from './XrayLinks';
-import { getTemplateSrv, config } from '@grafana/runtime';
-import { useAccountIds } from './useAccountIds';
+import { getTemplateSrv } from '@grafana/runtime';
+import { AccountIdDropdown } from './AccountIdDropdown';
 
 function findOptionForQueryType(queryType: XrayQueryType, options: any = queryTypeOptions): QueryTypeOption[] {
   for (const option of options) {
@@ -103,13 +103,8 @@ export function QueryEditorForm({
   useInitQuery(query, onChange, groups, allRegions, datasource);
 
   const selectedOptions = queryTypeToQueryTypeOptions(query.queryType);
-  const accountIds = useAccountIds(datasource, query, range);
   const allGroups = selectedOptions[0] === insightsOption ? [dummyAllGroup, ...groups] : groups;
   const styles = getStyles();
-  const hasStoredAccountIdFilter = !!(query.accountIds && query.accountIds.length);
-  const showAccountIdDropdown =
-    [serviceMapOption].includes(selectedOptions[0]) &&
-    (config.featureToggles.cloudWatchCrossAccountQuerying || hasStoredAccountIdFilter);
 
   return (
     <div>
@@ -184,26 +179,18 @@ export function QueryEditorForm({
           />
         </div>
 
-        {showAccountIdDropdown && (
-          <div className="gf-form">
-            <InlineFormLabel className="query-keyword" width="auto">
-              AccountId
-            </InlineFormLabel>
-            <MultiSelect
-              options={(accountIds || []).map((accountId: string) => ({
-                value: accountId,
-                label: accountId,
-              }))}
-              value={query.accountIds}
-              onChange={(items) => {
-                onChange({
-                  ...query,
-                  accountIds: items.map((item) => item.value),
-                } as any);
-              }}
-              placeholder={'All'}
-            />
-          </div>
+        {[serviceMapOption].includes(selectedOptions[0]) && (
+          <AccountIdDropdown
+            datasource={datasource}
+            query={query}
+            range={range}
+            onChange={(accountIds) =>
+              onChange({
+                ...query,
+                accountIds,
+              })
+            }
+          />
         )}
 
         <div className="gf-form">