Fix: Wrap value of multi variable in array when coming from URL (#16992)

*  Wrap variable value in array if multi

* Fix typo

* Fix case with multiple custom values

public/app/features/templating/specs/variable_srv.test.ts

@@ -3,6 +3,7 @@ import { VariableSrv } from '../variable_srv';
 import { DashboardModel } from '../../dashboard/state/DashboardModel';
 import $q from 'q';
 import { dateTime } from '@grafana/ui/src/utils/moment_wrapper';
+import { CustomVariable } from '../custom_variable';
 
 describe('VariableSrv', function(this: any) {
   const ctx = {
@@ -589,8 +590,55 @@ describe('VariableSrv', function(this: any) {
       expect(unknownSet).toEqual(false);
     });
   });
+
+  describe('setOptionFromUrl', () => {
+    it('sets single value as string if not multi choice', async () => {
+      const [setValueMock, setFromUrl] = setupSetFromUrlTest(ctx);
+      await setFromUrl('one');
+      expect(setValueMock).toHaveBeenCalledWith({ text: 'one', value: 'one' });
+    });
+
+    it('sets single value as array if multi choice', async () => {
+      const [setValueMock, setFromUrl] = setupSetFromUrlTest(ctx, { multi: true });
+      await setFromUrl('one');
+      expect(setValueMock).toHaveBeenCalledWith({ text: 'one', value: ['one'] });
+    });
+
+    it('sets both text and value as array if multiple values in url', async () => {
+      const [setValueMock, setFromUrl] = setupSetFromUrlTest(ctx, { multi: true });
+      await setFromUrl(['one', 'two']);
+      expect(setValueMock).toHaveBeenCalledWith({ text: ['one', 'two'], value: ['one', 'two'] });
+    });
+
+    it('sets text and value even if it does not match any option', async () => {
+      const [setValueMock, setFromUrl] = setupSetFromUrlTest(ctx);
+      await setFromUrl('none');
+      expect(setValueMock).toHaveBeenCalledWith({ text: 'none', value: 'none' });
+    });
+
+    it('sets text and value even if it does not match any option and it is array', async () => {
+      const [setValueMock, setFromUrl] = setupSetFromUrlTest(ctx);
+      await setFromUrl(['none', 'none2']);
+      expect(setValueMock).toHaveBeenCalledWith({ text: ['none', 'none2'], value: ['none', 'none2'] });
+    });
+  });
 });
 
+function setupSetFromUrlTest(ctx, model = {}) {
+  const variableSrv = new VariableSrv($q, ctx.$location, ctx.$injector, ctx.templateSrv, ctx.timeSrv);
+  const finalModel = {
+    type: 'custom',
+    options: ['one', 'two', 'three'].map(v => ({ text: v, value: v })),
+    name: 'test',
+    ...model,
+  };
+  const variable = new CustomVariable(finalModel, variableSrv);
+  // We are mocking the setValue here instead of just checking the final variable.current value because there is lots
+  // of stuff going when the setValue is called that is hard to mock out.
+  variable.setValue = jest.fn();
+  return [variable.setValue, val => variableSrv.setOptionFromUrl(variable, val)];
+}
+
 function getVarMockConstructor(variable, model, ctx) {
   switch (model.model.type) {
     case 'datasource':

public/app/features/templating/variable_srv.ts

@@ -214,7 +214,14 @@ export class VariableSrv {
     }
   }
 
-  setOptionFromUrl(variable, urlValue) {
+  /**
+   * Sets the current selected option (or options) based on the query params in the url. It is possible for values
+   * in the url to not match current options of the variable. In that case the variables current value will be still set
+   * to that value.
+   * @param variable Instance of Variable
+   * @param urlValue Value of the query parameter
+   */
+  setOptionFromUrl(variable: any, urlValue: string | string[]): Promise<any> {
     let promise = this.$q.when();
 
     if (variable.refresh) {
@@ -222,28 +229,41 @@ export class VariableSrv {
     }
 
     return promise.then(() => {
+      // Simple case. Value in url matches existing options text or value.
       let option: any = _.find(variable.options, op => {
         return op.text === urlValue || op.value === urlValue;
       });
 
-      let defaultText = urlValue;
-      const defaultValue = urlValue;
+      // No luck either it is array or value does not exist in the variables options.
+      if (!option) {
+        let defaultText = urlValue;
+        const defaultValue = urlValue;
 
-      if (!option && _.isArray(urlValue)) {
-        defaultText = [];
+        if (_.isArray(urlValue)) {
+          // Multiple values in the url. We construct text as a list of texts from all matched options.
+          defaultText = urlValue.reduce((acc, item) => {
+            const t: any = _.find(variable.options, { value: item });
+            if (t) {
+              acc.push(t.text);
+            } else {
+              acc.push(item);
+            }
 
-        for (let n = 0; n < urlValue.length; n++) {
-          const t: any = _.find(variable.options, op => {
-            return op.value === urlValue[n];
-          });
-
-          if (t) {
-            defaultText.push(t.text);
-          }
+            return acc;
+          }, []);
         }
+
+        // It is possible that we did not match the value to any existing option. In that case the url value will be
+        // used anyway for both text and value.
+        option = { text: defaultText, value: defaultValue };
+      }
+
+      if (variable.multi) {
+        // In case variable is multiple choice, we cast to array to preserve the same behaviour as when selecting
+        // the option directly, which will return even single value in an array.
+        option = { ...option, value: _.castArray(option.value) };
       }
 
-      option = option || { text: defaultText, value: defaultValue };
       return variable.setValue(option);
     });
   }