AzureMonitor: Move DefaultSubscriptions field to separate component (#61633)

* Move DefaultSubscriptions field to separate component

- Add separate component
- Update CredentialsForm
- Update tests
- Remove redundant test

* Remove defaultSubscriptionId

- Update MonitorConfig to appropriately set subscriptionId
- Update DefaultSubscription component to make use of JsonData
- Remove references to defaultSubscriptionId
- Update tests
- Update mockInstanceSettings for flexibility

* Review

- Add DefaultSubscription test and remove component from AzureCredentialsForm test
- Remove onCredentialsChange from DefaultSubscription
- Set subscription as a part of onSubscriptionChange

public/app/plugins/datasource/grafana-azure-monitor-datasource/__mocks__/instanceSettings.ts

@@ -1,8 +1,11 @@
-import { DataSourcePluginMeta } from '@grafana/data';
+import { DataSourceInstanceSettings, DataSourcePluginMeta } from '@grafana/data';
 
-import { AzureDataSourceInstanceSettings } from '../types';
+import { AzureDataSourceInstanceSettings, AzureDataSourceJsonData } from '../types';
 
-export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings => ({
+export const createMockInstanceSetttings = (
+  overrides?: Partial<DataSourceInstanceSettings>,
+  jsonDataOverrides?: Partial<AzureDataSourceJsonData>
+): AzureDataSourceInstanceSettings => ({
   url: '/ds/1',
   id: 1,
   uid: 'abc',
@@ -11,6 +14,7 @@ export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings =
   meta: {} as DataSourcePluginMeta,
   name: 'azure',
   readOnly: false,
+  ...overrides,
 
   jsonData: {
     cloudName: 'azuremonitor',
@@ -20,5 +24,6 @@ export const createMockInstanceSetttings = (): AzureDataSourceInstanceSettings =
     tenantId: 'abc-123',
     clientId: 'def-456',
     subscriptionId: 'ghi-789',
+    ...jsonDataOverrides,
   },
 });

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.test.tsx

@@ -1,8 +1,6 @@
 import { render, screen, waitFor } from '@testing-library/react';
 import React from 'react';
 
-import { selectors } from '../e2e/selectors';
-
 import AzureCredentialsForm, { Props } from './AzureCredentialsForm';
 
 const setup = (propsFunc?: (props: Props) => Props) => {
@@ -14,7 +12,6 @@ const setup = (propsFunc?: (props: Props) => Props) => {
       tenantId: 'e7f3f661-a933-3h3f-0294-31c4f962ec48',
       clientId: '34509fad-c0r9-45df-9e25-f1ee34af6900',
       clientSecret: undefined,
-      defaultSubscriptionId: '44987801-6nn6-49he-9b2d-9106972f9789',
     },
     azureCloudOptions: [
       { value: 'azuremonitor', label: 'Azure' },
@@ -22,7 +19,6 @@ const setup = (propsFunc?: (props: Props) => Props) => {
       { value: 'chinaazuremonitor', label: 'Azure China' },
     ],
     onCredentialsChange: jest.fn(),
-    getSubscriptions: jest.fn(() => Promise.resolve([])),
   };
 
   if (propsFunc) {
@@ -54,22 +50,6 @@ describe('Render', () => {
     expect(screen.getByTestId('client-secret')).toBeDisabled();
   });
 
-  it('should enable azure monitor load subscriptions button', async () => {
-    setup((props) => ({
-      ...props,
-      credentials: {
-        authType: 'clientsecret',
-        azureCloud: 'azuremonitor',
-        tenantId: 'e7f3f661-a933-3h3f-0294-31c4f962ec48',
-        clientId: '34509fad-c0r9-45df-9e25-f1ee34af6900',
-        clientSecret: 'e7f3f661-a933-4b3f-8176-51c4f982ec48',
-      },
-    }));
-    await waitFor(() =>
-      expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).not.toBeDisabled()
-    );
-  });
-
   describe('when disabled', () => {
     it('should disable inputs', async () => {
       setup((props) => ({
@@ -80,23 +60,5 @@ describe('Render', () => {
       await waitFor(() => screen.getByLabelText('Azure Cloud'));
       expect(screen.getByLabelText('Azure Cloud')).toBeDisabled();
     });
-
-    it('should remove buttons', async () => {
-      setup((props) => ({
-        ...props,
-        disabled: true,
-      }));
-      await waitFor(() =>
-        expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).toBeDisabled()
-      );
-    });
-
-    it('should render children components', () => {
-      setup((props) => ({
-        ...props,
-        children: <button>click me</button>,
-      }));
-      expect(screen.getByText('click me')).toBeInTheDocument();
-    });
   });
 });

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/AzureCredentialsForm.tsx

@@ -1,9 +1,8 @@
-import React, { ChangeEvent, FunctionComponent, useEffect, useReducer, useState } from 'react';
+import React, { ChangeEvent, FunctionComponent } from 'react';
 
 import { SelectableValue } from '@grafana/data';
 import { LegacyForms, Button, Select, InlineField } from '@grafana/ui';
 
-import { isCredentialsComplete } from '../credentials';
 import { selectors } from '../e2e/selectors';
 import { AzureAuthType, AzureCredentials } from '../types';
 
@@ -14,7 +13,6 @@ export interface Props {
   credentials: AzureCredentials;
   azureCloudOptions?: SelectableValue[];
   onCredentialsChange?: (updatedCredentials: AzureCredentials) => void;
-  getSubscriptions?: () => Promise<SelectableValue[]>;
   disabled?: boolean;
   children?: JSX.Element;
 }
@@ -33,52 +31,13 @@ const authTypeOptions: Array<SelectableValue<AzureAuthType>> = [
 const LABEL_WIDTH = 18;
 
 export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) => {
-  const { credentials, azureCloudOptions, onCredentialsChange, getSubscriptions, disabled } = props;
+  const { credentials, azureCloudOptions, onCredentialsChange, disabled, managedIdentityEnabled } = props;
-  const hasRequiredFields = isCredentialsComplete(credentials);
-
-  const [subscriptions, setSubscriptions] = useState<Array<SelectableValue<string>>>([]);
-  const [loadSubscriptionsClicked, onLoadSubscriptions] = useReducer((val) => val + 1, 0);
-  useEffect(() => {
-    if (!getSubscriptions || !hasRequiredFields) {
-      updateSubscriptions([]);
-      return;
-    }
-    let canceled = false;
-    getSubscriptions().then((result) => {
-      if (!canceled) {
-        updateSubscriptions(result, loadSubscriptionsClicked);
-      }
-    });
-    return () => {
-      canceled = true;
-    };
-    // This effect is intended to be called only once initially and on Load Subscriptions click
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [loadSubscriptionsClicked]);
-
-  const updateSubscriptions = (received: Array<SelectableValue<string>>, autoSelect = false) => {
-    setSubscriptions(received);
-    if (getSubscriptions) {
-      if (autoSelect && !credentials.defaultSubscriptionId && received.length > 0) {
-        // Selecting the default subscription if subscriptions received but no default subscription selected
-        onSubscriptionChange(received[0]);
-      } else if (credentials.defaultSubscriptionId) {
-        const found = received.find((opt) => opt.value === credentials.defaultSubscriptionId);
-        if (!found) {
-          // Unselecting the default subscription if it isn't found among the received subscriptions
-          onSubscriptionChange(undefined);
-        }
-      }
-    }
-  };
 
   const onAuthTypeChange = (selected: SelectableValue<AzureAuthType>) => {
     if (onCredentialsChange) {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         authType: selected.value || 'msi',
-        defaultSubscriptionId: undefined,
       };
       onCredentialsChange(updated);
     }
@@ -86,11 +45,9 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   const onAzureCloudChange = (selected: SelectableValue<string>) => {
     if (onCredentialsChange && credentials.authType === 'clientsecret') {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         azureCloud: selected.value,
-        defaultSubscriptionId: undefined,
       };
       onCredentialsChange(updated);
     }
@@ -98,11 +55,9 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   const onTenantIdChange = (event: ChangeEvent<HTMLInputElement>) => {
     if (onCredentialsChange && credentials.authType === 'clientsecret') {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         tenantId: event.target.value,
-        defaultSubscriptionId: undefined,
       };
       onCredentialsChange(updated);
     }
@@ -110,11 +65,9 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   const onClientIdChange = (event: ChangeEvent<HTMLInputElement>) => {
     if (onCredentialsChange && credentials.authType === 'clientsecret') {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         clientId: event.target.value,
-        defaultSubscriptionId: undefined,
       };
       onCredentialsChange(updated);
     }
@@ -122,11 +75,9 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   const onClientSecretChange = (event: ChangeEvent<HTMLInputElement>) => {
     if (onCredentialsChange && credentials.authType === 'clientsecret') {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         clientSecret: event.target.value,
-        defaultSubscriptionId: undefined,
       };
       onCredentialsChange(updated);
     }
@@ -134,21 +85,9 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   const onClientSecretReset = () => {
     if (onCredentialsChange && credentials.authType === 'clientsecret') {
-      setSubscriptions([]);
       const updated: AzureCredentials = {
         ...credentials,
         clientSecret: '',
-        defaultSubscriptionId: undefined,
-      };
-      onCredentialsChange(updated);
-    }
-  };
-
-  const onSubscriptionChange = (selected: SelectableValue<string> | undefined) => {
-    if (onCredentialsChange) {
-      const updated: AzureCredentials = {
-        ...credentials,
-        defaultSubscriptionId: selected?.value,
       };
       onCredentialsChange(updated);
     }
@@ -156,7 +95,7 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
 
   return (
     <div className="gf-form-group">
-      {props.managedIdentityEnabled && (
+      {managedIdentityEnabled && (
         <InlineField
           label="Authentication"
           labelWidth={LABEL_WIDTH}
@@ -263,38 +202,6 @@ export const AzureCredentialsForm: FunctionComponent<Props> = (props: Props) =>
             ))}
         </>
       )}
-      {getSubscriptions && (
-        <InlineField
-          label="Default Subscription"
-          labelWidth={LABEL_WIDTH}
-          data-testid={selectors.components.configEditor.defaultSubscription.input}
-          htmlFor="default-subscription"
-        >
-          <div className="width-30" style={{ display: 'flex', gap: '4px' }}>
-            <Select
-              inputId="default-subscription"
-              aria-label="Default Subscription"
-              value={
-                credentials.defaultSubscriptionId
-                  ? subscriptions.find((opt) => opt.value === credentials.defaultSubscriptionId)
-                  : undefined
-              }
-              options={subscriptions}
-              onChange={onSubscriptionChange}
-              disabled={disabled}
-            />
-            <Button
-              variant="secondary"
-              type="button"
-              onClick={onLoadSubscriptions}
-              disabled={!hasRequiredFields || disabled}
-              data-testid={selectors.components.configEditor.loadSubscriptions.button}
-            >
-              Load Subscriptions
-            </Button>
-          </div>
-        </InlineField>
-      )}
       {props.children}
     </div>
   );

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/DefaultSubscription.test.tsx

@@ -0,0 +1,58 @@
+import { render, screen } from '@testing-library/react';
+import React from 'react';
+
+import { createMockInstanceSetttings } from '../__mocks__/instanceSettings';
+import { selectors } from '../e2e/selectors';
+
+import { DefaultSubscription, Props } from './DefaultSubscription';
+
+const mockInstanceSettings = createMockInstanceSetttings();
+
+const defaultProps: Props = {
+  options: mockInstanceSettings.jsonData,
+  credentials: {
+    authType: 'clientsecret',
+    azureCloud: 'azuremonitor',
+    tenantId: 'e7f3f661-a933-3h3f-0294-31c4f962ec48',
+    clientId: '34509fad-c0r9-45df-9e25-f1ee34af6900',
+    clientSecret: undefined,
+  },
+  subscriptions: [],
+  getSubscriptions: jest.fn().mockResolvedValue([{ label: 'subscriptionId', value: 'subscriptionId' }]),
+  onSubscriptionsChange: jest.fn(),
+  onSubscriptionChange: jest.fn(),
+};
+
+describe('DefaultSubscription', () => {
+  it('should render component', () => {
+    render(<DefaultSubscription {...defaultProps} />);
+
+    expect(screen.getByText('Default Subscription')).toBeInTheDocument();
+  });
+
+  it('should disable load subscriptions if credentials are incomplete', () => {
+    render(<DefaultSubscription {...{ ...defaultProps, credentials: { authType: 'clientsecret' } }} />);
+
+    expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).toBeDisabled();
+  });
+
+  it('should enable load subscriptions if credentials are complete and set default subscription', () => {
+    const props = {
+      ...defaultProps,
+      credentials: { ...defaultProps.credentials, clientSecret: 'client_secret' },
+      options: { ...defaultProps.options, subscriptionId: undefined },
+    };
+    const { rerender } = render(<DefaultSubscription {...props} />);
+
+    expect(screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button)).not.toBeDisabled();
+    screen.getByTestId(selectors.components.configEditor.loadSubscriptions.button).click();
+    rerender(
+      <DefaultSubscription
+        {...props}
+        subscriptions={[{ label: 'subscriptionId', value: 'subscriptionId' }]}
+        options={{ ...defaultProps.options, subscriptionId: 'subscriptionId' }}
+      />
+    );
+    expect(document.body).toHaveTextContent('subscriptionId');
+  });
+});

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/DefaultSubscription.tsx

@@ -0,0 +1,103 @@
+import React, { FunctionComponent, useEffect, useReducer } from 'react';
+
+import { SelectableValue } from '@grafana/data';
+import { InlineField, Select, Button } from '@grafana/ui';
+
+import { isCredentialsComplete } from '../credentials';
+import { selectors } from '../e2e/selectors';
+import { AzureCredentials, AzureDataSourceJsonData } from '../types';
+
+const LABEL_WIDTH = 18;
+
+export interface Props {
+  options: AzureDataSourceJsonData;
+  credentials: AzureCredentials;
+  getSubscriptions?: () => Promise<SelectableValue[]>;
+  subscriptions: Array<SelectableValue<string>>;
+  onSubscriptionsChange: (receivedSubscriptions: Array<SelectableValue<string>>) => void;
+  onSubscriptionChange: (subscriptionId?: string) => void;
+  disabled?: boolean;
+}
+
+export const DefaultSubscription: FunctionComponent<Props> = (props: Props) => {
+  const {
+    credentials,
+    disabled,
+    options,
+    subscriptions,
+    getSubscriptions,
+    onSubscriptionChange,
+    onSubscriptionsChange,
+  } = props;
+  const hasRequiredFields = isCredentialsComplete(credentials);
+  const [loadSubscriptionsClicked, onLoadSubscriptions] = useReducer((val) => val + 1, 0);
+
+  useEffect(() => {
+    if (!getSubscriptions || !hasRequiredFields) {
+      updateSubscriptions([]);
+      return;
+    }
+    let canceled = false;
+    getSubscriptions().then((result) => {
+      if (!canceled) {
+        updateSubscriptions(result, loadSubscriptionsClicked);
+      }
+    });
+    return () => {
+      canceled = true;
+    };
+    // This effect is intended to be called only once initially and on Load Subscriptions click
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [loadSubscriptionsClicked]);
+
+  const updateSubscriptions = (received: Array<SelectableValue<string>>, autoSelect = false) => {
+    onSubscriptionsChange(received);
+    if (getSubscriptions) {
+      if (autoSelect && !options.subscriptionId && received.length > 0) {
+        // Selecting the default subscription if subscriptions received but no default subscription selected
+        onChange(received[0]);
+      } else if (options.subscriptionId) {
+        const found = received.find((opt) => opt.value === options.subscriptionId);
+        if (!found) {
+          // Unselecting the default subscription if it isn't found among the received subscriptions
+          onChange(undefined);
+        }
+      }
+    }
+  };
+
+  const onChange = (selected: SelectableValue<string> | undefined) => onSubscriptionChange(selected?.value);
+
+  return (
+    <>
+      <InlineField
+        label="Default Subscription"
+        labelWidth={LABEL_WIDTH}
+        data-testid={selectors.components.configEditor.defaultSubscription.input}
+        htmlFor="default-subscription"
+      >
+        <div className="width-30" style={{ display: 'flex', gap: '4px' }}>
+          <Select
+            inputId="default-subscription"
+            aria-label="Default Subscription"
+            value={
+              options.subscriptionId ? subscriptions.find((opt) => opt.value === options.subscriptionId) : undefined
+            }
+            options={subscriptions}
+            onChange={onChange}
+            disabled={disabled}
+          />
+          <Button
+            variant="secondary"
+            type="button"
+            onClick={onLoadSubscriptions}
+            disabled={!hasRequiredFields || disabled}
+            data-testid={selectors.components.configEditor.loadSubscriptions.button}
+          >
+            Load Subscriptions
+          </Button>
+        </div>
+      </InlineField>
+    </>
+  );
+};

public/app/plugins/datasource/grafana-azure-monitor-datasource/components/MonitorConfig.tsx

@@ -1,4 +1,4 @@
-import React, { FunctionComponent, useMemo } from 'react';
+import React, { FunctionComponent, useMemo, useState } from 'react';
 
 import { SelectableValue } from '@grafana/data';
 import { config } from '@grafana/runtime';
@@ -7,6 +7,7 @@ import { getCredentials, updateCredentials } from '../credentials';
 import { AzureDataSourceSettings, AzureCredentials } from '../types';
 
 import { AzureCredentialsForm } from './AzureCredentialsForm';
+import { DefaultSubscription } from './DefaultSubscription';
 
 const azureClouds = [
   { value: 'azuremonitor', label: 'Azure' },
@@ -21,13 +22,25 @@ export interface Props {
 }
 
 export const MonitorConfig: FunctionComponent<Props> = (props: Props) => {
-  const { updateOptions, getSubscriptions } = props;
+  const { updateOptions, getSubscriptions, options } = props;
+  const [subscriptions, setSubscriptions] = useState<Array<SelectableValue<string>>>([]);
   const credentials = useMemo(() => getCredentials(props.options), [props.options]);
 
-  const onCredentialsChange = (credentials: AzureCredentials): void => {
+  const onCredentialsChange = (credentials: AzureCredentials, subscriptionId?: string): void => {
-    updateOptions((options) => updateCredentials(options, credentials));
+    if (!subscriptionId) {
+      setSubscriptions([]);
+    }
+    updateOptions((options) =>
+      updateCredentials({ ...options, jsonData: { ...options.jsonData, subscriptionId } }, credentials)
+    );
   };
 
+  const onSubscriptionsChange = (receivedSubscriptions: Array<SelectableValue<string>>) =>
+    setSubscriptions(receivedSubscriptions);
+
+  const onSubscriptionChange = (subscriptionId?: string) =>
+    updateOptions((options) => ({ ...options, jsonData: { ...options.jsonData, subscriptionId } }));
+
   return (
     <>
       <h3 className="page-heading">Authentication</h3>
@@ -36,9 +49,18 @@ export const MonitorConfig: FunctionComponent<Props> = (props: Props) => {
         credentials={credentials}
         azureCloudOptions={azureClouds}
         onCredentialsChange={onCredentialsChange}
-        getSubscriptions={getSubscriptions}
         disabled={props.options.readOnly}
-      />
+      >
+        <DefaultSubscription
+          subscriptions={subscriptions}
+          credentials={credentials}
+          getSubscriptions={getSubscriptions}
+          disabled={props.options.readOnly}
+          onSubscriptionsChange={onSubscriptionsChange}
+          onSubscriptionChange={onSubscriptionChange}
+          options={options.jsonData}
+        />
+      </AzureCredentialsForm>
     </>
   );
 };

public/app/plugins/datasource/grafana-azure-monitor-datasource/credentials.ts

@@ -92,7 +92,6 @@ export function getCredentials(options: AzureDataSourceSettings): AzureCredentia
       if (config.azure.managedIdentityEnabled) {
         return {
           authType: 'msi',
-          defaultSubscriptionId: options.jsonData.subscriptionId,
         };
       } else {
         // If authentication type is managed identity but managed identities were disabled in Grafana config,
@@ -109,7 +108,6 @@ export function getCredentials(options: AzureDataSourceSettings): AzureCredentia
         tenantId: options.jsonData.tenantId,
         clientId: options.jsonData.clientId,
         clientSecret: getSecret(options),
-        defaultSubscriptionId: options.jsonData.subscriptionId,
       };
   }
 }
@@ -129,7 +127,6 @@ export function updateCredentials(
         jsonData: {
           ...options.jsonData,
           azureAuthType: 'msi',
-          subscriptionId: credentials.defaultSubscriptionId,
         },
       };
 
@@ -144,7 +141,6 @@ export function updateCredentials(
           cloudName: credentials.azureCloud || getDefaultAzureCloud(),
           tenantId: credentials.tenantId,
           clientId: credentials.clientId,
-          subscriptionId: credentials.defaultSubscriptionId,
         },
         secureJsonData: {
           ...options.secureJsonData,

public/app/plugins/datasource/grafana-azure-monitor-datasource/types/types.ts

@@ -35,7 +35,6 @@ export type ConcealedSecret = symbol;
 
 interface AzureCredentialsBase {
   authType: AzureAuthType;
-  defaultSubscriptionId?: string;
 }
 
 export interface AzureManagedIdentityCredentials extends AzureCredentialsBase {