From c095d45330dc46af5c4ac178daea8514f6cd7eee Mon Sep 17 00:00:00 2001 From: Elias Nahum Date: Wed, 19 Aug 2020 19:19:51 -0400 Subject: [PATCH] MM-25832 Prevent permalink view from loading posts multiple times (#4696) * Prevent permalink view from loading posts multiple times * Android unhandled exception and initial loader indicator * Fix permalink tests --- app/mm-redux/client/client4.ts | 2 + app/screens/permalink/permalink.js | 138 +++++++----------------- app/screens/permalink/permalink.test.js | 43 +------- 3 files changed, 43 insertions(+), 140 deletions(-) diff --git a/app/mm-redux/client/client4.ts b/app/mm-redux/client/client4.ts index b118bdf38..9e3272257 100644 --- a/app/mm-redux/client/client4.ts +++ b/app/mm-redux/client/client4.ts @@ -3126,6 +3126,7 @@ export class ClientError extends Error { intl: { defaultMessage: string; id: string } | { defaultMessage: string; id: string } | { id: string; defaultMessage: string; values: any } | { id: string; defaultMessage: string }; server_error_id: any; status_code: any; + details: Error; constructor(baseUrl: string, data: any) { super(data.message + ': ' + cleanUrlForLogging(baseUrl, data.url)); @@ -3134,6 +3135,7 @@ export class ClientError extends Error { this.intl = data.intl; this.server_error_id = data.server_error_id; this.status_code = data.status_code; + this.details = data.details; // Ensure message is treated as a property of this class when object spreading. Without this, // copying the object by using `{...error}` would not include the message. diff --git a/app/screens/permalink/permalink.js b/app/screens/permalink/permalink.js index 106e4fd8e..2e63f5cfc 100644 --- a/app/screens/permalink/permalink.js +++ b/app/screens/permalink/permalink.js @@ -15,27 +15,24 @@ import MaterialIcon from 'react-native-vector-icons/MaterialIcons'; import AwesomeIcon from 'react-native-vector-icons/FontAwesome'; import {Navigation} from 'react-native-navigation'; -import {General} from '@mm-redux/constants'; -import EventEmitter from '@mm-redux/utils/event_emitter'; -import {getLastPostIndex} from '@mm-redux/utils/post_list'; - -import FormattedText from 'app/components/formatted_text'; -import Loading from 'app/components/loading'; -import PostList from 'app/components/post_list'; -import PostListRetry from 'app/components/post_list_retry'; -import SafeAreaView from 'app/components/safe_area_view'; -import {marginHorizontal as margin} from 'app/components/safe_area_view/iphone_x_spacing'; - -import {preventDoubleTap} from 'app/utils/tap'; -import {changeOpacity, makeStyleSheetFromTheme} from 'app/utils/theme'; - import { resetToChannel, goToScreen, dismissModal, dismissAllModals, popToRoot, -} from 'app/actions/navigation'; +} from '@actions/navigation'; +import FormattedText from '@components/formatted_text'; +import Loading from '@components/loading'; +import PostList from '@components/post_list'; +import PostListRetry from '@components/post_list_retry'; +import SafeAreaView from '@components/safe_area_view'; +import {marginHorizontal as margin} from '@components/safe_area_view/iphone_x_spacing'; +import {General} from '@mm-redux/constants'; +import EventEmitter from '@mm-redux/utils/event_emitter'; +import {getLastPostIndex} from '@mm-redux/utils/post_list'; +import {preventDoubleTap} from '@utils/tap'; +import {changeOpacity, makeStyleSheetFromTheme} from '@utils/theme'; Animatable.initializeRegistryWithDefinitions({ growOut: { @@ -76,7 +73,6 @@ export default class Permalink extends PureComponent { isPermalink: PropTypes.bool, myMembers: PropTypes.object.isRequired, onClose: PropTypes.func, - onPress: PropTypes.func, postIds: PropTypes.array, theme: PropTypes.object.isRequired, isLandscape: PropTypes.bool.isRequired, @@ -84,7 +80,6 @@ export default class Permalink extends PureComponent { }; static defaultProps = { - onPress: () => true, postIds: [], }; @@ -92,50 +87,10 @@ export default class Permalink extends PureComponent { intl: intlShape.isRequired, }; - static getDerivedStateFromProps(nextProps, prevState) { - const newState = {}; - if (nextProps.focusedPostId !== prevState.focusedPostIdState) { - newState.focusedPostIdState = nextProps.focusedPostId; - } - - if (nextProps.channelId && nextProps.channelId !== prevState.channelIdState) { - newState.channelIdState = nextProps.channelId; - } - - if (nextProps.channelName && nextProps.channelName !== prevState.channelNameState) { - newState.channelNameState = nextProps.channelName; - } - - if (nextProps.postIds && nextProps.postIds.length > 0 && nextProps.postIds !== prevState.postIdsState) { - newState.postIdsState = nextProps.postIds; - } - - if (nextProps.focusedPostId !== prevState.focusedPostIdState) { - let loading = true; - if (nextProps.postIds && nextProps.postIds.length >= 10) { - loading = false; - } - - newState.loading = loading; - } - - if (Object.keys(newState).length === 0) { - return null; - } - - return newState; - } - constructor(props) { super(props); - const { - postIds, - channelId, - channelName, - focusedPostId, - error, - } = props; + const {error, postIds} = props; let loading = true; if (postIds && postIds.length >= 10) { @@ -143,14 +98,10 @@ export default class Permalink extends PureComponent { } this.state = { - title: channelName, + title: '', loading, error: error || '', retry: false, - channelIdState: channelId, - channelNameState: channelName, - focusedPostIdState: focusedPostId, - postIdsState: postIds, }; } @@ -159,14 +110,15 @@ export default class Permalink extends PureComponent { this.mounted = true; - if (this.state.loading) { - this.loadPosts(this.props); + if (this.state.loading && this.props.focusedPostId) { + this.initialLoad = true; + this.loadPosts(); } } componentDidUpdate() { - if (this.state.loading) { - this.loadPosts(this.props); + if (this.state.loading && this.props.focusedPostId && !this.initialLoad) { + this.loadPosts(); } } @@ -225,23 +177,17 @@ export default class Permalink extends PureComponent { }; handlePress = () => { - const {channelIdState} = this.state; - if (this.viewRef) { this.viewRef.growOut().then(() => { - this.jumpToChannel(channelIdState); + this.jumpToChannel(this.props.channelId); }); } }; jumpToChannel = async (channelId) => { if (channelId) { - const {actions, channelTeamId, currentTeamId, onClose} = this.props; - const currentChannelId = this.props.channelId; - const { - handleSelectChannel, - handleTeamChange, - } = actions; + const {actions, channelId: currentChannelId, channelTeamId, currentTeamId, onClose} = this.props; + const {handleSelectChannel, handleTeamChange} = actions; actions.selectPost(''); @@ -269,16 +215,22 @@ export default class Permalink extends PureComponent { } }; - loadPosts = async (props) => { + loadPosts = async () => { const {intl} = this.context; - const {actions, channelId, currentUserId, focusedPostId, isPermalink, postIds} = props; + const {actions, channelId, currentUserId, focusedPostId, isPermalink, postIds} = this.props; const {formatMessage} = intl; let focusChannelId = channelId; + if (this.mounted && !this.initialLoad) { + this.setState({loading: false}); + } + if (focusedPostId) { const post = await actions.getPostThread(focusedPostId, false); if (post.error && (!postIds || !postIds.length)) { - if (this.mounted && isPermalink && post.error.message.toLowerCase() !== 'network request failed') { + const error = post.error.message.toLowerCase() === 'network request failed'; + const connectionError = post.error.details?.message?.toLowerCase() === 'could not connect to the server.'; + if (this.mounted && isPermalink && !error && !connectionError) { this.setState({ error: formatMessage({ id: 'permalink.error.access', @@ -290,7 +242,7 @@ export default class Permalink extends PureComponent { }), }); } else if (this.mounted) { - this.setState({error: post.error.message, retry: true}); + this.setState({error: post.error.message, retry: true, loading: false}); } return; @@ -309,7 +261,8 @@ export default class Permalink extends PureComponent { await actions.getPostsAround(focusChannelId, focusedPostId, 10); - if (this.mounted) { + if (this.initialLoad) { + this.initialLoad = false; this.setState({loading: false}); } } @@ -317,8 +270,8 @@ export default class Permalink extends PureComponent { retry = () => { if (this.mounted) { + this.initialLoad = false; this.setState({loading: true, error: null, retry: false}); - this.loadPosts(this.props); } }; @@ -340,19 +293,8 @@ export default class Permalink extends PureComponent { }; render() { - const { - currentUserId, - focusedPostId, - theme, - isLandscape, - } = this.props; - const { - error, - retry, - loading, - postIdsState, - title, - } = this.state; + const {channelName, currentUserId, focusedPostId, isLandscape, postIds, theme} = this.props; + const {error, loading, retry, title} = this.state; const style = getStyleSheet(theme); let postList; @@ -384,8 +326,8 @@ export default class Permalink extends PureComponent { onHashtagPress={this.handleHashtagPress} onPermalinkPress={this.handlePermalinkPress} onPostPress={this.goToThread} - postIds={postIdsState} - lastPostIndex={Platform.OS === 'android' ? getLastPostIndex(postIdsState) : -1} + postIds={postIds} + lastPostIndex={Platform.OS === 'android' ? getLastPostIndex(postIds || []) : -1} currentUserId={currentUserId} lastViewedAt={0} highlightPinnedOrFlagged={false} @@ -430,7 +372,7 @@ export default class Permalink extends PureComponent { style={style.title} > {this.archivedIcon()} - {title} + {title || channelName} diff --git a/app/screens/permalink/permalink.test.js b/app/screens/permalink/permalink.test.js index b9ab182b4..b1f0be07c 100644 --- a/app/screens/permalink/permalink.test.js +++ b/app/screens/permalink/permalink.test.js @@ -34,7 +34,6 @@ describe('Permalink', () => { isPermalink: true, myMembers: {}, onClose: jest.fn(), - onPress: jest.fn(), postIds: ['post_id_1', 'focused_post_id', 'post_id_3'], theme: Preferences.THEMES.default, componentId: 'component-id', @@ -62,8 +61,7 @@ describe('Permalink', () => { wrapper.instance().loadPosts = jest.fn(); wrapper.instance().retry(); - expect(wrapper.instance().loadPosts).toHaveBeenCalledTimes(2); - expect(wrapper.instance().loadPosts).toBeCalledWith(baseProps); + expect(wrapper.instance().loadPosts).toHaveBeenCalledTimes(1); }); test('should call handleClose on onNavigatorEvent(backPress)', () => { @@ -76,43 +74,4 @@ describe('Permalink', () => { wrapper.instance().navigationButtonPressed({buttonId: 'backPress'}); expect(wrapper.instance().handleClose).toHaveBeenCalledTimes(1); }); - - test('should match state', () => { - const wrapper = shallow( - , - {context: {intl: {formatMessage: jest.fn()}}}, - ); - - expect(wrapper.state('channelIdState')).toEqual(baseProps.channelId); - expect(wrapper.state('channelNameState')).toEqual(baseProps.channelName); - expect(wrapper.state('focusedPostIdState')).toEqual(baseProps.focusedPostId); - expect(wrapper.state('postIdsState')).toEqual(baseProps.postIds); - - wrapper.setProps({channelId: ''}); - expect(wrapper.state('channelIdState')).toEqual(baseProps.channelId); - wrapper.setProps({channelId: null}); - expect(wrapper.state('channelIdState')).toEqual(baseProps.channelId); - wrapper.setProps({channelId: 'new_channel_id'}); - expect(wrapper.state('channelIdState')).toEqual('new_channel_id'); - - wrapper.setProps({channelName: ''}); - expect(wrapper.state('channelNameState')).toEqual(baseProps.channelName); - wrapper.setProps({channelName: null}); - expect(wrapper.state('channelNameState')).toEqual(baseProps.channelName); - wrapper.setProps({channelName: 'new_channel_name'}); - expect(wrapper.state('channelNameState')).toEqual('new_channel_name'); - - wrapper.setProps({focusedPostId: 'new_focused_post_id'}); - expect(wrapper.state('focusedPostIdState')).toEqual('new_focused_post_id'); - - wrapper.setProps({postIds: []}); - expect(wrapper.state('postIdsState')).toEqual(baseProps.postIds); - wrapper.setProps({postIds: ['post_id_1', 'focused_post_id']}); - expect(wrapper.state('postIdsState')).toEqual(['post_id_1', 'focused_post_id']); - - wrapper.setProps({postIds: baseProps.postIds, focusedPostId: baseProps.focusedPostId}); - expect(wrapper.state('loading')).toEqual(true); - wrapper.setProps({postIds: ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10', '11'], focusedPostId: 'new_focused_post_id'}); - expect(wrapper.state('loading')).toEqual(false); - }); });