Dashboard: Migration - Edit Variable Settings: Implement "Add new" and Remove "Discard Changes" (#81660)

* enable "add" new variables and implement logic

* Remove discard changes code as we will not follow that pattern for now

* Add unit test of the onAdd function

* add unit test for utils functions

public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx

@@ -1,6 +1,18 @@
+import { of } from 'rxjs';
+
+import {
+  FieldType,
+  LoadingState,
+  PanelData,
+  VariableSupportType,
+  getDefaultTimeRange,
+  toDataFrame,
+} from '@grafana/data';
 import { getPanelPlugin } from '@grafana/data/test/__mocks__/pluginMocks';
-import { setPluginImportUtils } from '@grafana/runtime';
+import { setPluginImportUtils, setRunRequest } from '@grafana/runtime';
 import { SceneVariableSet, CustomVariable, SceneGridItem, SceneGridLayout, VizPanel } from '@grafana/scenes';
+import { mockDataSource } from 'app/features/alerting/unified/mocks';
+import { LegacyVariableQueryEditor } from 'app/features/variables/editor/LegacyVariableQueryEditor';
 
 import { DashboardScene } from '../scene/DashboardScene';
 import { activateFullSceneTree } from '../utils/test-utils';
@@ -12,6 +24,46 @@ setPluginImportUtils({
   getPanelPluginFromCache: (id: string) => undefined,
 });
 
+const defaultDatasource = mockDataSource({
+  name: 'Default Test Data Source',
+  type: 'test',
+});
+
+const promDatasource = mockDataSource({
+  name: 'Prometheus',
+  type: 'prometheus',
+});
+
+jest.mock('@grafana/runtime/src/services/dataSourceSrv', () => ({
+  ...jest.requireActual('@grafana/runtime/src/services/dataSourceSrv'),
+  getDataSourceSrv: () => ({
+    get: async () => ({
+      ...defaultDatasource,
+      variables: {
+        getType: () => VariableSupportType.Custom,
+        query: jest.fn(),
+        editor: jest.fn().mockImplementation(LegacyVariableQueryEditor),
+      },
+    }),
+    getList: () => [defaultDatasource, promDatasource],
+    getInstanceSettings: () => ({ ...defaultDatasource }),
+  }),
+}));
+
+const runRequestMock = jest.fn().mockReturnValue(
+  of<PanelData>({
+    state: LoadingState.Done,
+    series: [
+      toDataFrame({
+        fields: [{ name: 'text', type: FieldType.string, values: ['val1', 'val2', 'val11'] }],
+      }),
+    ],
+    timeRange: getDefaultTimeRange(),
+  })
+);
+
+setRunRequest(runRequestMock);
+
 describe('VariablesEditView', () => {
   describe('Dashboard Variables state', () => {
     let dashboard: DashboardScene;
@@ -130,40 +182,15 @@ describe('VariablesEditView', () => {
       expect(variableView.state.editIndex).toBeUndefined();
     });
 
-    it('should reset editing variable when discarding changes', () => {
-      variableView.onEdit('customVar2');
-      const editIndex = variableView.state.editIndex!;
-      const variable = variableView.getVariables()[editIndex];
-      const originalState = { ...variable.state };
-
-      variable.setState({ name: 'newName' });
-      variableView.onDiscardChanges();
-
-      const newVariable = variableView.getVariables()[editIndex];
-      expect(newVariable.state).toEqual(originalState);
+    it('should add default new query variable when onAdd is called', () => {
+      variableView.onAdd();
+      expect(variableView.getVariables()).toHaveLength(3);
+      expect(variableView.getVariables()[2].state.name).toBe('query0');
+      expect(variableView.getVariables()[2].state.type).toBe('query');
     });
 
-    it('should reset editing variable when discarding changes after the type being changed', () => {
-      variableView.onEdit('customVar2');
-      const editIndex = variableView.state.editIndex!;
-      const variable = variableView.getVariables()[editIndex];
-      const originalState = { ...variable.state };
-
-      variableView.onTypeChange('constant');
-      variableView.onDiscardChanges();
-
-      const newVariable = variableView.getVariables()[editIndex];
-      expect(newVariable.state).toEqual(originalState);
-    });
-
-    it('should go back when discarding changes', () => {
-      variableView.onEdit('customVar2');
-      const editIndex = variableView.state.editIndex!;
-      expect(editIndex).toBeDefined();
-
-      variableView.onDiscardChanges();
-
-      expect(variableView.state.editIndex).toBeUndefined();
+    afterEach(() => {
+      jest.clearAllMocks();
     });
   });
 

public/app/features/dashboard-scene/settings/VariablesEditView.tsx

@@ -5,7 +5,6 @@ import {
   SceneComponentProps,
   SceneObjectBase,
   SceneVariable,
-  SceneVariableState,
   SceneVariables,
   sceneGraph,
   AdHocFilterSet,
@@ -20,10 +19,9 @@ import { EditListViewSceneUrlSync } from './EditListViewSceneUrlSync';
 import { DashboardEditView, DashboardEditViewState, useDashboardEditPageNav } from './utils';
 import { VariableEditorForm } from './variables/VariableEditorForm';
 import { VariableEditorList } from './variables/VariableEditorList';
-import { EditableVariableType, getVariableScene, isEditableVariableType } from './variables/utils';
+import { EditableVariableType, getVariableDefault, getVariableScene } from './variables/utils';
 export interface VariablesEditViewState extends DashboardEditViewState {
   editIndex?: number | undefined;
-  originalVariableState?: SceneVariableState;
 }
 
 export class VariablesEditView extends SceneObjectBase<VariablesEditViewState> implements DashboardEditView {
@@ -104,19 +102,18 @@ export class VariablesEditView extends SceneObjectBase<VariablesEditViewState> i
       return;
     }
 
-    const originalVariable = variables[variableIndex];
+    const variableToUpdate = variables[variableIndex];
     let copyNumber = 0;
-    let newName = `copy_of_${originalVariable.state.name}`;
+    let newName = `copy_of_${variableToUpdate.state.name}`;
 
     // Check if the name is unique, if not, increment the copy number
     while (variables.some((v) => v.state.name === newName)) {
       copyNumber++;
-      newName = `copy_of_${originalVariable.state.name}_${copyNumber}`;
+      newName = `copy_of_${variableToUpdate.state.name}_${copyNumber}`;
     }
 
     //clone the original variable
-
-    const newVariable = originalVariable.clone(originalVariable.state);
+    const newVariable = variableToUpdate.clone(variableToUpdate.state);
     // update state name of the new variable
     newVariable.setState({ name: newName });
 
@@ -153,7 +150,20 @@ export class VariablesEditView extends SceneObjectBase<VariablesEditViewState> i
       console.error('Variable not found');
       return;
     }
-    this.setState({ editIndex: variableIndex, originalVariableState: { ...this.getVariables()[variableIndex].state } });
+    this.setState({ editIndex: variableIndex });
+  };
+
+  public onAdd = () => {
+    const variables = this.getVariables();
+    const variableIndex = variables.length;
+    //add the new variable to the end of the array
+    const defaultNewVariable = getVariableDefault(variables);
+    if (defaultNewVariable instanceof AdHocFilterSet) {
+      // TODO: Update controls in adding this fiter set to the dashboard
+    } else {
+      this.getVariableSet().setState({ variables: [...this.getVariables(), defaultNewVariable] });
+      this.setState({ editIndex: variableIndex });
+    }
   };
 
   public onTypeChange = (type: EditableVariableType) => {
@@ -176,36 +186,13 @@ export class VariablesEditView extends SceneObjectBase<VariablesEditViewState> i
   public onGoBack = () => {
     this.setState({ editIndex: undefined });
   };
-
-  public onDiscardChanges: () => void = () => {
-    const variables = this.getVariableSet().state.variables;
-    const { editIndex, originalVariableState } = this.state;
-    if (editIndex === undefined || !originalVariableState) {
-      return;
-    }
-    const variable = variables[editIndex];
-    if (!variable) {
-      return;
-    }
-    if (isEditableVariableType(originalVariableState.type)) {
-      const newVariable = getVariableScene(originalVariableState.type, originalVariableState);
-      if (newVariable instanceof AdHocFilterSet) {
-        // TODO: Update controls in adding this fiter set to the dashboard
-      } else {
-        const updatedVariables = [...variables.slice(0, editIndex), newVariable, ...variables.slice(editIndex + 1)];
-        this.getVariableSet().setState({ variables: updatedVariables });
-      }
-    }
-
-    this.setState({ editIndex: undefined, originalVariableState: undefined });
-  };
 }
 
 function VariableEditorSettingsListView({ model }: SceneComponentProps<VariablesEditView>) {
   const dashboard = model.getDashboard();
   const { navModel, pageNav } = useDashboardEditPageNav(dashboard, model.getUrlKey());
   // get variables from dashboard state
-  const { onDelete, onDuplicated, onOrderChanged, onEdit, onTypeChange, onGoBack, onDiscardChanges } = model;
+  const { onDelete, onDuplicated, onOrderChanged, onEdit, onTypeChange, onGoBack, onAdd } = model;
   const { variables } = model.getVariableSet().useState();
   const { editIndex } = model.useState();
 
@@ -217,7 +204,6 @@ function VariableEditorSettingsListView({ model }: SceneComponentProps<Variables
           variable={variable}
           onTypeChange={onTypeChange}
           onGoBack={onGoBack}
-          onDiscardChanges={onDiscardChanges}
           pageNav={pageNav}
           navModel={navModel}
           dashboard={dashboard}
@@ -234,7 +220,7 @@ function VariableEditorSettingsListView({ model }: SceneComponentProps<Variables
         onDelete={onDelete}
         onDuplicate={onDuplicated}
         onChangeOrder={onOrderChanged}
-        onAdd={() => {}}
+        onAdd={onAdd}
         onEdit={onEdit}
       />
     </Page>
@@ -248,7 +234,6 @@ interface VariableEditorSettingsEditViewProps {
   dashboard: DashboardScene;
   onTypeChange: (variableType: EditableVariableType) => void;
   onGoBack: () => void;
-  onDiscardChanges: () => void;
 }
 
 function VariableEditorSettingsView({
@@ -258,7 +243,6 @@ function VariableEditorSettingsView({
   dashboard,
   onTypeChange,
   onGoBack,
-  onDiscardChanges,
 }: VariableEditorSettingsEditViewProps) {
   const parentTab = pageNav.children!.find((p) => p.active)!;
   parentTab.parentItem = pageNav;
@@ -271,12 +255,7 @@ function VariableEditorSettingsView({
   return (
     <Page navModel={navModel} pageNav={editVariablePageNav} layout={PageLayoutType.Standard}>
       <NavToolbarActions dashboard={dashboard} />
-      <VariableEditorForm
-        variable={variable}
-        onTypeChange={onTypeChange}
-        onGoBack={onGoBack}
-        onDiscardChanges={onDiscardChanges}
-      />
+      <VariableEditorForm variable={variable} onTypeChange={onTypeChange} onGoBack={onGoBack} />
     </Page>
   );
 }

public/app/features/dashboard-scene/settings/variables/VariableEditorForm.tsx

@@ -22,10 +22,9 @@ interface VariableEditorFormProps {
   variable: SceneVariable;
   onTypeChange: (type: EditableVariableType) => void;
   onGoBack: () => void;
-  onDiscardChanges: () => void;
 }
 
-export function VariableEditorForm({ variable, onTypeChange, onGoBack, onDiscardChanges }: VariableEditorFormProps) {
+export function VariableEditorForm({ variable, onTypeChange, onGoBack }: VariableEditorFormProps) {
   const { name, type, label, description, hide } = variable.useState();
   const EditorToRender = isEditableVariableType(type) ? getVariableEditor(type) : undefined;
   const [runQueryState, onRunQuery] = useAsyncFn(async () => {
@@ -106,13 +105,6 @@ export function VariableEditorForm({ variable, onTypeChange, onGoBack, onDiscard
                 {runQueryState.loading ? <LoadingPlaceholder text="Running query..." /> : `Run query`}
               </Button>
             )}
-            <Button
-              variant="destructive"
-              data-testid={selectors.pages.Dashboard.Settings.Variables.Edit.General.applyButton}
-              onClick={onDiscardChanges}
-            >
-              Discard changes
-            </Button>
           </HorizontalGroup>
         </div>
       </form>

public/app/features/dashboard-scene/settings/variables/VariableEditorList.tsx

@@ -5,7 +5,7 @@ import { DragDropContext, Droppable, DropResult } from 'react-beautiful-dnd';
 import { selectors } from '@grafana/e2e-selectors';
 import { reportInteraction } from '@grafana/runtime';
 import { SceneVariable, SceneVariableState } from '@grafana/scenes';
-import { useStyles2, Stack } from '@grafana/ui';
+import { useStyles2, Stack, Button } from '@grafana/ui';
 import EmptyListCTA from 'app/core/components/EmptyListCTA/EmptyListCTA';
 
 import { VariableEditorListRow } from './VariableEditorListRow';
@@ -80,6 +80,15 @@ export function VariableEditorList({
                 </DragDropContext>
               </table>
             </div>
+            <Stack>
+              <Button
+                data-testid={selectors.pages.Dashboard.Settings.Variables.List.newButton}
+                onClick={onAdd}
+                icon="plus"
+              >
+                New variable
+              </Button>
+            </Stack>
           </Stack>
         )}
       </div>
@@ -94,7 +103,6 @@ function EmptyVariablesList({ onAdd }: { onAdd: () => void }): ReactElement {
         title="There are no variables yet"
         buttonIcon="calculator-alt"
         buttonTitle="Add variable"
-        buttonDisabled
         infoBox={{
           __html: ` <p>
                     Variables enable more interactive and dynamic dashboards. Instead of hard-coding things like server

public/app/features/dashboard-scene/settings/variables/utils.test.ts

@@ -8,6 +8,7 @@ import {
   DataSourceVariable,
   AdHocFiltersVariable,
   TextBoxVariable,
+  SceneVariableSet,
 } from '@grafana/scenes';
 import { DataQuery, DataSourceJsonData, VariableType } from '@grafana/schema';
 import { SHARED_DASHBOARD_QUERY } from 'app/plugins/datasource/dashboard';
@@ -31,6 +32,8 @@ import {
   EditableVariableType,
   getDefinition,
   getOptionDataSourceTypes,
+  getNextAvailableId,
+  getVariableDefault,
 } from './utils';
 
 const templateSrv = {
@@ -249,3 +252,47 @@ describe('getOptionDataSourceTypes', () => {
     expect(optionTypes[1].label).toBe('ds1');
   });
 });
+
+describe('getNextAvailableId', () => {
+  it('should return the initial ID for an empty array', () => {
+    const sceneVariables = new SceneVariableSet({
+      variables: [],
+    });
+
+    expect(getNextAvailableId('query', sceneVariables.state.variables)).toBe('query0');
+  });
+
+  it('should return a non-conflicting ID for a non-empty array', () => {
+    const variable = new QueryVariable({
+      name: 'query0',
+      label: 'test-label',
+      description: 'test-desc',
+      value: ['selected-value'],
+      text: ['selected-value-text'],
+      datasource: { uid: 'fake-std', type: 'fake-std' },
+      query: 'query',
+      includeAll: true,
+      allValue: 'test-all',
+      isMulti: true,
+    });
+
+    const sceneVariables = new SceneVariableSet({
+      variables: [variable],
+    });
+
+    expect(getNextAvailableId('query', sceneVariables.state.variables)).toBe('query1');
+  });
+});
+
+describe('getVariableDefault', () => {
+  it('should return a QueryVariable instance with the correct name', () => {
+    const sceneVariables = new SceneVariableSet({
+      variables: [],
+    });
+
+    const defaultVariable = getVariableDefault(sceneVariables.state.variables);
+
+    expect(defaultVariable).toBeInstanceOf(QueryVariable);
+    expect(defaultVariable.state.name).toBe('query0');
+  });
+});

public/app/features/dashboard-scene/settings/variables/utils.ts

@@ -12,6 +12,7 @@ import {
   AdHocFilterSet,
   SceneVariable,
   MultiValueVariable,
+  SceneVariableState,
 } from '@grafana/scenes';
 import { VariableType } from '@grafana/schema';
 
@@ -122,6 +123,25 @@ export function getVariableScene(type: EditableVariableType, initialState: Commo
   }
 }
 
+export function getVariableDefault(variables: Array<SceneVariable<SceneVariableState>>) {
+  const defaultVariableType = 'query';
+  const nextVariableIdName = getNextAvailableId(defaultVariableType, variables);
+  return new QueryVariable({
+    name: nextVariableIdName,
+  });
+}
+
+export function getNextAvailableId(type: VariableType, variables: Array<SceneVariable<SceneVariableState>>): string {
+  let counter = 0;
+  let nextId = `${type}${counter}`;
+
+  while (variables.find((variable) => variable.state.name === nextId)) {
+    nextId = `${type}${++counter}`;
+  }
+
+  return nextId;
+}
+
 export function hasVariableOptions(variable: SceneVariable): variable is MultiValueVariable {
   // variable options can be defined by state.options or state.intervals in case of interval variable
   return 'options' in variable.state || 'intervals' in variable.state;