From 47f5187bd2aedd6966d34c3e0769ebc6b9753383 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 12 Sep 2018 15:52:26 -0400
Subject: [PATCH 01/22] Add UI for Filter objects

Fixes #176
---
 package.json                                  |   1 +
 src/components/data/QueryRecipeFilters.js     |  27 +
 src/less/pages/recipe-details.less            |   5 +
 src/setupTests.js                             |   8 +-
 src/state/network/actions.js                  |   2 +-
 src/state/recipes/actions.js                  |  29 +-
 .../components/FilterObjectForm.test.js       | 509 +++++++++++
 .../recipes/components/FilterObjectForm.js    | 803 ++++++++++++++++++
 .../recipes/components/RecipeDetails.js       | 146 +++-
 .../recipes/components/RecipeForm.js          |  39 +-
 yarn.lock                                     |  26 +
 11 files changed, 1575 insertions(+), 20 deletions(-)
 create mode 100644 src/components/data/QueryRecipeFilters.js
 create mode 100644 src/tests/workflows/recipes/components/FilterObjectForm.test.js
 create mode 100644 src/workflows/recipes/components/FilterObjectForm.js

diff --git a/package.json b/package.json
index e7e7e7154..7bb31ccd2 100644
--- a/package.json
+++ b/package.json
@@ -84,6 +84,7 @@
     "promise": "8.0.2",
     "raf": "3.4.0",
     "react-dev-utils": "6.0.0-next.3e165448",
+    "react-testing-library": "5.1.0",
     "resolve": "1.8.1",
     "style-loader": "0.23.0",
     "stylelint": "9.6.0",
diff --git a/src/components/data/QueryRecipeFilters.js b/src/components/data/QueryRecipeFilters.js
new file mode 100644
index 000000000..f50b6fdd9
--- /dev/null
+++ b/src/components/data/QueryRecipeFilters.js
@@ -0,0 +1,27 @@
+import PropTypes from 'prop-types';
+import React from 'react';
+import { connect } from 'react-redux';
+
+import { fetchRecipeFilters } from 'console/state/recipes/actions';
+
+@connect(
+  null,
+  {
+    fetchRecipeFilters,
+  },
+)
+class QueryRecipeFilters extends React.PureComponent {
+  static propTypes = {
+    fetchRecipeFilters: PropTypes.func.isRequired,
+  };
+
+  componentDidMount() {
+    this.props.fetchRecipeFilters();
+  }
+
+  render() {
+    return null;
+  }
+}
+
+export default QueryRecipeFilters;
diff --git a/src/less/pages/recipe-details.less b/src/less/pages/recipe-details.less
index 039453b87..1b8f8427c 100644
--- a/src/less/pages/recipe-details.less
+++ b/src/less/pages/recipe-details.less
@@ -32,6 +32,11 @@ dl.details {
     }
   }
 
+  dt.no-padding,
+  dd.no-padding {
+    padding: 0;
+  }
+
   .arguments-value {
     .value {
       float: left;
diff --git a/src/setupTests.js b/src/setupTests.js
index 2844a7926..f39c9a033 100644
--- a/src/setupTests.js
+++ b/src/setupTests.js
@@ -1,10 +1,11 @@
 import React from 'react';
 import ReactDOM from 'react-dom';
 import { configure, mount, shallow } from 'enzyme';
+import { cleanup } from 'react-testing-library';
 import Adapter from 'enzyme-adapter-react-16';
 import * as immutableMatchers from 'jest-immutable-matchers';
 import fetchMock from 'fetch-mock';
-
+import { Headers } from 'whatwg-fetch';
 // Configure Enzyme adapter
 configure({ adapter: new Adapter() });
 
@@ -40,10 +41,12 @@ global.auth0 = {
   })),
 };
 global.localStorage = mockLocalStorage();
+global.sessionStorage = mockLocalStorage();
 global.mount = mount;
 global.shallow = shallow;
 global.React = React;
 global.ReactDOM = ReactDOM;
+global.Headers = Headers;
 
 // Set up a global fetch mock, and assert that all calls to it are expected.
 beforeEach(() => {
@@ -60,4 +63,7 @@ afterEach(() => {
   } finally {
     fetchMock.restore();
   }
+
+  // Unmounts React trees that were mounted with react-testing-library's render.
+  cleanup();
 });
diff --git a/src/state/network/actions.js b/src/state/network/actions.js
index d24d46429..a60082520 100644
--- a/src/state/network/actions.js
+++ b/src/state/network/actions.js
@@ -38,7 +38,7 @@ export function makeApiRequest(requestId, root, endpoint, options = {}) {
         error,
       });
 
-      throw error;
+      throw new Error(error);
     }
 
     dispatch({
diff --git a/src/state/recipes/actions.js b/src/state/recipes/actions.js
index c9a1abce6..6b301bee8 100644
--- a/src/state/recipes/actions.js
+++ b/src/state/recipes/actions.js
@@ -145,13 +145,30 @@ export function fetchRecipeHistory(pk) {
 
 export function fetchRecipeFilters() {
   return async dispatch => {
+    const localStorageKey = 'recipe_filters';
+    const localFilters = window.localStorage.getItem(localStorageKey);
+    if (localFilters) {
+      // If the filters *were* in localStorage, it's a JSON *string*.
+      dispatch({
+        type: RECIPE_FILTERS_RECEIVE,
+        filters: JSON.parse(localFilters),
+      });
+    }
+    // XXX If we already had the filters in localStorage, can we avoid using this
+    // makeNormandyApiRequest() with an ID so that it doesn't cause any spinners to appear
+    // when the localStorage version almost definitely is good enough.
     const requestId = 'fetch-recipe-filters';
-    const filters = await dispatch(makeNormandyApiRequest(requestId, 'v2/filters/'));
-
-    dispatch({
-      type: RECIPE_FILTERS_RECEIVE,
-      filters,
-    });
+    const filters = await dispatch(makeNormandyApiRequest(requestId, 'v3/filters/'));
+    // After it has been retrieved remotely, it's very possible that the lists
+    // haven't changed. If it hasn't changed compared to what we had in local Storage, then
+    // don't bother dispatching again.
+    if (!localFilters || JSON.stringify(filters) !== localFilters) {
+      dispatch({
+        type: RECIPE_FILTERS_RECEIVE,
+        filters,
+      });
+      window.localStorage.setItem(localStorageKey, JSON.stringify(filters));
+    }
   };
 }
 
diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
new file mode 100644
index 000000000..eac652835
--- /dev/null
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -0,0 +1,509 @@
+import { List, fromJS } from 'immutable';
+import { render, fireEvent } from 'react-testing-library';
+import fetchMock from 'fetch-mock';
+import { NORMANDY_ADMIN_API_ROOT_URL } from 'console/settings';
+import FilterObjectForm, {
+  serializeFilterObjectToMap,
+  deserializeFilterObjectToList,
+  InputsWidget,
+  rewriteSamplingInput,
+  SamplingInput,
+  VersionsInput,
+  parseIntOrNull,
+} from 'console/workflows/recipes/components/FilterObjectForm';
+import { createForm } from 'console/utils/forms';
+import { wrapMockStore } from 'console/tests/mockStore';
+
+describe('<FilterObjectForm>', () => {
+  it('render editing form with an empty existing filter object', () => {
+    fetchMock.getOnce(`${NORMANDY_ADMIN_API_ROOT_URL}v3/filters/`, {
+      channels: [
+        {
+          key: 'beta',
+          value: 'Beta',
+        },
+      ],
+      countries: [{ key: 'SE', value: 'Sweden' }],
+      locales: [{ key: 'sv', value: 'Swedish' }],
+    });
+
+    const filterObject = serializeFilterObjectToMap(fromJS([]));
+    const props = {
+      disabled: false,
+      form: {
+        getFieldValue: key => {
+          // Remember, 'key' is something like "filter_object.locales"
+          const value = filterObject.get(key.split('.')[1]);
+          if (value) {
+            return value.toJS();
+          }
+        },
+        getFieldDecorator: (key, config) => {
+          return Component => {
+            return;
+          };
+        },
+      },
+      filterObject,
+      onSubmit: jest.fn(),
+    };
+
+    const FakeForm = createForm({})(FilterObjectForm);
+    const { getByText } = render(wrapMockStore(<FakeForm {...props} />));
+    expect(getByText('Geo')).toBeTruthy();
+    expect(getByText('Browser')).toBeTruthy();
+    expect(getByText('Sampling')).toBeTruthy();
+    expect(fetchMock.called()).toBeTruthy();
+  });
+
+  it('render editing form with a full existing filter object', () => {
+    fetchMock.getOnce(`${NORMANDY_ADMIN_API_ROOT_URL}v3/filters/`, {
+      channels: [
+        {
+          key: 'beta',
+          value: 'Beta',
+        },
+      ],
+      countries: [{ key: 'SE', value: 'Sweden' }],
+      locales: [{ key: 'sv', value: 'Swedish' }],
+    });
+
+    const filterObject = serializeFilterObjectToMap(
+      fromJS([
+        { type: 'channel', channels: ['beta'] },
+        { type: 'country', countries: ['SE'] },
+        { type: 'locale', locales: ['sv'] },
+        { type: 'channel', channels: ['beta'] },
+        { type: 'version', versions: [67, 68, 69] },
+        { type: 'bucketSample', start: 10, count: 20, total: 30, input: [] },
+      ]),
+    );
+    const props = {
+      disabled: false,
+      form: {
+        getFieldValue: key => {
+          // Remember, 'key' is something like "filter_object.locales"
+          const value = filterObject.get(key.split('.')[1]);
+          if (value) {
+            return value.toJS();
+          }
+        },
+        getFieldDecorator: (key, config) => {
+          return Component => {
+            return;
+          };
+        },
+      },
+      allChannels: fromJS([{ value: 'Release', key: 'release' }]),
+      allCountries: fromJS([{ value: 'Sweden', key: 'SE' }]),
+      allLocales: fromJS([{ value: 'Swedish', key: 'sv' }]),
+      filterObject,
+      onSubmit: jest.fn(),
+    };
+
+    const FakeForm = createForm({})(FilterObjectForm);
+    const { getByText } = render(wrapMockStore(<FakeForm {...props} />));
+
+    // Expect to see "Geo (2)" because a country and a locale is chosen.
+    expect(getByText('Geo (2)')).toBeTruthy();
+    // Same with "Browser (2)" because both channels and versions is chosen.
+    expect(getByText('Browser (2)')).toBeTruthy();
+    // And "Sampling (1)" because a sampling method has been chosen.
+    expect(getByText('Sampling (1)')).toBeTruthy();
+
+    expect(fetchMock.called()).toBeTruthy();
+  });
+
+  describe('<serializeFilterObjectToMap>', () => {
+    it('serialize an empty List to an empty Map', () => {
+      const map = serializeFilterObjectToMap(List());
+      expect(map).toBeImmutableMap();
+      expect(map.size).toEqual(0);
+    });
+
+    it('serialize an non-empty List to a Map', () => {
+      const list = fromJS([
+        { type: 'channel', channels: ['beta'] },
+        {
+          type: 'bucketSample',
+          start: 50,
+          count: 70,
+          total: 100,
+          input: ['normandy.request_time'],
+        },
+      ]);
+      const map = serializeFilterObjectToMap(list);
+      expect(map).toBeImmutableMap();
+      expect(map.size).toEqual(2);
+      expect(map.get('channels')).toEqual(List(['beta']));
+      expect(map.get('_sampling')).toEqual(
+        fromJS({
+          type: 'bucketSample',
+          start: 50,
+          count: 70,
+          total: 100,
+          input: ['normandy.request_time'],
+        }),
+      );
+    });
+
+    it('serialize an undefined List to an empty Map', () => {
+      const map = serializeFilterObjectToMap(undefined);
+      expect(map).toBeImmutableMap();
+      expect(map.size).toEqual(0);
+    });
+
+    it('throw error on something truthy that is not a List', () => {
+      expect(() => {
+        serializeFilterObjectToMap([]);
+      }).toThrow("'' (object) is not a List");
+    });
+    it('throw error on an unrecognized key', () => {
+      const list = fromJS([{ type: 'color', colors: ['red'] }]);
+      expect(() => {
+        serializeFilterObjectToMap(list);
+      }).toThrow("Not sure how to convert 'color'");
+    });
+  });
+
+  describe('<deserializeFilterObjectToList>', () => {
+    it('deserialize an undefined Map to an empty List', () => {
+      const map = deserializeFilterObjectToList(undefined);
+      expect(map).toBeImmutableList();
+      expect(map.size).toEqual(0);
+    });
+
+    it('serialize an non-empty Map to a List', () => {
+      const obj = {
+        channels: ['beta'],
+      };
+      const list = deserializeFilterObjectToList(obj);
+      expect(list).toBeImmutableList();
+      expect(list.size).toEqual(1);
+      expect(list.first()).toEqual(fromJS({ type: 'channel', channels: ['beta'] }));
+    });
+
+    it('throw an error on unrecognized names', () => {
+      const obj = {
+        colors: ['red'],
+      };
+      expect(() => {
+        deserializeFilterObjectToList(obj);
+      }).toThrow("Key 'colors' no in known mapping.");
+    });
+
+    it('serialize a map with exception _sampling', () => {
+      const obj = {
+        _sampling: {
+          type: 'bucketSample',
+          start: 50,
+          count: 70,
+          total: 100,
+          input: ['normandy.request_time'],
+        },
+      };
+      const list = deserializeFilterObjectToList(obj);
+      expect(list).toBeImmutableList();
+      expect(list.size).toEqual(1);
+      expect(list.first()).toEqual(
+        fromJS({
+          type: 'bucketSample',
+          start: 50,
+          count: 70,
+          total: 100,
+          input: ['normandy.request_time'],
+        }),
+      );
+    });
+  });
+
+  describe('utility functions', () => {
+    it('should parse inputs to either integer or null', () => {
+      expect(parseIntOrNull(1)).toEqual(1);
+      expect(parseIntOrNull('2')).toEqual(2);
+      expect(parseIntOrNull('2.0')).toBeNull();
+      expect(parseIntOrNull(Infinity)).toBeNull();
+      expect(parseIntOrNull('junk')).toBeNull();
+      expect(parseIntOrNull(1.1)).toBeNull();
+      expect(parseIntOrNull([])).toBeNull();
+    });
+
+    it('should rewrite sampling input strings in a smart way', () => {
+      const options = ['normandy.request_time', 'normandy.clientId'];
+      expect(rewriteSamplingInput('normandy.request_time', options)).toEqual(
+        'normandy.request_time',
+      );
+      expect(rewriteSamplingInput('string', options)).toEqual('"string"');
+      expect(rewriteSamplingInput('123', options)).toEqual('123');
+    });
+  });
+
+  describe('<InputsWidget>', () => {
+    it('happy path adding more different inputs', () => {
+      const props = {
+        disabled: false,
+        bubbleUp: jest.fn(),
+        value: {
+          input: ['normandy.request_time'],
+        },
+        onSubmit: jest.fn(),
+      };
+
+      // Create a Form wrapper to get the child contexts automatically passed.
+      const FakeForm = createForm({})(InputsWidget);
+      const { container } = render(<FakeForm {...props} />);
+      // Have to use querySelector because there's no text on the button.
+      const button = container.querySelector('button[type="button"]');
+      expect(button.disabled).toBeTruthy();
+      fireEvent.click(button);
+      // Because it's disabled. Because nothing has been typed in.
+      expect(props.bubbleUp).toHaveBeenCalledTimes(0);
+      const input = container.querySelector('input');
+      // Can't use `fireEvent.change(input)` because it's a controlled component.
+      fireEvent.change(input, {
+        target: { value: '   ' },
+      });
+      // Input is just whitespace, so the button is still disabled.
+      expect(button.disabled).toBeTruthy();
+      fireEvent.change(input, {
+        target: { value: ' freetext ' },
+      });
+      expect(button.disabled).toBeFalsy();
+      fireEvent.click(button);
+      expect(props.bubbleUp).toHaveBeenCalledTimes(1);
+      expect(props.bubbleUp).toBeCalledWith({ input: ['"freetext"', 'normandy.request_time'] });
+
+      // Type something else in.
+      expect(input.value).toEqual(''); // should have been wiped
+      fireEvent.change(input, {
+        target: { value: '123' },
+      });
+      expect(button.disabled).toBeFalsy();
+      fireEvent.click(button);
+      expect(props.bubbleUp).toHaveBeenCalledTimes(2);
+      expect(props.bubbleUp).toBeCalledWith({
+        input: ['"freetext"', 123, 'normandy.request_time'],
+      });
+
+      expect(input.value).toEqual(''); // should have been wiped
+      fireEvent.change(input, {
+        target: { value: 'normandy.clientId' },
+      });
+      expect(button.disabled).toBeFalsy();
+      fireEvent.click(button);
+      expect(props.bubbleUp).toHaveBeenCalledTimes(3);
+      expect(props.bubbleUp).toBeCalledWith({
+        input: ['"freetext"', 123, 'normandy.clientId', 'normandy.request_time'],
+      });
+    });
+
+    it('autocomplete suggestions', () => {
+      const props = {
+        disabled: false,
+        bubbleUp: jest.fn(),
+        value: {},
+        defaultInputOptions: ['normandy.foo', 'normandy.man'],
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+      const FakeForm = createForm({})(InputsWidget);
+      const { container, queryByText, getByText } = render(<FakeForm {...props} />);
+      const input = container.querySelector('input');
+      // Can't use `fireEvent.change(input)` because it's a controlled component.
+      fireEvent.change(input, {
+        target: { value: 'm' },
+      });
+      expect(getByText('normandy.man')).not.toBeNull();
+      // If should have filtered out 'normandy.foo' even though it naively contains
+      // the string 'm'.
+      expect(queryByText('normandy.foo')).toBeNull();
+    });
+
+    it('editing by removing an existing input', () => {
+      const props = {
+        disabled: false,
+        bubbleUp: jest.fn(),
+        value: {
+          input: ['normandy.clientId', 'normandy.request_time'],
+        },
+        defaultInputOptions: ['normandy.foo', 'normandy.man'],
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+      const FakeForm = createForm({})(InputsWidget);
+      const { getByText } = render(<FakeForm {...props} />);
+      const tag = getByText('normandy.clientId');
+      const closeIcon = tag.querySelector('.anticon-close');
+      fireEvent.click(closeIcon);
+      expect(props.bubbleUp).toHaveBeenCalledTimes(1);
+      expect(props.bubbleUp).toBeCalledWith({
+        input: ['normandy.request_time'],
+      });
+    });
+  });
+
+  describe('<SampingInput>', () => {
+    // Deliberately commented out for now.
+    // In Antd the <Select/> component is actually a special <div> construct and not a
+    // pretty version of regular <select><option/>..
+    // Because of that it's very hard to successfully manage to click and select something.
+    // Basically, I could never get this test to pass but I'm keeping the attempt for
+    // a day when this gets easier or we get more seasoned with Ant and
+    // react-testing-library.
+    // it('happy path with blank default inmput', () => {
+    //   const props = {
+    //     disabled: false,
+    //     onChange: jest.fn(),
+    //     value: {},
+    //   };
+    //   const FakeForm = createForm({})(SamplingInput);
+    //   const { container, getByLabelText, queryByText, getByText, debug } = render(
+    //     <FakeForm {...props} />,
+    //   );
+    //   fireEvent.click(container.querySelector('div.ant-select'));
+    //   // const bucketChoice = queryByText('Bucket');
+    //   fireEvent.click(queryByText('Bucket'));
+    //   debug();
+    //   // const option = queryByText('Bucket');
+    //   // const dropdown = queryByText('None');
+    //   // debug(dropdown);
+    //   // debug(dropdown.parentElement.parentElement);
+    //   // fireEvent.keyDown(dropdown);
+    // });
+
+    it('editing bucket sampling', () => {
+      const props = {
+        disabled: false,
+        onChange: jest.fn(),
+        value: {
+          type: 'bucketSample',
+          start: 50,
+          count: 70,
+          total: 90,
+        },
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+      const FakeForm = createForm({})(SamplingInput);
+      const { getByValue } = render(<FakeForm {...props} />);
+      fireEvent.change(getByValue('50'), {
+        target: { value: '55' },
+      });
+      expect(props.onChange).toHaveBeenCalledTimes(1);
+      expect(props.onChange).toBeCalledWith({
+        count: 70,
+        start: 55,
+        total: 90,
+        type: 'bucketSample',
+      });
+    });
+
+    it('editing stable sampling', () => {
+      const props = {
+        disabled: false,
+        onChange: jest.fn(),
+        value: {
+          type: 'stableSample',
+          rate: 0.3333,
+        },
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+      const FakeForm = createForm({})(SamplingInput);
+      const { getByValue } = render(<FakeForm {...props} />);
+      fireEvent.change(getByValue('33%'), {
+        target: { value: '75' },
+      });
+      expect(props.onChange).toHaveBeenCalledTimes(1);
+      expect(props.onChange).toBeCalledWith({
+        rate: 0.75,
+        type: 'stableSample',
+      });
+    });
+  });
+
+  describe('<VersionsInput>', () => {
+    it('edit by adding 1 version', () => {
+      const props = {
+        disabled: false,
+        onChange: jest.fn(),
+        value: [],
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+
+      const FakeForm = createForm({})(VersionsInput);
+      const { getByValue, container } = render(<FakeForm {...props} />);
+
+      // If you enter some junk the submit button will be disabled.
+      const firstInput = getByValue('');
+      fireEvent.change(firstInput, {
+        target: { value: 'junk' },
+      });
+      const button = container.querySelector('button[type="button"]');
+      expect(button.disabled).toBeTruthy();
+      fireEvent.click(button);
+      expect(props.onChange).toHaveBeenCalledTimes(0);
+
+      // Now put something acceptable in
+      fireEvent.change(firstInput, {
+        target: { value: '75' },
+      });
+      expect(button.disabled).toBeFalsy();
+      fireEvent.click(button);
+      expect(props.onChange).toHaveBeenCalledTimes(1);
+      expect(props.onChange).toBeCalledWith([75]);
+    });
+
+    it('edit by adding a range of versions', () => {
+      const props = {
+        disabled: false,
+        onChange: jest.fn(),
+        value: [],
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+
+      const FakeForm = createForm({})(VersionsInput);
+      const { getByValue, container } = render(<FakeForm {...props} />);
+
+      const changeRadioButton = getByValue('range');
+      fireEvent.click(changeRadioButton);
+
+      const firstInput = getByValue('');
+      fireEvent.change(firstInput, {
+        target: { value: '75' },
+      });
+      fireEvent.blur(firstInput);
+      const button = container.querySelector('button[type="button"]');
+      // The submit button is now *not* disabled, because typing something into the
+      // first number will automatically suggest something in the second input.
+      const secondInput = getByValue('76'); // getting this proves it was populated
+      expect(button.disabled).toBeFalsy();
+
+      // Let's put in something that is not right. A number *less* than the first one.
+      fireEvent.change(secondInput, {
+        target: { value: '70' },
+      });
+      expect(button.disabled).toBeTruthy();
+
+      // Be more sensible.
+      fireEvent.change(secondInput, {
+        target: { value: '77' },
+      });
+      expect(button.disabled).toBeFalsy();
+
+      fireEvent.click(button);
+      expect(props.onChange).toBeCalledWith([75, 76, 77]);
+    });
+
+    it('edit by removing a version', () => {
+      const props = {
+        disabled: false,
+        onChange: jest.fn(),
+        value: [67, 68, 69],
+        onSubmit: jest.fn(), // otherwise set by createForm()
+      };
+      const FakeForm = createForm({})(VersionsInput);
+      const { getByText } = render(<FakeForm {...props} />);
+      const tag = getByText('67');
+      const closeIcon = tag.querySelector('.anticon-close');
+      fireEvent.click(closeIcon);
+      expect(props.onChange).toBeCalledWith([68, 69]);
+    });
+  });
+});
diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
new file mode 100644
index 000000000..3af022b99
--- /dev/null
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -0,0 +1,803 @@
+/* eslint-disable react/jsx-boolean-value */
+import {
+  AutoComplete,
+  Button,
+  Checkbox,
+  Col,
+  InputNumber,
+  Radio,
+  Row,
+  Select,
+  Tabs,
+  Tag,
+} from 'antd';
+import { fromJS, List, Map } from 'immutable';
+import PropTypes from 'prop-types';
+import React from 'react';
+
+import QueryRecipeFilters from 'console/components/data/QueryRecipeFilters';
+import FormItem from 'console/components/forms/FormItem';
+
+// XXX Why is this needed?!?
+const TabsPane = Tabs.TabPane;
+const CheckboxGroup = Checkbox.Group;
+const RadioGroup = Radio.Group;
+const Option = Select.Option;
+
+// Every filter object is an Map (in an List) that looks something like this:
+//  {"type": "channel", "channels": ["beta"]}
+// We need a mapping from "channel" to "channels", for each, to know what to
+// call it when we serialize this list into a Map where entries look like this:
+//  {"channels": ["beta"]}
+// It is not safe to assume that the "type" can just be the plural form of the word.
+export const MAPPING = {
+  channel: 'channels',
+  country: 'countries',
+  locale: 'locales',
+  version: 'versions',
+};
+
+// In the UI we present the labels but in the data structure we store it by its value.
+// This mapping needs to be ordered to generate the <Select><Option/><Option/>... components.
+// Also, this is used to *display* the human readable version of each type when viewing
+// the details of the filter object.
+export const SAMPLING_TYPES = [
+  { value: 'bucketSample', label: 'Bucket' },
+  { value: 'stableSample', label: 'Stable' },
+  { value: '', label: 'None' },
+];
+
+/* Return a Map from a List.
+  Each item in the list is expected to be something like this:
+
+      [{"type": "age", "ages": [1,2,3]}, ]
+
+  Which is then transformed to this:
+
+      {"ages": [1,2,3]}
+
+  The list of valid keys needs to be known in advance. */
+export const serializeFilterObjectToMap = list => {
+  if (!list) {
+    return Map();
+  }
+  if (!List.isList(list)) {
+    throw new Error(`'${list}' (${typeof list}) is not a List`);
+  }
+
+  return Map(
+    list.map(item => {
+      const type = item.get('type');
+      if (type === 'bucketSample' || type === 'stableSample') {
+        return ['_sampling', item];
+      }
+      const name = MAPPING[type];
+      if (name === undefined) {
+        throw new Error(`Not sure how to convert '${type}'`);
+      }
+      return [name, item.get(name)];
+    }),
+  );
+};
+
+/* Return a List from a Map.
+  Each item in the map is expected to be key'ed based on the reverse of the MAPPING
+  constant. For example, from a list like this:
+
+       {locales: ["sv", "en-US"], ...}
+
+  Transforms to this:
+
+    [{type: "locale", locales: ["sv", "en-US"]}, ...]
+
+  The list of keys have to be known in advance. */
+export const deserializeFilterObjectToList = obj => {
+  if (!obj) {
+    return List();
+  }
+  const reverseMapping = Object.assign(
+    {},
+    ...Object.entries(MAPPING).map(([key, value]) => ({ [value]: key })),
+  );
+  return fromJS(
+    Object.entries(obj)
+      .filter(([key, value]) => {
+        return value && ((key === '_sampling' && value.type) || !!value.length);
+      })
+      .map(([key, value]) => {
+        if (key === '_sampling') {
+          // Special one that needs to be rewritten based on all the keys in the value.
+          // The 'value' should have
+          return Object.assign({}, value);
+        } else {
+          const name = reverseMapping[key];
+          if (name === undefined) {
+            throw new Error(`Key '${key}' no in known mapping.`);
+          }
+          return { [key]: value, type: name };
+        }
+      }),
+  );
+};
+
+class FilterObjectForm extends React.PureComponent {
+  static propTypes = {
+    allChannels: PropTypes.instanceOf(List),
+    allCountries: PropTypes.instanceOf(List),
+    allLocales: PropTypes.instanceOf(List),
+    disabled: PropTypes.bool,
+    filterObject: PropTypes.instanceOf(Map),
+  };
+
+  static defaultProps = {
+    allChannels: new List(),
+    allCountries: new List(),
+    allLocales: new List(),
+  };
+
+  allChannelOptions = () => {
+    return this.props.allChannels
+      .map(channel => {
+        return { label: channel.get('value'), value: channel.get('key') };
+      })
+      .toArray();
+  };
+
+  rememberActiveTabKey = key => {
+    window.sessionStorage.setItem('filterObjectActiveTabKey', key);
+  };
+
+  getDefaultActiveTabKey = (default_ = 'geo') => {
+    return window.sessionStorage.getItem('filterObjectActiveTabKey') || default_;
+  };
+
+  checkVersions = (rule, value, callback) => {
+    // Not doing anything here. The trust of validation is ultimately inside the VersionsInput
+    // component *and* the server-side checking.
+    // This method is remains as a stub in case we later decide to add some last-minute
+    // validation and by leaving it we remind ourselves how this magic with
+    // getFieldDecorator stuff works.
+    callback();
+  };
+
+  checkSampling = (rule, value, callback) => {
+    // XXX nothing at the moment
+    callback();
+  };
+
+  filterOption = (inputValue, option) => {
+    // Rather tongue-twistingly, the option.props.value is the "key" (e.g. "en-US")
+    // and "name" is "English". That is because of the nomenclature of Antd Select and the
+    // fact that "key" is a React attribute to distinguish nodes.
+    // Return true if @inputValue is in either. In a smart way.
+    const regex = makeAutocompleteRegex(inputValue);
+    return regex.test(option.props.value) || regex.test(option.props.name);
+  };
+
+  getSettingsCounts = () => {
+    // Return an object keyed by 'geo', 'browser', and 'sampling' that counts how many
+    // distinct settings have been made there.
+
+    const counts = {
+      geo: 0,
+      browser: 0,
+      sampling: 0,
+    };
+    const { getFieldValue } = this.props.form;
+
+    function any(key) {
+      const value = getFieldValue(`filter_object.${key}`);
+      // 'value' here is either undefined, a JS object or a JS array.
+      // The task is to conclude if it's truthy.
+      // XXX Is there a better utility function for this?
+      if (value) {
+        return (Array.isArray(value) && !!value.length) || !!Object.keys(value).length;
+      }
+      return false;
+    }
+    any('locales') && counts.geo++;
+    any('countries') && counts.geo++;
+    any('channels') && counts.browser++;
+    any('versions') && counts.browser++;
+    any('_sampling') && counts.sampling++;
+    return counts;
+  };
+
+  render() {
+    const { disabled, filterObject } = this.props;
+    const { getFieldDecorator } = this.props.form;
+
+    // This is a simple object for counting how many settings have been set per tab.
+    const countSettings = this.getSettingsCounts();
+
+    const initialLocales = filterObject.get('locales', List()).toArray();
+    const initialCountries = filterObject.get('countries', List()).toArray();
+    const initialChannels = filterObject.get('channels', List()).toArray();
+    const initialVersions = filterObject.get('versions', List()).toArray();
+    const initialSampling = filterObject.get('_sampling', Map()).toJS();
+
+    return (
+      <div>
+        <QueryRecipeFilters />
+
+        <Tabs
+          defaultActiveKey={this.getDefaultActiveTabKey()}
+          onChange={this.rememberActiveTabKey}
+        >
+          <TabsPane tab={countSettings.geo ? `Geo (${countSettings.geo})` : 'Geo'} key="geo">
+            <Row gutter={16} type="flex">
+              <Col span={12}>
+                <FormItem
+                  label="Locale"
+                  name="filter_object.locales"
+                  required={false}
+                  rules={[{ required: false }]}
+                  initialValue={initialLocales}
+                >
+                  <Select
+                    disabled={disabled}
+                    mode="multiple"
+                    allowClear
+                    showArrow={false}
+                    filterOption={this.filterOption}
+                  >
+                    {this.props.allLocales.map(locale => {
+                      return (
+                        <Select.Option
+                          key={locale.get('key')}
+                          value={locale.get('key')}
+                          name={locale.get('value')}
+                        >
+                          {locale.get('key')} ({locale.get('value')})
+                        </Select.Option>
+                      );
+                    })}
+                  </Select>
+                </FormItem>
+                {/* This little spacer is necessary so that the row extends all the way. */}
+                <p> </p>
+              </Col>
+              <Col span={12}>
+                <FormItem
+                  label="Country"
+                  name="filter_object.countries"
+                  required={false}
+                  rules={[{ required: false }]}
+                  initialValue={initialCountries}
+                >
+                  <Select
+                    disabled={disabled}
+                    mode="multiple"
+                    allowClear
+                    showArrow={false}
+                    filterOption={this.filterOption}
+                  >
+                    {this.props.allCountries.map(country => {
+                      return (
+                        <Select.Option
+                          key={country.get('key')}
+                          value={country.get('key')}
+                          name={country.get('value')}
+                        >
+                          {country.get('value')} ({country.get('key')})
+                        </Select.Option>
+                      );
+                    })}
+                  </Select>
+                </FormItem>
+              </Col>
+            </Row>
+          </TabsPane>
+          <TabsPane
+            tab={countSettings.browser ? `Browser (${countSettings.browser})` : 'Browser'}
+            key="browser"
+          >
+            <Row gutter={16}>
+              <Col span={12}>
+                <FormItem
+                  label="Channel"
+                  name="filter_object.channels"
+                  required={false}
+                  rules={[{ required: false }]}
+                  initialValue={initialChannels}
+                >
+                  <CheckboxGroup disabled={disabled} options={this.allChannelOptions()} />
+                </FormItem>
+              </Col>
+              <Col span={12}>
+                <FormItem
+                  label="Version"
+                  name="filter_object.versions"
+                  required={false}
+                  connectToForm={false}
+                >
+                  {getFieldDecorator('filter_object.versions', {
+                    initialValue: initialVersions,
+                    rules: [{ validator: this.checkVersions }],
+                  })(<VersionsInput disabled={disabled} />)}
+                </FormItem>
+              </Col>
+            </Row>
+          </TabsPane>
+          <TabsPane
+            tab={countSettings.sampling ? `Sampling (${countSettings.sampling})` : 'Sampling'}
+            key="sampling"
+          >
+            <FormItem
+              label=""
+              name="filter_object._sampling"
+              required={false}
+              connectToForm={false}
+            >
+              {getFieldDecorator('filter_object._sampling', {
+                initialValue: initialSampling,
+                rules: [{ validator: this.checkSampling }],
+              })(<SamplingInput disabled={disabled} />)}
+            </FormItem>
+          </TabsPane>
+        </Tabs>
+      </div>
+    );
+  }
+}
+
+export default FilterObjectForm;
+
+/**
+ * Return an integer or null. Parse it if you have to.
+ */
+export function parseIntOrNull(number) {
+  if (typeof number === 'string') {
+    // The problem with parseInt() is that `parseInt('1x2')` becomes `1` and not NaN.
+    const asInteger = parseInt(number, 10);
+    return (!isNaN(asInteger) && number === asInteger.toString() && asInteger) || null;
+  } else if (typeof number === 'number') {
+    // Then it's either an integer or a floating point.
+    // But it's 123.0 then just return 123 and for 123.1 return null;
+    return ((number | 0) === number && number) || null;
+  }
+  return null;
+}
+
+export class VersionsInput extends React.PureComponent {
+  MIN_VERSION = 45;
+  INPUT_TYPE_SPECIFIC = 'specific';
+  INPUT_TYPE_RANGE = 'range';
+
+  static propTypes = {
+    disabled: PropTypes.bool,
+    onChange: PropTypes.func,
+    value: PropTypes.array,
+  };
+
+  state = {
+    newVersions: [],
+    number1: null,
+    number2: null,
+    probablyValid: false,
+    versionInputType: this.INPUT_TYPE_SPECIFIC,
+  };
+
+  onChangeVersionInputType = event => {
+    this.setState({ versionInputType: event.target.value });
+  };
+
+  onChangeNumber1 = value => {
+    this.setState({ number1: parseIntOrNull(value) }, this.toggleProbablyValid);
+  };
+  onChangeNumber2 = value => {
+    this.setState({ number2: parseIntOrNull(value) }, this.toggleProbablyValid);
+  };
+
+  toggleProbablyValid = () => {
+    // Remember, these two numbers are definitely integers or null.
+    const { number1, number2 } = this.state;
+    const existingVersions = this.props.value;
+    const isSaneNumber = number => {
+      return number !== null && number >= this.MIN_VERSION;
+    };
+    let probablyValid = false; // assume the worst
+    if (this.state.versionInputType === this.INPUT_TYPE_RANGE) {
+      // The second number must be greater than the first number.
+      probablyValid =
+        isSaneNumber(number1) &&
+        isSaneNumber(number2) &&
+        number2 > number1 &&
+        (number1 < Math.min(...existingVersions) || number2 > Math.max(...existingVersions));
+    } else {
+      // The first number just needs to be sane.
+      // But it can't be one of the existing ones.
+      probablyValid = isSaneNumber(number1) && !existingVersions.includes(number1);
+    }
+    this.setState({ probablyValid });
+  };
+
+  addInputVersions = event => {
+    let lastNumber = this.state.number1;
+    const existingVersions = this.props.value;
+    const range = [lastNumber];
+    while (this.state.number2 !== null && lastNumber++ < this.state.number2) {
+      range.push(lastNumber);
+    }
+    // Merge the range being asked to add with the existing versions.
+    const merged = [...new Set([...existingVersions, ...range])].sort();
+    this.setState({ number1: '', number2: '', probablyValid: false }, () => {
+      this.props.onChange(merged);
+    });
+  };
+
+  removeVersion = version => {
+    const versions = this.props.value.filter(v => v !== version);
+    this.props.onChange(versions);
+  };
+
+  render() {
+    const { disabled } = this.props;
+
+    return (
+      <div>
+        <RadioGroup
+          disabled={disabled}
+          onChange={this.onChangeVersionInputType}
+          value={this.state.versionInputType}
+        >
+          <Radio value={this.INPUT_TYPE_SPECIFIC}>Specific</Radio>
+          <Radio value={this.INPUT_TYPE_RANGE}>Range</Radio>
+        </RadioGroup>
+        <div>
+          <InputNumber
+            min={this.MIN_VERSION}
+            disabled={disabled}
+            onChange={this.onChangeNumber1}
+            value={this.state.number1 || ''}
+            title={`Must be an integer number of at least ${this.MIN_VERSION}`}
+            onBlur={() => {
+              if (
+                this.state.number1 &&
+                this.state.versionInputType === this.INPUT_TYPE_RANGE &&
+                !this.state.number2
+              ) {
+                // Suggest a number of the second input right away.
+                this.setState({ number2: this.state.number1 + 1 }, this.toggleProbablyValid);
+              }
+            }}
+          />
+          {this.state.versionInputType === this.INPUT_TYPE_RANGE ? (
+            <InputNumber
+              min={this.MIN_VERSION}
+              disabled={disabled}
+              value={this.state.number2 || ''}
+              onChange={this.onChangeNumber2}
+              title={`Must be an integer number of at least ${
+                this.MIN_VERSION
+              } and greater than the other number`}
+            />
+          ) : null}{' '}
+          <Button
+            type="primary"
+            shape="circle"
+            disabled={disabled || !this.state.probablyValid}
+            icon="plus"
+            onClick={this.addInputVersions}
+          />
+        </div>
+
+        {this.props.value.map(version => {
+          return (
+            <Tag closable={!disabled} key={version} onClose={() => this.removeVersion(version)}>
+              {version}
+            </Tag>
+          );
+        })}
+      </div>
+    );
+  }
+}
+
+export class SamplingInput extends React.PureComponent {
+  static propTypes = {
+    disabled: PropTypes.bool,
+    value: PropTypes.object,
+    // The reason this can't be `.isRequired` (even though it is!) is because
+    // getFieldDecorator is weird. It never actually calls the component without first
+    // adding the extra props `onChange` and `value` but because you spell it out as
+    // the argument to the partial call of getFieldDecorator you get prop type warnings.
+    // See https://codesandbox.io/s/r51k0y1y5p for an exanple. If you watch the console
+    // you should see a `Failed prop type: The prop `onChange` is marked as required...`
+    // but the component is actually never rendered without that prop.
+    // Just weird! That's why these implicit props are NOT required.
+    onChange: PropTypes.func,
+  };
+
+  bubbleUp = newData => {
+    this.props.onChange(Object.assign({}, this.props.value, newData));
+  };
+
+  typeChange = type => {
+    if (type) {
+      // If you switched input type, it's highly likely that you want to keep the
+      // inputs you had earlier. Bold assumption.
+      const defaultInputs = this.props.value.input || [];
+      this.props.onChange({ type, input: defaultInputs });
+    } else {
+      this.props.onChange({});
+    }
+  };
+
+  formItemLayout = {
+    labelCol: { span: 4 },
+    wrapperCol: { span: 14 },
+  };
+
+  renderBucketInputs = () => {
+    const { disabled, value } = this.props;
+
+    return (
+      <div>
+        <FormItem
+          label="Start"
+          name="_sampling.start"
+          required={true}
+          connectToForm={false}
+          {...this.formItemLayout}
+        >
+          <InputNumber
+            min={0}
+            disabled={disabled}
+            onChange={value => {
+              this.bubbleUp({ start: value });
+            }}
+            value={value.start || ''}
+          />
+        </FormItem>
+        <FormItem
+          label="Count"
+          name="_sampling.count"
+          required={true}
+          connectToForm={false}
+          {...this.formItemLayout}
+        >
+          <InputNumber
+            min={0}
+            disabled={disabled}
+            onChange={value => {
+              this.bubbleUp({ count: value });
+            }}
+            value={value.count || ''}
+          />
+        </FormItem>
+        <FormItem
+          label="Total"
+          name="_sampling.total"
+          required={true}
+          connectToForm={false}
+          {...this.formItemLayout}
+        >
+          <InputNumber
+            min={0}
+            disabled={disabled}
+            onChange={value => {
+              this.bubbleUp({ total: value });
+            }}
+            value={value.total || ''}
+          />
+        </FormItem>
+
+        <InputsWidget
+          disabled={disabled}
+          value={value}
+          bubbleUp={this.bubbleUp}
+          formItemLayout={this.formItemLayout}
+        />
+      </div>
+    );
+  };
+
+  renderStableInputs = () => {
+    const { disabled, value } = this.props;
+
+    return (
+      <div>
+        <FormItem
+          label="Rate"
+          name="_sampling.rate"
+          required={true}
+          connectToForm={false}
+          {...this.formItemLayout}
+        >
+          <InputNumber
+            min={0}
+            max={100}
+            disabled={disabled}
+            title="Value between 1 and 100"
+            formatter={value => `${value}%`}
+            parser={value => value.replace('%', '')}
+            onChange={value => {
+              this.bubbleUp({ rate: value / 100 });
+            }}
+            value={(value.rate && Math.trunc(value.rate * 100)) || ''}
+          />
+        </FormItem>
+        <InputsWidget
+          disabled={disabled}
+          value={value}
+          bubbleUp={this.bubbleUp}
+          formItemLayout={this.formItemLayout}
+        />
+      </div>
+    );
+  };
+
+  render() {
+    const { disabled } = this.props;
+    return (
+      <div>
+        <FormItem
+          label="Type"
+          name="_sampling.type"
+          required={false}
+          connectToForm={false}
+          {...this.formItemLayout}
+        >
+          <Select
+            disabled={disabled}
+            style={{ width: 200 }}
+            value={this.props.value.type || ''}
+            onChange={this.typeChange}
+          >
+            {SAMPLING_TYPES.map(samplingType => {
+              return (
+                <Option value={samplingType.value} key={samplingType.value}>
+                  {samplingType.label}
+                </Option>
+              );
+            })}
+          </Select>
+        </FormItem>
+        {this.props.value.type === 'bucketSample' ? this.renderBucketInputs() : null}
+        {this.props.value.type === 'stableSample' ? this.renderStableInputs() : null}
+      </div>
+    );
+  }
+}
+
+function makeAutocompleteRegex(value) {
+  const escaped = value.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');
+  return new RegExp(`\\b${escaped}`, 'gi');
+}
+
+export function rewriteSamplingInput(input, defaultOptions) {
+  // If you chosen your own input and ignored any of the suggestions and that input is
+  // not a number or something that looks like an identifier, assume it to a string that
+  // needs to be quoted.
+  if (
+    !defaultOptions.includes(input) &&
+    isNaN(parseInt(input, 10)) &&
+    (/\s/.test(input) || !/\./.test(input))
+  ) {
+    input = `"${input}"`;
+  }
+  return input;
+}
+
+export class InputsWidget extends React.PureComponent {
+  state = {
+    inputSearch: '',
+    probablyValid: false,
+  };
+
+  static propTypes = {
+    disabled: PropTypes.bool,
+    bubbleUp: PropTypes.func.isRequired,
+    value: PropTypes.object,
+    formItemLayout: PropTypes.object.isRequired,
+    defaultInputOptions: PropTypes.array,
+  };
+
+  static defaultProps = {
+    formItemLayout: {
+      labelCol: { span: 4 },
+      wrapperCol: { span: 14 },
+    },
+    // XXX THIS LIST IS BASED ON SCRAPING https://normandy.cdn.mozilla.net/api/v1/recipe/
+    // AND IT'S VERY DIFFERENT FROM THE AVAILABLE OPTIONS HERE:
+    // https://normandy.readthedocs.io/en/latest/user/filters.html#filter-context
+    defaultInputOptions: [
+      'normandy.clientId',
+      'normandy.recipe.id',
+      'normandy.request_time',
+      'normandy.userId',
+    ],
+  };
+
+  onSelectSamplingInput = value => {
+    if (!isNaN(parseInt(value, 10))) {
+      value = parseInt(value, 10);
+    }
+    const inputs = this.props.value.input || [];
+    inputs.push(value);
+    inputs.sort((inputA, inputB) => {
+      const smallA = (typeof inputA === 'string' && inputA.toLowerCase()) || inputA.toString();
+      const smallB = (typeof inputB === 'string' && inputB.toLowerCase()) || inputB.toString();
+      if (smallA > smallB) {
+        return 1;
+      } else if (smallA < smallB) {
+        return -1;
+      }
+      return 0;
+    });
+    this.props.bubbleUp({ input: inputs });
+    this.setState({ inputSearch: '', probablyValid: false });
+  };
+
+  onSubmitSamplingInput = () => {
+    // The function rewriteSamplingInput might change the input to make it look like a
+    // string string. I.e. if the input is 'foobar' the output becomes '"foobar"'.
+    this.onSelectSamplingInput(
+      rewriteSamplingInput(this.state.inputSearch.trim(), this.props.defaultInputOptions),
+    );
+  };
+
+  removeInput = value => {
+    const inputs = this.props.value.input.filter(input => input !== value);
+    this.props.bubbleUp({ input: inputs });
+  };
+
+  onSearchInput = value => {
+    // Reason for keeping this as local state is to be able to clear it when something
+    // is chosen.
+    this.setState({ inputSearch: value, probablyValid: !!value.trim() });
+  };
+
+  filterOption = (inputValue, option) => {
+    const regex = makeAutocompleteRegex(inputValue);
+    return regex.test(option.props.children);
+  };
+
+  render() {
+    const { value, disabled } = this.props;
+    const inputs = value.input || [];
+    const inputOptions = this.props.defaultInputOptions.filter(opt => {
+      return !inputs.includes(opt);
+    });
+    return (
+      <FormItem
+        label="Input"
+        name="_sampling.input"
+        required={true}
+        connectToForm={false}
+        {...this.props.formItemLayout}
+      >
+        <AutoComplete
+          dataSource={inputOptions}
+          style={{ width: 200 }}
+          onSelect={this.onSelectSamplingInput}
+          onChange={this.onSearchInput}
+          disabled={disabled}
+          value={this.state.inputSearch}
+          filterOption={this.filterOption}
+          allowClear={true}
+        />
+        <Button
+          type="primary"
+          shape="circle"
+          disabled={disabled || !this.state.probablyValid}
+          icon="plus"
+          onClick={this.onSubmitSamplingInput}
+          // Needed to be able to find button DOM in testing.
+          // https://github.com/kentcdodds/dom-testing-library/issues/108
+          // data-testid="add-button"
+        />
+
+        <br />
+        {inputs.map(input => {
+          return (
+            <Tag closable={!disabled} key={input} onClose={() => this.removeInput(input)}>
+              {input}
+            </Tag>
+          );
+        })}
+      </FormItem>
+    );
+  }
+}
diff --git a/src/workflows/recipes/components/RecipeDetails.js b/src/workflows/recipes/components/RecipeDetails.js
index 2313b8664..681b77038 100644
--- a/src/workflows/recipes/components/RecipeDetails.js
+++ b/src/workflows/recipes/components/RecipeDetails.js
@@ -1,9 +1,14 @@
-import { Card, Icon, Tag, Tooltip } from 'antd';
+import { Card, Icon, Table, Tag, Tooltip } from 'antd';
 import { List, Map } from 'immutable';
 import PropTypes from 'prop-types';
 import React from 'react';
 import CopyToClipboard from 'react-copy-to-clipboard';
 
+import {
+  serializeFilterObjectToMap,
+  SAMPLING_TYPES,
+} from 'console/workflows/recipes/components/FilterObjectForm';
+
 export default class RecipeDetails extends React.PureComponent {
   static propTypes = {
     recipe: PropTypes.instanceOf(Map).isRequired,
@@ -12,7 +17,8 @@ export default class RecipeDetails extends React.PureComponent {
   render() {
     const { recipe } = this.props;
     const actionName = recipe.getIn(['action', 'name']);
-
+    const filterObject = recipe.get('filter_object');
+    const extraFilterExpression = recipe.get('extra_filter_expression');
     return (
       <div className="recipe-details">
         <Card className="noHovering" key="recipe-details" title="Recipe">
@@ -20,10 +26,17 @@ export default class RecipeDetails extends React.PureComponent {
             <dt>Name</dt>
             <ArgumentsValue name="name" value={recipe.get('name')} />
 
-            <dt>Filters</dt>
+            {filterObject && filterObject.size ? <dt>Filters</dt> : null}
+            {filterObject && filterObject.size ? (
+              <ArgumentsValue name="filter_object" value={recipe.get('filter_object')} />
+            ) : null}
+
+            <dt>
+              {filterObject && filterObject.size ? 'Extra Filter Expression' : 'Filter Expression'}
+            </dt>
             <ArgumentsValue
               name="extra_filter_expression"
-              value={recipe.get('extra_filter_expression')}
+              value={extraFilterExpression ? extraFilterExpression : <em>none</em>}
             />
           </dl>
         </Card>
@@ -131,6 +144,113 @@ export class ArgumentsValue extends React.PureComponent {
     );
   }
 
+  renderFilterObject(values) {
+    if (!values) {
+      return null;
+    }
+
+    // The reason for hardcoding this ordered list of keys is so that it more resembles
+    // the order of which these inputs appear when editing.
+    const KEYS = [
+      ['locales', 'Locale'],
+      ['countries', 'Country'],
+      ['channels', 'Channel'],
+      ['versions', 'Version'],
+      ['_sampling', 'Sampling'],
+    ];
+    const samplingTypes = {};
+    for (const { value, label } of SAMPLING_TYPES) {
+      samplingTypes[value] = label;
+    }
+    const serialized = serializeFilterObjectToMap(values);
+    const dataSource = [];
+    const columns = [
+      {
+        title: 'Name',
+        dataIndex: 'name',
+      },
+      {
+        title: 'Value(s)',
+        dataIndex: 'value',
+      },
+    ];
+    for (const [key, label] of KEYS) {
+      const value = serialized.get(key);
+      // All others except special '_sampling' is a List instance.
+      if (value && (key === '_sampling' || value.size)) {
+        if (key === '_sampling') {
+          const samplingColumns = [
+            { title: 'Name', dataIndex: 'name' },
+            { title: 'Value', dataIndex: 'value' },
+          ];
+          const samplingType = value.get('type');
+          const samplingSource = [
+            { key: 'type', name: 'Type', value: samplingTypes[samplingType] },
+          ];
+          if (samplingType === 'stableSample') {
+            samplingSource.push({
+              key: 'rate',
+              name: 'Rate',
+              value: Math.trunc(value.get('rate') * 100) + '%',
+            });
+          } else if (samplingType === 'bucketSample') {
+            samplingSource.push({
+              key: 'start',
+              name: 'Start',
+              value: value.get('start'),
+            });
+            samplingSource.push({
+              key: 'count',
+              name: 'Count',
+              value: value.get('count'),
+            });
+            samplingSource.push({
+              key: 'total',
+              name: 'Total',
+              value: value.get('total'),
+            });
+          } else {
+            throw new Error(`Unrecognized sampling type (${samplingType})`);
+          }
+          samplingSource.push({
+            key: 'input',
+            name: 'Input',
+            value: <code>{value.get('input').join(', ')}</code>,
+          });
+          dataSource.push({
+            key: key,
+            name: label,
+            value: (
+              <Table
+                dataSource={samplingSource}
+                columns={samplingColumns}
+                pagination={false}
+                showHeader={false}
+                size="middle"
+              />
+            ),
+          });
+        } else {
+          dataSource.push({
+            key: key,
+            name: label,
+            value: <code>{value.join(', ')}</code>,
+          });
+        }
+      }
+    }
+    return (
+      <Table
+        dataSource={dataSource}
+        columns={columns}
+        pagination={false}
+        showHeader={false}
+        bordered={false}
+        size="middle"
+      />
+    );
+  }
+
   renderCode(code) {
     return (
       <pre>
@@ -147,9 +267,13 @@ export class ArgumentsValue extends React.PureComponent {
     const { name, actionName, value } = this.props;
 
     let valueRender = x => (typeof x === 'object' ? JSON.stringify(x, null, 2) : x);
+    let argumentsValueClassName = 'arguments-value';
 
     if (name === 'branches') {
       valueRender = this.renderBranchTable;
+    } else if (name === 'filter_object') {
+      valueRender = this.renderFilterObject;
+      argumentsValueClassName += ' no-padding';
     } else if (name === 'extra_filter_expression') {
       valueRender = this.renderCode;
     } else if (typeof value === 'boolean') {
@@ -164,13 +288,15 @@ export class ArgumentsValue extends React.PureComponent {
     }
 
     return (
-      <dd className="arguments-value">
+      <dd className={argumentsValueClassName}>
         <div className="value">{valueRender(value)}</div>
-        <Tooltip mouseEnterDelay={1} title="Copy to Clipboard" placement="top">
-          <CopyToClipboard className="copy-icon" text={textToCopy}>
-            <Icon type="copy" />
-          </CopyToClipboard>
-        </Tooltip>
+        {value ? (
+          <Tooltip mouseEnterDelay={1} title="Copy to Clipboard" placement="top">
+            <CopyToClipboard className="copy-icon" text={textToCopy}>
+              <Icon type="copy" />
+            </CopyToClipboard>
+          </Tooltip>
+        ) : null}
       </dd>
     );
   }
diff --git a/src/workflows/recipes/components/RecipeForm.js b/src/workflows/recipes/components/RecipeForm.js
index 2ab09694a..38416fb7c 100644
--- a/src/workflows/recipes/components/RecipeForm.js
+++ b/src/workflows/recipes/components/RecipeForm.js
@@ -9,6 +9,10 @@ import FormItem from 'console/components/forms/FormItem';
 import FormActions from 'console/components/forms/FormActions';
 import ActionSelect from 'console/workflows/recipes/components/ActionSelect';
 import ConsoleLogFields from 'console/workflows/recipes/components/ConsoleLogFields';
+import FilterObjectForm, {
+  serializeFilterObjectToMap,
+  deserializeFilterObjectToList,
+} from 'console/workflows/recipes/components/FilterObjectForm';
 import JSONArgumentsField from 'console/workflows/recipes/components/JSONArgumentsField';
 import PreferenceExperimentFields from 'console/workflows/recipes/components/PreferenceExperimentFields';
 import PreferenceRolloutFields, {
@@ -17,6 +21,7 @@ import PreferenceRolloutFields, {
 import ShowHeartbeatFields from 'console/workflows/recipes/components/ShowHeartbeatFields';
 import OptOutStudyFields from 'console/workflows/recipes/components/OptOutStudyFields';
 import { getAction } from 'console/state/actions/selectors';
+import { getRecipeFilters } from 'console/state/recipes/selectors';
 import { areAnyRequestsInProgress } from 'console/state/network/selectors';
 import { createForm } from 'console/utils/forms';
 import IdenticonField from 'console/components/forms/IdenticonField';
@@ -35,6 +40,16 @@ export function cleanRecipeData(data) {
     // field. In that case, rewrite the list of rows into an expected list of preferences.
     data.arguments.preferences = deserializePreferenceRows(data.arguments.preferences);
   }
+  // Have to turn the filter_object object into an array.
+  // When submitted it's an object like `{locales: ['sv']}` and it needs to become:
+  // `[{type: 'locale': locales: ['sv']}]` for example.
+  data.filter_object = deserializeFilterObjectToList(data.filter_object);
+  if (!data.filter_object.size && !data.extra_filter_expression) {
+    // You can't submit a recipe with no extra filter expression and no filter object rules.
+    // Error thrown here will be automatically turned into a notification message in the UI.
+    throw new Error('Have you have at least one filter object or a filter expression.');
+  }
+  // console.log('DATA.filter_object', JSON.stringify(data.filter_object));
 
   // Make sure the action ID is an integer
   try {
@@ -57,11 +72,13 @@ export function cleanRecipeData(data) {
   return {
     selectedActionName: selectedAction.get('name'),
     isLoading: areAnyRequestsInProgress(state),
+    filters: getRecipeFilters(state, new Map()),
   };
 })
 @autobind
 class RecipeForm extends React.PureComponent {
   static propTypes = {
+    filters: PropTypes.object,
     form: PropTypes.object.isRequired,
     isLoading: PropTypes.bool,
     isCreationForm: PropTypes.bool,
@@ -121,13 +138,20 @@ class RecipeForm extends React.PureComponent {
   }
 
   render() {
-    const { isCreationForm, isLoading, onSubmit, recipe } = this.props;
+    const { filters, isCreationForm, isLoading, onSubmit, recipe } = this.props;
     const { defaultIdenticonSeed } = this.state;
 
     // If creating, the 'default' seed is randomly generated. We store it in memory
     // to prevent the form from generating a new identicon on each render.
     const identiconSeed = isCreationForm ? defaultIdenticonSeed : null;
 
+    // The recipe object has an **array of objects** which we need to turn around
+    // to an object of keys and values. For example,
+    //   [{type: 'country', countries:['sv']}, ...]
+    // becomes
+    //   {countries: ['sv'], ...}
+    const filterObject = serializeFilterObjectToMap(recipe.get('filter_object'));
+
     return (
       <Form onSubmit={onSubmit} className="recipe-form">
         <Row gutter={16}>
@@ -147,9 +171,20 @@ class RecipeForm extends React.PureComponent {
           </Col>
         </Row>
 
+        <FilterObjectForm
+          form={this.props.form}
+          disabled={isLoading}
+          filterObject={filterObject}
+          allLocales={filters.get('locales')}
+          allCountries={filters.get('countries')}
+          allChannels={filters.get('channels')}
+        />
+
         <FormItem
           name="extra_filter_expression"
-          label="Filter Expression"
+          label="Additional Filter Expression"
+          required={false}
+          rules={[{ required: false }]}
           initialValue={recipe.get('extra_filter_expression')}
         >
           <Input.TextArea disabled={isLoading} rows={4} />
diff --git a/yarn.lock b/yarn.lock
index 3941b8b9b..b2ef51fb1 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -2855,6 +2855,14 @@ dom-serializer@0, dom-serializer@~0.1.0:
     domelementtype "~1.1.1"
     entities "~1.1.1"
 
+dom-testing-library@^3.1.0:
+  version "3.6.0"
+  resolved "https://registry.yarnpkg.com/dom-testing-library/-/dom-testing-library-3.6.0.tgz#e5598fae9477f3918e22aee44f2262727203b5bd"
+  dependencies:
+    mutationobserver-shim "^0.3.2"
+    pretty-format "^22.4.3"
+    wait-for-expect "^1.0.0"
+
 dom-urls@^1.1.0:
   version "1.1.0"
   resolved "https://registry.yarnpkg.com/dom-urls/-/dom-urls-1.1.0.tgz#001ddf81628cd1e706125c7176f53ccec55d918e"
@@ -7267,6 +7275,13 @@ pretty-error@^2.0.2:
     renderkid "^2.0.1"
     utila "~0.4"
 
+pretty-format@^22.4.3:
+  version "22.4.3"
+  resolved "http://registry.npmjs.org/pretty-format/-/pretty-format-22.4.3.tgz#f873d780839a9c02e9664c8a082e9ee79eaac16f"
+  dependencies:
+    ansi-regex "^3.0.0"
+    ansi-styles "^3.2.0"
+
 pretty-format@^23.6.0:
   version "23.6.0"
   resolved "https://registry.yarnpkg.com/pretty-format/-/pretty-format-23.6.0.tgz#5eaac8eeb6b33b987b7fe6097ea6a8a146ab5760"
@@ -8016,6 +8031,13 @@ react-test-renderer@^16.0.0-0:
     prop-types "^15.6.0"
     react-is "^16.4.1"
 
+react-testing-library@5.1.0:
+  version "5.1.0"
+  resolved "https://registry.yarnpkg.com/react-testing-library/-/react-testing-library-5.1.0.tgz#ae208d9e3b7faae233409d205e287b304ec8ffdc"
+  dependencies:
+    dom-testing-library "^3.1.0"
+    wait-for-expect "^1.0.0"
+
 react@16.5.2:
   version "16.5.2"
   resolved "https://registry.yarnpkg.com/react/-/react-16.5.2.tgz#19f6b444ed139baa45609eee6dc3d318b3895d42"
@@ -9867,6 +9889,10 @@ w3c-hr-time@^1.0.1:
   dependencies:
     browser-process-hrtime "^0.1.2"
 
+wait-for-expect@^1.0.0:
+  version "1.0.1"
+  resolved "https://registry.yarnpkg.com/wait-for-expect/-/wait-for-expect-1.0.1.tgz#73ab346ed56ed2ef66c380a59fd623755ceac0ce"
+
 walker@~1.0.5:
   version "1.0.7"
   resolved "https://registry.yarnpkg.com/walker/-/walker-1.0.7.tgz#2f7f9b8fd10d677262b18a884e28d19618e028fb"

From 73523264857985fde76dbfa2973fba706b58a671 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 2 Oct 2018 14:35:17 -0400
Subject: [PATCH 02/22] addin whatwg-fetch for ability to polyfill
 window.Headers

---
 package.json | 3 ++-
 yarn.lock    | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/package.json b/package.json
index 7bb31ccd2..a311f10bd 100644
--- a/package.json
+++ b/package.json
@@ -96,7 +96,8 @@
     "webpack": "4.20.2",
     "webpack-dev-server": "3.1.9",
     "webpack-manifest-plugin": "2.0.4",
-    "webpack-merge": "4.1.4"
+    "webpack-merge": "4.1.4",
+    "whatwg-fetch": "3.0.0"
   },
   "jest": {
     "collectCoverageFrom": [
diff --git a/yarn.lock b/yarn.lock
index b2ef51fb1..e1dea2234 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -10071,6 +10071,10 @@ whatwg-encoding@^1.0.1, whatwg-encoding@^1.0.3:
   dependencies:
     iconv-lite "0.4.19"
 
+whatwg-fetch@3.0.0:
+  version "3.0.0"
+  resolved "https://registry.yarnpkg.com/whatwg-fetch/-/whatwg-fetch-3.0.0.tgz#fc804e458cc460009b1a2b966bc8817d2578aefb"
+
 whatwg-fetch@>=0.10.0:
   version "2.0.4"
   resolved "https://registry.yarnpkg.com/whatwg-fetch/-/whatwg-fetch-2.0.4.tgz#dde6a5df315f9d39991aa17621853d720b85566f"

From 0328c4ed2c44e13d034105ed32e730d3331a552b Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 3 Oct 2018 10:49:14 -0400
Subject: [PATCH 03/22] feedbacked

---
 src/setupTests.js                             |  1 +
 .../components/FilterObjectForm.test.js       | 50 +++++++++----------
 .../recipes/components/FilterObjectForm.js    |  3 --
 .../recipes/components/RecipeForm.js          |  1 -
 4 files changed, 26 insertions(+), 29 deletions(-)

diff --git a/src/setupTests.js b/src/setupTests.js
index f39c9a033..2918d9f17 100644
--- a/src/setupTests.js
+++ b/src/setupTests.js
@@ -6,6 +6,7 @@ import Adapter from 'enzyme-adapter-react-16';
 import * as immutableMatchers from 'jest-immutable-matchers';
 import fetchMock from 'fetch-mock';
 import { Headers } from 'whatwg-fetch';
+
 // Configure Enzyme adapter
 configure({ adapter: new Adapter() });
 
diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
index eac652835..469c84a53 100644
--- a/src/tests/workflows/recipes/components/FilterObjectForm.test.js
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -1,4 +1,4 @@
-import { List, fromJS } from 'immutable';
+import { Map, List, fromJS } from 'immutable';
 import { render, fireEvent } from 'react-testing-library';
 import fetchMock from 'fetch-mock';
 import { NORMANDY_ADMIN_API_ROOT_URL } from 'console/settings';
@@ -15,7 +15,7 @@ import { createForm } from 'console/utils/forms';
 import { wrapMockStore } from 'console/tests/mockStore';
 
 describe('<FilterObjectForm>', () => {
-  it('render editing form with an empty existing filter object', () => {
+  it('should render the form with an empty existing filter object', () => {
     fetchMock.getOnce(`${NORMANDY_ADMIN_API_ROOT_URL}v3/filters/`, {
       channels: [
         {
@@ -56,7 +56,7 @@ describe('<FilterObjectForm>', () => {
     expect(fetchMock.called()).toBeTruthy();
   });
 
-  it('render editing form with a full existing filter object', () => {
+  it('should render the form with a full existing filter object', () => {
     fetchMock.getOnce(`${NORMANDY_ADMIN_API_ROOT_URL}v3/filters/`, {
       channels: [
         {
@@ -114,14 +114,13 @@ describe('<FilterObjectForm>', () => {
     expect(fetchMock.called()).toBeTruthy();
   });
 
-  describe('<serializeFilterObjectToMap>', () => {
-    it('serialize an empty List to an empty Map', () => {
+  describe('serializeFilterObjectToMap function', () => {
+    it('should serialize an empty List to an empty Map', () => {
       const map = serializeFilterObjectToMap(List());
-      expect(map).toBeImmutableMap();
-      expect(map.size).toEqual(0);
+      expect(map).toEqualImmutable(Map());
     });
 
-    it('serialize an non-empty List to a Map', () => {
+    it('should serialize an non-empty List to a Map', () => {
       const list = fromJS([
         { type: 'channel', channels: ['beta'] },
         {
@@ -147,18 +146,19 @@ describe('<FilterObjectForm>', () => {
       );
     });
 
-    it('serialize an undefined List to an empty Map', () => {
+    it('should serialize an undefined List to an empty Map', () => {
       const map = serializeFilterObjectToMap(undefined);
       expect(map).toBeImmutableMap();
       expect(map.size).toEqual(0);
     });
 
-    it('throw error on something truthy that is not a List', () => {
+    it('should throw error on something truthy that is not a List', () => {
       expect(() => {
         serializeFilterObjectToMap([]);
       }).toThrow("'' (object) is not a List");
     });
-    it('throw error on an unrecognized key', () => {
+
+    it('should throw error on an unrecognized key', () => {
       const list = fromJS([{ type: 'color', colors: ['red'] }]);
       expect(() => {
         serializeFilterObjectToMap(list);
@@ -166,14 +166,14 @@ describe('<FilterObjectForm>', () => {
     });
   });
 
-  describe('<deserializeFilterObjectToList>', () => {
-    it('deserialize an undefined Map to an empty List', () => {
+  describe('deserializeFilterObjectToList function', () => {
+    it('should deserialize an undefined Map to an empty List', () => {
       const map = deserializeFilterObjectToList(undefined);
       expect(map).toBeImmutableList();
       expect(map.size).toEqual(0);
     });
 
-    it('serialize an non-empty Map to a List', () => {
+    it('should serialize an non-empty Map to a List', () => {
       const obj = {
         channels: ['beta'],
       };
@@ -183,7 +183,7 @@ describe('<FilterObjectForm>', () => {
       expect(list.first()).toEqual(fromJS({ type: 'channel', channels: ['beta'] }));
     });
 
-    it('throw an error on unrecognized names', () => {
+    it('should throw an error on unrecognized names', () => {
       const obj = {
         colors: ['red'],
       };
@@ -192,7 +192,7 @@ describe('<FilterObjectForm>', () => {
       }).toThrow("Key 'colors' no in known mapping.");
     });
 
-    it('serialize a map with exception _sampling', () => {
+    it('should serialize a map with the exceptional _sampling', () => {
       const obj = {
         _sampling: {
           type: 'bucketSample',
@@ -218,7 +218,7 @@ describe('<FilterObjectForm>', () => {
   });
 
   describe('utility functions', () => {
-    it('should parse inputs to either integer or null', () => {
+    it('should should parse inputs to either integer or null', () => {
       expect(parseIntOrNull(1)).toEqual(1);
       expect(parseIntOrNull('2')).toEqual(2);
       expect(parseIntOrNull('2.0')).toBeNull();
@@ -239,7 +239,7 @@ describe('<FilterObjectForm>', () => {
   });
 
   describe('<InputsWidget>', () => {
-    it('happy path adding more different inputs', () => {
+    it('should be possible to add more inputs', () => {
       const props = {
         disabled: false,
         bubbleUp: jest.fn(),
@@ -297,7 +297,7 @@ describe('<FilterObjectForm>', () => {
       });
     });
 
-    it('autocomplete suggestions', () => {
+    it('should suggest good matches in autocomplete suggestions', () => {
       const props = {
         disabled: false,
         bubbleUp: jest.fn(),
@@ -318,7 +318,7 @@ describe('<FilterObjectForm>', () => {
       expect(queryByText('normandy.foo')).toBeNull();
     });
 
-    it('editing by removing an existing input', () => {
+    it('should be possible to remove an existing input', () => {
       const props = {
         disabled: false,
         bubbleUp: jest.fn(),
@@ -369,7 +369,7 @@ describe('<FilterObjectForm>', () => {
     //   // fireEvent.keyDown(dropdown);
     // });
 
-    it('editing bucket sampling', () => {
+    it('should be possible to edit bucket sampling', () => {
       const props = {
         disabled: false,
         onChange: jest.fn(),
@@ -395,7 +395,7 @@ describe('<FilterObjectForm>', () => {
       });
     });
 
-    it('editing stable sampling', () => {
+    it('should be possible to edit stable sampling', () => {
       const props = {
         disabled: false,
         onChange: jest.fn(),
@@ -419,7 +419,7 @@ describe('<FilterObjectForm>', () => {
   });
 
   describe('<VersionsInput>', () => {
-    it('edit by adding 1 version', () => {
+    it('should be possible to add a version', () => {
       const props = {
         disabled: false,
         onChange: jest.fn(),
@@ -450,7 +450,7 @@ describe('<FilterObjectForm>', () => {
       expect(props.onChange).toBeCalledWith([75]);
     });
 
-    it('edit by adding a range of versions', () => {
+    it('should be possible to add a range of versions', () => {
       const props = {
         disabled: false,
         onChange: jest.fn(),
@@ -491,7 +491,7 @@ describe('<FilterObjectForm>', () => {
       expect(props.onChange).toBeCalledWith([75, 76, 77]);
     });
 
-    it('edit by removing a version', () => {
+    it('should be possible to remove a version', () => {
       const props = {
         disabled: false,
         onChange: jest.fn(),
diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 3af022b99..88161302c 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -784,9 +784,6 @@ export class InputsWidget extends React.PureComponent {
           disabled={disabled || !this.state.probablyValid}
           icon="plus"
           onClick={this.onSubmitSamplingInput}
-          // Needed to be able to find button DOM in testing.
-          // https://github.com/kentcdodds/dom-testing-library/issues/108
-          // data-testid="add-button"
         />
 
         <br />
diff --git a/src/workflows/recipes/components/RecipeForm.js b/src/workflows/recipes/components/RecipeForm.js
index 38416fb7c..c8a906c71 100644
--- a/src/workflows/recipes/components/RecipeForm.js
+++ b/src/workflows/recipes/components/RecipeForm.js
@@ -49,7 +49,6 @@ export function cleanRecipeData(data) {
     // Error thrown here will be automatically turned into a notification message in the UI.
     throw new Error('Have you have at least one filter object or a filter expression.');
   }
-  // console.log('DATA.filter_object', JSON.stringify(data.filter_object));
 
   // Make sure the action ID is an integer
   try {

From 9c866580a4ee4499a012407dca6ff337d51122d2 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 3 Oct 2018 11:19:49 -0400
Subject: [PATCH 04/22] tests for QueryRecipeFilters

---
 .../components/data/QueryRecipeFilters.test.js   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 src/tests/components/data/QueryRecipeFilters.test.js

diff --git a/src/tests/components/data/QueryRecipeFilters.test.js b/src/tests/components/data/QueryRecipeFilters.test.js
new file mode 100644
index 000000000..afa80f281
--- /dev/null
+++ b/src/tests/components/data/QueryRecipeFilters.test.js
@@ -0,0 +1,16 @@
+import { render } from 'react-testing-library';
+import TestComponent from 'console/components/data/QueryRecipeFilters';
+
+const { WrappedComponent: QueryRecipeFilters } = TestComponent;
+
+describe('<QueryRecipeFilters>', () => {
+  it('should work', () => {
+    const props = {
+      fetchRecipeFilters: jest.fn(),
+    };
+    const { container } = render(<QueryRecipeFilters {...props} />);
+    expect(props.fetchRecipeFilters).toHaveBeenCalledTimes(1);
+    // Should render nothing.
+    expect(container.querySelectorAll('*').length).toEqual(0);
+  });
+});

From 87abd55337a116aa16e7115d9c6bdb20ff1ec0a2 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 3 Oct 2018 11:57:28 -0400
Subject: [PATCH 05/22] started actions.test.js

---
 src/tests/state/actions/actions.test.js | 44 +++++++++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 src/tests/state/actions/actions.test.js

diff --git a/src/tests/state/actions/actions.test.js b/src/tests/state/actions/actions.test.js
new file mode 100644
index 000000000..2d16d7701
--- /dev/null
+++ b/src/tests/state/actions/actions.test.js
@@ -0,0 +1,44 @@
+import { fetchRecipeFilters } from 'console/state/recipes/actions';
+
+describe('Actions', () => {
+  describe('fetchRecipeFilters action', () => {
+    it('should work (and update) on an empty localStorage', async () => {
+      const fakeFilters = { countries: [] };
+      let state = {};
+      let dispatches = 0;
+      const dispatcher = action => {
+        if (typeof action === 'function') {
+          // Let's pretend we call out to the network.
+          return fakeFilters;
+        }
+        dispatches++;
+        state = action;
+      };
+      const func = fetchRecipeFilters();
+      await func(dispatcher);
+      expect(state.filters).toEqual(fakeFilters);
+      expect(dispatches).toEqual(1);
+      expect(JSON.parse(global.localStorage.getItem('recipe_filters'))).toEqual(fakeFilters);
+    });
+
+    it('should update existing localStorage', async () => {
+      global.localStorage.setItem('recipe_filters', JSON.stringify({ countries: [] }));
+      const fakeFilters = { countries: [{ key: 'SE', value: 'Sweeeden' }] };
+      let state = {};
+      let dispatches = 0;
+      const dispatcher = action => {
+        if (typeof action === 'function') {
+          // Let's pretend we call out to the network.
+          return fakeFilters;
+        }
+        dispatches++;
+        state = action;
+      };
+      const func = fetchRecipeFilters();
+      await func(dispatcher);
+      expect(state.filters).toEqual(fakeFilters);
+      expect(dispatches).toEqual(2);
+      expect(JSON.parse(global.localStorage.getItem('recipe_filters'))).toEqual(fakeFilters);
+    });
+  });
+});

From a41166d21aa7a9c0d5b4cb7d94da6d1760e1abae Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Mon, 15 Oct 2018 15:36:40 -0400
Subject: [PATCH 06/22] change order of tabs

---
 .../recipes/components/FilterObjectForm.js    | 70 +++++++++----------
 1 file changed, 32 insertions(+), 38 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 88161302c..8eb0ab8b6 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -147,7 +147,7 @@ class FilterObjectForm extends React.PureComponent {
     window.sessionStorage.setItem('filterObjectActiveTabKey', key);
   };
 
-  getDefaultActiveTabKey = (default_ = 'geo') => {
+  getDefaultActiveTabKey = default_ => {
     return window.sessionStorage.getItem('filterObjectActiveTabKey') || default_;
   };
 
@@ -221,10 +221,38 @@ class FilterObjectForm extends React.PureComponent {
         <QueryRecipeFilters />
 
         <Tabs
-          defaultActiveKey={this.getDefaultActiveTabKey()}
+          defaultActiveKey={this.getDefaultActiveTabKey('browser')}
           onChange={this.rememberActiveTabKey}
         >
-          <TabsPane tab={countSettings.geo ? `Geo (${countSettings.geo})` : 'Geo'} key="geo">
+          <TabsPane tab={`Browser (${countSettings.browser})`} key="browser">
+            <Row gutter={16}>
+              <Col span={12}>
+                <FormItem
+                  label="Channel"
+                  name="filter_object.channels"
+                  required={false}
+                  rules={[{ required: false }]}
+                  initialValue={initialChannels}
+                >
+                  <CheckboxGroup disabled={disabled} options={this.allChannelOptions()} />
+                </FormItem>
+              </Col>
+              <Col span={12}>
+                <FormItem
+                  label="Version"
+                  name="filter_object.versions"
+                  required={false}
+                  connectToForm={false}
+                >
+                  {getFieldDecorator('filter_object.versions', {
+                    initialValue: initialVersions,
+                    rules: [{ validator: this.checkVersions }],
+                  })(<VersionsInput disabled={disabled} />)}
+                </FormItem>
+              </Col>
+            </Row>
+          </TabsPane>
+          <TabsPane tab={`Geo (${countSettings.geo})`} key="geo">
             <Row gutter={16} type="flex">
               <Col span={12}>
                 <FormItem
@@ -288,41 +316,7 @@ class FilterObjectForm extends React.PureComponent {
               </Col>
             </Row>
           </TabsPane>
-          <TabsPane
-            tab={countSettings.browser ? `Browser (${countSettings.browser})` : 'Browser'}
-            key="browser"
-          >
-            <Row gutter={16}>
-              <Col span={12}>
-                <FormItem
-                  label="Channel"
-                  name="filter_object.channels"
-                  required={false}
-                  rules={[{ required: false }]}
-                  initialValue={initialChannels}
-                >
-                  <CheckboxGroup disabled={disabled} options={this.allChannelOptions()} />
-                </FormItem>
-              </Col>
-              <Col span={12}>
-                <FormItem
-                  label="Version"
-                  name="filter_object.versions"
-                  required={false}
-                  connectToForm={false}
-                >
-                  {getFieldDecorator('filter_object.versions', {
-                    initialValue: initialVersions,
-                    rules: [{ validator: this.checkVersions }],
-                  })(<VersionsInput disabled={disabled} />)}
-                </FormItem>
-              </Col>
-            </Row>
-          </TabsPane>
-          <TabsPane
-            tab={countSettings.sampling ? `Sampling (${countSettings.sampling})` : 'Sampling'}
-            key="sampling"
-          >
+          <TabsPane tab={`Sampling (${countSettings.sampling})`} key="sampling">
             <FormItem
               label=""
               name="filter_object._sampling"

From 615c8a8d478df0ae80a31a1169ab43fe02d4c160 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Mon, 15 Oct 2018 15:39:16 -0400
Subject: [PATCH 07/22] and tests

---
 .../workflows/recipes/components/FilterObjectForm.test.js   | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
index 469c84a53..06e1dba68 100644
--- a/src/tests/workflows/recipes/components/FilterObjectForm.test.js
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -50,9 +50,9 @@ describe('<FilterObjectForm>', () => {
 
     const FakeForm = createForm({})(FilterObjectForm);
     const { getByText } = render(wrapMockStore(<FakeForm {...props} />));
-    expect(getByText('Geo')).toBeTruthy();
-    expect(getByText('Browser')).toBeTruthy();
-    expect(getByText('Sampling')).toBeTruthy();
+    expect(getByText('Geo (0)')).toBeTruthy();
+    expect(getByText('Browser (0)')).toBeTruthy();
+    expect(getByText('Sampling (0)')).toBeTruthy();
     expect(fetchMock.called()).toBeTruthy();
   });
 

From d51c9cd9a131122ba3f9591695ff4fea269e1568 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 16 Oct 2018 12:50:18 -0400
Subject: [PATCH 08/22] update yarn integrities

---
 yarn.lock | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/yarn.lock b/yarn.lock
index 148f3bc96..61aa7d2cb 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -3299,6 +3299,7 @@ dom-serializer@0, dom-serializer@~0.1.0:
 dom-testing-library@^3.1.0:
   version "3.6.0"
   resolved "https://registry.yarnpkg.com/dom-testing-library/-/dom-testing-library-3.6.0.tgz#e5598fae9477f3918e22aee44f2262727203b5bd"
+  integrity sha512-BakKX0AI0QPVHwoncpMCx+DFzQ3zT3u7bIqPmzOm1WFfKVd278WlanGuKNOJz5/h07u2EzZKNkYUro7/21dy0Q==
   dependencies:
     mutationobserver-shim "^0.3.2"
     pretty-format "^22.4.3"
@@ -8470,6 +8471,7 @@ pretty-error@^2.0.2:
 pretty-format@^22.4.3:
   version "22.4.3"
   resolved "http://registry.npmjs.org/pretty-format/-/pretty-format-22.4.3.tgz#f873d780839a9c02e9664c8a082e9ee79eaac16f"
+  integrity sha512-S4oT9/sT6MN7/3COoOy+ZJeA92VmOnveLHgrwBE3Z1W5N9S2A1QGNYiE1z75DAENbJrXXUb+OWXhpJcg05QKQQ==
   dependencies:
     ansi-regex "^3.0.0"
     ansi-styles "^3.2.0"
@@ -9376,6 +9378,7 @@ react-test-renderer@^16.0.0-0:
 react-testing-library@5.1.0:
   version "5.1.0"
   resolved "https://registry.yarnpkg.com/react-testing-library/-/react-testing-library-5.1.0.tgz#ae208d9e3b7faae233409d205e287b304ec8ffdc"
+  integrity sha512-rmOJgUoscmZiGgcILoWhRh20zIqVzuh4JHvw/91nKvzyh/r56pN234hiatEiW1EzGIiM7Wg4i2Jbb0ydJgYD+A==
   dependencies:
     dom-testing-library "^3.1.0"
     wait-for-expect "^1.0.0"
@@ -11551,6 +11554,7 @@ w3c-hr-time@^1.0.1:
 wait-for-expect@^1.0.0:
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/wait-for-expect/-/wait-for-expect-1.0.1.tgz#73ab346ed56ed2ef66c380a59fd623755ceac0ce"
+  integrity sha512-TPZMSxGWUl2DWmqdspLDEy97/S1Mqq0pzbh2A7jTq0WbJurUb5GKli+bai6ayeYdeWTF0rQNWZmUvCVZ9gkrfA==
 
 walker@~1.0.5:
   version "1.0.7"
@@ -11753,6 +11757,7 @@ whatwg-encoding@^1.0.1, whatwg-encoding@^1.0.3:
 whatwg-fetch@3.0.0:
   version "3.0.0"
   resolved "https://registry.yarnpkg.com/whatwg-fetch/-/whatwg-fetch-3.0.0.tgz#fc804e458cc460009b1a2b966bc8817d2578aefb"
+  integrity sha512-9GSJUgz1D4MfyKU7KRqwOjXCXTqWdFNvEr7eUBYchQiVc744mqK/MzXPNR2WsPkmkOa4ywfg8C2n8h+13Bey1Q==
 
 whatwg-fetch@>=0.10.0:
   version "2.0.4"

From 5398297ad8ea7659f97b29e6a2a0270f17e6f1a0 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 16 Oct 2018 12:56:24 -0400
Subject: [PATCH 09/22] 'Sampling' tab first

---
 .../recipes/components/FilterObjectForm.js    | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 8eb0ab8b6..01c6ddb8f 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -221,9 +221,23 @@ class FilterObjectForm extends React.PureComponent {
         <QueryRecipeFilters />
 
         <Tabs
-          defaultActiveKey={this.getDefaultActiveTabKey('browser')}
+          defaultActiveKey={this.getDefaultActiveTabKey('sampling')}
           onChange={this.rememberActiveTabKey}
         >
+          <TabsPane tab={`Sampling (${countSettings.sampling})`} key="sampling">
+            <FormItem
+              label=""
+              name="filter_object._sampling"
+              required={false}
+              connectToForm={false}
+            >
+              {getFieldDecorator('filter_object._sampling', {
+                initialValue: initialSampling,
+                rules: [{ validator: this.checkSampling }],
+              })(<SamplingInput disabled={disabled} />)}
+            </FormItem>
+          </TabsPane>
+
           <TabsPane tab={`Browser (${countSettings.browser})`} key="browser">
             <Row gutter={16}>
               <Col span={12}>
@@ -316,19 +330,6 @@ class FilterObjectForm extends React.PureComponent {
               </Col>
             </Row>
           </TabsPane>
-          <TabsPane tab={`Sampling (${countSettings.sampling})`} key="sampling">
-            <FormItem
-              label=""
-              name="filter_object._sampling"
-              required={false}
-              connectToForm={false}
-            >
-              {getFieldDecorator('filter_object._sampling', {
-                initialValue: initialSampling,
-                rules: [{ validator: this.checkSampling }],
-              })(<SamplingInput disabled={disabled} />)}
-            </FormItem>
-          </TabsPane>
         </Tabs>
       </div>
     );

From 81b5801809a38a7841dd3260ae24a9e618aa8561 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 16 Oct 2018 13:20:47 -0400
Subject: [PATCH 10/22] one version at a time

---
 .../components/FilterObjectForm.test.js       | 41 --------
 .../recipes/components/FilterObjectForm.js    | 95 +++----------------
 2 files changed, 13 insertions(+), 123 deletions(-)

diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
index 06e1dba68..c576d3d1e 100644
--- a/src/tests/workflows/recipes/components/FilterObjectForm.test.js
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -450,47 +450,6 @@ describe('<FilterObjectForm>', () => {
       expect(props.onChange).toBeCalledWith([75]);
     });
 
-    it('should be possible to add a range of versions', () => {
-      const props = {
-        disabled: false,
-        onChange: jest.fn(),
-        value: [],
-        onSubmit: jest.fn(), // otherwise set by createForm()
-      };
-
-      const FakeForm = createForm({})(VersionsInput);
-      const { getByValue, container } = render(<FakeForm {...props} />);
-
-      const changeRadioButton = getByValue('range');
-      fireEvent.click(changeRadioButton);
-
-      const firstInput = getByValue('');
-      fireEvent.change(firstInput, {
-        target: { value: '75' },
-      });
-      fireEvent.blur(firstInput);
-      const button = container.querySelector('button[type="button"]');
-      // The submit button is now *not* disabled, because typing something into the
-      // first number will automatically suggest something in the second input.
-      const secondInput = getByValue('76'); // getting this proves it was populated
-      expect(button.disabled).toBeFalsy();
-
-      // Let's put in something that is not right. A number *less* than the first one.
-      fireEvent.change(secondInput, {
-        target: { value: '70' },
-      });
-      expect(button.disabled).toBeTruthy();
-
-      // Be more sensible.
-      fireEvent.change(secondInput, {
-        target: { value: '77' },
-      });
-      expect(button.disabled).toBeFalsy();
-
-      fireEvent.click(button);
-      expect(props.onChange).toBeCalledWith([75, 76, 77]);
-    });
-
     it('should be possible to remove a version', () => {
       const props = {
         disabled: false,
diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 01c6ddb8f..80f4a053c 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -1,16 +1,5 @@
 /* eslint-disable react/jsx-boolean-value */
-import {
-  AutoComplete,
-  Button,
-  Checkbox,
-  Col,
-  InputNumber,
-  Radio,
-  Row,
-  Select,
-  Tabs,
-  Tag,
-} from 'antd';
+import { AutoComplete, Button, Checkbox, Col, InputNumber, Row, Select, Tabs, Tag } from 'antd';
 import { fromJS, List, Map } from 'immutable';
 import PropTypes from 'prop-types';
 import React from 'react';
@@ -21,7 +10,6 @@ import FormItem from 'console/components/forms/FormItem';
 // XXX Why is this needed?!?
 const TabsPane = Tabs.TabPane;
 const CheckboxGroup = Checkbox.Group;
-const RadioGroup = Radio.Group;
 const Option = Select.Option;
 
 // Every filter object is an Map (in an List) that looks something like this:
@@ -237,7 +225,6 @@ class FilterObjectForm extends React.PureComponent {
               })(<SamplingInput disabled={disabled} />)}
             </FormItem>
           </TabsPane>
-
           <TabsPane tab={`Browser (${countSettings.browser})`} key="browser">
             <Row gutter={16}>
               <Col span={12}>
@@ -356,8 +343,6 @@ export function parseIntOrNull(number) {
 
 export class VersionsInput extends React.PureComponent {
   MIN_VERSION = 45;
-  INPUT_TYPE_SPECIFIC = 'specific';
-  INPUT_TYPE_RANGE = 'range';
 
   static propTypes = {
     disabled: PropTypes.bool,
@@ -367,56 +352,31 @@ export class VersionsInput extends React.PureComponent {
 
   state = {
     newVersions: [],
-    number1: null,
-    number2: null,
+    number: null,
     probablyValid: false,
     versionInputType: this.INPUT_TYPE_SPECIFIC,
   };
 
-  onChangeVersionInputType = event => {
-    this.setState({ versionInputType: event.target.value });
-  };
-
-  onChangeNumber1 = value => {
-    this.setState({ number1: parseIntOrNull(value) }, this.toggleProbablyValid);
-  };
-  onChangeNumber2 = value => {
-    this.setState({ number2: parseIntOrNull(value) }, this.toggleProbablyValid);
+  onChangeNumber = value => {
+    this.setState({ number: parseIntOrNull(value) }, this.toggleProbablyValid);
   };
 
   toggleProbablyValid = () => {
-    // Remember, these two numbers are definitely integers or null.
-    const { number1, number2 } = this.state;
+    // Remember, this 'number' is definitely either an integer or null.
+    const { number } = this.state;
     const existingVersions = this.props.value;
-    const isSaneNumber = number => {
-      return number !== null && number >= this.MIN_VERSION;
-    };
-    let probablyValid = false; // assume the worst
-    if (this.state.versionInputType === this.INPUT_TYPE_RANGE) {
-      // The second number must be greater than the first number.
-      probablyValid =
-        isSaneNumber(number1) &&
-        isSaneNumber(number2) &&
-        number2 > number1 &&
-        (number1 < Math.min(...existingVersions) || number2 > Math.max(...existingVersions));
-    } else {
-      // The first number just needs to be sane.
-      // But it can't be one of the existing ones.
-      probablyValid = isSaneNumber(number1) && !existingVersions.includes(number1);
-    }
+    const probablyValid =
+      number !== null && number >= this.MIN_VERSION && !existingVersions.includes(number);
     this.setState({ probablyValid });
   };
 
   addInputVersions = event => {
-    let lastNumber = this.state.number1;
+    const { number } = this.state;
     const existingVersions = this.props.value;
-    const range = [lastNumber];
-    while (this.state.number2 !== null && lastNumber++ < this.state.number2) {
-      range.push(lastNumber);
-    }
+    const range = [number];
     // Merge the range being asked to add with the existing versions.
     const merged = [...new Set([...existingVersions, ...range])].sort();
-    this.setState({ number1: '', number2: '', probablyValid: false }, () => {
+    this.setState({ number: '', probablyValid: false }, () => {
       this.props.onChange(merged);
     });
   };
@@ -431,43 +391,14 @@ export class VersionsInput extends React.PureComponent {
 
     return (
       <div>
-        <RadioGroup
-          disabled={disabled}
-          onChange={this.onChangeVersionInputType}
-          value={this.state.versionInputType}
-        >
-          <Radio value={this.INPUT_TYPE_SPECIFIC}>Specific</Radio>
-          <Radio value={this.INPUT_TYPE_RANGE}>Range</Radio>
-        </RadioGroup>
         <div>
           <InputNumber
             min={this.MIN_VERSION}
             disabled={disabled}
-            onChange={this.onChangeNumber1}
-            value={this.state.number1 || ''}
+            onChange={this.onChangeNumber}
+            value={this.state.number || ''}
             title={`Must be an integer number of at least ${this.MIN_VERSION}`}
-            onBlur={() => {
-              if (
-                this.state.number1 &&
-                this.state.versionInputType === this.INPUT_TYPE_RANGE &&
-                !this.state.number2
-              ) {
-                // Suggest a number of the second input right away.
-                this.setState({ number2: this.state.number1 + 1 }, this.toggleProbablyValid);
-              }
-            }}
           />
-          {this.state.versionInputType === this.INPUT_TYPE_RANGE ? (
-            <InputNumber
-              min={this.MIN_VERSION}
-              disabled={disabled}
-              value={this.state.number2 || ''}
-              onChange={this.onChangeNumber2}
-              title={`Must be an integer number of at least ${
-                this.MIN_VERSION
-              } and greater than the other number`}
-            />
-          ) : null}{' '}
           <Button
             type="primary"
             shape="circle"

From daa56c7bffbf6c9dcaeafcf24ac2aa833965b048 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 16 Oct 2018 14:21:37 -0400
Subject: [PATCH 11/22] channels displayed alphabetically by label

---
 .../components/FilterObjectForm.test.js       | 40 ++++++++++++++++++-
 .../recipes/components/FilterObjectForm.js    | 30 +++++++++++---
 2 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
index c576d3d1e..26e5c088d 100644
--- a/src/tests/workflows/recipes/components/FilterObjectForm.test.js
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -234,7 +234,7 @@ describe('<FilterObjectForm>', () => {
         'normandy.request_time',
       );
       expect(rewriteSamplingInput('string', options)).toEqual('"string"');
-      expect(rewriteSamplingInput('123', options)).toEqual('123');
+      expect(rewriteSamplingInput('123', options)).toEqual(123);
     });
   });
 
@@ -297,6 +297,44 @@ describe('<FilterObjectForm>', () => {
       });
     });
 
+    it('should not be possible to add inputs that already are set', () => {
+      const props = {
+        disabled: false,
+        bubbleUp: jest.fn(),
+        value: {
+          input: ['normandy.request_time', '"foobar"', 456],
+        },
+        onSubmit: jest.fn(),
+      };
+
+      // Create a Form wrapper to get the child contexts automatically passed.
+      const FakeForm = createForm({})(InputsWidget);
+      const { container } = render(<FakeForm {...props} />);
+      // Have to use querySelector because there's no text on the button.
+      const button = container.querySelector('button[type="button"]');
+      expect(button.disabled).toBeTruthy();
+
+      const input = container.querySelector('input');
+      // Can't use `fireEvent.change(input)` because it's a controlled component.
+      fireEvent.change(input, {
+        target: { value: 'normandy.request_time' },
+      });
+      // Current input isn't new!
+      expect(button.disabled).toBeTruthy();
+      // Even if you enter something that would be eventually reformatted it shouldn't allow it.
+      fireEvent.change(input, {
+        target: { value: 'foobar' },
+      });
+      // Current input still isn't new!
+      expect(button.disabled).toBeTruthy();
+      // As an string-to-int it should notice that that integer is already entered.
+      fireEvent.change(input, {
+        target: { value: '456' },
+      });
+      // Current input still isn't new!
+      expect(button.disabled).toBeTruthy();
+    });
+
     it('should suggest good matches in autocomplete suggestions', () => {
       const props = {
         disabled: false,
diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 80f4a053c..1e2adea9f 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -124,11 +124,23 @@ class FilterObjectForm extends React.PureComponent {
   };
 
   allChannelOptions = () => {
+    console.log('this.props.allChannels:', this.props.allChannels);
     return this.props.allChannels
       .map(channel => {
         return { label: channel.get('value'), value: channel.get('key') };
       })
-      .toArray();
+      .toArray()
+      .sort((a, b) => {
+        // The reason why we sort my *label* is because that's the only thing presented
+        // to users' eyes. What the default sort is or what the order by 'key' is not
+        // important to the user.
+        if (a.label > b.label) {
+          return 1;
+        } else if (a.label < b.label) {
+          return -1;
+        }
+        return 0;
+      });
   };
 
   rememberActiveTabKey = key => {
@@ -603,6 +615,10 @@ export function rewriteSamplingInput(input, defaultOptions) {
   ) {
     input = `"${input}"`;
   }
+  // This that are just numbers are converted to true integers.
+  if (!isNaN(parseInt(input, 10))) {
+    input = parseInt(input, 10);
+  }
   return input;
 }
 
@@ -637,9 +653,6 @@ export class InputsWidget extends React.PureComponent {
   };
 
   onSelectSamplingInput = value => {
-    if (!isNaN(parseInt(value, 10))) {
-      value = parseInt(value, 10);
-    }
     const inputs = this.props.value.input || [];
     inputs.push(value);
     inputs.sort((inputA, inputB) => {
@@ -672,7 +685,14 @@ export class InputsWidget extends React.PureComponent {
   onSearchInput = value => {
     // Reason for keeping this as local state is to be able to clear it when something
     // is chosen.
-    this.setState({ inputSearch: value, probablyValid: !!value.trim() });
+    let probablyValid = !!value.trim();
+    const existingValues = this.props.value.input;
+    if (existingValues && probablyValid) {
+      const newValue = rewriteSamplingInput(value, this.props.defaultInputOptions);
+      probablyValid &= !existingValues.includes(newValue);
+    }
+
+    this.setState({ inputSearch: value, probablyValid });
   };
 
   filterOption = (inputValue, option) => {

From 3ac4de55c4b36e3a649cdc4dcbf17586817ced70 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Tue, 16 Oct 2018 14:32:03 -0400
Subject: [PATCH 12/22] leftover console log

---
 src/workflows/recipes/components/FilterObjectForm.js | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 1e2adea9f..a7d98fb5e 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -124,7 +124,6 @@ class FilterObjectForm extends React.PureComponent {
   };
 
   allChannelOptions = () => {
-    console.log('this.props.allChannels:', this.props.allChannels);
     return this.props.allChannels
       .map(channel => {
         return { label: channel.get('value'), value: channel.get('key') };

From 6cbdf96aa0b7c072fc3c0b41ff84e2acd184cf71 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 17 Oct 2018 15:56:30 -0400
Subject: [PATCH 13/22] fix validation error rendering

---
 src/state/network/actions.js                  |   2 +-
 .../components/FilterObjectForm.test.js       |   1 +
 .../recipes/components/FilterObjectForm.js    | 143 ++++++++++++++++--
 .../recipes/components/RecipeForm.js          |   5 +-
 4 files changed, 134 insertions(+), 17 deletions(-)

diff --git a/src/state/network/actions.js b/src/state/network/actions.js
index a60082520..d24d46429 100644
--- a/src/state/network/actions.js
+++ b/src/state/network/actions.js
@@ -38,7 +38,7 @@ export function makeApiRequest(requestId, root, endpoint, options = {}) {
         error,
       });
 
-      throw new Error(error);
+      throw error;
     }
 
     dispatch({
diff --git a/src/tests/workflows/recipes/components/FilterObjectForm.test.js b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
index 26e5c088d..1cdf26bb6 100644
--- a/src/tests/workflows/recipes/components/FilterObjectForm.test.js
+++ b/src/tests/workflows/recipes/components/FilterObjectForm.test.js
@@ -45,6 +45,7 @@ describe('<FilterObjectForm>', () => {
         },
       },
       filterObject,
+      filterObjectErrors: {},
       onSubmit: jest.fn(),
     };
 
diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index a7d98fb5e..09c89a8ef 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -1,5 +1,16 @@
 /* eslint-disable react/jsx-boolean-value */
-import { AutoComplete, Button, Checkbox, Col, InputNumber, Row, Select, Tabs, Tag } from 'antd';
+import {
+  AutoComplete,
+  Button,
+  Checkbox,
+  Col,
+  Icon,
+  InputNumber,
+  Row,
+  Select,
+  Tabs,
+  Tag,
+} from 'antd';
 import { fromJS, List, Map } from 'immutable';
 import PropTypes from 'prop-types';
 import React from 'react';
@@ -45,7 +56,7 @@ export const SAMPLING_TYPES = [
       {"ages": [1,2,3]}
 
   The list of valid keys needs to be known in advance. */
-export const serializeFilterObjectToMap = list => {
+export function serializeFilterObjectToMap(list) {
   if (!list) {
     return Map();
   }
@@ -66,7 +77,7 @@ export const serializeFilterObjectToMap = list => {
       return [name, item.get(name)];
     }),
   );
-};
+}
 
 /* Return a List from a Map.
   Each item in the map is expected to be key'ed based on the reverse of the MAPPING
@@ -79,7 +90,7 @@ export const serializeFilterObjectToMap = list => {
     [{type: "locale", locales: ["sv", "en-US"]}, ...]
 
   The list of keys have to be known in advance. */
-export const deserializeFilterObjectToList = obj => {
+export function deserializeFilterObjectToList(obj) {
   if (!obj) {
     return List();
   }
@@ -106,7 +117,7 @@ export const deserializeFilterObjectToList = obj => {
         }
       }),
   );
-};
+}
 
 class FilterObjectForm extends React.PureComponent {
   static propTypes = {
@@ -115,6 +126,7 @@ class FilterObjectForm extends React.PureComponent {
     allLocales: PropTypes.instanceOf(List),
     disabled: PropTypes.bool,
     filterObject: PropTypes.instanceOf(Map),
+    filterObjectErrors: PropTypes.object,
   };
 
   static defaultProps = {
@@ -160,7 +172,7 @@ class FilterObjectForm extends React.PureComponent {
   };
 
   checkSampling = (rule, value, callback) => {
-    // XXX nothing at the moment
+    // XXX Check that all input fields are filled.
     callback();
   };
 
@@ -202,9 +214,72 @@ class FilterObjectForm extends React.PureComponent {
     return counts;
   };
 
+  getFormErrorsByTab = formErrors => {
+    // Return an object keyed by 'geo', 'browser', and 'sampling' whose values are
+    // true or false depending on their being any current form errors within.
+    // distinct settings have been made there.
+    const tabs = {
+      geo: false,
+      browser: false,
+      sampling: false,
+    };
+    if (formErrors._sampling) {
+      tabs.sampling = true;
+    }
+    if (formErrors.filter_object) {
+      if (formErrors.filter_object.channels || formErrors.filter_object.versions) {
+        tabs.browser = true;
+      }
+      if (formErrors.filter_object.countries || formErrors.filter_object.locales) {
+        tabs.geo = true;
+      }
+    }
+    return tabs;
+  };
+
   render() {
     const { disabled, filterObject } = this.props;
-    const { getFieldDecorator } = this.props.form;
+    const { getFieldDecorator, getFieldsValue } = this.props.form;
+
+    const filterObjectErrors = this.props.filterObjectErrors || {};
+    // This is where we build up a suitable object to pass to the FormItem
+    // components. It's got a very different structure than the filterObjectErrors.
+    // Basically, this object it sent to <FormItem/> components and they look up
+    // errors based on the "name". E.g. <FormItem name="foo.bar" formErrors={formErrors}>
+    // will search for errors in `formErrors.foo.bar`.
+    const formErrors = {};
+    if (Object.keys(filterObjectErrors).length) {
+      // The filter object form errors (i.e. validation errors from the server)
+      // comes in an object where each key is a the index number of the filter
+      // object *sent*. Suppose that we sent this:
+      //   [FilterObjectX, FilterObjectY, FilterObjectZ]
+      // and the server responded with:
+      //  {1: FormError}
+      // then, we know the error lies with 'FilterObjectY'.
+      // We need to turn those form errors into an object, keyed by the same
+      // way we sent it and using names that are the names we use for <FormItem/>
+      // Let's figure this out.
+      const filterObjectsAsList = deserializeFilterObjectToList(
+        getFieldsValue(['filter_object']).filter_object,
+      );
+      formErrors.filter_object = {};
+      filterObjectsAsList.forEach((filterObject, i) => {
+        if (filterObjectErrors[i]) {
+          const type = filterObject.get('type');
+          if (type === 'bucketSample' || type === 'stableSample') {
+            // Put this in the root!
+            formErrors._sampling = filterObjectErrors[i];
+          } else {
+            const name = MAPPING[type];
+            formErrors.filter_object[name] = filterObjectErrors[i][name];
+          }
+        }
+      });
+    }
+
+    // Now that we have all the active form errors, we can try to extract from these,
+    // which *tabs* then have errors in them.
+    const formErrorsByTab = this.getFormErrorsByTab(formErrors);
 
     // This is a simple object for counting how many settings have been set per tab.
     const countSettings = this.getSettingsCounts();
@@ -215,6 +290,27 @@ class FilterObjectForm extends React.PureComponent {
     const initialVersions = filterObject.get('versions', List()).toArray();
     const initialSampling = filterObject.get('_sampling', Map()).toJS();
 
+    const tabLabels = {
+      sampling: (
+        <span>
+          {formErrorsByTab.sampling && <Icon type="warning" theme="outlined" />}
+          Sampling ({countSettings.sampling})
+        </span>
+      ),
+      browser: (
+        <span>
+          {formErrorsByTab.browser && <Icon type="warning" theme="outlined" />}
+          Browser ({countSettings.browser})
+        </span>
+      ),
+      geo: (
+        <span>
+          {formErrorsByTab.geo && <Icon type="warning" theme="outlined" />}
+          Geo ({countSettings.geo})
+        </span>
+      ),
+    };
+
     return (
       <div>
         <QueryRecipeFilters />
@@ -223,25 +319,27 @@ class FilterObjectForm extends React.PureComponent {
           defaultActiveKey={this.getDefaultActiveTabKey('sampling')}
           onChange={this.rememberActiveTabKey}
         >
-          <TabsPane tab={`Sampling (${countSettings.sampling})`} key="sampling">
+          <TabsPane tab={tabLabels.sampling} key="sampling">
             <FormItem
               label=""
               name="filter_object._sampling"
               required={false}
               connectToForm={false}
+              formErrors={formErrors}
             >
               {getFieldDecorator('filter_object._sampling', {
                 initialValue: initialSampling,
                 rules: [{ validator: this.checkSampling }],
-              })(<SamplingInput disabled={disabled} />)}
+              })(<SamplingInput disabled={disabled} formErrors={formErrors} />)}
             </FormItem>
           </TabsPane>
-          <TabsPane tab={`Browser (${countSettings.browser})`} key="browser">
+          <TabsPane tab={tabLabels.browser} key="browser">
             <Row gutter={16}>
               <Col span={12}>
                 <FormItem
                   label="Channel"
                   name="filter_object.channels"
+                  formErrors={formErrors}
                   required={false}
                   rules={[{ required: false }]}
                   initialValue={initialChannels}
@@ -253,6 +351,7 @@ class FilterObjectForm extends React.PureComponent {
                 <FormItem
                   label="Version"
                   name="filter_object.versions"
+                  formErrors={formErrors}
                   required={false}
                   connectToForm={false}
                 >
@@ -264,12 +363,13 @@ class FilterObjectForm extends React.PureComponent {
               </Col>
             </Row>
           </TabsPane>
-          <TabsPane tab={`Geo (${countSettings.geo})`} key="geo">
+          <TabsPane tab={tabLabels.geo} key="geo">
             <Row gutter={16} type="flex">
               <Col span={12}>
                 <FormItem
                   label="Locale"
                   name="filter_object.locales"
+                  formErrors={formErrors}
                   required={false}
                   rules={[{ required: false }]}
                   initialValue={initialLocales}
@@ -301,6 +401,7 @@ class FilterObjectForm extends React.PureComponent {
                 <FormItem
                   label="Country"
                   name="filter_object.countries"
+                  formErrors={formErrors}
                   required={false}
                   rules={[{ required: false }]}
                   initialValue={initialCountries}
@@ -446,6 +547,10 @@ export class SamplingInput extends React.PureComponent {
     onChange: PropTypes.func,
   };
 
+  static defaultProps = {
+    formErrors: {},
+  };
+
   bubbleUp = newData => {
     this.props.onChange(Object.assign({}, this.props.value, newData));
   };
@@ -467,8 +572,7 @@ export class SamplingInput extends React.PureComponent {
   };
 
   renderBucketInputs = () => {
-    const { disabled, value } = this.props;
-
+    const { disabled, value, formErrors } = this.props;
     return (
       <div>
         <FormItem
@@ -476,6 +580,7 @@ export class SamplingInput extends React.PureComponent {
           name="_sampling.start"
           required={true}
           connectToForm={false}
+          formErrors={formErrors}
           {...this.formItemLayout}
         >
           <InputNumber
@@ -492,6 +597,7 @@ export class SamplingInput extends React.PureComponent {
           name="_sampling.count"
           required={true}
           connectToForm={false}
+          formErrors={formErrors}
           {...this.formItemLayout}
         >
           <InputNumber
@@ -508,6 +614,7 @@ export class SamplingInput extends React.PureComponent {
           name="_sampling.total"
           required={true}
           connectToForm={false}
+          formErrors={formErrors}
           {...this.formItemLayout}
         >
           <InputNumber
@@ -523,6 +630,7 @@ export class SamplingInput extends React.PureComponent {
         <InputsWidget
           disabled={disabled}
           value={value}
+          formErrors={formErrors}
           bubbleUp={this.bubbleUp}
           formItemLayout={this.formItemLayout}
         />
@@ -531,7 +639,7 @@ export class SamplingInput extends React.PureComponent {
   };
 
   renderStableInputs = () => {
-    const { disabled, value } = this.props;
+    const { disabled, value, formErrors } = this.props;
 
     return (
       <div>
@@ -540,6 +648,7 @@ export class SamplingInput extends React.PureComponent {
           name="_sampling.rate"
           required={true}
           connectToForm={false}
+          formErrors={formErrors}
           {...this.formItemLayout}
         >
           <InputNumber
@@ -558,6 +667,7 @@ export class SamplingInput extends React.PureComponent {
         <InputsWidget
           disabled={disabled}
           value={value}
+          formErrors={formErrors}
           bubbleUp={this.bubbleUp}
           formItemLayout={this.formItemLayout}
         />
@@ -640,6 +750,7 @@ export class InputsWidget extends React.PureComponent {
       labelCol: { span: 4 },
       wrapperCol: { span: 14 },
     },
+    formErrors: {},
     // XXX THIS LIST IS BASED ON SCRAPING https://normandy.cdn.mozilla.net/api/v1/recipe/
     // AND IT'S VERY DIFFERENT FROM THE AVAILABLE OPTIONS HERE:
     // https://normandy.readthedocs.io/en/latest/user/filters.html#filter-context
@@ -700,7 +811,8 @@ export class InputsWidget extends React.PureComponent {
   };
 
   render() {
-    const { value, disabled } = this.props;
+    const { value, disabled, formErrors } = this.props;
+
     const inputs = value.input || [];
     const inputOptions = this.props.defaultInputOptions.filter(opt => {
       return !inputs.includes(opt);
@@ -709,6 +821,7 @@ export class InputsWidget extends React.PureComponent {
       <FormItem
         label="Input"
         name="_sampling.input"
+        formErrors={formErrors}
         required={true}
         connectToForm={false}
         {...this.props.formItemLayout}
diff --git a/src/workflows/recipes/components/RecipeForm.js b/src/workflows/recipes/components/RecipeForm.js
index c8a906c71..39862b706 100644
--- a/src/workflows/recipes/components/RecipeForm.js
+++ b/src/workflows/recipes/components/RecipeForm.js
@@ -137,7 +137,8 @@ class RecipeForm extends React.PureComponent {
   }
 
   render() {
-    const { filters, isCreationForm, isLoading, onSubmit, recipe } = this.props;
+    const { filters, isCreationForm, isLoading, onSubmit, recipe, errors } = this.props;
+
     const { defaultIdenticonSeed } = this.state;
 
     // If creating, the 'default' seed is randomly generated. We store it in memory
@@ -173,7 +174,9 @@ class RecipeForm extends React.PureComponent {
         <FilterObjectForm
           form={this.props.form}
           disabled={isLoading}
+          recipe={recipe}
           filterObject={filterObject}
+          filterObjectErrors={errors.filter_object}
           allLocales={filters.get('locales')}
           allCountries={filters.get('countries')}
           allChannels={filters.get('channels')}

From 7765403a954e0b432cf5548ce780216bedf50a84 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 17 Oct 2018 16:13:07 -0400
Subject: [PATCH 14/22] ability to select exactly '0'

---
 src/workflows/recipes/components/FilterObjectForm.js | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 09c89a8ef..5318c822a 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -589,7 +589,7 @@ export class SamplingInput extends React.PureComponent {
             onChange={value => {
               this.bubbleUp({ start: value });
             }}
-            value={value.start || ''}
+            value={value.start !== undefined ? value.start : ''}
           />
         </FormItem>
         <FormItem
@@ -606,7 +606,7 @@ export class SamplingInput extends React.PureComponent {
             onChange={value => {
               this.bubbleUp({ count: value });
             }}
-            value={value.count || ''}
+            value={value.count !== undefined ? value.count : ''}
           />
         </FormItem>
         <FormItem
@@ -623,7 +623,7 @@ export class SamplingInput extends React.PureComponent {
             onChange={value => {
               this.bubbleUp({ total: value });
             }}
-            value={value.total || ''}
+            value={value.total !== undefined ? value.total : ''}
           />
         </FormItem>
 
@@ -661,7 +661,7 @@ export class SamplingInput extends React.PureComponent {
             onChange={value => {
               this.bubbleUp({ rate: value / 100 });
             }}
-            value={(value.rate && Math.trunc(value.rate * 100)) || ''}
+            value={(value.rate !== undefined && Math.trunc(value.rate * 100)) || ''}
           />
         </FormItem>
         <InputsWidget

From 55890966769b11890734f0115cdc3ff8be949d13 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 17 Oct 2018 16:31:39 -0400
Subject: [PATCH 15/22] inputs now require clicking the plus button

---
 src/workflows/recipes/components/FilterObjectForm.js | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 5318c822a..9449bb017 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -775,8 +775,9 @@ export class InputsWidget extends React.PureComponent {
       }
       return 0;
     });
-    this.props.bubbleUp({ input: inputs });
-    this.setState({ inputSearch: '', probablyValid: false });
+    this.setState({ inputSearch: '', probablyValid: false }, () => {
+      this.props.bubbleUp({ input: inputs });
+    });
   };
 
   onSubmitSamplingInput = () => {
@@ -829,7 +830,6 @@ export class InputsWidget extends React.PureComponent {
         <AutoComplete
           dataSource={inputOptions}
           style={{ width: 200 }}
-          onSelect={this.onSelectSamplingInput}
           onChange={this.onSearchInput}
           disabled={disabled}
           value={this.state.inputSearch}

From 43f4cdd29838b4b6d736affc2764d32e06e35def Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 17 Oct 2018 16:39:04 -0400
Subject: [PATCH 16/22] the right defaults for 'defaultInputOptions'

---
 src/workflows/recipes/components/FilterObjectForm.js | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 9449bb017..a98d8f9e3 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -751,15 +751,7 @@ export class InputsWidget extends React.PureComponent {
       wrapperCol: { span: 14 },
     },
     formErrors: {},
-    // XXX THIS LIST IS BASED ON SCRAPING https://normandy.cdn.mozilla.net/api/v1/recipe/
-    // AND IT'S VERY DIFFERENT FROM THE AVAILABLE OPTIONS HERE:
-    // https://normandy.readthedocs.io/en/latest/user/filters.html#filter-context
-    defaultInputOptions: [
-      'normandy.clientId',
-      'normandy.recipe.id',
-      'normandy.request_time',
-      'normandy.userId',
-    ],
+    defaultInputOptions: ['normandy.recipe.id', 'normandy.userId'],
   };
 
   onSelectSamplingInput = value => {

From a02ea370ddb125e4be634551b1097d8565b884ab Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Wed, 17 Oct 2018 16:46:51 -0400
Subject: [PATCH 17/22] ability to leave extra_filter_expression blank

---
 src/workflows/recipes/components/RecipeForm.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/workflows/recipes/components/RecipeForm.js b/src/workflows/recipes/components/RecipeForm.js
index 39862b706..c994b1312 100644
--- a/src/workflows/recipes/components/RecipeForm.js
+++ b/src/workflows/recipes/components/RecipeForm.js
@@ -44,6 +44,10 @@ export function cleanRecipeData(data) {
   // When submitted it's an object like `{locales: ['sv']}` and it needs to become:
   // `[{type: 'locale': locales: ['sv']}]` for example.
   data.filter_object = deserializeFilterObjectToList(data.filter_object);
+  if (data.filter_object.size && data.extra_filter_expression === null) {
+    // Normandy gets crumpy if the extra_filter_expression is null.
+    data.extra_filter_expression = '';
+  }
   if (!data.filter_object.size && !data.extra_filter_expression) {
     // You can't submit a recipe with no extra filter expression and no filter object rules.
     // Error thrown here will be automatically turned into a notification message in the UI.

From 4fe13f9e2d7f11ca2dc7f1708c543c1a00306640 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Thu, 18 Oct 2018 10:31:44 -0400
Subject: [PATCH 18/22] stealth api requests

---
 src/state/network/actions.js | 17 +++++++++++++----
 src/state/recipes/actions.js | 13 +++++++++----
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/src/state/network/actions.js b/src/state/network/actions.js
index d24d46429..75bf01761 100644
--- a/src/state/network/actions.js
+++ b/src/state/network/actions.js
@@ -18,14 +18,23 @@ export function makeApiRequest(requestId, root, endpoint, options = {}) {
     const api = new APIClient(root, accessToken);
     const request = getRequest(state, requestId);
 
+    // A "stealth API request" is one that doesn't update the state.
+    // This is useful when you generally "don't care" about the *state* of the request.
+    const stealth = options.stealth || false;
+    if (stealth) {
+      // Doing it this way because JavaScript doesn't have a `obj.pop(key, default)`
+      delete options.stealth;
+    }
+
     if (request.inProgress) {
       return true;
     }
 
-    dispatch({
-      type: REQUEST_SEND,
-      requestId,
-    });
+    !stealth &&
+      dispatch({
+        type: REQUEST_SEND,
+        requestId,
+      });
 
     let data;
 
diff --git a/src/state/recipes/actions.js b/src/state/recipes/actions.js
index 6b301bee8..2111826d6 100644
--- a/src/state/recipes/actions.js
+++ b/src/state/recipes/actions.js
@@ -154,11 +154,16 @@ export function fetchRecipeFilters() {
         filters: JSON.parse(localFilters),
       });
     }
-    // XXX If we already had the filters in localStorage, can we avoid using this
-    // makeNormandyApiRequest() with an ID so that it doesn't cause any spinners to appear
-    // when the localStorage version almost definitely is good enough.
+
+    const options = {};
+    // If we already had the filters in localStorage, we can make a "stealth API request"
+    // which is the same as a regular request except it doesn't update the global state
+    // that there's a request we're waiting for.
+    if (localFilters) {
+      options.stealth = true;
+    }
     const requestId = 'fetch-recipe-filters';
-    const filters = await dispatch(makeNormandyApiRequest(requestId, 'v3/filters/'));
+    const filters = await dispatch(makeNormandyApiRequest(requestId, 'v3/filters/', options));
     // After it has been retrieved remotely, it's very possible that the lists
     // haven't changed. If it hasn't changed compared to what we had in local Storage, then
     // don't bother dispatching again.

From e787283c6819e86dc324b82358bd2fe5c6886641 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Thu, 18 Oct 2018 10:47:58 -0400
Subject: [PATCH 19/22] getting rid of component shortchuts

---
 .../recipes/components/FilterObjectForm.js    | 23 ++++++++-----------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index a98d8f9e3..7a74ebfce 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -18,11 +18,6 @@ import React from 'react';
 import QueryRecipeFilters from 'console/components/data/QueryRecipeFilters';
 import FormItem from 'console/components/forms/FormItem';
 
-// XXX Why is this needed?!?
-const TabsPane = Tabs.TabPane;
-const CheckboxGroup = Checkbox.Group;
-const Option = Select.Option;
-
 // Every filter object is an Map (in an List) that looks something like this:
 //  {"type": "channel", "channels": ["beta"]}
 // We need a mapping from "channel" to "channels", for each, to know what to
@@ -319,7 +314,7 @@ class FilterObjectForm extends React.PureComponent {
           defaultActiveKey={this.getDefaultActiveTabKey('sampling')}
           onChange={this.rememberActiveTabKey}
         >
-          <TabsPane tab={tabLabels.sampling} key="sampling">
+          <Tabs.TabPane tab={tabLabels.sampling} key="sampling">
             <FormItem
               label=""
               name="filter_object._sampling"
@@ -332,8 +327,8 @@ class FilterObjectForm extends React.PureComponent {
                 rules: [{ validator: this.checkSampling }],
               })(<SamplingInput disabled={disabled} formErrors={formErrors} />)}
             </FormItem>
-          </TabsPane>
-          <TabsPane tab={tabLabels.browser} key="browser">
+          </Tabs.TabPane>
+          <Tabs.TabPane tab={tabLabels.browser} key="browser">
             <Row gutter={16}>
               <Col span={12}>
                 <FormItem
@@ -344,7 +339,7 @@ class FilterObjectForm extends React.PureComponent {
                   rules={[{ required: false }]}
                   initialValue={initialChannels}
                 >
-                  <CheckboxGroup disabled={disabled} options={this.allChannelOptions()} />
+                  <Checkbox.Group disabled={disabled} options={this.allChannelOptions()} />
                 </FormItem>
               </Col>
               <Col span={12}>
@@ -362,8 +357,8 @@ class FilterObjectForm extends React.PureComponent {
                 </FormItem>
               </Col>
             </Row>
-          </TabsPane>
-          <TabsPane tab={tabLabels.geo} key="geo">
+          </Tabs.TabPane>
+          <Tabs.TabPane tab={tabLabels.geo} key="geo">
             <Row gutter={16} type="flex">
               <Col span={12}>
                 <FormItem
@@ -428,7 +423,7 @@ class FilterObjectForm extends React.PureComponent {
                 </FormItem>
               </Col>
             </Row>
-          </TabsPane>
+          </Tabs.TabPane>
         </Tabs>
       </div>
     );
@@ -694,9 +689,9 @@ export class SamplingInput extends React.PureComponent {
           >
             {SAMPLING_TYPES.map(samplingType => {
               return (
-                <Option value={samplingType.value} key={samplingType.value}>
+                <Select.Option value={samplingType.value} key={samplingType.value}>
                   {samplingType.label}
-                </Option>
+                </Select.Option>
               );
             })}
           </Select>

From 0dc0f2d38bb49e4d4408dd8ca8aa2c8057dc398e Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Thu, 18 Oct 2018 11:22:31 -0400
Subject: [PATCH 20/22] remove XXX comment on checkSampling validation function

---
 .../recipes/components/FilterObjectForm.js    | 21 ++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 7a74ebfce..9ec085aef 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -160,15 +160,26 @@ class FilterObjectForm extends React.PureComponent {
   checkVersions = (rule, value, callback) => {
     // Not doing anything here. The trust of validation is ultimately inside the VersionsInput
     // component *and* the server-side checking.
-    // This method is remains as a stub in case we later decide to add some last-minute
-    // validation and by leaving it we remind ourselves how this magic with
-    // getFieldDecorator stuff works.
     callback();
   };
 
   checkSampling = (rule, value, callback) => {
-    // XXX Check that all input fields are filled.
-    callback();
+    // The reason for deliberately not doing any validation here is because this
+    // validation function is called on every single onChange to any of the sub-widgets
+    // of the sampling input.
+    // We *could* check that every value is an integer. We *could* check that the
+    // input field is not empty. However, we can't specific exactly which field is the
+    // bad one. For example, if we discover that "bucketSample.start === 'junk'" the best
+    // we can do with this is something like this: `callback("'start' is not a number")`
+    // but there are many caveats with this. It will make every input field in the sub-widgets
+    // read and the error message string (e.g. "'start' is not a number") will be displayed
+    // for *all* fields. Also, it's quite annoying to validate input whilst the user isn't
+    // finished with it. For example, they might fumble on the keyboard and haven't yet
+    // typed in what they want. Ideally, that's solved by doing the validation onBlur
+    // or (traditional) onChange when the user is generally done with the input field.
+    // Lastly, the server-side does good validation and that validation is on a per-field
+    // basis.
+    return callback();
   };
 
   filterOption = (inputValue, option) => {

From 7f41d96532a7048e792dfa94e7a655aff432866e Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Thu, 18 Oct 2018 11:24:16 -0400
Subject: [PATCH 21/22] remove last XXX comment

---
 src/workflows/recipes/components/FilterObjectForm.js | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/workflows/recipes/components/FilterObjectForm.js b/src/workflows/recipes/components/FilterObjectForm.js
index 9ec085aef..95db9a324 100644
--- a/src/workflows/recipes/components/FilterObjectForm.js
+++ b/src/workflows/recipes/components/FilterObjectForm.js
@@ -206,7 +206,6 @@ class FilterObjectForm extends React.PureComponent {
       const value = getFieldValue(`filter_object.${key}`);
       // 'value' here is either undefined, a JS object or a JS array.
       // The task is to conclude if it's truthy.
-      // XXX Is there a better utility function for this?
       if (value) {
         return (Array.isArray(value) && !!value.length) || !!Object.keys(value).length;
       }

From 3425188a5939b95efd3752bfe03deaf7448e4787 Mon Sep 17 00:00:00 2001
From: Peter Bengtsson <mail@peterbe.com>
Date: Thu, 18 Oct 2018 13:27:55 -0400
Subject: [PATCH 22/22] feedbacked

---
 src/state/network/actions.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/state/network/actions.js b/src/state/network/actions.js
index 75bf01761..1b394d9bf 100644
--- a/src/state/network/actions.js
+++ b/src/state/network/actions.js
@@ -30,11 +30,12 @@ export function makeApiRequest(requestId, root, endpoint, options = {}) {
       return true;
     }
 
-    !stealth &&
+    if (!stealth) {
       dispatch({
         type: REQUEST_SEND,
         requestId,
       });
+    }
 
     let data;