From 64d9f7e7f729b7aa9307606803b93d2fd3c97070 Mon Sep 17 00:00:00 2001 From: Vladimir Lebedev Date: Sun, 12 Jan 2020 14:37:15 +0300 Subject: [PATCH] MM-18900 - Remove search unused status flags (#3772) * Remove search unused status flags * fix lint, fix wrong variable usage * remove comment * add test, fix bug with wrong PropTypes * make test cleaner --- app/components/no_results.js | 2 +- .../__snapshots__/flagged_posts.test.js.snap | 40 +++++++++- app/screens/flagged_posts/flagged_posts.js | 26 ++++-- .../flagged_posts/flagged_posts.test.js | 42 +++++++++- app/screens/flagged_posts/index.js | 6 -- .../__snapshots__/pinned_posts.test.js.snap | 79 +++++++++++++++++++ app/screens/pinned_posts/index.js | 6 -- app/screens/pinned_posts/pinned_posts.js | 33 ++++++-- app/screens/pinned_posts/pinned_posts.test.js | 75 ++++++++++++++++++ .../recent_mentions.test.js.snap | 40 +++++++++- app/screens/recent_mentions/index.js | 6 -- .../recent_mentions/recent_mentions.js | 24 +++++- .../recent_mentions/recent_mentions.test.js | 38 +++++++++ app/screens/search/index.js | 2 - app/screens/search/search.js | 77 ++++++++++-------- 15 files changed, 417 insertions(+), 79 deletions(-) create mode 100644 app/screens/pinned_posts/__snapshots__/pinned_posts.test.js.snap create mode 100644 app/screens/pinned_posts/pinned_posts.test.js diff --git a/app/components/no_results.js b/app/components/no_results.js index c8699231e..76a580489 100644 --- a/app/components/no_results.js +++ b/app/components/no_results.js @@ -12,7 +12,7 @@ export default class NoResults extends PureComponent { static propTypes = { description: PropTypes.string, iconName: PropTypes.string, - image: PropTypes.number, + image: PropTypes.object, theme: PropTypes.object.isRequired, title: PropTypes.string.isRequired, }; diff --git a/app/screens/flagged_posts/__snapshots__/flagged_posts.test.js.snap b/app/screens/flagged_posts/__snapshots__/flagged_posts.test.js.snap index 6c2a19af1..4bcfbf8cf 100644 --- a/app/screens/flagged_posts/__snapshots__/flagged_posts.test.js.snap +++ b/app/screens/flagged_posts/__snapshots__/flagged_posts.test.js.snap @@ -9,9 +9,42 @@ exports[`FlaggedPosts should match snapshot 1`] = ` } > - + +`; + +exports[`FlaggedPosts should match snapshot when component waiting for response 1`] = ` + + + + +`; + +exports[`FlaggedPosts should match snapshot when getFlaggedPosts failed 1`] = ` + + + `; diff --git a/app/screens/flagged_posts/flagged_posts.js b/app/screens/flagged_posts/flagged_posts.js index 61e570c32..58d596536 100644 --- a/app/screens/flagged_posts/flagged_posts.js +++ b/app/screens/flagged_posts/flagged_posts.js @@ -41,8 +41,6 @@ export default class FlaggedPosts extends PureComponent { selectFocusedPostId: PropTypes.func.isRequired, selectPost: PropTypes.func.isRequired, }).isRequired, - didFail: PropTypes.bool, - isLoading: PropTypes.bool, postIds: PropTypes.array, theme: PropTypes.object.isRequired, }; @@ -59,11 +57,28 @@ export default class FlaggedPosts extends PureComponent { super(props); props.actions.clearSearch(); - props.actions.getFlaggedPosts(); + + this.state = { + didFail: false, + isLoading: false, + }; + } + + getFlaggedPosts = async () => { + const {actions} = this.props; + this.setState({isLoading: true}); + const {error} = await actions.getFlaggedPosts(); + + this.setState({ + isLoading: false, + didFail: Boolean(error), + }); } componentDidMount() { this.navigationEventListener = Navigation.events().bindComponent(this); + + this.getFlaggedPosts(); } navigationButtonPressed({buttonId}) { @@ -195,11 +210,12 @@ export default class FlaggedPosts extends PureComponent { }; retry = () => { - this.props.actions.getFlaggedPosts(); + this.getFlaggedPosts(); }; render() { - const {didFail, isLoading, postIds, theme} = this.props; + const {postIds, theme} = this.props; + const {didFail, isLoading} = this.state; let component; if (didFail) { diff --git a/app/screens/flagged_posts/flagged_posts.test.js b/app/screens/flagged_posts/flagged_posts.test.js index 586e73749..a9b750ef6 100644 --- a/app/screens/flagged_posts/flagged_posts.test.js +++ b/app/screens/flagged_posts/flagged_posts.test.js @@ -29,12 +29,50 @@ describe('FlaggedPosts', () => { test('should match snapshot', () => { const wrapper = shallowWithIntl( - + , ); expect(wrapper.getElement()).toMatchSnapshot(); }); + test('should match snapshot when getFlaggedPosts failed', async () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getFlaggedPosts: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + await wrapper.instance().getFlaggedPosts(); + expect(wrapper.state('didFail')).toBe(true); + expect(wrapper.getElement()).toMatchSnapshot(); + }); + + test('should match snapshot when component waiting for response', () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getFlaggedPosts: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + wrapper.instance().getFlaggedPosts(); + expect(wrapper.getElement()).toMatchSnapshot(); + expect(wrapper.state('isLoading')).toBe(true); + }); + test('should call showSearchModal after awaiting dismissModal on handleHashtagPress', async () => { const error = new Error('foo'); const dismissModal = jest.spyOn(NavigationActions, 'dismissModal'); @@ -42,7 +80,7 @@ describe('FlaggedPosts', () => { const hashtag = 'test'; const wrapper = shallowWithIntl( - + , ); dismissModal.mockImplementation(async () => { diff --git a/app/screens/flagged_posts/index.js b/app/screens/flagged_posts/index.js index 57a9337f2..d8f837f67 100644 --- a/app/screens/flagged_posts/index.js +++ b/app/screens/flagged_posts/index.js @@ -6,7 +6,6 @@ import {connect} from 'react-redux'; import {selectFocusedPostId, selectPost} from 'mattermost-redux/actions/posts'; import {clearSearch, getFlaggedPosts} from 'mattermost-redux/actions/search'; -import {RequestStatus} from 'mattermost-redux/constants'; import {getTheme} from 'mattermost-redux/selectors/entities/preferences'; import {loadChannelsByTeamName, loadThreadIfNecessary} from 'app/actions/views/channel'; @@ -18,14 +17,9 @@ function makeMapStateToProps() { const preparePostIds = makePreparePostIdsForSearchPosts(); return (state) => { const postIds = preparePostIds(state, state.entities.search.flagged); - const {flaggedPosts: flaggedPostsRequest} = state.requests.search; - const isLoading = flaggedPostsRequest.status === RequestStatus.STARTED || - flaggedPostsRequest.status === RequestStatus.NOT_STARTED; return { postIds, - isLoading, - didFail: flaggedPostsRequest.status === RequestStatus.FAILURE, theme: getTheme(state), }; }; diff --git a/app/screens/pinned_posts/__snapshots__/pinned_posts.test.js.snap b/app/screens/pinned_posts/__snapshots__/pinned_posts.test.js.snap new file mode 100644 index 000000000..e712788cd --- /dev/null +++ b/app/screens/pinned_posts/__snapshots__/pinned_posts.test.js.snap @@ -0,0 +1,79 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`PinnedPosts should match snapshot 1`] = ` + + + + +`; + +exports[`PinnedPosts should match snapshot when component waiting for response 1`] = ` + + + + +`; + +exports[`PinnedPosts should match snapshot when getPinnedPosts failed 1`] = ` + + + + +`; diff --git a/app/screens/pinned_posts/index.js b/app/screens/pinned_posts/index.js index 4cfb165c6..b2accffbc 100644 --- a/app/screens/pinned_posts/index.js +++ b/app/screens/pinned_posts/index.js @@ -6,7 +6,6 @@ import {connect} from 'react-redux'; import {selectFocusedPostId, selectPost} from 'mattermost-redux/actions/posts'; import {clearSearch, getPinnedPosts} from 'mattermost-redux/actions/search'; -import {RequestStatus} from 'mattermost-redux/constants'; import {getTheme} from 'mattermost-redux/selectors/entities/preferences'; import {loadChannelsByTeamName, loadThreadIfNecessary} from 'app/actions/views/channel'; @@ -20,14 +19,9 @@ function makeMapStateToProps() { const {pinned} = state.entities.search; const channelPinnedPosts = pinned[ownProps.currentChannelId] || []; const postIds = preparePostIds(state, channelPinnedPosts); - const {pinnedPosts: pinnedPostsRequest} = state.requests.search; - const isLoading = pinnedPostsRequest.status === RequestStatus.STARTED || - pinnedPostsRequest.status === RequestStatus.NOT_STARTED; return { postIds, - isLoading, - didFail: pinnedPostsRequest.status === RequestStatus.FAILURE, theme: getTheme(state), }; }; diff --git a/app/screens/pinned_posts/pinned_posts.js b/app/screens/pinned_posts/pinned_posts.js index c064e5880..1f6a997e2 100644 --- a/app/screens/pinned_posts/pinned_posts.js +++ b/app/screens/pinned_posts/pinned_posts.js @@ -43,12 +43,19 @@ export default class PinnedPosts extends PureComponent { selectPost: PropTypes.func.isRequired, }).isRequired, currentChannelId: PropTypes.string.isRequired, - didFail: PropTypes.bool, - isLoading: PropTypes.bool, postIds: PropTypes.array, theme: PropTypes.object.isRequired, }; + constructor(props) { + super(props); + + this.state = { + didFail: false, + isLoading: false, + }; + } + static defaultProps = { postIds: [], }; @@ -60,9 +67,9 @@ export default class PinnedPosts extends PureComponent { componentDidMount() { this.navigationEventListener = Navigation.events().bindComponent(this); - const {actions, currentChannelId} = this.props; + const {actions} = this.props; actions.clearSearch(); - actions.getPinnedPosts(currentChannelId); + this.getPinnedPosts(); } navigationButtonPressed({buttonId}) { @@ -71,6 +78,18 @@ export default class PinnedPosts extends PureComponent { } } + getPinnedPosts = async () => { + const {actions, currentChannelId} = this.props; + + this.setState({isLoading: true}); + const {error} = await actions.getPinnedPosts(currentChannelId); + + this.setState({ + isLoading: false, + didFail: Boolean(error), + }); + } + setListRef = (ref) => { this.listRef = ref; } @@ -191,12 +210,12 @@ export default class PinnedPosts extends PureComponent { }; retry = () => { - const {actions, currentChannelId} = this.props; - actions.getPinnedPosts(currentChannelId); + this.getPinnedPosts(); }; render() { - const {didFail, isLoading, postIds, theme} = this.props; + const {postIds, theme} = this.props; + const {didFail, isLoading} = this.state; let component; if (didFail) { diff --git a/app/screens/pinned_posts/pinned_posts.test.js b/app/screens/pinned_posts/pinned_posts.test.js new file mode 100644 index 000000000..bba24703f --- /dev/null +++ b/app/screens/pinned_posts/pinned_posts.test.js @@ -0,0 +1,75 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import React from 'react'; + +import Preferences from 'mattermost-redux/constants/preferences'; + +import {shallowWithIntl} from 'test/intl-test-helper'; + +import PinnedPosts from './pinned_posts'; + +jest.mock('rn-placeholder', () => ({ + ImageContent: () => {}, +})); + +describe('PinnedPosts', () => { + const baseProps = { + actions: { + clearSearch: jest.fn(), + loadChannelsByTeamName: jest.fn(), + loadThreadIfNecessary: jest.fn(), + getPinnedPosts: jest.fn(), + selectFocusedPostId: jest.fn(), + selectPost: jest.fn(), + }, + theme: Preferences.THEMES.default, + currentChannelId: 'channelId', + }; + + test('should match snapshot', () => { + const wrapper = shallowWithIntl( + , + ); + + expect(wrapper.getElement()).toMatchSnapshot(); + }); + + test('should match snapshot when getPinnedPosts failed', async () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getPinnedPosts: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + await wrapper.instance().getPinnedPosts(); + expect(wrapper.state('didFail')).toBe(true); + expect(wrapper.getElement()).toMatchSnapshot(); + }); + + test('should match snapshot when component waiting for response', () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getPinnedPosts: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + wrapper.instance().getPinnedPosts(); + expect(wrapper.getElement()).toMatchSnapshot(); + expect(wrapper.state('isLoading')).toBe(true); + }); +}); diff --git a/app/screens/recent_mentions/__snapshots__/recent_mentions.test.js.snap b/app/screens/recent_mentions/__snapshots__/recent_mentions.test.js.snap index 4972ac358..3f9f0b578 100644 --- a/app/screens/recent_mentions/__snapshots__/recent_mentions.test.js.snap +++ b/app/screens/recent_mentions/__snapshots__/recent_mentions.test.js.snap @@ -9,9 +9,42 @@ exports[`RecentMentions should match snapshot 1`] = ` } > - + +`; + +exports[`RecentMentions should match snapshot when component waiting for response 1`] = ` + + + + +`; + +exports[`RecentMentions should match snapshot when getRecentMentions failed 1`] = ` + + + `; diff --git a/app/screens/recent_mentions/index.js b/app/screens/recent_mentions/index.js index 83198213c..f9dd4b749 100644 --- a/app/screens/recent_mentions/index.js +++ b/app/screens/recent_mentions/index.js @@ -6,7 +6,6 @@ import {connect} from 'react-redux'; import {selectFocusedPostId, selectPost} from 'mattermost-redux/actions/posts'; import {clearSearch, getRecentMentions} from 'mattermost-redux/actions/search'; -import {RequestStatus} from 'mattermost-redux/constants'; import {getTheme} from 'mattermost-redux/selectors/entities/preferences'; import {loadChannelsByTeamName, loadThreadIfNecessary} from 'app/actions/views/channel'; @@ -18,14 +17,9 @@ function makeMapStateToProps() { const preparePostIds = makePreparePostIdsForSearchPosts(); return (state) => { const postIds = preparePostIds(state, state.entities.search.results); - const {recentMentions: recentMentionsRequest} = state.requests.search; - const isLoading = recentMentionsRequest.status === RequestStatus.STARTED || - recentMentionsRequest.status === RequestStatus.NOT_STARTED; return { postIds, - isLoading, - didFail: recentMentionsRequest.status === RequestStatus.FAILURE, theme: getTheme(state), }; }; diff --git a/app/screens/recent_mentions/recent_mentions.js b/app/screens/recent_mentions/recent_mentions.js index ecae86faa..1ad43eab7 100644 --- a/app/screens/recent_mentions/recent_mentions.js +++ b/app/screens/recent_mentions/recent_mentions.js @@ -59,11 +59,28 @@ export default class RecentMentions extends PureComponent { super(props); props.actions.clearSearch(); - props.actions.getRecentMentions(); + this.state = { + didFail: false, + isLoading: false, + }; + } + + getRecentMentions = async () => { + const {actions} = this.props; + + this.setState({isLoading: true}); + const {error} = await actions.getRecentMentions(); + + this.setState({ + isLoading: false, + didFail: Boolean(error), + }); } componentDidMount() { this.navigationEventListener = Navigation.events().bindComponent(this); + + this.getRecentMentions(); } setListRef = (ref) => { @@ -193,11 +210,12 @@ export default class RecentMentions extends PureComponent { }; retry = () => { - this.props.actions.getRecentMentions(); + this.getRecentMentions(); }; render() { - const {didFail, isLoading, postIds, theme} = this.props; + const {postIds, theme} = this.props; + const {didFail, isLoading} = this.state; let component; if (didFail) { diff --git a/app/screens/recent_mentions/recent_mentions.test.js b/app/screens/recent_mentions/recent_mentions.test.js index 418f9f24a..7d2498408 100644 --- a/app/screens/recent_mentions/recent_mentions.test.js +++ b/app/screens/recent_mentions/recent_mentions.test.js @@ -35,6 +35,44 @@ describe('RecentMentions', () => { expect(wrapper.getElement()).toMatchSnapshot(); }); + test('should match snapshot when getRecentMentions failed', async () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getRecentMentions: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + await wrapper.instance().getRecentMentions(); + expect(wrapper.state('didFail')).toBe(true); + expect(wrapper.getElement()).toMatchSnapshot(); + }); + + test('should match snapshot when component waiting for response', () => { + const error = new Error('foo'); + + const newProps = { + ...baseProps, + actions: { + ...baseProps.actions, + getRecentMentions: jest.fn().mockResolvedValue({error}), + }, + }; + const wrapper = shallowWithIntl( + , + ); + + wrapper.instance().getRecentMentions(); + expect(wrapper.getElement()).toMatchSnapshot(); + expect(wrapper.state('isLoading')).toBe(true); + }); + test('should call showSearchModal after awaiting dismissModal on handleHashtagPress', async () => { const error = new Error('foo'); const dismissModal = jest.spyOn(NavigationActions, 'dismissModal'); diff --git a/app/screens/search/index.js b/app/screens/search/index.js index b727f547b..9ab08383f 100644 --- a/app/screens/search/index.js +++ b/app/screens/search/index.js @@ -43,7 +43,6 @@ function makeMapStateToProps() { const currentTeamId = getCurrentTeamId(state); const currentChannelId = getCurrentChannelId(state); const {recent} = state.entities.search; - const {searchPosts: searchRequest} = state.requests.search; const currentUser = getCurrentUser(state); const enableTimezone = isTimezoneEnabled(state); @@ -62,7 +61,6 @@ function makeMapStateToProps() { postIds, archivedPostIds, recent: recent[currentTeamId], - searchingStatus: searchRequest.status, isSearchGettingMore, theme: getTheme(state), enableDateSuggestion, diff --git a/app/screens/search/search.js b/app/screens/search/search.js index a9d596028..76009b402 100644 --- a/app/screens/search/search.js +++ b/app/screens/search/search.js @@ -15,7 +15,6 @@ import AwesomeIcon from 'react-native-vector-icons/FontAwesome'; import {Navigation} from 'react-native-navigation'; import {debounce} from 'mattermost-redux/actions/helpers'; -import {RequestStatus} from 'mattermost-redux/constants'; import {isDateLine, getDateForDateLine} from 'mattermost-redux/utils/post_list'; import Autocomplete from 'app/components/autocomplete'; @@ -49,6 +48,7 @@ const RECENT_SEPARATOR_HEIGHT = 3; const SCROLL_UP_MULTIPLIER = 6; const SEARCHING = 'searching'; const NO_RESULTS = 'no results'; +const FAILURE = 'failure'; export default class Search extends PureComponent { static propTypes = { @@ -70,7 +70,6 @@ export default class Search extends PureComponent { postIds: PropTypes.array, archivedPostIds: PropTypes.arrayOf(PropTypes.string), recent: PropTypes.array.isRequired, - searchingStatus: PropTypes.string, isSearchGettingMore: PropTypes.bool.isRequired, theme: PropTypes.object.isRequired, enableDateSuggestion: PropTypes.bool, @@ -98,6 +97,10 @@ export default class Search extends PureComponent { cursorPosition: 0, value: props.initialValue, recent: props.recent, + didFail: false, + isLoading: false, + isLoaded: false, + status: 'not_started', }; } @@ -116,11 +119,11 @@ export default class Search extends PureComponent { } componentDidUpdate(prevProps, prevState) { - const {searchingStatus: status, enableDateSuggestion} = this.props; - const {recent} = this.state; - const {searchingStatus: prevStatus} = prevProps; + const {enableDateSuggestion} = this.props; + const {recent, didFail, isLoaded, status} = this.state; + const {status: prevStatus} = prevState; const shouldScroll = prevStatus !== status && - (status === RequestStatus.SUCCESS || status === RequestStatus.FAILURE) && + (isLoaded || didFail) && !this.props.isSearchGettingMore && !prevProps.isSearchGettingMore && prevState.recent.length === recent.length; if (this.props.isLandscape !== prevProps.isLandscape) { @@ -273,11 +276,13 @@ export default class Search extends PureComponent { }); handleTextChanged = (value, selectionChanged) => { - const {actions, searchingStatus, isSearchGettingMore} = this.props; + const {actions, isSearchGettingMore} = this.props; + const {isLoaded} = this.state; + this.setState({value}); actions.handleSearchDraftChanged(value); - if (!value && searchingStatus === RequestStatus.SUCCESS && !isSearchGettingMore) { + if (!value && isLoaded && !isSearchGettingMore) { actions.clearSearch(); this.scrollToTop(); } @@ -485,7 +490,7 @@ export default class Search extends PureComponent { } }; - search = (text, isOrSearch) => { + search = async (text, isOrSearch) => { const {actions, currentTeamId, viewArchivedChannels} = this.props; const recent = [...this.state.recent]; const terms = text.trim(); @@ -510,13 +515,21 @@ export default class Search extends PureComponent { per_page: 20, include_deleted_channels: viewArchivedChannels, }; - actions.searchPostsWithParams(currentTeamId, params, true); + this.setState({isLoading: true, isLoaded: false, status: 'isLoading'}); + const {error} = await actions.searchPostsWithParams(currentTeamId, params, true); + if (!recent.find((r) => r.terms === terms)) { recent.push({ terms, }); this.setState({recent}); } + this.setState({ + isLoading: false, + didFail: Boolean(error), + isLoaded: true, + status: error ? 'didFail' : 'isLoaded', + }); }; setModifierValue = preventDoubleTap((modifier) => { @@ -549,7 +562,6 @@ export default class Search extends PureComponent { const { isLandscape, postIds, - searchingStatus, theme, isSearchGettingMore, } = this.props; @@ -559,6 +571,9 @@ export default class Search extends PureComponent { cursorPosition, recent, value, + didFail, + isLoading, + isLoaded, } = this.state; const style = getStyleFromTheme(theme); @@ -627,8 +642,23 @@ export default class Search extends PureComponent { } let results; - switch (searchingStatus) { - case RequestStatus.STARTED: + if (didFail) { + if (postIds.length) { + results = postIds; + } else { + results = [{ + id: FAILURE, + component: ( + + + + ), + }]; + } + } else if (isLoading) { if (isSearchGettingMore) { results = postIds; } else { @@ -641,8 +671,7 @@ export default class Search extends PureComponent { ), }]; } - break; - case RequestStatus.SUCCESS: + } else if (isLoaded) { if (postIds.length) { results = postIds; } else if (this.state.value) { @@ -657,24 +686,6 @@ export default class Search extends PureComponent { ), }]; } - break; - case RequestStatus.FAILURE: - if (postIds.length) { - results = postIds; - } else { - results = [{ - id: RequestStatus.FAILURE, - component: ( - - - - ), - }]; - } - break; } if (results) {