Skip to content
This repository has been archived by the owner on May 27, 2021. It is now read-only.

Filter objects UI #489

Merged
merged 23 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.1",
"stylelint": "9.6.0",
Expand All @@ -95,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": [
Expand Down
27 changes: 27 additions & 0 deletions src/components/data/QueryRecipeFilters.js
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 5 additions & 0 deletions src/less/pages/recipe-details.less
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ dl.details {
}
}

dt.no-padding,
dd.no-padding {
padding: 0;
}

.arguments-value {
.value {
float: left;
Expand Down
7 changes: 7 additions & 0 deletions src/setupTests.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { configure, mount, shallow } from 'enzyme';
import { cleanup } from 'react-testing-library';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't clear below what cleanup refers to. Can this be import * as reactTestingLibrary from 'react-testing-library instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because JavaScript.
Look at the line above. It's called configure :)

I don't like import * as ... from '...' because when tree shaking and partial imports can be used that's an anti-pattern.

Also, I've seen a lot of code where they accept this mess and leave it as import { commonword } from 'javascript-lacks-namespaces'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally and whimsically, how about is :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency is good, but repeating mistakes for the sake of consistency isn't useful. We can improve over time. Just because there are bad examples already in the code doesn't mean we can't do better going forward.

import Adapter from 'enzyme-adapter-react-16';
import * as immutableMatchers from 'jest-immutable-matchers';
import fetchMock from 'fetch-mock';
import { Headers } from 'whatwg-fetch';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be good to leave the space between imports and the rest of the code that used to be here.


// Configure Enzyme adapter
configure({ adapter: new Adapter() });
Expand Down Expand Up @@ -40,10 +42,12 @@ global.auth0 = {
})),
};
global.localStorage = mockLocalStorage();
global.sessionStorage = mockLocalStorage();
global.mount = mount;
global.shallow = shallow;
global.React = React;
global.ReactDOM = ReactDOM;
global.Headers = Headers;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't do this you get unhandled errors in api.js. In Jest they appear as warnings like this:

(node:646) UnhandledPromiseRejectionWarning: ReferenceError: Headers is not defined
(node:646) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 4)
(node:646) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:646) UnhandledPromiseRejectionWarning: ReferenceError: Headers is not defined
(node:646) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 8)

You can hopefully see an example of that here: https://circleci.com/gh/mozilla/delivery-console/3515

It saddens me that the tests don't fail on this blatant error (that global Headers obviously doesn't exist in NodeJS).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when using globals, should one use window. instead? E.g. window.localStorage instead of localStorage?
You know, to avoid this happening again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather, to be more explicit.


// Set up a global fetch mock, and assert that all calls to it are expected.
beforeEach(() => {
Expand All @@ -60,4 +64,7 @@ afterEach(() => {
} finally {
fetchMock.restore();
}

// Unmounts React trees that were mounted with react-testing-library's render.
cleanup();
});
29 changes: 23 additions & 6 deletions src/state/recipes/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we address this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check this out. In practice it works.
Basically, if there was data in localStorage it makes the next XHR request without setting {[requestId]: {inProgress:true}}.

See this:
screen shot 2018-10-18 at 10 18 08 am

I put in a time.sleep(10) in the Filters(views.APIView).get method. Note how all the fields are not disabled whilst waiting for this request.

Then I delete the stuff from localStorage and refreshed. This time, all the fields are disabled whilst waiting.
screen shot 2018-10-18 at 10 31 02 am

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));
}
};
}

Expand Down
16 changes: 16 additions & 0 deletions src/tests/components/data/QueryRecipeFilters.test.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
44 changes: 44 additions & 0 deletions src/tests/state/actions/actions.test.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
Loading