Merge branch 'fix-templating-variables-set-from-url' into 'master'

Fix templating vars set from URL

See merge request gitlab-org/gitlab!34668

app/assets/javascripts/monitoring/stores/utils.js

@@ -2,7 +2,7 @@ import { slugify } from '~/lib/utils/text_utility';
 import createGqClient, { fetchPolicies } from '~/lib/graphql';
 import { SUPPORTED_FORMATS } from '~/lib/utils/unit_format';
 import { getIdFromGraphQLId } from '~/graphql_shared/utils';
-import { parseTemplatingVariables } from './variable_mapping';
+import { mergeURLVariables, parseTemplatingVariables } from './variable_mapping';
 import { DATETIME_RANGE_TYPES } from '~/lib/utils/constants';
 import { timeRangeToParams, getRangeType } from '~/lib/utils/datetime_range';
 import { isSafeURL, mergeUrlParams } from '~/lib/utils/url_utility';
@@ -289,7 +289,7 @@ export const mapToDashboardViewModel = ({
 }) => {
   return {
     dashboard,
-    variables: parseTemplatingVariables(templating),
+    variables: mergeURLVariables(parseTemplatingVariables(templating)),
     links: links.map(mapLinksToViewModel),
     panelGroups: panel_groups.map(mapToPanelGroupViewModel),
   };

app/assets/javascripts/monitoring/stores/variable_mapping.js

 import { isString } from 'lodash';
+import { templatingVariablesFromUrl } from '../utils';
 import { VARIABLE_TYPES } from '../constants';
 
 /**
@@ -164,4 +165,39 @@ export const parseTemplatingVariables = ({ variables = {} } = {}) =>
     return acc;
   }, {});
 
+/**
+ * Custom variables are defined in the dashboard yml file
+ * and their values can be passed through the URL.
+ *
+ * On component load, this method merges variables data
+ * from the yml file with URL data to store in the Vuex store.
+ * Not all params coming from the URL need to be stored. Only
+ * the ones that have a corresponding variable defined in the
+ * yml file.
+ *
+ * This ensures that there is always a single source of truth
+ * for variables
+ *
+ * This method can be improved further. See the below issue
+ * https://gitlab.com/gitlab-org/gitlab/-/issues/217713
+ *
+ * @param {Object} varsFromYML template variables from yml file
+ * @returns {Object}
+ */
+export const mergeURLVariables = (varsFromYML = {}) => {
+  const varsFromURL = templatingVariablesFromUrl();
+  const variables = {};
+  Object.keys(varsFromYML).forEach(key => {
+    if (Object.prototype.hasOwnProperty.call(varsFromURL, key)) {
+      variables[key] = {
+        ...varsFromYML[key],
+        value: varsFromURL[key],
+      };
+    } else {
+      variables[key] = varsFromYML[key];
+    }
+  });
+  return variables;
+};
+
 export default {};

app/assets/javascripts/monitoring/utils.js

@@ -170,11 +170,10 @@ export const convertVariablesForURL = variables =>
  * begin with a constant prefix so that it doesn't collide with
  * other URL params.
  *
- * @param {String} New URL
+ * @param {String} search URL
  * @returns {Object} The custom variables defined by the user in the URL
  */
-
-export const getPromCustomVariablesFromUrl = (search = window.location.search) => {
+export const templatingVariablesFromUrl = (search = window.location.search) => {
   const params = queryToObject(search);
   // pick the params with variable prefix
   const paramsWithVars = pickBy(params, (val, key) => key.startsWith(VARIABLE_PREFIX));
@@ -353,39 +352,4 @@ export const barChartsDataParser = (data = []) =>
     {},
   );
 
-/**
- * Custom variables are defined in the dashboard yml file
- * and their values can be passed through the URL.
- *
- * On component load, this method merges variables data
- * from the yml file with URL data to store in the Vuex store.
- * Not all params coming from the URL need to be stored. Only
- * the ones that have a corresponding variable defined in the
- * yml file.
- *
- * This ensures that there is always a single source of truth
- * for variables
- *
- * This method can be improved further. See the below issue
- * https://gitlab.com/gitlab-org/gitlab/-/issues/217713
- *
- * @param {Object} varsFromYML template variables from yml file
- * @returns {Object}
- */
-export const mergeURLVariables = (varsFromYML = {}) => {
-  const varsFromURL = getPromCustomVariablesFromUrl();
-  const variables = {};
-  Object.keys(varsFromYML).forEach(key => {
-    if (Object.prototype.hasOwnProperty.call(varsFromURL, key)) {
-      variables[key] = {
-        ...varsFromYML[key],
-        value: varsFromURL[key],
-      };
-    } else {
-      variables[key] = varsFromYML[key];
-    }
-  });
-  return variables;
-};
-
 export default {};

changelogs/unreleased/fix-templating-variables-set-from-url.yml

+---
+title: Fix missing templating vars set from URL in metrics dashboard
+merge_request: 34668
+author:
+type: fixed

spec/frontend/monitoring/store/utils_spec.js

@@ -10,6 +10,7 @@ import {
   addDashboardMetaDataToLink,
   normalizeCustomDashboardPath,
 } from '~/monitoring/stores/utils';
+import * as urlUtils from '~/lib/utils/url_utility';
 import { annotationsData } from '../mock_data';
 import { NOT_IN_DB_PREFIX } from '~/monitoring/constants';
 
@@ -399,6 +400,118 @@ describe('mapToDashboardViewModel', () => {
       });
     });
   });
+
+  describe('templating variables mapping', () => {
+    beforeEach(() => {
+      jest.spyOn(urlUtils, 'queryToObject');
+    });
+
+    afterEach(() => {
+      urlUtils.queryToObject.mockRestore();
+    });
+
+    it('sets variables as-is from yml file if URL has no variables', () => {
+      const response = {
+        dashboard: 'Dashboard Name',
+        links: [],
+        templating: {
+          variables: {
+            pod: 'kubernetes',
+            pod_2: 'kubernetes-2',
+          },
+        },
+      };
+
+      urlUtils.queryToObject.mockReturnValueOnce();
+
+      expect(mapToDashboardViewModel(response)).toMatchObject({
+        dashboard: 'Dashboard Name',
+        links: [],
+        variables: {
+          pod: {
+            label: 'pod',
+            type: 'text',
+            value: 'kubernetes',
+          },
+          pod_2: {
+            label: 'pod_2',
+            type: 'text',
+            value: 'kubernetes-2',
+          },
+        },
+      });
+    });
+
+    it('sets variables as-is from yml file if URL has no matching variables', () => {
+      const response = {
+        dashboard: 'Dashboard Name',
+        links: [],
+        templating: {
+          variables: {
+            pod: 'kubernetes',
+            pod_2: 'kubernetes-2',
+          },
+        },
+      };
+
+      urlUtils.queryToObject.mockReturnValueOnce({
+        'var-environment': 'POD',
+      });
+
+      expect(mapToDashboardViewModel(response)).toMatchObject({
+        dashboard: 'Dashboard Name',
+        links: [],
+        variables: {
+          pod: {
+            label: 'pod',
+            type: 'text',
+            value: 'kubernetes',
+          },
+          pod_2: {
+            label: 'pod_2',
+            type: 'text',
+            value: 'kubernetes-2',
+          },
+        },
+      });
+    });
+
+    it('merges variables from URL with the ones from yml file', () => {
+      const response = {
+        dashboard: 'Dashboard Name',
+        links: [],
+        templating: {
+          variables: {
+            pod: 'kubernetes',
+            pod_2: 'kubernetes-2',
+          },
+        },
+      };
+
+      urlUtils.queryToObject.mockReturnValueOnce({
+        'var-environment': 'POD',
+        'var-pod': 'POD1',
+        'var-pod_2': 'POD2',
+      });
+
+      expect(mapToDashboardViewModel(response)).toMatchObject({
+        dashboard: 'Dashboard Name',
+        links: [],
+        variables: {
+          pod: {
+            label: 'pod',
+            type: 'text',
+            value: 'POD1',
+          },
+          pod_2: {
+            label: 'pod_2',
+            type: 'text',
+            value: 'POD2',
+          },
+        },
+      });
+    });
+  });
 });
 
 describe('uniqMetricsId', () => {

spec/frontend/monitoring/store/variable_mapping_spec.js

-import { parseTemplatingVariables } from '~/monitoring/stores/variable_mapping';
+import { parseTemplatingVariables, mergeURLVariables } from '~/monitoring/stores/variable_mapping';
+import * as urlUtils from '~/lib/utils/url_utility';
 import { mockTemplatingData, mockTemplatingDataResponses } from '../mock_data';
 
 describe('parseTemplatingVariables', () => {
@@ -21,3 +22,73 @@ describe('parseTemplatingVariables', () => {
     expect(parseTemplatingVariables(input?.dashboard?.templating)).toEqual(expected);
   });
 });
+
+describe('mergeURLVariables', () => {
+  beforeEach(() => {
+    jest.spyOn(urlUtils, 'queryToObject');
+  });
+
+  afterEach(() => {
+    urlUtils.queryToObject.mockRestore();
+  });
+
+  it('returns empty object if variables are not defined in yml or URL', () => {
+    urlUtils.queryToObject.mockReturnValueOnce({});
+
+    expect(mergeURLVariables({})).toEqual({});
+  });
+
+  it('returns empty object if variables are defined in URL but not in yml', () => {
+    urlUtils.queryToObject.mockReturnValueOnce({
+      'var-env': 'one',
+      'var-instance': 'localhost',
+    });
+
+    expect(mergeURLVariables({})).toEqual({});
+  });
+
+  it('returns yml variables if variables defined in yml but not in the URL', () => {
+    urlUtils.queryToObject.mockReturnValueOnce({});
+
+    const params = {
+      env: 'one',
+      instance: 'localhost',
+    };
+
+    expect(mergeURLVariables(params)).toEqual(params);
+  });
+
+  it('returns yml variables if variables defined in URL do not match with yml variables', () => {
+    const urlParams = {
+      'var-env': 'one',
+      'var-instance': 'localhost',
+    };
+    const ymlParams = {
+      pod: { value: 'one' },
+      service: { value: 'database' },
+    };
+    urlUtils.queryToObject.mockReturnValueOnce(urlParams);
+
+    expect(mergeURLVariables(ymlParams)).toEqual(ymlParams);
+  });
+
+  it('returns merged yml and URL variables if there is some match', () => {
+    const urlParams = {
+      'var-env': 'one',
+      'var-instance': 'localhost:8080',
+    };
+    const ymlParams = {
+      instance: { value: 'localhost' },
+      service: { value: 'database' },
+    };
+
+    const merged = {
+      instance: { value: 'localhost:8080' },
+      service: { value: 'database' },
+    };
+
+    urlUtils.queryToObject.mockReturnValueOnce(urlParams);
+
+    expect(mergeURLVariables(ymlParams)).toEqual(merged);
+  });
+});

spec/frontend/monitoring/utils_spec.js

@@ -169,8 +169,8 @@ describe('monitoring/utils', () => {
     });
   });
 
-  describe('getPromCustomVariablesFromUrl', () => {
-    const { getPromCustomVariablesFromUrl } = monitoringUtils;
+  describe('templatingVariablesFromUrl', () => {
+    const { templatingVariablesFromUrl } = monitoringUtils;
 
     beforeEach(() => {
       jest.spyOn(urlUtils, 'queryToObject');
@@ -195,7 +195,7 @@ describe('monitoring/utils', () => {
         'var-pod': 'POD',
       });
 
-      expect(getPromCustomVariablesFromUrl()).toEqual(expect.objectContaining({ pod: 'POD' }));
+      expect(templatingVariablesFromUrl()).toEqual(expect.objectContaining({ pod: 'POD' }));
     });
 
     it('returns an empty object when no custom variables are present', () => {
@@ -203,7 +203,7 @@ describe('monitoring/utils', () => {
         dashboard: '.gitlab/dashboards/custom_dashboard.yml',
       });
 
-      expect(getPromCustomVariablesFromUrl()).toStrictEqual({});
+      expect(templatingVariablesFromUrl()).toStrictEqual({});
     });
   });
 
@@ -427,76 +427,6 @@ describe('monitoring/utils', () => {
     });
   });
 
-  describe('mergeURLVariables', () => {
-    beforeEach(() => {
-      jest.spyOn(urlUtils, 'queryToObject');
-    });
-
-    afterEach(() => {
-      urlUtils.queryToObject.mockRestore();
-    });
-
-    it('returns empty object if variables are not defined in yml or URL', () => {
-      urlUtils.queryToObject.mockReturnValueOnce({});
-
-      expect(monitoringUtils.mergeURLVariables({})).toEqual({});
-    });
-
-    it('returns empty object if variables are defined in URL but not in yml', () => {
-      urlUtils.queryToObject.mockReturnValueOnce({
-        'var-env': 'one',
-        'var-instance': 'localhost',
-      });
-
-      expect(monitoringUtils.mergeURLVariables({})).toEqual({});
-    });
-
-    it('returns yml variables if variables defined in yml but not in the URL', () => {
-      urlUtils.queryToObject.mockReturnValueOnce({});
-
-      const params = {
-        env: 'one',
-        instance: 'localhost',
-      };
-
-      expect(monitoringUtils.mergeURLVariables(params)).toEqual(params);
-    });
-
-    it('returns yml variables if variables defined in URL do not match with yml variables', () => {
-      const urlParams = {
-        'var-env': 'one',
-        'var-instance': 'localhost',
-      };
-      const ymlParams = {
-        pod: { value: 'one' },
-        service: { value: 'database' },
-      };
-      urlUtils.queryToObject.mockReturnValueOnce(urlParams);
-
-      expect(monitoringUtils.mergeURLVariables(ymlParams)).toEqual(ymlParams);
-    });
-
-    it('returns merged yml and URL variables if there is some match', () => {
-      const urlParams = {
-        'var-env': 'one',
-        'var-instance': 'localhost:8080',
-      };
-      const ymlParams = {
-        instance: { value: 'localhost' },
-        service: { value: 'database' },
-      };
-
-      const merged = {
-        instance: { value: 'localhost:8080' },
-        service: { value: 'database' },
-      };
-
-      urlUtils.queryToObject.mockReturnValueOnce(urlParams);
-
-      expect(monitoringUtils.mergeURLVariables(ymlParams)).toEqual(merged);
-    });
-  });
-
   describe('convertVariablesForURL', () => {
     it.each`
       input                               | expected