Skip to content

Commit

Permalink
Use Redux-Saga for Search Actions (amundsen-io#265)
Browse files Browse the repository at this point in the history 
* Added redux actions and sagas instead for each search action: `submitSearch`, `setResource`, `setPageIndex`, `loadPreviousSearch`, and `UrlDidUpdate`. This greatly simplifies the `SearchPage` logic in preparation for adding filters.
* Added `navigation-utils`.
  • Loading branch information
Daniel authored Sep 4, 2019
1 parent 415627b commit 4a47bb9
Show file tree
Hide file tree
Showing 17 changed files with 732 additions and 729 deletions.
26 changes: 18 additions & 8 deletions frontend/amundsen_application/static/js/components/SearchPage/SearchBar/index.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import { bindActionCreators } from 'redux'
import { connect } from 'react-redux';
import { RouteComponentProps, withRouter } from 'react-router';

// TODO: Use css-modules instead of 'import'
import './styles.scss';
Expand All @@ -14,17 +14,23 @@ import {
SYNTAX_ERROR_SPACING_SUFFIX,
} from './constants';
import { GlobalState } from 'ducks/rootReducer';
import { submitSearch } from 'ducks/search/reducer';
import { SubmitSearchRequest } from 'ducks/search/types';

export interface StateFromProps {
searchTerm: string;
}

export interface DispatchFromProps {
submitSearch: (searchTerm: string) => SubmitSearchRequest;
}

export interface OwnProps {
placeholder?: string;
subText?: string;
}

export type SearchBarProps = StateFromProps & OwnProps & RouteComponentProps<any>;
export type SearchBarProps = StateFromProps & DispatchFromProps & OwnProps;

interface SearchBarState {
subTextClassName: string;
Expand Down Expand Up @@ -61,15 +67,15 @@ export class SearchBar extends React.Component<SearchBarProps, SearchBarState> {
const searchTerm = this.state.searchTerm.trim();
event.preventDefault();
if (this.isFormValid(searchTerm)) {
let pathName = '/';
if (searchTerm !== '') {
pathName = `/search?searchTerm=${searchTerm}`;
}
this.props.history.push(pathName);
this.props.submitSearch(searchTerm);
}
};

isFormValid = (searchTerm: string) : boolean => {
if (searchTerm.length === 0) {
return false;
}

const hasAtMostOneCategory = searchTerm.split(':').length <= 2;
if (!hasAtMostOneCategory) {
this.setState({
Expand Down Expand Up @@ -126,4 +132,8 @@ export const mapStateToProps = (state: GlobalState) => {
};
};

export default connect<StateFromProps>(mapStateToProps, null)(withRouter(SearchBar));
export const mapDispatchToProps = (dispatch: any) => {
return bindActionCreators({ submitSearch }, dispatch);
};

export default connect<StateFromProps>(mapStateToProps, mapDispatchToProps)(SearchBar);
45 changes: 25 additions & 20 deletions frontend/amundsen_application/static/js/components/SearchPage/SearchBar/tests/index.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';

import { shallow } from 'enzyme';

import { mapStateToProps, SearchBar, SearchBarProps } from '../';
import { mapStateToProps, mapDispatchToProps, SearchBar, SearchBarProps } from '../';
import {
ERROR_CLASSNAME,
SUBTEXT_DEFAULT,
Expand All @@ -11,18 +11,15 @@ import {
SYNTAX_ERROR_SPACING_SUFFIX,
} from '../constants';
import globalState from 'fixtures/globalState';
import { getMockRouterProps } from 'fixtures/mockRouter';

describe('SearchBar', () => {
const valueChangeMockEvent = { target: { value: 'Data Resources' } };
const submitMockEvent = { preventDefault: jest.fn() };
const setStateSpy = jest.spyOn(SearchBar.prototype, 'setState');
const routerProps = getMockRouterProps<any>(null, null);
const historyPushSpy = jest.spyOn(routerProps.history, 'push');
const setup = (propOverrides?: Partial<SearchBarProps>) => {
const props: SearchBarProps = {
searchTerm: '',
...routerProps,
submitSearch: jest.fn(),
...propOverrides
};
const wrapper = shallow<SearchBar>(<SearchBar {...props} />)
Expand Down Expand Up @@ -82,27 +79,18 @@ describe('SearchBar', () => {
expect(submitMockEvent.preventDefault).toHaveBeenCalled();
});

it('redirects back to home if searchTerm is empty', () => {
historyPushSpy.mockClear();
// @ts-ignore: mocked events throw type errors
wrapper.instance().handleValueSubmit(submitMockEvent);
expect(historyPushSpy).toHaveBeenCalledWith('/');
});

it('submits with correct props if isFormValid()', () => {
historyPushSpy.mockClear();
const { props, wrapper } = setup({ searchTerm: 'testTerm' });
// @ts-ignore: mocked events throw type errors
wrapper.instance().handleValueSubmit(submitMockEvent);
expect(historyPushSpy).toHaveBeenCalledWith(`/search?searchTerm=${wrapper.state().searchTerm}`);
expect(props.submitSearch).toHaveBeenCalledWith(props.searchTerm);
});

it('does not submit if !isFormValid()', () => {
historyPushSpy.mockClear();
const { props, wrapper } = setup({ searchTerm: 'tag:tag1 tag:tag2' });
// @ts-ignore: mocked events throw type errors
wrapper.instance().handleValueSubmit(submitMockEvent);
expect(historyPushSpy).not.toHaveBeenCalled();
expect(props.submitSearch).not.toHaveBeenCalled();
});
});

Expand All @@ -111,12 +99,16 @@ describe('SearchBar', () => {
let wrapper;
beforeAll(() => {
wrapper = setup().wrapper;
})
});

it('returns false', () => {
it('does not accept multiple search categories', () => {
expect(wrapper.instance().isFormValid('tag:tag1 tag:tag2')).toEqual(false);
});

it('does not accept empty search term', () => {
expect(wrapper.instance().isFormValid('')).toEqual(false);
});

it('sets state.subText correctly', () => {
expect(wrapper.state().subText).toEqual(SYNTAX_ERROR_CATEGORY);
});
Expand All @@ -130,7 +122,7 @@ describe('SearchBar', () => {
let wrapper;
beforeAll(() => {
wrapper = setup({ searchTerm: 'tag : tag1' }).wrapper;
})
});

it('returns false', () => {
expect(wrapper.instance().isFormValid('tag : tag1')).toEqual(false);
Expand All @@ -149,7 +141,7 @@ describe('SearchBar', () => {
let wrapper;
beforeAll(() => {
wrapper = setup().wrapper;
})
});

it('returns true', () => {
expect(wrapper.instance().isFormValid('tag:tag1')).toEqual(true);
Expand Down Expand Up @@ -237,6 +229,19 @@ describe('SearchBar', () => {
});
});

describe('mapDispatchToProps', () => {
let dispatch;
let result;
beforeAll(() => {
dispatch = jest.fn(() => Promise.resolve());
result = mapDispatchToProps(dispatch);
});

it('sets searchAll on the props', () => {
expect(result.submitSearch).toBeInstanceOf(Function);
});
});

describe('mapStateToProps', () => {
let result;
beforeAll(() => {
Expand Down
141 changes: 14 additions & 127 deletions frontend/amundsen_application/static/js/components/SearchPage/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ import * as React from 'react';
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';
import * as DocumentTitle from 'react-document-title';
import * as qs from 'simple-query-string';
import { RouteComponentProps } from 'react-router';
import { Search } from 'history';
import { Search as UrlSearch } from 'history';

import AppConfig from 'config/config';
import LoadingSpinner from 'components/common/LoadingSpinner';
Expand All @@ -14,14 +13,14 @@ import TabsComponent from 'components/common/Tabs';
import SearchBar from './SearchBar';

import { GlobalState } from 'ducks/rootReducer';
import { searchAll, searchResource, updateSearchTab } from 'ducks/search/reducer';
import { setPageIndex, setResource, urlDidUpdate } from 'ducks/search/reducer';
import {
DashboardSearchResults,
SearchAllRequest,
SearchResourceRequest,
SearchResults,
SetPageIndexRequest,
SetResourceRequest,
TableSearchResults,
UpdateSearchTabRequest,
UrlDidUpdateRequest,
UserSearchResults,
} from 'ducks/search/types';

Expand Down Expand Up @@ -54,9 +53,9 @@ export interface StateFromProps {
}

export interface DispatchFromProps {
searchAll: (term: string, selectedTab: ResourceType, pageIndex: number) => SearchAllRequest;
searchResource: (term: string, resource: ResourceType, pageIndex: number) => SearchResourceRequest;
updateSearchTab: (selectedTab: ResourceType) => UpdateSearchTabRequest;
setResource: (resource: ResourceType) => SetResourceRequest;
setPageIndex: (pageIndex: number) => SetPageIndexRequest;
urlDidUpdate: (urlSearch: UrlSearch) => UrlDidUpdateRequest;
}

export type SearchPageProps = StateFromProps & DispatchFromProps & RouteComponentProps<any>;
Expand All @@ -69,127 +68,15 @@ export class SearchPage extends React.Component<SearchPageProps> {
}

componentDidMount() {
const urlSearchParams = this.getUrlParams(this.props.location.search);
const globalStateParams = this.getGlobalStateParams();

if (this.shouldUpdateFromGlobalState(urlSearchParams, globalStateParams)) {
this.updatePageUrl(globalStateParams.term, globalStateParams.tab, globalStateParams.index, true);

} else if (this.shouldUpdateFromUrlParams(urlSearchParams, globalStateParams)) {
this.props.searchAll(urlSearchParams.term, urlSearchParams.tab, urlSearchParams.index);
this.updatePageUrl(urlSearchParams.term, urlSearchParams.tab, urlSearchParams.index, true);
}
}

shouldUpdateFromGlobalState(urlParams, globalStateParams): boolean {
return urlParams.term === '' && globalStateParams.term !== '';
}

shouldUpdateFromUrlParams(urlParams, globalStateParams): boolean {
return urlParams.term !== '' && urlParams.term !== globalStateParams.term;
this.props.urlDidUpdate(this.props.location.search);
}

componentDidUpdate(prevProps: SearchPageProps) {
const nextUrlParams = this.getUrlParams(this.props.location.search);
const prevUrlParams = this.getUrlParams(prevProps.location.search);

// If urlParams and globalState are synced, no need to update
if (this.isUrlStateSynced(nextUrlParams)) return;

// Capture any updates in URL
if (this.shouldUpdateSearchTerm(nextUrlParams, prevUrlParams)) {
this.props.searchAll(nextUrlParams.term, nextUrlParams.tab, nextUrlParams.index);

} else if (this.shouldUpdateTab(nextUrlParams, prevUrlParams)) {
this.props.updateSearchTab(nextUrlParams.tab)

} else if (this.shouldUpdatePageIndex(nextUrlParams, prevUrlParams)) {
this.props.searchResource(nextUrlParams.term, nextUrlParams.tab, nextUrlParams.index);
}
}

isUrlStateSynced(urlParams): boolean {
const globalStateParams = this.getGlobalStateParams();

return urlParams.term === globalStateParams.term &&
urlParams.tab === globalStateParams.tab &&
urlParams.index === globalStateParams.index;
}

shouldUpdateSearchTerm(nextUrlParams, prevUrlParams): boolean {
return nextUrlParams.term !== prevUrlParams.term;
}

shouldUpdateTab(nextUrlParams, prevUrlParams): boolean {
return nextUrlParams.tab !== prevUrlParams.tab;
}

shouldUpdatePageIndex(nextUrlParams, prevUrlParams): boolean {
return nextUrlParams.index !== prevUrlParams.index;
}

getSelectedTabByResourceType = (newTab: ResourceType): ResourceType => {
switch(newTab) {
case ResourceType.table:
case ResourceType.user:
return newTab;
case ResourceType.dashboard:
default:
return this.props.selectedTab;
if (this.props.location.search !== prevProps.location.search) {
this.props.urlDidUpdate(this.props.location.search);
}
};

getUrlParams(search: Search) {
const urlParams = qs.parse(search);
const { searchTerm, pageIndex, selectedTab } = urlParams;
const index = parseInt(pageIndex, 10);
return {
term: (searchTerm || '').trim(),
tab: this.getSelectedTabByResourceType(selectedTab),
index: isNaN(index) ? 0 : index,
};
};

getGlobalStateParams() {
return {
term: this.props.searchTerm,
tab: this.props.selectedTab,
index: this.getPageIndexByResourceType(this.props.selectedTab),
};
}


getPageIndexByResourceType = (tab: ResourceType): number => {
switch(tab) {
case ResourceType.table:
return this.props.tables.page_index;
case ResourceType.user:
return this.props.users.page_index;
case ResourceType.dashboard:
return this.props.dashboards.page_index;
}
return 0;
};

onPaginationChange = (index: number): void => {
this.updatePageUrl(this.props.searchTerm, this.props.selectedTab, index);
};

onTabChange = (tab: ResourceType): void => {
const newTab = this.getSelectedTabByResourceType(tab);
this.updatePageUrl(this.props.searchTerm, newTab, this.getPageIndexByResourceType(newTab));
};

updatePageUrl = (searchTerm: string, tab: ResourceType, pageIndex: number, replace: boolean = false): void => {
const pathName = `/search?searchTerm=${searchTerm}&selectedTab=${tab}&pageIndex=${pageIndex}`;

if (replace) {
this.props.history.replace(pathName);
} else {
this.props.history.push(pathName);
}
};

renderSearchResults = () => {
const tabConfig = [
{
Expand All @@ -212,7 +99,7 @@ export class SearchPage extends React.Component<SearchPageProps> {
tabs={ tabConfig }
defaultTab={ ResourceType.table }
activeKey={ this.props.selectedTab }
onSelect={ this.onTabChange }
onSelect={ this.props.setResource }
/>
</div>
);
Expand Down Expand Up @@ -282,7 +169,7 @@ export class SearchPage extends React.Component<SearchPageProps> {
source={ SEARCH_SOURCE_NAME }
itemsPerPage={ RESULTS_PER_PAGE }
activePage={ page_index }
onPagination={ this.onPaginationChange }
onPagination={ this.props.setPageIndex }
/>
</div>
);
Expand Down Expand Up @@ -330,7 +217,7 @@ export const mapStateToProps = (state: GlobalState) => {
};

export const mapDispatchToProps = (dispatch: any) => {
return bindActionCreators({ searchAll, searchResource, updateSearchTab }, dispatch);
return bindActionCreators({ setResource, setPageIndex, urlDidUpdate }, dispatch);
};

export default connect<StateFromProps, DispatchFromProps>(mapStateToProps, mapDispatchToProps)(SearchPage);
Loading

0 comments on commit 4a47bb9

Please sign in to comment.