Variables: shows warning when user tries to save erroneous variables (#110154)

* Variables: shows warning when saving variables with errors

* chore: updates after PR feedback

* chore: removes changes in non-scenes parts

packages/grafana-e2e-selectors/src/selectors/pages.ts

@@ -638,6 +638,9 @@ export const versionedPages = {
     saveRefresh: {
       '11.1.0': 'Dashboard settings Save Dashboard Modal Save refresh checkbox',
     },
+    variablesWarningAlert: {
+      '12.2.0': 'Dashboard settings Save Dashboard Modal Save variables Variables With Errors Warning Alert',
+    },
   },
   SharePanelModal: {
     linkToRenderedImage: {

public/app/features/dashboard-scene/saving/SaveDashboardDrawer.test.tsx

@@ -1,10 +1,11 @@
-import { screen, render } from '@testing-library/react';
+import { screen, render, waitFor } from '@testing-library/react';
 import userEvent from '@testing-library/user-event';
 import { TestProvider } from 'test/helpers/TestProvider';
+import { byTestId, byText } from 'testing-library-selector';
 
 import { selectors } from '@grafana/e2e-selectors';
 import { config } from '@grafana/runtime';
-import { sceneGraph, SceneRefreshPicker } from '@grafana/scenes';
+import { ConstantVariable, sceneGraph, SceneRefreshPicker } from '@grafana/scenes';
 import { AnnoKeyManagerKind, ManagerKind } from 'app/features/apiserver/types';
 import { SaveDashboardResponseDTO } from 'app/types/dashboard';
 
@@ -27,12 +28,18 @@ jest.mock('app/features/browse-dashboards/api/browseDashboardsAPI', () => ({
   useSaveDashboardMutation: () => [saveDashboardMutationMock],
 }));
 
+const ui = {
+  saveDashbordText: byText('Save dashboard'),
+  saveVariablesCheckbox: byTestId(selectors.pages.SaveDashboardModal.saveVariables),
+  variablesWarningAlert: byTestId(selectors.pages.SaveDashboardModal.variablesWarningAlert),
+};
+
 describe('SaveDashboardDrawer', () => {
   describe('Given an already saved dashboard', () => {
     it('should render save drawer with only message textarea', async () => {
       setup().openAndRender();
 
-      expect(await screen.findByText('Save dashboard')).toBeInTheDocument();
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
       expect(screen.queryByTestId(selectors.pages.SaveDashboardModal.saveTimerange)).not.toBeInTheDocument();
       expect(screen.getByText('No changes to save')).toBeInTheDocument();
       expect(screen.queryByRole('tab', { name: /Changes/ })).not.toBeInTheDocument();
@@ -50,10 +57,57 @@ describe('SaveDashboardDrawer', () => {
 
       openAndRender();
 
-      expect(await screen.findByText('Save dashboard')).toBeInTheDocument();
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
       expect(screen.queryByTestId(selectors.pages.SaveDashboardModal.saveTimerange)).toBeInTheDocument();
     });
 
+    it('When variable changed show save variables option', async () => {
+      const { dashboard, openAndRender } = setup();
+
+      sceneGraph
+        .getVariables(dashboard)
+        .setState({ variables: [new ConstantVariable({ name: 'constant', type: 'constant', value: 'new value' })] });
+
+      openAndRender();
+
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
+      expect(ui.saveVariablesCheckbox.get()).toBeInTheDocument();
+      expect(ui.variablesWarningAlert.query()).not.toBeInTheDocument(); // the alert shouldn't show as default
+
+      // checking the checkbox shouldn't show the alert because there are no variables with errors
+      await userEvent.click(ui.saveVariablesCheckbox.get());
+      expect(ui.variablesWarningAlert.query()).not.toBeInTheDocument();
+    });
+
+    it('When variable has error show save variables warning', async () => {
+      const { dashboard, openAndRender } = setup();
+
+      sceneGraph.getVariables(dashboard).setState({
+        variables: [
+          new ConstantVariable({
+            name: 'constant',
+            type: 'constant',
+            value: 'new value',
+            error: new Error('Some error'),
+          }),
+        ],
+      });
+
+      openAndRender();
+
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
+      expect(ui.saveVariablesCheckbox.get()).toBeInTheDocument();
+      expect(ui.variablesWarningAlert.query()).not.toBeInTheDocument(); // the alert shouldn't show as default
+
+      // checking the save variables checkbox should show the alert
+      await userEvent.click(ui.saveVariablesCheckbox.get());
+      await waitFor(() => expect(ui.variablesWarningAlert.query()).toBeInTheDocument());
+
+      // unchecking the save variables checkbox should hide the alert
+      await userEvent.click(ui.saveVariablesCheckbox.get());
+      expect(ui.variablesWarningAlert.query()).not.toBeInTheDocument();
+    });
+
     it('Should update diff when including time range is', async () => {
       const { dashboard, openAndRender } = setup();
 
@@ -61,7 +115,7 @@ describe('SaveDashboardDrawer', () => {
 
       openAndRender();
 
-      expect(await screen.findByText('Save dashboard')).toBeInTheDocument();
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
       expect(screen.queryByTestId(selectors.pages.SaveDashboardModal.saveTimerange)).toBeInTheDocument();
       expect(screen.queryByRole('tab', { name: /Changes/ })).not.toBeInTheDocument();
 
@@ -80,7 +134,7 @@ describe('SaveDashboardDrawer', () => {
 
       openAndRender();
 
-      expect(await screen.findByText('Save dashboard')).toBeInTheDocument();
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
       expect(screen.queryByTestId(selectors.pages.SaveDashboardModal.saveRefresh)).toBeInTheDocument();
     });
 
@@ -94,7 +148,7 @@ describe('SaveDashboardDrawer', () => {
 
       openAndRender();
 
-      expect(await screen.findByText('Save dashboard')).toBeInTheDocument();
+      expect(await ui.saveDashbordText.find()).toBeInTheDocument();
       expect(screen.getByTestId(selectors.pages.SaveDashboardModal.saveRefresh)).toBeInTheDocument();
       expect(screen.queryByRole('tab', { name: /Changes/ })).not.toBeInTheDocument();
 
@@ -274,6 +328,15 @@ function setup(overrides?: Partial<DashboardSceneState>) {
       schemaVersion: 30,
       panels: [],
       version: 10,
+      templating: {
+        list: [
+          {
+            name: 'constant',
+            query: 'a constant value',
+            type: 'constant',
+          },
+        ],
+      },
     },
     meta: {},
     ...overrides,

public/app/features/dashboard-scene/saving/SaveDashboardDrawer.tsx

@@ -18,6 +18,7 @@ interface SaveDashboardDrawerState extends SceneObjectState {
   saveVariables?: boolean;
   saveRefresh?: boolean;
   saveAsCopy?: boolean;
+  showVariablesWarning?: boolean;
   onSaveSuccess?: () => void;
 }
 

public/app/features/dashboard-scene/saving/SaveDashboardForm.tsx

@@ -203,7 +203,12 @@ export interface SaveDashboardFormCommonOptionsProps {
 }
 
 export function SaveDashboardFormCommonOptions({ drawer, changeInfo }: SaveDashboardFormCommonOptionsProps) {
-  const { saveVariables = false, saveTimeRange = false, saveRefresh = false } = drawer.useState();
+  const {
+    saveVariables = false,
+    saveTimeRange = false,
+    saveRefresh = false,
+    showVariablesWarning = false,
+  } = drawer.useState();
   const { hasTimeChanges, hasVariableValueChanges, hasRefreshChange } = changeInfo;
 
   return (
@@ -241,20 +246,38 @@ export function SaveDashboardFormCommonOptions({ drawer, changeInfo }: SaveDashb
         />
       )}
       {hasVariableValueChanges && (
-        <Checkbox
-          id="save-variables"
-          label={t(
-            'dashboard-scene.save-dashboard-form-common-options.save-variables-label-update-default-variable-values',
-            'Update default variable values'
+        <>
+          <Checkbox
+            id="save-variables"
+            label={t(
+              'dashboard-scene.save-dashboard-form-common-options.save-variables-label-update-default-variable-values',
+              'Update default variable values'
+            )}
+            description={t(
+              'dashboard-scene.save-dashboard-form-common-options.save-variables-description-current-values-default',
+              'Will make the current values the new default'
+            )}
+            checked={saveVariables}
+            onChange={drawer.onToggleSaveVariables}
+            data-testid={selectors.pages.SaveDashboardModal.saveVariables}
+          />
+          {saveVariables && showVariablesWarning && (
+            <Alert
+              data-testid={selectors.pages.SaveDashboardModal.variablesWarningAlert}
+              title={t(
+                'dashboard-scene.save-dashboard-form-common-options.show-variables-warning-alert-title',
+                'Variable queries failed'
+              )}
+              severity="warning"
+            >
+              <Trans i18nKey="dashboard-scene.save-dashboard-form-common-options.show-variables-warning-alert-body">
+                Some variables failed to load. If you keep “Update default variable values” checked, the current
+                (failed) values will become the dashboard defaults. You can save anyway or uncheck the option to avoid
+                storing those (failed) values.
+              </Trans>
+            </Alert>
           )}
-          description={t(
-            'dashboard-scene.save-dashboard-form-common-options.save-variables-description-current-values-default',
-            'Will make the current values the new default'
-          )}
-          checked={saveVariables}
-          onChange={drawer.onToggleSaveVariables}
-          data-testid={selectors.pages.SaveDashboardModal.saveVariables}
-        />
+        </>
       )}
     </Stack>
   );

public/app/features/dashboard-scene/scene/DashboardScene.tsx

@@ -414,6 +414,7 @@ export class DashboardScene extends SceneObjectBase<DashboardSceneState> impleme
         dashboardRef: this.getRef(),
         saveAsCopy,
         onSaveSuccess,
+        showVariablesWarning: this.hasVariableErrors(),
       }),
     });
   }
@@ -792,6 +793,10 @@ export class DashboardScene extends SceneObjectBase<DashboardSceneState> impleme
   getPath() {
     return this.state.meta.k8s?.annotations?.[AnnoKeySourcePath];
   }
+
+  private hasVariableErrors(): boolean {
+    return Boolean(this.state.$variables?.state.variables.find((v) => Boolean(v.state.error)));
+  }
 }
 
 export class DashboardVariableDependency implements SceneVariableDependencyConfigLike {

public/locales/en-US/grafana.json

@@ -6096,7 +6096,9 @@
       "save-timerange-description-current-range-default": "Will make current time range the new default",
       "save-timerange-label-update-default-time-range": "Update default time range",
       "save-variables-description-current-values-default": "Will make the current values the new default",
-      "save-variables-label-update-default-variable-values": "Update default variable values"
+      "save-variables-label-update-default-variable-values": "Update default variable values",
+      "show-variables-warning-alert-body": "Some variables failed to load. If you keep “Update default variable values” checked, the current (failed) values will become the dashboard defaults. You can save anyway or uncheck the option to avoid storing those (failed) values.",
+      "show-variables-warning-alert-title": "Variable queries failed"
     },
     "save-library-viz-panel-modal": {
       "affected-dashboards_one": "This update will affect <1>{{count}} dashboards.</1> The following dashboards using the panel will be affected:",